HDFS-15417. RBF: Get the datanode report from cache for federation WebHDFS operations #2080
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
goiri
changed the title
|
💔 -1 overall
This message was automatically generated. |
sunchao
reviewed
Jun 18, 2020
| LocatedBlock location0 = locations.get(0); | ||
| return bestNode(location0.getLocations(), excludes); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (dns == null) { |
There was a problem hiding this comment.
Thanks. I follow the previous logic now with cached DN report
| if (dns == null) { | ||
| dns = getDatanodeReport(router); | ||
| } | ||
|
|
||
| return getRandomDatanode(dns, excludes); |
There was a problem hiding this comment.
In this case, excludes is always empty. We need to compute it using the dns obtained.
| /** | ||
| * Get the datanode report from all namespaces that are registered | ||
| * and active in the federation. | ||
| * @param router Router from which to get the report. |
There was a problem hiding this comment.
nit: leave a blank line before the @param line.
| private static DatanodeInfo[] getDatanodeReport( | ||
| final Router router) throws IOException { | ||
| // We need to get the DNs as a privileged user | ||
| final RouterRpcServer rpcServer = getRPCServer(router); |
There was a problem hiding this comment.
should we wrap these in the try .. catch block? and what is the return value if rpcServer.getDatanodeReport fail? null?
NickyYe
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
sunchao
reviewed
Jun 19, 2020
| @@ -18,6 +18,8 @@ | |||
| package org.apache.hadoop.hdfs.server.federation.router; | |||
|
|
|||
| import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.HADOOP_SECURITY_AUTHORIZATION; | |||
| import static org.apache.hadoop.hdfs.server.federation.metrics.NamenodeBeanMetrics.DN_REPORT_CACHE_EXPIRE; | |||
There was a problem hiding this comment.
This will cause compilation error. Also I'm wondering whether it makes sense to move the DN cache logic from NamenodeBeanMetrics to here and have the former to depend on this. This way we don't have to keep two copies of cache.
There was a problem hiding this comment.
Thanks for catching this. I was working on an old branch, now it is fixed. I prefer to move the NamenodeBeanMetrics logic by a separate change. Let's make this change cohesive. NamenodeBeanMetrics cached report needs a different API or at least some extra work since it needs a String.
...-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterWebHdfsMethods.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
sunchao
reviewed
Jun 24, 2020
...-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcServer.java
Show resolved
Hide resolved
| * @return List of datanodes. | ||
| * @throws IOException If it cannot get the report. | ||
| */ | ||
| public DatanodeInfo[] getCachedDatanodeReport(DatanodeReportType type) |
There was a problem hiding this comment.
nit: this can be package-private?
| } | ||
|
|
||
| private DatanodeInfo[] getCachedDatanodeReportImpl | ||
| (final DatanodeReportType type) throws IOException{ |
...-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcServer.java
Show resolved
Hide resolved
| final DatanodeReportType type, DatanodeInfo[] oldValue) | ||
| throws Exception { | ||
| ListenableFuture<DatanodeInfo[]> listenableFuture = |
There was a problem hiding this comment.
nit: variable listenableFuture is redundant - you can just return from submit call.
There was a problem hiding this comment.
Thank you for the comments. I've addressed all of them.
|
sunchao
approved these changes
Jun 24, 2020
There was a problem hiding this comment.
LGTM. @goiri @Hexiaoqiao do you also want to check this before I committing it?
|
Thanks @sunchao involve me here. In my internal version I try to turn off |
|
💔 -1 overall
This message was automatically generated. |
|
Thanks @Hexiaoqiao . Similarly, in our case we also disabled this for router metrics but need webhdfs on router. Relying on @NickyYe thanks for addressing the checkstyle issue. Could you also add a unit test as well? I think we need one for |
|
💔 -1 overall
This message was automatically generated. |
|
Any update @NickyYe ? we are pretty close to get this in with the UT. |
|
💔 -1 overall
This message was automatically generated. |
sunchao
reviewed
Jul 5, 2020
| assertArrayEquals(datanodeReport1, datanodeReport); | ||
|
|
||
| // Add one datanode | ||
| getCluster().getCluster().startDataNodes(getCluster().getCluster().getConfiguration(0), |
There was a problem hiding this comment.
I think we need to clear state after the test case so that it won't affect others like testNamenodeMetrics.
Also long line.
There was a problem hiding this comment.
Done. Thanks. @sunchao
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Merged. Thanks @NickyYe for the contribution and @Hexiaoqiao for helping review! |
Xushaohong
pushed a commit
to Xushaohong/hadoop
that referenced
this pull request
Jul 20, 2022
…deration WebHDFS operations (apache#2080)
NyteKnight
pushed a commit
to NyteKnight/hadoop
that referenced
this pull request
Jun 25, 2024
…bHDFS operations (apache#2080)
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.
https://issues.apache.org/jira/browse/HDFS-15417
NOTICE
Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HADOOP-XXXXX. Fix a typo in YYY.)
For more details, please see https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute