HBASE-25980 Master table.jsp pointed at meta throws 500 when no all r… #3374
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…eplicas are online
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Jun 11, 2021
| try { | ||
| metaLocation = MetaTableLocator.waitMetaRegionLocation(master.getZooKeeper(), j, 1); | ||
| } catch (NotAllMetaRegionsOnlineException e) { | ||
| //Should ignore this Exception for we don't need to display rit meta region info in UI |
There was a problem hiding this comment.
Should ignore this Exception for we don't need to display rit meta region info in UI
=>
Should ignore this Exception as we don't need to display rit meta region info in UI
Use 'as' instead of 'for'.
And mind explaining a bit more on why we do not need to dispay rit meta region info? And what is going on if metaLocation is null?
There was a problem hiding this comment.
Ok! Thansk for your comments
GeorryHuang
commented
Jun 12, 2021
| try { | ||
| metaLocation = MetaTableLocator.waitMetaRegionLocation(master.getZooKeeper(), j, 1); | ||
| } catch (NotAllMetaRegionsOnlineException e) { | ||
| //Since a region in transition state throw a NotAllMetaRegionsOnlineException then |
There was a problem hiding this comment.
@Apache9 What about this explanation here? By the way, I added a 'continue' here to skip to display rit region. IMHO, we do not need to show region info here since the home page of master status UI has show which region is in transition and why
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
GeorryHuang
commented
Jun 15, 2021
| //could cause the whole UI crash, and the operator could have learned that this | ||
| //region is in transition state from the master status UI. We should ignore to display | ||
| //this region info. | ||
| continue; |
There was a problem hiding this comment.
In this case, the user is looking at the meta table specifically. Maybe the below if (metaLocation != null) {... needs an else block that prints a message stating that some region location(s) could not be identified?
@ndimiduk Since the user may learn the meta region is in transition state from the transition list in Master status UI， Should we still show any details here?
There was a problem hiding this comment.
After I check the current implementation it shows that If a region of user-level table is in transition, it does not be displayed in its table details page. So I think the meta region should be the same.
Apache9
approved these changes
Jun 15, 2021
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…eplicas are online