Skip to content

Conversation

@ArafatKhan2198
Copy link
Contributor

@ArafatKhan2198 ArafatKhan2198 commented Jun 15, 2022

What changes were proposed in this pull request?

Add Group-Id, Current-Node Role, Election Count, Leader Election Elapsed time & the Ratis-Ring information for OM UI.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-6863

How was this patch tested?

Tested on docker OM ha env
image

For a single node cluster it will print STANDALONE
image

Mohammad Arafat Khan and others added 2 commits June 15, 2022 18:39
Add Group-Id & Ratis-Ring information for OM UI.
@ArafatKhan2198
Copy link
Contributor Author

ArafatKhan2198 commented Jun 15, 2022

@siddhantsangwan @sadanand48 @jojochuang @adoroszlai can you please review this patch !!

@ArafatKhan2198 ArafatKhan2198 changed the title HDDS-6863 Add Group-Id & Ratis-Roles Information for OM UI HDDS-6863 Add Group-Id & Ratis-Roles Information for OM UI. Jun 15, 2022
@ArafatKhan2198 ArafatKhan2198 changed the title HDDS-6863 Add Group-Id & Ratis-Roles Information for OM UI. HDDS-6863. Add Group-Id & Ratis-Roles Information for OM UI. Jun 15, 2022
@adoroszlai
Copy link
Contributor

@ArafatKhan2198
Copy link
Contributor Author

ensure CI run in one's own fork succeeds, before opening a PR

Thanks I was not aware of that !!
I will do that next time :)

Mohammad Arafat Khan added 2 commits July 19, 2022 10:00
Copy link
Contributor

@sadanand48 sadanand48 left a comment

Choose a reason for hiding this comment

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

Thanks @ArafatKhan2198 for addressing the comments. Overall changes look good, dropped some minor comments inline.

Copy link
Contributor

@sadanand48 sadanand48 left a comment

Choose a reason for hiding this comment

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

Thanks @ArafatKhan2198 for addressing comments, +1 LGTM

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @ArafatKhan2198 for the patch. Mostly looks good, but I think there are some edge cases, and some place for improvement.

Copy link
Contributor

@jojochuang jojochuang left a comment

Choose a reason for hiding this comment

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

Looks okay after Attila's comments are addressed.

@jojochuang
Copy link
Contributor

For future reference, it was be nice to show current term and how long has pasted since the last election. (Useful to know if there was any recent election and whether or not the OMs were unstable)

@adoroszlai
Copy link
Contributor

Thanks @ArafatKhan2198 for updating the patch. The order of nodes is still "random", the rest looks good.

@ArafatKhan2198 ArafatKhan2198 requested review from adoroszlai and jojochuang and removed request for adoroszlai and jojochuang September 20, 2022 14:27
@ArafatKhan2198
Copy link
Contributor Author

@adoroszlai the OM's are now printed in order

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @ArafatKhan2198 for updating the patch with sorting.

@adoroszlai
Copy link
Contributor

Note: Although we used to recommend pushing a new commit to re-trigger the tests, Github now allows committers to re-run only the failed jobs. This also prevents the case where previously passed checks fail in a new run.

@adoroszlai adoroszlai merged commit 55efab1 into apache:master Sep 28, 2022
@adoroszlai
Copy link
Contributor

Thanks @ArafatKhan2198 for the patch, @jojochuang, @sadanand48 for the review.

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