HBASE-25469 Add detailed RIT info in JSON format for consumption as metrics #3535
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
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
apurtell
requested changes
Aug 2, 2021
| @@ -659,6 +660,7 @@ protected RSRpcServices createRpcServices() throws IOException { | |||
| @Override | |||
| protected void configureInfoServer() { | |||
| infoServer.addUnprivilegedServlet("master-status", "/master-status", MasterStatusServlet.class); | |||
| infoServer.addPrivilegedServlet("rits", "/rits", RitServlet.class); | |||
There was a problem hiding this comment.
Nit: Let's call it "/rit".
Not a big deal, but other paths are not in the plural form.
| } | ||
|
|
||
| if (rits.isEmpty()) { | ||
| out.write("There are currently no regions in transition."); |
There was a problem hiding this comment.
is it okay to return an empty map then?
bharathv
requested changes
Aug 2, 2021
|
@bharathv I considered consolidating, but mainly wrote it this way so that 1) the url isn't so complex, and 2) the code is not dependent on this page/the complex url (we can also attach the servlet elsewhere if we want). |
|
🎊 +1 overall
This message was automatically generated. |
|
The problem is that the two places can now diverge. If someone is not familiar with the other page, they just update one of them. As an operator it is now confusing whether to use /rit or /rit.jsp or both or how they differ apart from the output type is not obvious. Is it difficult to clean up rit.jsp or move rit.jsp logic into /rit servlet (and add a redirect in branch-2) ? |
|
@bharathv there already are multiple places where RIT info can be found: the main |
|
actually, I think it makes more sense to link |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
I'm not convinced by the explanation. I can see how /master-status has a summary in the footer and is a bit different from the other pages but it is usually a common pattern to do something like /rits?type=html /rits?type=json /rits?type=txt etc and all are served by the backing page (especially when they are serving the same information presented in different ways). Not a big deal though, don't want to block the progress here. I'm ok if @apurtell has already +1ed the patch. |
|
@bharathv ok, I moved the JSON logic into |
bharathv
approved these changes
Aug 3, 2021
| @@ -69,24 +75,29 @@ | |||
| <div class="page-header"> | |||
| <a href="/rits.jsp?format=txt&filter=region&table=<%=table%>&state=<%=state%>" class="btn btn-primary">Regions in text format</a> | |||
| <a href="/rits.jsp?format=txt&filter=procedure&table=<%=table%>&state=<%=state%>" class="btn btn-info">Procedures in text format</a> | |||
| <a href="/rits.jsp?format=json" class="btn btn-info">RIT info as JSON</a> | |||
There was a problem hiding this comment.
Add table and state filters (=null) for completeness?
|
🎊 +1 overall
This message was automatically generated. |
caroliney14
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
caroliney14
force-pushed
the
HBASE-25469
branch
from
78e97fd
to
cc426dc
Compare
apurtell
approved these changes
Aug 4, 2021
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +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.
No description provided.