HDFS-16369. RBF: Fix the retry logic of RouterRpcServer#invokeAtAvailableNs. #3745
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
goiri
reviewed
Dec 3, 2021
| @@ -668,14 +674,16 @@ public void testInvokeAtAvailableNs() throws IOException { | |||
| // Make one subcluster unavailable. | |||
| MiniDFSCluster dfsCluster = cluster.getCluster(); | |||
| dfsCluster.shutdownNameNode(0); | |||
| dfsCluster.shutdownNameNode(1); | |||
| try { | |||
| // Verify that #invokeAtAvailableNs works by calling #getServerDefaults. | |||
| RemoteMethod method = new RemoteMethod("getServerDefaults"); | |||
| FsServerDefaults serverDefaults = | |||
| rpcServer.invokeAtAvailableNs(method, FsServerDefaults.class); | |||
| assertNotNull(serverDefaults); | |||
There was a problem hiding this comment.
Is there something else we can assert? Any metrics?
There was a problem hiding this comment.
I thought of using the NamenodeMetrics, but it doesn't track getServerDefaults. Second is RBFClientMetrics. It tracks all the invokes for getServerDefault, irrespective of success and it isn't per namespace as well. So, in case the invocation order by any chance changes, from ns0->ns1->n2 to ns0->ns2->ns1 in that case the test will become flaky.
In general without the fix, the getServerDefault call will fail only if 2 NS is down, so I thought if the call is successful we can conclude things work. Let me know if you have any ideas around what we can assert
There was a problem hiding this comment.
Yes, the flakiness is not ideal.
Let's go with this.
| for (FederationNamespaceInfo ns : nss) { | ||
| if (!nsId.equals(ns.getNameserviceId())) { | ||
| namespaceInfos.add(ns); | ||
| for (Iterator<FederationNamespaceInfo> it = nss.iterator(); it |
There was a problem hiding this comment.
If we are going to loop over, can we just do:
for (String nsId : nss)
?
|
🎊 +1 overall
This message was automatically generated. |
tomscut
approved these changes
Dec 3, 2021
goiri
approved these changes
Dec 3, 2021
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…ableNs. (apache#3745). Contributed by Ayush Saxena. Reviewed-by: litao <tomleescut@gmail.com> Reviewed-by: Inigo Goiri <inigoiri@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
Fixes the retry the logic of invokeAtAvailableNs (RBF
How was this patch tested?
Through UT
For code changes: