HBASE-25895 Implement a Cluster Metrics JSON endpoint #4177
+1,112
−11
Conversation
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
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
reviewed
Mar 7, 2022
|
|
||
| @Override | ||
| public JsonElement serialize(byte[] src, Type typeOfSrc, JsonSerializationContext context) { | ||
| return new JsonPrimitive(Bytes.toString(src)); |
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.
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.
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
There was a problem hiding this comment.
Thanks @ndimiduk . I was mainly worried that we might break parsing.
apurtell
reviewed
Mar 7, 2022
| @@ -689,9 +691,16 @@ protected MasterRpcServices createRpcServices() throws IOException { | |||
| @Override | |||
| protected void configureInfoServer(InfoServer infoServer) { | |||
| infoServer.addUnprivilegedServlet("master-status", "/master-status", MasterStatusServlet.class); | |||
| infoServer.addUnprivilegedServlet("api_v1", "/api/v1/*", buildApiV1Servlet()); | |||
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.
ndimiduk
force-pushed
the
25895-cluster-metrics-json-endpoint
branch
from
e50fff6 to
1c62e64
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
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 ? |
|
This failure does not reproduce locally when I run this new test and |
ndimiduk
force-pushed
the
25895-cluster-metrics-json-endpoint
branch
from
1c62e64 to
5446762
Compare
|
🎊 +1 overall
This message was automatically generated. |
apurtell
approved these changes
Mar 9, 2022
|
💔 -1 overall
This message was automatically generated. |
|
💔 -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>
ndimiduk
force-pushed
the
25895-cluster-metrics-json-endpoint
branch
from
5446762 to
7b50d86
Compare
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.
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