Copilot commented on code in PR #10862:
URL: https://github.com/apache/ozone/pull/10862#discussion_r3714504984
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -4733,6 +4734,15 @@ public boolean isLeaderReady() {
return ratisServer != null && ratisServer.getLeaderStatus() ==
LEADER_AND_READY;
}
+ /**
+ * Return true, if the current OM node is leader.
+ * Note that it also returns true if the OM is leader but is not ready.
+ */
Review Comment:
Minor grammar fix: 'Return true, if' reads awkwardly in Javadoc. Consider
'Returns true if the current OM node is the leader.'
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOMDBCheckpointServletInodeBasedXferNonLeader.java:
##########
@@ -77,4 +88,46 @@ void
processMetadataSnapshotRequestSetsStatusWhenSendErrorFails() throws Excepti
verify(response).setStatus(HttpServletResponse.SC_SERVICE_UNAVAILABLE);
}
+
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ void processMetadataSnapshotRequestDoesNotReturn503WhenLeader(boolean
isLeaderReady) throws Exception {
+ OMDBCheckpointServletInodeBasedXfer servlet =
+ spy(new OMDBCheckpointServletInodeBasedXfer());
+ OzoneManager om = mock(OzoneManager.class);
+ when(om.isLeader()).thenReturn(true);
+ when(om.isLeaderReady()).thenReturn(isLeaderReady);
+
+ ServletContext ctx = mock(ServletContext.class);
+ when(ctx.getAttribute(OzoneConsts.OM_CONTEXT_ATTRIBUTE)).thenReturn(om);
+ doReturn(ctx).when(servlet).getServletContext();
+ // Force a failure after leader check so this unit test can stay
lightweight
+ // (no full servlet/bootstrap setup) while still proving that leader
requests
+ // are not rejected with 503.
+ doThrow(new IOException("test collect failure"))
+ .when(servlet).collectDbDataToTransfer(any(), anySet(), any());
+
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ HttpServletResponse response = mock(HttpServletResponse.class);
+
+ servlet.processMetadataSnapshotRequest(request, response, false, true);
+
+ verify(response, never())
+ .sendError(eq(HttpServletResponse.SC_SERVICE_UNAVAILABLE),
anyString());
+ verify(om).isLeader();
+ verify(om, never()).isLeaderReady();
+ // Internal error comes from the forced collect failure above.
+ verify(response).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
Review Comment:
This assertion makes the test depend on the servlet’s specific
error-handling mechanism (eg `setStatus(500)` vs `sendError(500, ...)`). Since
the purpose of the test is to prove 'no 503 on leader', consider narrowing the
assertion to that intent (and/or asserting a 5xx in a less
implementation-coupled way) to reduce brittleness when error handling changes.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOMDBCheckpointServletInodeBasedXferNonLeader.java:
##########
@@ -77,4 +88,46 @@ void
processMetadataSnapshotRequestSetsStatusWhenSendErrorFails() throws Excepti
verify(response).setStatus(HttpServletResponse.SC_SERVICE_UNAVAILABLE);
}
+
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ void processMetadataSnapshotRequestDoesNotReturn503WhenLeader(boolean
isLeaderReady) throws Exception {
+ OMDBCheckpointServletInodeBasedXfer servlet =
+ spy(new OMDBCheckpointServletInodeBasedXfer());
+ OzoneManager om = mock(OzoneManager.class);
+ when(om.isLeader()).thenReturn(true);
+ when(om.isLeaderReady()).thenReturn(isLeaderReady);
+
+ ServletContext ctx = mock(ServletContext.class);
+ when(ctx.getAttribute(OzoneConsts.OM_CONTEXT_ATTRIBUTE)).thenReturn(om);
+ doReturn(ctx).when(servlet).getServletContext();
+ // Force a failure after leader check so this unit test can stay
lightweight
+ // (no full servlet/bootstrap setup) while still proving that leader
requests
+ // are not rejected with 503.
+ doThrow(new IOException("test collect failure"))
+ .when(servlet).collectDbDataToTransfer(any(), anySet(), any());
+
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ HttpServletResponse response = mock(HttpServletResponse.class);
+
+ servlet.processMetadataSnapshotRequest(request, response, false, true);
+
+ verify(response, never())
+ .sendError(eq(HttpServletResponse.SC_SERVICE_UNAVAILABLE),
anyString());
+ verify(om).isLeader();
+ verify(om, never()).isLeaderReady();
+ // Internal error comes from the forced collect failure above.
+ verify(response).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+ }
+
+ @ParameterizedTest
+ @EnumSource(RaftServerStatus.class)
+ void isLeaderReflectsRaftServerStatus(RaftServerStatus raftServerStatus) {
+ OzoneManager om = mock(OzoneManager.class, CALLS_REAL_METHODS);
+ OzoneManagerRatisServer ratisServer = mock(OzoneManagerRatisServer.class);
+ when(ratisServer.getLeaderStatus()).thenReturn(raftServerStatus);
+ HddsWhiteboxTestUtils.setInternalState(om, "omRatisServer", ratisServer);
+
+ assertEquals(raftServerStatus != RaftServerStatus.NOT_LEADER,
om.isLeader());
+ }
Review Comment:
This test encodes the same 'anything but NOT_LEADER is leader' assumption as
production. If you take the suggested approach of explicitly enumerating leader
statuses, this test should be updated to assert only the known leader statuses
map to `true`, and all known non-leader statuses map to `false` (rather than
relying on `EnumSource` + `!= NOT_LEADER`). This will make the test intention
clearer and reduce the chance of accidentally classifying a newly added status
as leader.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -4733,6 +4734,15 @@ public boolean isLeaderReady() {
return ratisServer != null && ratisServer.getLeaderStatus() ==
LEADER_AND_READY;
}
+ /**
+ * Return true, if the current OM node is leader.
+ * Note that it also returns true if the OM is leader but is not ready.
+ */
+ public boolean isLeader() {
+ final OzoneManagerRatisServer ratisServer = omRatisServer;
+ return ratisServer != null && ratisServer.getLeaderStatus() != NOT_LEADER;
+ }
Review Comment:
The `!= NOT_LEADER` check makes `isLeader()` implicitly treat any new future
`RaftServerStatus` value (other than `NOT_LEADER`) as leader, which is brittle
if the enum ever grows non-leader states. Prefer explicitly enumerating the
leader states (eg `LEADER_AND_READY` / `LEADER_AND_NOT_READY`), or adding a
dedicated helper on the enum (eg `status.isLeader()`) and delegating to that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
