Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding 'Previous Role' to the reconfiguration text #874

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

vpavlovicMSFT
Copy link

Currently, if a partition is undergoing reconfiguration, there is no means to determine the primary replica, as the only information available to customers via SFX is the target role of the replica. The introduction of 'Previous Role' will significantly reduce the risk of accidentally terminating the primary replica, a scenario that could potentially lead to data loss.

While a partition is undergoing reconfiguration, in addition to the 'Target Role', 'Previous Role' will also be displayed.

PreviousReplicaRole SFX

ADO work item

Copy link
Collaborator

@chensation chensation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the same logic is also in DeplolyedReplica, that will also need to be changed. In addition, let's add a unit test for this change in replica.cy.ts and deployedReplica.cy.ts

src/SfxWeb/src/app/Models/DataModels/Replica.ts Outdated Show resolved Hide resolved
@jeffj6123
Copy link
Member

could we also add a new column to the replica list for partition which shows previous replica role? this could be useful in investigation

@vpavlovicMSFT
Copy link
Author

vpavlovicMSFT commented Nov 7, 2023

@jeffj6123

could we also add a new column to the replica list for partition which shows previous replica role? this could be useful in investigation

We can't do that, since the API used for getting previous replica role Get-ServiceFabricReplica
only returns previous role if the partition is undergoing a reconfiguration.

But we can add Previous Role in partition view if the partition is reconfiguring, and when it's not reconfiguring, we just show current role.
Something like this:
image

@vpavlovicMSFT
Copy link
Author

vpavlovicMSFT commented Nov 7, 2023

Latest changes to the UI:
image

Also, I need help with adjusting the CSS for replica-tile component. Tile's height doesn't automatically adjust to the added field... It looks like the relevant styles are in src\SfxWeb\src\app\modules\partition-replication\replica-status-container\replica-status-container.component.scss

@jeffj6123
Copy link
Member

Latest changes to the UI: image

Also, I need help with adjusting the CSS for replica-tile component. Tile's height doesn't automatically adjust to the added field... It looks like the relevant styles are in src\SfxWeb\src\app\modules\partition-replication\replica-status-container*replica-status-container.component.scss*

I only meant the table at the bottom, like an extra column added next to replica role

@vpavlovicMSFT
Copy link
Author

vpavlovicMSFT commented Nov 8, 2023

Latest changes to the UI: image
Also, I need help with adjusting the CSS for replica-tile component. Tile's height doesn't automatically adjust to the added field... It looks like the relevant styles are in src\SfxWeb\src\app\modules\partition-replication\replica-status-container*replica-status-container.component.scss*

I only meant the table at the bottom, like an extra column added next to replica role

Oh, we can do that, but like I already mentioned, we can only show previous role if a partition is undergoing a reconfig.

@jeffj6123
Copy link
Member

Latest changes to the UI: image
Also, I need help with adjusting the CSS for replica-tile component. Tile's height doesn't automatically adjust to the added field... It looks like the relevant styles are in src\SfxWeb\src\app\modules\partition-replication\replica-status-container*replica-status-container.component.scss*

I only meant the table at the bottom, like an extra column added next to replica role

Oh, we can do that, but like I already mentioned, we can only show previous role if a partition is undergoing a reconfig.

my mistake I didnt see that in the previous message.

@vpavlovicMSFT
Copy link
Author

Screenshot 2024-04-04 180414

@vpavlovicMSFT
Copy link
Author

@jeffj6123 @chensation After a considerable break, this PR has been revived! 😊 Please take a moment to review the most recent updates.

@chensation
Copy link
Collaborator

@vpavlovicMSFT , I think my requested changes still stands. The same role logic is inside DeployedReplica.ts and we should change it as well unless there's a reason to not change it.
In addition, although you added a PreviousReplicaRole field to the cypress fixtures, replica.cy.ts and deployedReplica.cy.ts don't have any tests to check on if roles are displaying properly. I think it's good practice to always update and add tests with every feature we add so we can maintain and increase our test coverage.

@jeffj6123 , I know your original comment said you only wanted a "Previous Role" column at the bottom table, but would you say that adding a "Previous Role" row to the Replicator Status would still make it easier to read?
image

this particular change has been reverted, and now looks like this
image

vpavlovicMSFT and others added 2 commits November 26, 2024 14:38
…case) (#2)

* DeployedReplica role during reconfiguration

* Handle when partition is undefined

* Update DeployedReplica.ts

* Remove empty line
@mssusnjar
Copy link

Hi @jeffj6123 and @chensation, we addressed the requested change and adjusted the logic in DeployedReplica.ts. We would like to complete this change soon. Can you please review the latest version?

@chensation
Copy link
Collaborator

@mssusnjar can you add tests into replica.cy.ts and deployedReplica.cy.ts to test for this change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants