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

HDDS-9303. Display leader in table and highlight current node in OM web UI #5311

Merged
merged 13 commits into from
Dec 6, 2023

Conversation

smitajoshi12
Copy link
Contributor

@smitajoshi12 smitajoshi12 commented Sep 18, 2023

What changes were proposed in this pull request?

SCM web UI displays the leader information on the webUI and it makes debugging easier. Need to do similar in Ozone Manager.

What is the link to the Apache JIRA

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

How was this patch tested?

Manually

Old Ozone UI
image

New Changes
image

When it is Standalone with no data hiding OM Roles (HA) Table with ozone-ha
image

Running ozone
image

Exception Handling

image

@dombizita dombizita self-requested a review September 18, 2023 16:12
@devmadhuu
Copy link
Contributor

@smitajoshi12 thanks for working on this patch, PR description says need to show leader information in ozone manager webUI, however I think we already show LEADER and FOLLOWER info in OM Web UI, but in JSON format, so pls correct the PR description and PR title.

@dombizita
Copy link
Contributor

dombizita commented Sep 19, 2023

thanks for working on this @smitajoshi12!

@smitajoshi12 thanks for working on this patch, PR description says need to show leader information in ozone manager webUI, however I think we already show LEADER and FOLLOWER info in OM Web UI, but in JSON format, so pls correct the PR description and PR title.

I agree with Devesh, the OM roles are already present in the OM UI, it was added by @ArafatKhan2198 in HDDS-6863. For me it looks good already as it is, all the necessary information is present there.
This new table that is nested in another table looks a bit weird for me, but maybe that's just for me :) If we want to have a change like this, maybe it should be moved to a new table, but I'm not sure if that would improve the OM UI that much. In the SCM UI it also presents this information in JSON format. Let me know what you think!

@smitajoshi12 smitajoshi12 changed the title HDDS-9303. Ozone Manager : Add Ozone leader information on the Ozone manager UI HDDS-9303. Ozone Manager : Display leader information on readable format and highlight current node. Sep 20, 2023
@smitajoshi12 smitajoshi12 changed the title HDDS-9303. Ozone Manager : Display leader information on readable format and highlight current node. HDDS-9303. Ozone Manager : Display leader information in tanle format and highlight current node. Sep 20, 2023
@smitajoshi12
Copy link
Contributor Author

thanks for working on this @smitajoshi12!

@smitajoshi12 thanks for working on this patch, PR description says need to show leader information in ozone manager webUI, however I think we already show LEADER and FOLLOWER info in OM Web UI, but in JSON format, so pls correct the PR description and PR title.

I agree with Devesh, the OM roles are already present in the OM UI, it was added by @ArafatKhan2198 in HDDS-6863. For me it looks good already as it is, all the necessary information is present there. This new table that is nested in another table looks a bit weird for me, but maybe that's just for me :) If we want to have a change like this, maybe it should be moved to a new table, but I'm not sure if that would improve the OM UI that much. In the SCM UI it also presents this information in JSON format. Let me know what you think!

@dombizita
Hi ZIta,
Done Changes as suggested by you. Attached new Screenshot.

@devmadhuu
Copy link
Contributor

thanks for working on this @smitajoshi12!

@smitajoshi12 thanks for working on this patch, PR description says need to show leader information in ozone manager webUI, however I think we already show LEADER and FOLLOWER info in OM Web UI, but in JSON format, so pls correct the PR description and PR title.

I agree with Devesh, the OM roles are already present in the OM UI, it was added by @ArafatKhan2198 in HDDS-6863. For me it looks good already as it is, all the necessary information is present there. This new table that is nested in another table looks a bit weird for me, but maybe that's just for me :) If we want to have a change like this, maybe it should be moved to a new table, but I'm not sure if that would improve the OM UI that much. In the SCM UI it also presents this information in JSON format. Let me know what you think!

@dombizita Hi ZIta, Done Changes as suggested by you. Attached new Screenshot.

