microsoft / service-fabric-explorer

Service Fabric Explorer is a web based dashboard for visualizing the state of a Service Fabric cluster.
MIT License
107 stars 69 forks source link

Adding 'Previous Role' to the reconfiguration text #874

Open vpavlovicMSFT opened 10 months ago

vpavlovicMSFT commented 10 months ago

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

jeffj6123 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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-containerreplica-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 commented 10 months ago

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-containerreplica-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 commented 5 months ago

Screenshot 2024-04-04 180414

vpavlovicMSFT commented 5 months ago

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

chensation commented 5 months ago

@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