-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-26192 Master UI hbck should provide a JSON formatted output option #5772
Conversation
Hi @virajjasani @apurtell @ndimiduk Can I get review on this JIRA? (I have tried to include all the comments mentioned in the PR #4470 ) |
🎊 +1 overall
This message was automatically generated. |
Endpoint list for each inconsistencies : Orphan Region on FS : Here is Sample Output for
|
private final HbckRegionDetails region1; | ||
private final HbckRegionDetails region2; | ||
|
||
public HbckOverlapRegions(HbckRegionDetails overlapRegion1, HbckRegionDetails overlapRegion2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be region1, region2 as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
|
||
@GET | ||
@Path("/orphan_regions_on_fs") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read at places using hyphens is a recommended approach in REST APIs - https://restfulapi.net/resource-naming/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
final String response = | ||
classRule.getTarget().path("region_holes").request(MediaType.APPLICATION_JSON_TYPE) | ||
.header("X-Jersey-Tracing-Accept", true).get(String.class); | ||
assertThat(response, allOf(startsWith("{\"data\":["), endsWith("]}"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of only checking for starts with "data". can we have some mocks to return a list of objects and validate if the response is proper?
similarly at other places too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Check 8177556
LGTM |
🎊 +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. |
🎊 +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. |
💔 -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. |
💔 -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, nice work.
Left one suggestion, hopefully simple, not a thread to pull on.
@ndimiduk check this out.
.register(ResponseEntityMapper.class).register(GsonSerializationFeature.class) | ||
.register(new MasterFeature(master)) | ||
|
||
// devs: enable TRACING to see how jersey is dispatching to resources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for adding this note.
* The root object exposing hbck.jsp page as JSON Output. | ||
*/ | ||
@Path("hbck-metrics") | ||
@Produces({ MediaType.APPLICATION_JSON }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add support for XML here too? (Is that MediaType.APPLICATION_XML
?) Should be as simple as that. Jersey will choose according to the contents of the client's Accept
header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on my understanding from #4177 by @ndimiduk and limited understanding of JERSEY, We would have to implement something similar to https://github.com/apache/hbase/tree/master/hbase-server/src/main/java/org/apache/hadoop/hbase/master/http/gson for XML too. And that would be an another JIRA itself.
@ndimiduk What do you think?
|
||
private static final MiniClusterRule miniClusterRule = MiniClusterRule.newBuilder() | ||
.setMiniClusterOption( | ||
StartTestingClusterOption.builder().numZkServers(3).numMasters(3).numDataNodes(3).build()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need 3 servers, or would the default 1 be good enough?
StartTestingClusterOption.builder().numZkServers(3).numMasters(3).numDataNodes(3).build()) | |
StartTestingClusterOption.builder().build()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have used similar configs from other tests for number of master and regionserver. I will look into more tests to see if it's beneficial to reduce number HM/RS and create JIRA if it's helpful.
No further comments, going to merge. Thank you @mihir6692 for the contribution |
…ion (#5772) Signed-off-by: Andrew Purtell <apurtell@apache.org>
…ion (apache#5772) Signed-off-by: Andrew Purtell <apurtell@apache.org>
/hbck/hbck-metrics/
will provide entire HBCK Report in JSON Format. Also each inconsistency can be requested separately using their endpoints.Endpoint list for each inconsistencies :
Orphan Region on FS :
/hbck/hbck-metrics/orphan-regions-on-fs
Orphan Region on RS :
/hbck/hbck-metrics/orphan-egions-on-rs
Inconsistent Regions :
/hbck/hbck-metrics/inconsistent-regions
Region Holes :
/hbck/hbck-metrics/region-holes
Region Overlaps :
/hbck/hbck-metrics/region-overlaps
Unknown Servers :
/hbck/hbck-metrics/unknown-servers
Empty Region Info :
/hbck/hbck-metrics/empty-regioninfo