octachoron commented on code in PR #9611:
URL: https://github.com/apache/ozone/pull/9611#discussion_r2678784429
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java:
##########
@@ -870,6 +874,50 @@ public boolean forceExitSafeMode() throws IOException {
}
+ @Override
+ public boolean inSafeModeForNode(String nodeId) throws IOException {
+ InSafeModeRequestProto request =
InSafeModeRequestProto.getDefaultInstance();
+
+ try {
+ StorageContainerLocationProtocolPB proxy = fpp.getProxyForNode(nodeId);
+ ScmContainerLocationRequest wrapper =
ScmContainerLocationRequest.newBuilder()
+ .setCmdType(Type.InSafeMode)
+ .setVersion(ClientVersion.CURRENT_VERSION)
+ .setTraceID(TracingUtil.exportCurrentSpan())
+ .setInSafeModeRequest(request)
+ .build();
+ ScmContainerLocationResponse response =
proxy.submitRequest(NULL_RPC_CONTROLLER, wrapper);
Review Comment:
What happens here is functionally the same thing as `submitRequest()`, other
than the way the proxy is selected. Would it make sense to make that official,
and refactor this part into a general `submitRequest()` variant that also takes
a node ID? I believe that would have many benefits: separation of concerns,
improving tests through said separation of concerns, DRY, readability
(especially when comparing to `inSafeMode()`), reusability (e.g., in this PR a
few lines down), etc.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java:
##########
@@ -870,6 +874,50 @@ public boolean forceExitSafeMode() throws IOException {
}
+ @Override
+ public boolean inSafeModeForNode(String nodeId) throws IOException {
+ InSafeModeRequestProto request =
InSafeModeRequestProto.getDefaultInstance();
+
+ try {
+ StorageContainerLocationProtocolPB proxy = fpp.getProxyForNode(nodeId);
+ ScmContainerLocationRequest wrapper =
ScmContainerLocationRequest.newBuilder()
+ .setCmdType(Type.InSafeMode)
+ .setVersion(ClientVersion.CURRENT_VERSION)
+ .setTraceID(TracingUtil.exportCurrentSpan())
+ .setInSafeModeRequest(request)
+ .build();
+ ScmContainerLocationResponse response =
proxy.submitRequest(NULL_RPC_CONTROLLER, wrapper);
+ return response.getInSafeModeResponse().getInSafeMode();
+ } catch (Exception e) {
+ throw new IOException("Failed to get safe mode status from SCM node " +
nodeId, e);
+ }
+ }
+
+ @Override
+ public Map<String, Pair<Boolean, String>>
getSafeModeRuleStatusesForNode(String nodeId) throws IOException {
+ GetSafeModeRuleStatusesRequestProto request =
GetSafeModeRuleStatusesRequestProto.getDefaultInstance();
+
+ try {
+ StorageContainerLocationProtocolPB proxy = fpp.getProxyForNode(nodeId);
+ ScmContainerLocationRequest wrapper =
ScmContainerLocationRequest.newBuilder()
+ .setCmdType(Type.GetSafeModeRuleStatuses)
+ .setVersion(ClientVersion.CURRENT_VERSION)
+ .setTraceID(TracingUtil.exportCurrentSpan())
+ .setGetSafeModeRuleStatusesRequest(request)
+ .build();
+ ScmContainerLocationResponse response =
proxy.submitRequest(NULL_RPC_CONTROLLER, wrapper);
Review Comment:
In this case, a `submitRequest()` helper with a node ID parameter (details
in the other comment) would even allow reusing the part of the existing
`getSafeModeRuleStatuses()` method that builds the map (as it is functionally
identical).
If we go with that approach in the end though, I vote to keep this new
iteration of the map-building part, as it makes some readability improvements
over the existing one.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1033,6 +1033,24 @@ public Map<String, Pair<Boolean, String>>
getSafeModeRuleStatuses()
}
}
+ @Override
+ public boolean inSafeModeForNode(String nodeId) throws IOException {
+ boolean result = inSafeMode();
+ AUDIT.logReadSuccess(
+ buildAuditMessageForSuccess(SCMAction.IN_SAFE_MODE, null)
+ );
+ return result;
+ }
+
+ @Override
+ public Map<String, Pair<Boolean, String>>
getSafeModeRuleStatusesForNode(String nodeId) throws IOException {
+ Map<String, Pair<Boolean, String>> result = getSafeModeRuleStatuses();
+ AUDIT.logReadSuccess(
+ buildAuditMessageForSuccess(SCMAction.GET_SAFE_MODE_RULE_STATUSES,
null)
+ );
+ return result;
+ }
Review Comment:
Just to make sure my understanding is correct (or find out that it is not
:slightly_smiling_face:), these implementations cannot be reached through an
actual client, right?
As in, we need the methods in the StorageContainerLocationProtocol interface
so that the calls can get all the way to the client-side translator, and so we
definitely need to implement the new methods here too. On the other hand, after
a target SCM is chosen in the client-side translator, the actual request it
makes is of the same no-node-ID kind as always (which makes sense).
If this sounds correct, then is there a way to reach this code regardless?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocolServerSideTranslatorPB.java:
##########
@@ -212,7 +213,8 @@ public ScmContainerLocationResponse
submitRequest(RpcController controller,
ScmContainerLocationRequest request) throws ServiceException {
// not leader or not belong to admin command.
Review Comment:
With this PR, read-only commands that can run on followers is a third case
here. I believe it would be beneficial to update the comment with this
information.
--
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]
