Repository navigation
Conversation
|
most of the addition are tests |
|
@echonesis Please help take a look as well since this concerns gRPC client. |
There was a problem hiding this comment.
Thanks @peterxcli for the patch, left some comments. There are some behavior change concerns.
| * | ||
| * @param nodeId the expected current node. | ||
| */ | ||
| protected synchronized void changeFollowerReadNodeId(String nodeId) { |
There was a problem hiding this comment.
Nit: Can annotate nodeId as @Nonnull and can probably add Objects.requireNonNull.
| protected synchronized String selectFollowerReadNodeId(ReadConsistency readConsistency) { | ||
| String nodeId = getCurrentFollowerReadNodeId(); | ||
| if (readConsistency != ReadConsistency.LOCAL_LEASE) { | ||
| return nodeId; | ||
| } | ||
|
|
||
| final String leaderNodeId = getLeaderProxy().getCurrentProxyOMNodeId(); | ||
| for (int i = 0; i < getOMNodeCount(); i++) { | ||
| if (!nodeId.equals(leaderNodeId)) { | ||
| return nodeId; | ||
| } | ||
| changeFollowerReadNodeId(nodeId); | ||
| nodeId = getCurrentFollowerReadNodeId(); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
In the previous logic, we don't necessary require client to send to a follower first (for perf reason since currently things linearizable read latency is significantly higher than leader read), but seems this patch changes the logic prioritize follower first.
I suggest we address behavior change in another ticket (and probably gate it in a configuration with name similar to follower affinity in OmMetadataGenerator).
| * The index of the OM node used for follower read, in the order of the leader proxy's OM nodes. | ||
| * Should only be accessed in synchronized methods. | ||
| */ | ||
| private int currentIndex = 0; |
There was a problem hiding this comment.
Before the refactor, HadoopRpcOMFollowerReadFailoverProxyProvider initialized currentIndex to -1, IIRC so that it changeProxy will trigger creation for index 0. If we set this to 0, will this skip the first OM proxy in changeFollowerReadNodeId?
echonesis
left a comment
echonesis
left a comment
There was a problem hiding this comment.
Thanks @peterxcli for the refactor.
Just leave some comments.
| public synchronized void changeInitialProxyForTest(String initialOmNodeId) { | ||
| currentIndex = getLeaderProxy().getOMProxyMap().indexOf(initialOmNodeId); |
There was a problem hiding this comment.
Could we retain the null check around indexOf(initialOmNodeId)?
An unknown node ID left the current selection unchanged. With this assignment, the returned Integer would be unboxed and throw an NPE.
| for (int i = 0; i < getOMNodeCount(); i++) { | ||
| final String nodeId = selectFollowerReadNodeId(readConsistency); | ||
| if (nodeId == null) { | ||
| break; | ||
| } | ||
| final OMProxyInfo<OzoneManagerProtocolPB> current = leaderProxy.createOMProxyIfNeeded(nodeId); | ||
| LOG.debug("Attempting to service submitRequest with cmdType {} using proxy {}", | ||
| omRequest.getCmdType(), current.proxyInfo); | ||
| try { |
There was a problem hiding this comment.
Would it be possible to add a regression test for that case?
It would be useful to verify that, after DNS refresh invalidates the current node’s cached proxy, the next read recreates it using the refreshed address and succeeds on the same node without moving to another OM.
What changes were proposed in this pull request?
The Hadoop RPC client and the gRPC client (used by the S3 Gateway) each have their own copy of the OM follower read logic. The Hadoop RPC one lives in
HadoopRpcOMFollowerReadFailoverProxyProvider, while the gRPC one was added straight intoGrpcOmTransportby HDDS-15492. So every follower read change, like the per-request read consistency in HDDS-15089, has to be made twice.As suggested in #11252 (comment), this PR moves the shared parts into a base class:
FollowerReadFailoverProxyProviderBasekeeps the follower read settings, decides whether a request may go to a follower, adds the default read consistency hint, and picks the OM node for follower reads (skipping the leader forLOCAL_LEASE).HadoopRpcOMFollowerReadFailoverProxyProvidernow extends it.GrpcOmTransportinto a newGrpcOMFollowerReadFailoverProxyProvider, which also extends it.GrpcOmTransportis left with just sending the requests.The behavior stays the same, with one small difference. The Hadoop RPC provider now calls
createOMProxyIfNeededevery time it picks a follower node, not only when it moves to the next node. So if a DNS refresh has dropped the cached proxy of the current node, the proxy is created again, instead of the request hitting an NPE on that node and moving on to the next one.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16737
How was this patch tested?
TestFollowerReadFailoverProxyProviderBaseandTestGrpcOMFollowerReadFailoverProxyProvider.TestHadoopRpcOMFollowerReadFailoverProxyProvider,TestS3GrpcOmTransport,TestOzoneManagerHAFollowerReadWithAllRunningandTestOzoneManagerHAFollowerReadWithStoppedNodes.