HADOOP-18365. Update the remote address when a change is detected #4692
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
Avoid reconnecting to the old address after detecting that the address has been updated.
|
💔 -1 overall
This message was automatically generated. |
ZanderXu
approved these changes
Aug 4, 2022
There was a problem hiding this comment.
LGTM, Thanks @snmvaughan for your report.
Please fix the checkstyle and check the failed UT hadoop.ipc.TestIPC
|
💔 -1 overall
This message was automatically generated. |
added 2 commits
August 6, 2022 10:41
The ConnectionId is used as a key in the connections map, and updating the remoteId caused problems with the cleanup of connections when the removeMethod was used. Instead of updating the address within the remoteId, use the removeMethod to cleanup references to the current identifier and then replace it with a new identifier using the updated address.
Mark non-test fields as private and final, and add a missing accessor.
|
💔 -1 overall
This message was automatically generated. |
|
I need to add an additional test to demonstrate that the in addition to the update being handled, it avoids having to repeat the process. The current patch fixed the existing unit test that correctly identified that the updated address caused problems with cleanup, but it didn't catch that the original (incorrect) IP address would be attempted again. |
|
🎊 +1 overall
This message was automatically generated. |
sokui
reviewed
Aug 8, 2022
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Client.java
Show resolved
Hide resolved
Once the address has been updated, it should be used in future calls. Check to ensure that a second request succeeds and that it uses the existing updated address instead of having to re-resolve.
|
🎊 +1 overall
This message was automatically generated. |
ndimiduk
approved these changes
Aug 12, 2022
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Client.java
Show resolved
Hide resolved
saintstack
approved these changes
Aug 18, 2022
| * | ||
| * @param address the updated address | ||
| * @throws IllegalArgumentException if the hostname or port doesn't match | ||
| * @see Connection#updateAddress() |
There was a problem hiding this comment.
The IllegalAddressException throw is unsettling but makes sense as long as the caller #updateAddress keeps making the currentAddr in the same way... I was going to say this method comment could be more explicit about what is happening in here and presumptions but I think the pointer to #updateAddress takes care of it....
| throws IOException { | ||
| return getProxy(protocol, clientVersion, connId.getAddress(), | ||
| connId.ticket, conf, factory, connId.getRpcTimeout(), | ||
| connId.getTicket(), conf, factory, connId.getRpcTimeout(), |
There was a problem hiding this comment.
Nice but unrelated change.
| } | ||
|
|
||
| /** | ||
| * The {@link ConnectionId#hashCode} has to be stable despite updates that occur as the the |
| server.stop(); | ||
| } | ||
| } | ||
|
|
snmvaughan
pushed a commit
to snmvaughan/hadoop
that referenced
this pull request
Aug 19, 2022
…ache#4692) Back port to branch-3.3, to avoid reconnecting to the old address after detecting that the address has been updated. * Use a stable hashCode to allow safe IP addr changes * Add test that updated address is used Once the address has been updated, it will be used in future calls. Test verifies that a second request succeeds and that it uses the existing updated address instead of having to re-resolve.
sunchao
pushed a commit
that referenced
this pull request
Aug 20, 2022
) (#4768) Back port to branch-3.3, to avoid reconnecting to the old address after detecting that the address has been updated. * Use a stable hashCode to allow safe IP addr changes * Add test that updated address is used Once the address has been updated, it will be used in future calls. Test verifies that a second request succeeds and that it uses the existing updated address instead of having to re-resolve. Co-authored-by: Steve Vaughan Jr <s_vaughan@apple.com>
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…ache#4692) Avoid reconnecting to the old address after detecting that the address has been updated. * Fix Checkstyle line length violation * Keep ConnectionId as Immutable for map key The ConnectionId is used as a key in the connections map, and updating the remoteId caused problems with the cleanup of connections when the removeMethod was used. Instead of updating the address within the remoteId, use the removeMethod to cleanup references to the current identifier and then replace it with a new identifier using the updated address. * Use final to protect immutable ConnectionId Mark non-test fields as private and final, and add a missing accessor. * Use a stable hashCode to allow safe IP addr changes * Add test that updated address is used Once the address has been updated, it should be used in future calls. Check to ensure that a second request succeeds and that it uses the existing updated address instead of having to re-resolve. Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: sokui Signed-off-by: XanderZu Signed-off-by: stack <stack@apache.org>
bbeaudreault
pushed a commit
to HubSpot/hadoop
that referenced
this pull request
Jul 12, 2023
…ge is detected (apache#4692) (apache#4768) Back port to branch-3.3, to avoid reconnecting to the old address after detecting that the address has been updated. * Use a stable hashCode to allow safe IP addr changes * Add test that updated address is used Once the address has been updated, it will be used in future calls. Test verifies that a second request succeeds and that it uses the existing updated address instead of having to re-resolve. Co-authored-by: Steve Vaughan Jr <s_vaughan@apple.com>
NyteKnight
pushed a commit
to NyteKnight/hadoop
that referenced
this pull request
Jun 25, 2024
…ache#4… (apache#356) * HADOOP-18365. Update the remote address when a change is detected (apache#4692) (apache#4768) Back port to branch-3.3, to avoid reconnecting to the old address after detecting that the address has been updated. * Use a stable hashCode to allow safe IP addr changes * Add test that updated address is used Once the address has been updated, it will be used in future calls. Test verifies that a second request succeeds and that it uses the existing updated address instead of having to re-resolve. Co-authored-by: Steve Vaughan Jr <s_vaughan@apple.com> * ACLOVERRIDE --------- Co-authored-by: Steve Vaughan <email@stevevaughan.me> Co-authored-by: Steve Vaughan Jr <s_vaughan@apple.com>
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.
Avoid reconnecting to the old address after detecting that the address has been updated.
Description of PR
When the IPC Client recognizes that an IP address has changed, it updates the server field and logs a message:
Address change detected. Old: journalnode-1.journalnode.hdfs.svc.cluster.local/10.1.0.178:8485 New: journalnode-1.journalnode.hdfs.svc.cluster.local/10.1.0.182:8485Although the change is detected, the client will continue to connect to the old IP address, resulting in repeated log messages. This is seen in managed environments when JournalNode syncing is enabled and a JournalNode is restarted, with the remaining nodes in the set repeatedly logging this message when syncing to the restarted JournalNode.
The source of the problem is that the remoteId.address is not updated.
How was this patch tested?
HA configuration deployed on a Kubernetes cluster, using Java 11.
JounalNodeSyncerto wrap back around to the Restarted JournalNode to ensure that the message wasn't repeated and communications went directly to the new instanceFor code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?