HBASE-26192 Master UI hbck should provide a JSON formatted output option #4470
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
|
Needs some basic tests; up next. |
|
🎊 +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. |
|
Rebase. Test implementation in progress. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
ndimiduk
reviewed
May 31, 2022
|
|
||
| private static final long serialVersionUID = 1L; | ||
| private static final Logger LOG = LoggerFactory.getLogger(MasterHbckServlet.class); | ||
| private static final Gson GSON = new Gson(); |
There was a problem hiding this comment.
You almost certainly don't want to use the Gson object defaults; I would expect some minor configuration. See also hbase-server/src/main/java/.../hbase/master/http/gson/GsonFactory.java
There was a problem hiding this comment.
GsonUtil would also work but yes GsonFactory#buildGson should be quite enough.
|
Rebased, with test |
|
Clarify something in test and apply spotless. |
|
🎊 +1 overall
This message was automatically generated. |
|
Example - healthy cluster:
|
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@ndimiduk Do you have any concerns with the current state of this? I think if someone wants to use this servlet to check status with a json aware tool it will provide all the necessary information, but we don't have an API to recover the original Java types from the GSON serialization. OTOH I do not think that is required. @virajjasani WDYT? |
virajjasani
approved these changes
Jun 2, 2022
| assertEquals(1, orphanRegionsOnFS.size()); | ||
| assertNull(orphanRegionsOnFS.get(FAKE_HRI.getEncodedName())); | ||
| assertNotNull(orphanRegionsOnFS.get(FAKE_HRI_3.getEncodedName())); | ||
| List holes = (List) result.get(MasterHbckServlet.HOLES); |
There was a problem hiding this comment.
Given that we are just comparing null vs non-null and size of the list, this raw use of List is fine.
ndimiduk
reviewed
Jun 6, 2022
There was a problem hiding this comment.
I think this is useful information to expose. I tentatively agree that exposing a report like this deserves its own endpoint, rather than jamming it into JMX.
On the implementation, I'm disappointed to see programming against the raw Servlet API. I did all the work to get Jersey setup and working properly, I hoped that it would be the path forward for new web service work. So I'm -0 on more hand-rolled Servlets.
As for the implementation itself, I think you could a do a little more around error handling. I have a question about using a 503 for an expected server state. In particular, I just got bit by a web UI not handling backup master properly, so that scenario is on my mind during this review.
| private static final Gson GSON = GsonFactory.buildGson(); | ||
|
|
||
| @Override | ||
| public void doGet(HttpServletRequest request, HttpServletResponse response) |
There was a problem hiding this comment.
Why use the raw Servlet API when we have Jersey? Instead of building a response by hand, you could instantiate a POJO and return it, be done.
There was a problem hiding this comment.
The other servlets are implemented using the Servlet API and I used them as template.
There was a problem hiding this comment.
I agree to Nick’s comment on the REST Endpoint returning JSON backed my Model which is a standard way of implementing REST APIs.
| @@ -707,6 +708,7 @@ protected MasterRpcServices createRpcServices() throws IOException { | |||
| protected void configureInfoServer(InfoServer infoServer) { | |||
| infoServer.addUnprivilegedServlet("master-status", "/master-status", MasterStatusServlet.class); | |||
| infoServer.addUnprivilegedServlet("api_v1", "/api/v1/*", buildApiV1Servlet()); | |||
| infoServer.addUnprivilegedServlet("hbck", "/hbck", MasterHbckServlet.class); | |||
There was a problem hiding this comment.
Why add a new endpoint instead of having the existing /hbck.jsp honor an "application/json" header and render its response there? Probably a bunch of that jsp's POJOs can be reused...
There was a problem hiding this comment.
I did not and still do not know how to make a JSP page honor different content-encodings. JSP is not the tool I would like to implement this with. If this is the preferred implementation that's fine but I would not want to pursue it.
| final HbckChore hbckChore = master.getHbckChore(); | ||
| if (hbckChore == null || hbckChore.isDisabled()) { | ||
| LOG.warn("Hbck chore is disabled"); | ||
| sendError(response, HttpServletResponse.SC_SERVICE_UNAVAILABLE, "Hbck chore is disabled"); |
There was a problem hiding this comment.
Is this really a 503? The server is fine. I'm no HTTP status code expert, but I'd say this is more like a 204 SC_NO_CONTENT or a 404 SC_NOT_FOUND meaning "hbck resource is not available".
There was a problem hiding this comment.
If the HBCK chore is not available the service is not available, was my thinking. However I can see 204 being quite appropriate.
There was a problem hiding this comment.
404 is usually for resource not found right - if hbck is the resource which is disabled, it means the requested resource is not found.
204 would mean that the resource is found but didn't have content to respond with.
May 404 is right? Given the message contains details that chore is disabled?
| public void doGet(HttpServletRequest request, HttpServletResponse response) | ||
| throws ServletException, IOException { | ||
| final HMaster master = (HMaster) getServletContext().getAttribute(HMaster.MASTER); | ||
| if (!master.isInitialized()) { |
There was a problem hiding this comment.
!isInitialized() could mean that this master instance is a backup master, in which case the response should be a redirect to this endpoint on the active master.
There was a problem hiding this comment.
Ah, this is not something I thought about. Seems reasonable to handle it with a redirect.
| LOG.warn("Failed generating a new catalogjanitor report; using cache", se); | ||
| } | ||
| } | ||
| Map<String, Object> result = new HashMap<>(); |
There was a problem hiding this comment.
If you define a POJO for the response object, we can have some hope of tracing API compatibility in the future using our existing compatibility report.
There was a problem hiding this comment.
Got it, this is a good reason.
There was a problem hiding this comment.
With POJO, we can achieve something like what OData does to even query for a sub document as mentioned here. I am not sure if there is a right equivalent support already available in HBase, else we can skip the OData part but the POJO adds value.
| } | ||
| Map<String, Object> result = new HashMap<>(); | ||
| result.put(START_TIMESTAMP, hbckChore.getCheckingStartTimestamp()); | ||
| result.put(END_TIMESTAMP, hbckChore.getCheckingEndTimestamp()); |
There was a problem hiding this comment.
Our HBCK chore should expose a report so that a reader can get a static view... or a read-write lock so that a reader can block a writer from clobbering its view.
| } | ||
| final CatalogJanitor janitor = master.getCatalogJanitor(); | ||
| if (janitor != null) { | ||
| final Report report = janitor.getLastReport(); |
There was a problem hiding this comment.
CatalogueJanitor exposes the report concept. Brilliant!
There was a problem hiding this comment.
Yes, and unfortunately the result is a mix of CatalogJanitor's nicely structured Report and HbckChore's other return types.
Unifying these things is out of scope, though. Or, we can make it a precondition, but I won't pursue it at this time.
There was a problem hiding this comment.
Down review I accepted your proposal to implement this with Jersey, so a clean unification of these two data sources can be done when working up the model (POJO) of the report encoding to return from the request handler.
|
|
||
| @SuppressWarnings({ "unchecked", "rawtypes" }) | ||
| @Test | ||
| public void testHbckServletWithMocks() throws Exception { |
| doReturn(hbckChore).when(master).getHbckChore(); | ||
| MasterRpcServices services = mock(MasterRpcServices.class); | ||
| doReturn(services).when(master).getRpcServices(); | ||
| doReturn(services).when(master).getMasterRpcServices(); |
There was a problem hiding this comment.
Why do all this mockery? At this point, it's it simpler to stand up a mini-cluster and manufacture the state you need? It looks like TestMetaFixer has some utility code for manufacturing faulty states.
There was a problem hiding this comment.
Because when I wrote this test this was the way I understood it could be implemented and I don't think too much is mocked here, just what is needed to produce a report that has exactly what I want to check for later.
ndimiduk
added a commit
to ndimiduk/hbase
that referenced
this pull request
Jun 7, 2022
In apache#4470 for HBASE-26192, it was noted that the HbckChore is kind of a pain to use and test because it maintains a bunch of local state. By contract, the CatalogJanitorChore makes a nice self-contained report. Let's update HbckChore to do the same.
|
I posted up #4498, which will give you a report to work from. |
apurtell
force-pushed
the
HBASE-26192
branch
2 times, most recently
from
cb11a74
to
f8892b0
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ndimiduk
added a commit
to ndimiduk/hbase
that referenced
this pull request
Jun 9, 2022
In apache#4470 for HBASE-26192, it was noted that the HbckChore is kind of a pain to use and test because it maintains a bunch of local state. By contract, the CatalogJanitorChore makes a nice self-contained report. Let's update HbckChore to do the same. Signed-off-by: Andrew Purtell <apurtell@apache.org>
ndimiduk
added a commit
to ndimiduk/hbase
that referenced
this pull request
Jun 9, 2022
In apache#4470 for HBASE-26192, it was noted that the HbckChore is kind of a pain to use and test because it maintains a bunch of local state. By contract, the CatalogJanitorChore makes a nice self-contained report. Let's update HbckChore to do the same. Signed-off-by: Andrew Purtell <apurtell@apache.org>
ndimiduk
added a commit
to ndimiduk/hbase
that referenced
this pull request
Jun 9, 2022
In apache#4470 for HBASE-26192, it was noted that the HbckChore is kind of a pain to use and test because it maintains a bunch of local state. By contract, the CatalogJanitorChore makes a nice self-contained report. Let's update HbckChore to do the same. Signed-off-by: Andrew Purtell <apurtell@apache.org>
ndimiduk
added a commit
to ndimiduk/hbase
that referenced
this pull request
Jun 9, 2022
In apache#4470 for HBASE-26192, it was noted that the HbckChore is kind of a pain to use and test because it maintains a bunch of local state. By contract, the CatalogJanitorChore makes a nice self-contained report. Let's update HbckChore to do the same. Signed-off-by: Andrew Purtell <apurtell@apache.org>
ndimiduk
added a commit
that referenced
this pull request
Jun 10, 2022
In #4470 for HBASE-26192, it was noted that the HbckChore is kind of a pain to use and test because it maintains a bunch of local state. By contract, the CatalogJanitorChore makes a nice self-contained report. Let's update HbckChore to do the same. Signed-off-by: Andrew Purtell <apurtell@apache.org>
ndimiduk
added a commit
that referenced
this pull request
Jun 10, 2022
In #4470 for HBASE-26192, it was noted that the HbckChore is kind of a pain to use and test because it maintains a bunch of local state. By contract, the CatalogJanitorChore makes a nice self-contained report. Let's update HbckChore to do the same. Signed-off-by: Andrew Purtell <apurtell@apache.org>
ndimiduk
added a commit
that referenced
this pull request
Jun 10, 2022
In #4470 for HBASE-26192, it was noted that the HbckChore is kind of a pain to use and test because it maintains a bunch of local state. By contract, the CatalogJanitorChore makes a nice self-contained report. Let's update HbckChore to do the same. Signed-off-by: Andrew Purtell <apurtell@apache.org>
ndimiduk
added a commit
that referenced
this pull request
Jun 10, 2022
In #4470 for HBASE-26192, it was noted that the HbckChore is kind of a pain to use and test because it maintains a bunch of local state. By contract, the CatalogJanitorChore makes a nice self-contained report. Let's update HbckChore to do the same. Signed-off-by: Andrew Purtell <apurtell@apache.org>
wenwj0
pushed a commit
to wenwj0/hbase
that referenced
this pull request
Jun 14, 2022
In apache#4470 for HBASE-26192, it was noted that the HbckChore is kind of a pain to use and test because it maintains a bunch of local state. By contract, the CatalogJanitorChore makes a nice self-contained report. Let's update HbckChore to do the same. Signed-off-by: Andrew Purtell <apurtell@apache.org>
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
GSON won't deserialize back into HBase native types so don't give the wrong impression by using them.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
vinayakphegde
pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
In apache#4470 for HBASE-26192, it was noted that the HbckChore is kind of a pain to use and test because it maintains a bunch of local state. By contract, the CatalogJanitorChore makes a nice self-contained report. Let's update HbckChore to do the same. Signed-off-by: Andrew Purtell <apurtell@apache.org> Change-Id: I69c1bf867b09b5b9b0d38f69f7cc37c8b48955d1
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.
This is a trivial recast of hbck.jsp as a servlet. It takes the same actions, using the same parameters, and instead of formatting for HTML constructs a simple map that is formatted as JSON via GSON. Nouns, inclusion, and ordering are the same as that of the jsp page.
It runs as a read-only unprivileged servlet on the master at
/hbck. Hbck JSP pretty printer remains at/hbck.jsp.