Transport client: Fix remove address to actually work #21743
Merged
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
The removeTransportAddress method of TransportClient removes the address from the list of nodes that the client pings to sniff for nodes. However, it does not remove it from the list of existing connected nodes. This means removing a node is not possible, as long as that node is still up. This change removes the node from the connected nodes list before triggering sampling (ie sniffing). While the fix is simple, testing was not because there were no existing tests for sniffing. This change also modifies the mocks used by transport client unit tests in order to allow mocking sniffing.
rjernst
added
:Core/Infra/Transport API
|
Note I have marked this for 5.1 and 6.0. It should backport to 5.0.2 fine (I don't think there have been many changes, if any), but I would only do that if there is a strong need for it. |
rjernst
commented
Nov 22, 2016
| @@ -547,6 +547,8 @@ protected void doRun() throws Exception { | |||
| holderToNotify.handler().handleException(sendRequestException); | |||
| } | |||
| }); | |||
| } else { | |||
| logger.debug("Exception while sending request, handler likely already notified due to timeout", e); | |||
There was a problem hiding this comment.
Note this debug statement is so that we do not completely swallow an exception where the holder has already been removed. This was causing an NPE I had while developing my mock code to be hidden. @jasontedor can explain more on why this is an "ok" case normally...
jasontedor
approved these changes
Nov 22, 2016
| } else { | ||
| logger.debug("removing address [{}]", otherNode); | ||
| logger.debug("removing address [{}] from list nodes", otherNode); |
There was a problem hiding this comment.
Nit: list nodes -> listed nodes?
| if (!otherNode.getAddress().equals(transportAddress)) { | ||
| nodesBuilder.add(otherNode); | ||
| } else { | ||
| logger.debug("removing address [{}] from connected nodes", otherNode); |
There was a problem hiding this comment.
I wonder if this should say something more like disconnecting from node with address [{}]?
| ClusterState clusterState = getMockClusterState(node); | ||
| transportResponseHandler.handleResponse(new ClusterStateResponse(clusterName, clusterState)); | ||
| } else { | ||
| throw new UnsupportedOperationException("Mock transport does not understand action " + action); |
| checkRemoveAddress(false); | ||
| } | ||
|
|
||
| private void checkRemoveAddress(boolean sniff) { |
|
Nice work @rjernst, thanks for adding the tests, I know that was where most of the work was and it's very much appreciated. |
|
retest this please |
rjernst
added a commit
that referenced
this pull request
Nov 23, 2016
* Transport client: Fix remove address to actually work The removeTransportAddress method of TransportClient removes the address from the list of nodes that the client pings to sniff for nodes. However, it does not remove it from the list of existing connected nodes. This means removing a node is not possible, as long as that node is still up. This change removes the node from the connected nodes list before triggering sampling (ie sniffing). While the fix is simple, testing was not because there were no existing tests for sniffing. This change also modifies the mocks used by transport client unit tests in order to allow mocking sniffing.
s1monw
pushed a commit
that referenced
this pull request
Nov 23, 2016
* Transport client: Fix remove address to actually work The removeTransportAddress method of TransportClient removes the address from the list of nodes that the client pings to sniff for nodes. However, it does not remove it from the list of existing connected nodes. This means removing a node is not possible, as long as that node is still up. This change removes the node from the connected nodes list before triggering sampling (ie sniffing). While the fix is simple, testing was not because there were no existing tests for sniffing. This change also modifies the mocks used by transport client unit tests in order to allow mocking sniffing.
s1monw
pushed a commit
that referenced
this pull request
Nov 23, 2016
* Transport client: Fix remove address to actually work The removeTransportAddress method of TransportClient removes the address from the list of nodes that the client pings to sniff for nodes. However, it does not remove it from the list of existing connected nodes. This means removing a node is not possible, as long as that node is still up. This change removes the node from the connected nodes list before triggering sampling (ie sniffing). While the fix is simple, testing was not because there were no existing tests for sniffing. This change also modifies the mocks used by transport client unit tests in order to allow mocking sniffing.
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.
The removeTransportAddress method of TransportClient removes the address
from the list of nodes that the client pings to sniff for nodes.
However, it does not remove it from the list of existing connected
nodes. This means removing a node is not possible, as long as that node
is still up.
This change removes the node from the connected nodes list before
triggering sampling (ie sniffing). While the fix is simple, testing was
not because there were no existing tests for sniffing. This change also
modifies the mocks used by transport client unit tests in order to allow
mocking sniffing.