Copilot commented on code in PR #11455:
URL: https://github.com/apache/ozone/pull/11455#discussion_r4228638441
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/upgrade/TestOMCompleteFinalizeUpgradeRequest.java:
##########
@@ -49,6 +57,27 @@
*/
public class TestOMCompleteFinalizeUpgradeRequest extends OMKeyRequestTests {
+ @Test
+ public void testExternalCompletionIsRejectedBeforeRatis() throws Exception {
+ OzoneManagerRatisServer ratisServer = mock(OzoneManagerRatisServer.class);
+ when(ozoneManager.getOmRatisServer()).thenReturn(ratisServer);
+ when(ozoneManager.getOmRpcServerAddr()).thenReturn(new
InetSocketAddress("localhost", 9862));
+
when(ratisServer.submitRequest(any(OzoneManagerProtocolProtos.OMRequest.class),
eq(true)))
+
.thenReturn(OMResponse.newBuilder().setCmdType(OzoneManagerProtocolProtos.Type.CompleteFinalizeUpgrade)
+ .setStatus(Status.OK).setSuccess(true).build());
+ OzoneManagerProtocolProtos.OMRequest request =
OzoneManagerProtocolProtos.OMRequest.newBuilder()
+ .setCmdType(OzoneManagerProtocolProtos.Type.CompleteFinalizeUpgrade)
+ .setClientId(ClientId.randomId().toString())
+ .build();
+
+ OMResponse response = new OMExecutionFlow(ozoneManager).submit(request,
true);
+
+ assertFalse(response.getSuccess());
+ assertEquals(Status.INVALID_REQUEST, response.getStatus());
+ verifyNoInteractions(ratisServer);
+ verify(ozoneManager, never()).finalizeUpgrade();
Review Comment:
`verifyNoInteractions(ratisServer)` makes the test brittle if
`OMExecutionFlow` starts consulting the Ratis server for non-submission
metadata as part of request handling (even though it still correctly avoids
`submitRequest`). A more robust assertion would be to verify that
`submitRequest(...)` is never invoked on `ratisServer`, instead of asserting
zero interactions overall.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/upgrade/OMCompleteFinalizeUpgradeRequest.java:
##########
@@ -53,6 +54,12 @@ public OMCompleteFinalizeUpgradeRequest(OMRequest omRequest)
{
super(omRequest);
}
+ @Override
+ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+ final OMRequest omRequest = super.preExecute(ozoneManager);
+ throw new OMException(omRequest.getCmdType() + " is an internal request",
OMException.ResultCodes.INVALID_REQUEST);
Review Comment:
`preExecute()` always throws, so calling `super.preExecute(ozoneManager)` is
unnecessary work and increases the chance of unintended side effects (e.g.,
mutating the request/audit context) despite the request being rejected.
Consider removing the `super.preExecute(...)` call and building the exception
message from `getOmRequest().getCmdType()` (or a constant message) directly
before throwing.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/upgrade/OMCompleteFinalizeUpgradeRequest.java:
##########
@@ -53,6 +54,12 @@ public OMCompleteFinalizeUpgradeRequest(OMRequest omRequest)
{
super(omRequest);
}
+ @Override
+ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+ final OMRequest omRequest = super.preExecute(ozoneManager);
+ throw new OMException(omRequest.getCmdType() + " is an internal request",
OMException.ResultCodes.INVALID_REQUEST);
Review Comment:
The error message is a bit vague for operators/API consumers. Consider
making it explicit that `CompleteFinalizeUpgrade` is only accepted via the
internal finalize service / internal pipeline (Ratis-applied request), and that
direct client submission is rejected, while still returning `INVALID_REQUEST`.
--
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]
