ivandika3 commented on code in PR #11760:
URL: https://github.com/apache/ozone/pull/11760#discussion_r4374318998
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java:
##########
@@ -208,7 +202,8 @@ private OMResponse submitRequestToLeader(OMRequest payload)
tryOtherHost = false;
Review Comment:
Nit: Let's take this opportunity to cleanup this `tryOtherHost` condition.
It's currently not used. We can also rename it to `shouldRetry`.
Also the
```java
if (!tryOtherHost) {
throw new OMException(resultCode);
}
```
is not needed since outside the loop we already throw OMException.
Additionally, `shouldRetry` actually does a failover so `shouldRetry` might
not be a good method name.
I'm OK if you defer this to a follow up ticket. The `GrpcOmTransport` is
readability is pretty poor IMO so we might need to have a wider cleanup /
refactor. To name a few
- Separate the concern for exception handling by using some kind of gRPC
StatusRuntimeException to Hadoop RemoteException adapter.
- Rename `host` to `currentHost` (or `leaderHost` if it's supposed to be
pointed to the leader).
- Separate the gRPC channel building to a separate method
- Do a contract test on the gRPC exception message so that it can be parsed
by the client).
--
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]
