HDFS-16535. SlotReleaser should reuse the domain socket based on socket paths #4158
Conversation
This file contains 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. |
|
@leosunli is this something you'd be interested in reviewing? |
|
@jojochuang thank for your reminder, I will review this PR. |
|
🎊 +1 overall
This message was automatically generated. |
leosunli
reviewed
Apr 15, 2022
There was a problem hiding this comment.
Thank @stiga-huang for catching it. I add two simple comments.
| @@ -240,7 +245,7 @@ public void run() { | |||
| } else { | |||
| shm.getEndpointShmManager().shutdown(shm); | |||
| IOUtilsClient.cleanupWithLogger(LOG, domainSocket, out); | |||
| domainSocket = null; | |||
There was a problem hiding this comment.
why do you remove the line 243? If remove it, the line 228 looks redundant.
There was a problem hiding this comment.
why do you remove the line 243?
domainSocket is now a local variable (defined at line 192). We don't need to update it at the end of this method. Note that this finally clause belongs to the try-clause starts at line 195.
If remove it, the line 228 looks redundant.
For line 228, it's needed in the next retry of the while-loop. The check at line 199 will catch it and trigger a re-connect.
| @@ -957,6 +958,83 @@ public void testDomainSocketClosedByDN() throws Exception { | |||
| } | |||
| } | |||
|
|
|||
| // Regression test for HDFS-16473 | |||
There was a problem hiding this comment.
I don't quite understand the connection between this UT and HDFS-16473.
There was a problem hiding this comment.
Oops, I gave a wrong JIRA. Thanks for catching this! It should be HDFS-16535.
|
Thank @leosunli! I updated the comments. |
|
🎊 +1 overall
This message was automatically generated. |
|
LGFM +1. |
asfgit
pushed a commit
that referenced
this pull request
Apr 18, 2022
…et paths (#4158) Reviewed-by: Lisheng Sun <[email protected]> (cherry picked from commit 35d4c02)
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…et paths (apache#4158) Reviewed-by: Lisheng Sun <[email protected]>
jojochuang
pushed a commit
to jojochuang/hadoop
that referenced
this pull request
May 23, 2023
…ased on socket paths (apache#4158) Reviewed-by: Lisheng Sun <[email protected]> (cherry picked from commit 35d4c02) Change-Id: Ie4609bf36ada0156af684ace80222f699dad41d1
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
HDFS-13639 (#1885) improves the performance of short-circuit shm slot releasing by reusing the domain socket that the client previously used to send release request to the DataNode.
This is good when there are only one DataNode locates with the client (truth in most of the production environment). However, if we launch multiple DataNodes on a machine (usually for testing, e.g. Impala's end-to-end tests), the request could be sent to the wrong DataNode. See an example in IMPALA-11234.
We should only reuse the domain socket when it corresponds to the same socket path. This change introduces a map in ShortCircuitCache to track the mapping from socket path to the domain socket. SlotReleaser will pick the correct domain socket (if exists) based on the socket path.
How was this patch tested?
I deploy the fix in my Impala minicluster with multiple DataNodes launched on my machine. Verified that the error logs of slot release failures disappeared.
Also add a regression test: TestShortCircuitCache#testDomainSocketClosedByMultipleDNs().
For code changes: