Uh oh!
There was an error while loading. Please reload this page.
HBASE-25895 Implement a Cluster Metrics JSON endpoint - #4177
Merged
ndimiduk merged 1 commit intoMar 10, 2022
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
busbey
approved these changes
Mar 7, 2022
apurtell
approved these changes
Mar 7, 2022
apurtell
left a comment
Contributor
There was a problem hiding this comment.
As I mentioned on the review of your draft PR this is mostly boilerplate wiring up gson to jersey, and it looks sane to me, although I do not understand the requirements in detail.
apurtell
reviewed
Mar 7, 2022
| @Override | ||
| public JsonElement serialize(byte[] src, Type typeOfSrc, JsonSerializationContext context) { | ||
| return new JsonPrimitive(Bytes.toString(src)); |
Contributor
There was a problem hiding this comment.
I assume nonprintable characters in the string are url escaped here downstream from this call to the JsonPrimitve constructor.
MemberAuthor
There was a problem hiding this comment.
@apurtell I cannot tell you. Actually, I'm embarrassed to see that this commit doesn't include any tests. I've added some most basic coverage, including a test that shows the behavior of the configured Gson instance produced by the factory. Please advise.
MemberAuthor
There was a problem hiding this comment.
After some research, my understanding is that Gson handles escaping characters according to the JSON RFC's definition of String values. We are free, according to the RFC, to encode more aggressively than what Gson does, but it appears to implement the minimum plus some additional characters related to html. My test demonstrates that there are some binary values that are not encoded and that do not render well (at least on this machine).
We may chose to escape a wider range of code points, or we my consider some alternative encoding for binary values, such as base64 with perhaps some prefix.
I have made every effort to declare this API as IA.Private, so we are free to evolve it as we choose, so long as we can maintain a sufficient level of backward compatibility for our guarantees. Thus I would prefer to press forward with landing this initial implementation of the feature and continue this encoding discussion as a follow-on effort.
Please advise.
https://datatracker.ietf.org/doc/html/rfc4627#section-2.5
https://github.com/google/gson/blob/gson-parent-2.8.9/gson/src/main/java/com/google/gson/stream/JsonWriter.java#L133-L163
Contributor
There was a problem hiding this comment.
Thanks @ndimiduk . I was mainly worried that we might break parsing.
apurtell
reviewed
Mar 7, 2022
| @Override | ||
| protected void configureInfoServer(InfoServer infoServer) { | ||
| infoServer.addUnprivilegedServlet("master-status", "/master-status", MasterStatusServlet.class); | ||
| infoServer.addUnprivilegedServlet("api_v1", "/api/v1/*", buildApiV1Servlet()); |
Contributor
There was a problem hiding this comment.
This is nice, something to build on
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
ndimidukforce-pushed
the
25895-cluster-metrics-json-endpoint
branch
from
March 8, 2022 20:32
e50fff6 to
1c62e64CompareApache-HBase
commented
Mar 8, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Mar 9, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Mar 9, 2022
💔 -1 overall
This message was automatically generated. |
ndimiduk
commented
Mar 9, 2022
MemberAuthor
I'm not sure what to make of the test failure in precommit checks / yetus jdk11 hadoop3 checks / testResolvePortConflict. ZK resulted in a port conflict. Is this somehow related to my conf customization in Ideas @busbey ? |
ndimiduk
commented
Mar 9, 2022
MemberAuthor
This failure does not reproduce locally when I run this new test and |
ndimidukforce-pushed
the
25895-cluster-metrics-json-endpoint
branch
from
March 9, 2022 16:36
1c62e64 to
5446762CompareApache-HBase
commented
Mar 9, 2022
🎊 +1 overall
This message was automatically generated. |
apurtell
approved these changes
Mar 9, 2022
Apache-HBase
commented
Mar 9, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Mar 9, 2022
💔 -1 overall
This message was automatically generated. |
Publishes a set of JSON endpoints following a RESTful structure, which expose a subset of the `o.a.h.h.ClusterMetrics` object tree. The URI structure is as follows /api/v1/admin/cluster_metrics /api/v1/admin/cluster_metrics/live_servers /api/v1/admin/cluster_metrics/dead_servers Signed-off-by: Sean Busbey <busbey@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
ndimidukforce-pushed
the
25895-cluster-metrics-json-endpoint
branch
from
March 10, 2022 13:57
5446762 to
7b50d86Compare
This file contains hidden or 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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Publishes a set of JSON endpoints following a RESTful structure, which expose a subset of the
o.a.h.h.ClusterMetricsobject tree. The URI structure is as follows