Repository navigation
Conversation
… exception class names
|
|
||
| optional string message = 1; | ||
| // Not set for any other error. | ||
| oneof error { |
There was a problem hiding this comment.
this allow us to add more error type and structural info in future
# Conflicts: # hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java # hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/protocolPB/TestS3GrpcOmTransport.java
chihsuan
left a comment
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for the patch! @peterxcli nice move away from reflection. Left two questions and a small nit inline.
| if (suggestedLeaderAddress != null && | ||
| suggestedNodeId != null && | ||
| omProxies.contains(suggestedNodeId, suggestedLeaderAddress)) { | ||
| if (suggestedNodeId != null && omProxies.get(suggestedNodeId) != null) { |
There was a problem hiding this comment.
I might be missing something, but if the suggested leader is down, the client seems to bounce between two OMs with no wait. Would it make sense to back off on repeated suggestions?
| } | ||
| return grpcException; | ||
| // exception generated by gRPC, e.g. a connection failure | ||
| return ex; |
There was a problem hiding this comment.
Just to confirm, do we need to handle a new S3G with an older OM? Without the trailer, NotLeader and LeaderNotReady become plain errors here.
| responseObserver.onError( | ||
| Status.INTERNAL.withDescription(ex.getMessage()) | ||
| .asRuntimeException()); | ||
| Throwable omException = e instanceof ServiceException ? e.getCause() : e; |
There was a problem hiding this comment.
nit: probably not reachable today, but if a ServiceException has no cause, toProto may throw NPE. Maybe fall back to e here, just to be defensive?
What changes were proposed in this pull request?
The OM gRPC server reports a failed
submitRequestasStatus.INTERNALwith the descriptionclass: message, andGrpcOmTransportrebuilds the exception from that text: it splits it at the first:, and the class is then created by reflection. This is fragile, it lets the server pick the class the client loads, and it keeps nothing but the message. In particular, the suggested leader inOMNotLeaderExceptionnever reaches the retry policy.With this change:
OMGrpcErrorProtomessage inOmClientProtocol.protocarries the message and aoneofwith a variant for each OM error the client handles:NotLeader(with the suggested leader node ID),LeaderNotReady,ReadIndexFailedandReadFailed. No variant means any other error. The OM gRPC server sends it in a binary gRPC trailer.OMGrpcError, its Java form, is the one place that maps an OM exception to the error on the server, and the variant back to the OM exception on the client. There is no text parsing and no reflection.unwrapExceptiongives the retry policy aServiceExceptionwith the OM exception as its cause. Theget*Exceptionhelpers inOMFailoverProxyProviderBasenow accept such a cause as well as aRemoteException, and the four of them share one implementation.InetAddress#toString(host/ip) while the proxy hashost:port, so it never matched.HDDS-17448 (#11760) already gives the retry policy the shape it recognizes, by parsing the description. This change keeps that behavior and replaces the parsing.
Compatibility: the description keeps its old format, so older gRPC clients still work. A new client talking to an older OM gets no trailer, treats the error as a plain failure and fails over to the next OM. The Hadoop RPC client is unchanged.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-17462
How was this patch tested?
TestOMGrpcError: every variant maps back to its exception type and message, including the suggested leader; any other exception becomesINTERNAL_ERROR; the description keeps the old format.TestGrpcOzoneManagerServer: the OM gRPC server sends the error with the suggested leader.TestOMFailoverProxyProvider: a not-leader error goes to the suggested leader, not just to the next OM.TestS3GrpcOmTransport: over gRPC, the client goes from a not-leader OM straight to the suggested leader.The fake OM servers in the gRPC tests now build their errors the same way as the OM gRPC server.