HDFS-17181 WebHDFS: Route all CREATE requests to the BlockManager #6108
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. |
|
All test failures seem unrelated and I didn't add any new tests because the code seems to be covered by tests already. |
|
💔 -1 overall
This message was automatically generated. |
|
This has now been running in production since September without a problem. I'll update the branch. |
lfrancke
commented
Feb 14, 2024
| HashSet<Node> excludes = new HashSet<Node>(); | ||
|
|
||
| Set<Node> excludes = new HashSet<>(); |
There was a problem hiding this comment.
This just removes a warning
| Node excludeNode = null; | ||
| if (idx != -1) { |
There was a problem hiding this comment.
This makes the code easier to read by just switching the != with the == condition. No other code changes
| @@ -311,25 +315,15 @@ static DatanodeInfo chooseDatanode(final NameNode namenode, | |||
| } | |||
| } | |||
|
|
|||
| if (op == PutOpParam.Op.CREATE) { | |||
There was a problem hiding this comment.
This is the actual change. It used to treat CREATE special by calling the BlockManager.
This removes the special case and instead makes calling the BlockManager the norm for all requests except OPEN, APPEND and GETFILECHECKSUM for which it will pick an actual replica.
| @@ -358,13 +364,13 @@ static DatanodeInfo chooseDatanode(final NameNode namenode, | |||
| * to return the first element of the node here. | |||
| */ | |||
| protected static DatanodeInfo bestNode(DatanodeInfo[] nodes, | |||
| HashSet<Node> excludes) throws IOException { | |||
There was a problem hiding this comment.
Unrelated: Just removes warnings again and makes code clearer
|
I have updated the PR and merged latest changes from trunk |
|
Makes sense to me. Assuming this patch is tested in production, then this one should be good to go. |
|
@yakirgb as far as I know you have been running with this in production for months, now. Correct? |
|
Correct |
|
The test build failed but I believe it's unrelated. I can't really read/interpret the logs. |
|
+1 and merged based on the fact it's been tested in production. |
|
Thank you very much! |
|
Thank you! |
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
https://issues.apache.org/jira/browse/HDFS-17181
When calling WebHDFS to create a file it will happily redirect to nodes that are in maintenance.
The reason is in the
chooseDatanodemethod inNamenodeWebHdfsMethodswhere it will only call theBlockPlacementPolicy(which considers all these edge cases) in case theremoteAddr(i.e. the address making the request to WebHDFS) is also running a DataNode.In all other cases it just refers to
NetworkTopology#chooseRandomwhich does not consider any of these circumstances (e.g. load, maintenance).I don't understand the reason to not just always refer to the placement policy so this PR fixes that
How was this patch tested?
We tested it in production and ran the unit tests (had some issues with those so let's see what the builds here say)
For code changes: