Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Give a unique id to each ping response #7769

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
4 participants
@bleskes
Copy link
Member

bleskes commented Sep 17, 2014

During discovery a node gossips with other nodes to discover the current state of the cluster - what nodes are out there, what version they use and most importantly whether there is an active master out there. During this ping process we may end up in a situation where old information is mixed with new. This is comment if a couple of master election happen in rapid succession.

This PR adds a monotonically increasing id to each ping response. This makes it easy to always select the last ping from every node.

bleskes added some commits Sep 12, 2014

wip
Added PingCollection
TODO: unit test it + java docs
@@ -108,16 +126,8 @@ public void readFrom(StreamInput in) throws IOException {
if (in.readBoolean()) {
master = readNode(in);
}
if (in.getVersion().onOrAfter(Version.V_1_4_0_Beta1)) {

This comment has been minimized.

Copy link
@martijnvg

martijnvg Sep 19, 2014

Member

I assume this version check remains in 1.x and 1.4, right?

This comment has been minimized.

Copy link
@bleskes

bleskes Sep 19, 2014

Author Member

Yes - the code in 1.4 has much bwc craft. It was decided to remove it from master but I missed this in previous work.

/**
* a utility collection of pings where only the most recent ping is stored per node
*/
public static class PingCollection {

This comment has been minimized.

Copy link
@martijnvg

martijnvg Sep 19, 2014

Member

+1 this really cleans up code in several places

*/
public static class PingCollection {

Map<DiscoveryNode, PingResponse> pings;

This comment has been minimized.

Copy link
@martijnvg

martijnvg Sep 19, 2014

Member

Maybe use ConcurrentHashMap? if we can then we don't the synchronized methods.

This comment has been minimized.

Copy link
@bleskes

bleskes Sep 19, 2014

Author Member

I chose for simplicity (synchronized methods + normal map) because we have a slightly complex update rule (update if id is >= existing id):

 PingResponse existingResponse = pings.get(ping.node());
 // in case both existing and new ping have the same id (probably because they come
 // from nodes from version <1.4.0) we prefer to use the last added one.
 if (existingResponse == null || existingResponse.id() <= ping.id()) {

This comment has been minimized.

Copy link
@martijnvg

martijnvg Sep 19, 2014

Member

ah ok that make sense

@@ -52,6 +55,12 @@

public static final PingResponse[] EMPTY = new PingResponse[0];

private static final AtomicLong idGenerator = new AtomicLong();

This comment has been minimized.

Copy link
@martijnvg

martijnvg Sep 19, 2014

Member

Would this be ok if multiple nodes run inside the same jvm?

This comment has been minimized.

Copy link
@bleskes

bleskes Sep 19, 2014

Author Member

I think so. The point is that always increases with each ping, doesn't matter by how much.

This comment has been minimized.

Copy link
@martijnvg

martijnvg Sep 19, 2014

Member

yes, that make sense

@martijnvg

This comment has been minimized.

Copy link
Member

martijnvg commented Sep 19, 2014

LGTM

@bleskes bleskes closed this in 41fd5d0 Sep 20, 2014

bleskes added a commit to bleskes/elasticsearch that referenced this pull request Sep 20, 2014

Discovery: Give a unique id to each ping response
During discovery a node gossips with other nodes to discover the current state of the cluster - what nodes are out there, what version they use and most importantly whether there is an active master out there. During this ping process we may end up in a situation where old information is mixed with new. This is comment if a couple of master election happen in rapid succession.

This commit adds a monotonically increasing id to each ping response. This makes it easy to always select the last ping from every node.

Closes elastic#7769

bleskes added a commit to bleskes/elasticsearch that referenced this pull request Sep 20, 2014

Discovery: Give a unique id to each ping response
During discovery a node gossips with other nodes to discover the current state of the cluster - what nodes are out there, what version they use and most importantly whether there is an active master out there. During this ping process we may end up in a situation where old information is mixed with new. This is comment if a couple of master election happen in rapid succession.

This commit adds a monotonically increasing id to each ping response. This makes it easy to always select the last ping from every node.

Closes elastic#7769

bleskes added a commit that referenced this pull request Sep 20, 2014

Discovery: Give a unique id to each ping response
During discovery a node gossips with other nodes to discover the current state of the cluster - what nodes are out there, what version they use and most importantly whether there is an active master out there. During this ping process we may end up in a situation where old information is mixed with new. This is comment if a couple of master election happen in rapid succession.

This commit adds a monotonically increasing id to each ping response. This makes it easy to always select the last ping from every node.

Closes #7769

@bleskes bleskes deleted the bleskes:unicast_ping_response_id branch Sep 20, 2014

@jpountz jpountz removed the review label Oct 21, 2014

@clintongormley clintongormley changed the title Discovery: Give a unique id to each ping response Give a unique id to each ping response Jun 7, 2015

mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015

Discovery: Give a unique id to each ping response
During discovery a node gossips with other nodes to discover the current state of the cluster - what nodes are out there, what version they use and most importantly whether there is an active master out there. During this ping process we may end up in a situation where old information is mixed with new. This is comment if a couple of master election happen in rapid succession.

This commit adds a monotonically increasing id to each ping response. This makes it easy to always select the last ping from every node.

Closes elastic#7769
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.