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

Persistent Node Ids #17987

Closed
wants to merge 34 commits into from

Conversation

Projects
None yet
5 participants
@ywelsch
Copy link
Contributor

commented Apr 26, 2016

Nodes are currently identified in the cluster by IDs that are randomly generated during node startup. That means they change every time the node is restarted (i.e. they correspond to process ids). While this doesn't matter for ES proper, it makes it hard for external services to track nodes. Persistent node ids ensure that the same id can be reused across restarts, facilitating tracking of nodes in node stats / cluster state APIs.

In contrast to #17811 (which this PR supersedes), this PR keeps process ids around for the code that relies on its semantics (mainly discovery-related parts).

bleskes and others added some commits Apr 16, 2016

when resetting shared test cluster, shut down new nodes first as they…
… might have reused an node id of the initial shared nodes.
* returns the unique uuid describing this node. The uuid is persisted in the data folder of this node
* and is reused across restarts.
**/
public String nodeId() {

This comment has been minimized.

Copy link
@s1monw

s1monw May 2, 2016

Contributor

can we make this a getNodeId()

This comment has been minimized.

Copy link
@ywelsch

ywelsch May 3, 2016

Author Contributor

ok, done in 7dbe1bf

import java.io.OutputStream;
import java.util.Objects;

/**

This comment has been minimized.

Copy link
@s1monw

s1monw May 2, 2016

Contributor

give us some chars that explain what this is and where it's used?

This comment has been minimized.

Copy link
@ywelsch

ywelsch May 3, 2016

Author Contributor

done in 7dbe1bf


private final String nodeId;

public NodeMetaData(final String nodeId) {

This comment has been minimized.

Copy link
@s1monw

s1monw May 2, 2016

Contributor

does this need to be public?

This comment has been minimized.

Copy link
@ywelsch

ywelsch May 3, 2016

Author Contributor

nope, made it package-private

final XContentType xContentType = XContentType.values()[indexInput.readInt()];
indexInput.readLong(); // version currently unused
if (fileVersion == 0) {

This comment has been minimized.

Copy link
@s1monw

s1monw May 2, 2016

Contributor

can you compare this against MIN_COMPATIBLE_STATE_FILE_VERSION instead? and maybe we should add a some comment when we can remove it?

This comment has been minimized.

Copy link
@ywelsch

ywelsch May 3, 2016

Author Contributor

I have made it more explicit what we're trying to achieve. Have a look at 7dbe1bf.

@@ -322,8 +332,9 @@ public Node start() {

validateNodeBeforeAcceptingRequests(settings, transportService.boundAddress());

DiscoveryNode localNode = injector.getInstance(DiscoveryNodeService.class)
.buildLocalNode(transportService.boundAddress().publishAddress());
String nodeId = injector.getInstance(NodeEnvironment.class).nodeId();

This comment has been minimized.

Copy link
@s1monw

s1monw May 2, 2016

Contributor

Can we just assign NodeEnvironment next to Environment and don't do the guice lookup?

This comment has been minimized.

Copy link
@ywelsch

ywelsch May 3, 2016

Author Contributor

NodeEnvironment is created in the constructor and localNode injection happens in the start method. I could make nodeEnvironment or nodeId a final field in the Node class, but I'm not sure that makes the code much nicer.

ywelsch added some commits May 10, 2016

*/
public DiscoveryNode(String nodeId, TransportAddress address, Map<String, String> attributes, Set<Role> roles, Version version) {
this("", nodeId, address.getHost(), address.getAddress(), address, attributes, roles, version);
public DiscoveryNode(String id, TransportAddress address, Map<String, String> attributes, Set<Role> roles,

This comment has been minimized.

Copy link
@bleskes

bleskes May 17, 2016

Member

hmm. I think this is tricky having no id string. How about using the supplied id as the nodeId and generate a random ephemeral one?

This comment has been minimized.

Copy link
@ywelsch

ywelsch May 17, 2016

Author Contributor

I don't understand what you mean here. This constructor is mainly used by tests where we don't care much about the actual values and construction of temporary nodes for pinging. I don't think we should just randomly generate a ephemeral id here. Better would be to get rid of this constructor altogether...

@@ -238,6 +251,13 @@ public String getId() {
}

/**
* The unique ephemeral id of the node.

This comment has been minimized.

Copy link
@bleskes

bleskes May 17, 2016

Member

can we add some docs as to what we mean with ephemeral - most notably when it changes, and it's implication to identity

if (address != null) {
sb.append('{').append(address).append('}');
}
sb.append('{').append(nodeId).append('}');

This comment has been minimized.

Copy link
@bleskes

bleskes May 17, 2016

Member

why did you change this?

This comment has been minimized.

Copy link
@ywelsch

ywelsch May 17, 2016

Author Contributor

nodeId is never null
ephemeralId is never null
hostName/address is never null

for (DiscoveryNode node : this) {
if (Regex.simpleMatch(nodeId, node.getName())) {
if (node.getEphemeralId().equals(nodeId)) {

This comment has been minimized.

Copy link
@bleskes

bleskes May 17, 2016

Member

why did add this extra resolveNode path? isn't ephemeralId supposed to be internal and shouldn't be used in an API?

This comment has been minimized.

Copy link
@ywelsch

ywelsch May 17, 2016

Author Contributor

sure, I can remove it.

@@ -980,7 +980,10 @@ public void run() {
public void onNodeAck(DiscoveryNode node, @Nullable Throwable t) {
if (!ackedTaskListener.mustAck(node)) {
//we always wait for the master ack anyway
if (!node.equals(nodes.getMasterNode())) {
DiscoveryNode masterNode = nodes.getMasterNode();

This comment has been minimized.

Copy link
@bleskes

bleskes May 17, 2016

Member

what was the rational behind this change? equals checks for all of these no?

This comment has been minimized.

Copy link
@ywelsch

ywelsch May 17, 2016

Author Contributor

previously, equals did only check for equality of ephemeral id. Extending this check now to all attributes felt:

  1. inefficient (always compare all attributes just to detect that it's same node)
  2. unsafe w.r.t. future extensions (where we could for example make it possible for node to have attributes changing at runtime).
@@ -241,6 +246,10 @@ static Settings buildClientSettings(String tribeName, Settings globalSettings, S
sb.put(Node.NODE_DATA_SETTING.getKey(), false);
sb.put(Node.NODE_MASTER_SETTING.getKey(), false);
sb.put(Node.NODE_INGEST_SETTING.getKey(), false);
// node id of a tribe client node is determined by node id of parent node and tribe name
int nodeIdSeed = StringHelper.murmurhash3_x86_32(new BytesRef(parentNodeId + "/" + tribeName), 0);

This comment has been minimized.

Copy link
@bleskes

bleskes May 17, 2016

Member

+1. nice solution.

@jasontedor jasontedor removed their assignment Jun 5, 2016

@ywelsch ywelsch referenced this pull request Jun 30, 2016

Merged

Persistent Node Ids #19140

@ywelsch

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2016

superseded by #19140

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.