@smitajoshi12 what is "tanle" format as you have mentioned in PR title. Is it by mistake ? Also PR description is not in line with what is being done in this patch. Also I agree with @dombizita , if this patch really needed as JSON format itself look fine and we want to keep any formatting or processing over OM UI as minimal as possible unless it is very much necessary. Can we keep same as in SCM webUI (in Json format) ? @dombizita @sumitagrawl pls suggest.

Copy link
Contributor

@ArafatKhan2198 ArafatKhan2198 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @smitajoshi12 for the OM-HA the table looks fine but what about when we have a standalone cluster will we want to display a table there as well, could you please attach a screenshot for that too.

@smitajoshi12 smitajoshi12 changed the title HDDS-9303. Ozone Manager : Display leader information in tanle format and highlight current node. HDDS-9303. Ozone Manager : Display leader information in table format and highlight current node. Sep 21, 2023
@smitajoshi12
Copy link
Contributor Author

Thanks for the changes @smitajoshi12 for the OM-HA the table looks fine but what about when we have a standalone cluster will we want to display a table there as well, could you please attach a screenshot for that too.

@ArafatKhan2198
Attached New Screenshot with Standalone cluster with no Data.

Copy link
Contributor

@sumitagrawl sumitagrawl left a comment

Choose a reason for hiding this comment

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

@smitajoshi12 Thanks for working over this, have few comments.

@sumitagrawl
Copy link
Contributor

ep same as in SCM webUI (in Json format)

Giving as JSON format, and showing in tablular format is good. I think its just render the page with data and may not have much impact for table.
Already given few comments for STANDALONE, non-ratis or exception cases, this is hidden now. That may need resolve for showing.

Copy link
Contributor

@dombizita dombizita left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @smitajoshi12, please take a look at my comments!

Copy link
Contributor

@dombizita dombizita left a comment

Choose a reason for hiding this comment

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

Thanks for updating you patch @smitajoshi12, I added a few comments. Also there are 2 lines that are longer than 30 characters, you can check it here: https://github.com/smitajoshi12/ozone/actions/runs/6657443633/job/18092176653

Copy link
Contributor

@sumitagrawl sumitagrawl left a comment

Choose a reason for hiding this comment

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

@smitajoshi12 Given few minor comments, please check

@adoroszlai
Copy link
Contributor

/pending If there is exception, how the output looks? do still accessing roles[1],... can have issue over UI in exception case?

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Marking this issue as un-mergeable as requested.

Please use /ready comment when it's resolved.

Please note that the PR will be closed after 21 days of inactivity from now. (But can be re-opened anytime later...)

If there is exception, how the output looks? do still accessing roles[1],... can have issue over UI in exception case?

@smitajoshi12
Copy link
Contributor Author

/ready
@adoroszlai

I have attached screenshot for Exception Handling.
There is no issue over UI in terms of Exception Handling.

Copy link
Contributor

@dombizita dombizita left a comment

Choose a reason for hiding this comment

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

Thanks for updating your PR and its description @smitajoshi12, I have one more comment, please take a look at it. Otherwise it's looking good to me!

@adoroszlai adoroszlai changed the title HDDS-9303. Ozone Manager : Display leader information in table format and highlight current node. HDDS-9303. Display leader in table and highlight current node in OM web UI Nov 27, 2023
Copy link
Contributor

@dombizita dombizita left a comment

Choose a reason for hiding this comment

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

Thanks for your continuous effort with this @smitajoshi12, could you take a look at my comments? Also please update the screenshots in the PR description with the changes.

Copy link
Contributor

@sumitagrawl sumitagrawl left a comment

Choose a reason for hiding this comment

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

@dombizita
Copy link
Contributor

/ready

@github-actions github-actions bot dismissed their stale review December 6, 2023 12:00

Blocking review request is removed.

@github-actions github-actions bot removed the pending label Dec 6, 2023
@dombizita dombizita merged commit dd431cd into apache:master Dec 6, 2023
34 checks passed
@dombizita
Copy link
Contributor

Thanks for working on this @smitajoshi12! Thank you for the review @devmadhuu, @ArafatKhan2198 and @sumitagrawl!

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.

6 participants