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

Core: Default node.name to the hostname #33677

Merged
merged 11 commits into from
Sep 19, 2018

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Sep 13, 2018

Changes the default of the node.name setting to the hostname of the
machine on which Elasticsearch is running. Previously it was the first 8
characters of the node id. This had the advantage of producing a unique
name even when the node name isn't configured but the disadvantage of
being unrecognizable and not being available until fairly late in the
startup process. Of particular interest is that it isn't available until
after logging is configured. This forces us to use a volatile read
whenever we add the node name to the log.

Using the hostname is available immediately on startup and is generally
recognizable but has the disadvantage of not being unique when run on
machines that don't set their hostname or when multiple elasticsearch
processes are run on the same host. I believe that, taken together, it
is better to default to the hostname.

  1. Running multiple copies of Elasticsearch on the same node is a fairly
    advanced feature. We do it all the as part of the elasticsearch build
    for testing but we make sure to set the node name then.
  2. That the node.name defaults to some flavor of "localhost" on an
    unconfigured box feels like it isn't going to come up too much in
    production. I expect most production deployments to at least set the
    hostname.

As a bonus, production deployments need no longer set the node name in
most cases. At least in my experience most folks set it to the hostname
anyway.

Changes the default of the `node.name` setting to the hostname of the
machine on which Elasticsearch is running. Previously it was the first 8
characters of the node id. This had the advantage of producing a unique
name even when the node name isn't configured but the disadvantage of
being unrecognizable and not being available until fairly late in the
startup process. Of particular interest is that it isn't available until
after logging is configured. This forces us to use a volatile read
whenever we add the node name to the log.

Using the hostname is available immediately on startup and is generally
recognizable but has the disadvantage of not being unique when run on
machines that don't set their hostname or when multiple elasticsearch
processes are run on the same host. I believe that, taken together, it
is better to default to the hostname.

1. Running multiple copies of Elasticsearch on the same node is a fairly
advanced feature. We do it all the as part of the elasticsearch build
for testing but we make sure to set the node name then.
2. That the node.name defaults to some flavor of "localhost" on an
unconfigured box feels like it isn't going to come up too much in
production. I expect most production deployments to at least set the
hostname.

As a bonus, production deployments need no longer set the node name in
most cases. At least in my experience most folks set it to the hostname
anyway.
@nik9000 nik9000 added >breaking :Core/Infra/Core Core issues without another label v7.0.0 labels Sep 13, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@nik9000
Copy link
Member Author

nik9000 commented Sep 13, 2018

I've marked this as discuss because I bunch of us talked about this and thought it was a good idea but we are aware that other contributors have thought otherwise in the past and might still not like this idea.

@jasontedor
Copy link
Member

We discussed this during Fix-it-Thursday and have consensus to move ahead with this proposal.

@nik9000
Copy link
Member Author

nik9000 commented Sep 13, 2018

We discussed this during Fix-it-Thursday and have consensus to move ahead with this proposal.

❤️

@nik9000
Copy link
Member Author

nik9000 commented Sep 13, 2018

The test failure is mine. I'm looking into it.

I still need to go through the docs some more. And write a breaking changes note for this.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one suggestion.

Elasticsearch uses `node.name` as a human readable identifier for a
particular instance of Elasticsearch so it is included in the response
of many APIs. It defaults to the hostname that the machine has when
Elasticsearch starts. If you'd prefer a different identifier then you
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would word this a little less colloquially:

It defaults to the hostname of the machine when Elasticsearch starts, but can be configured explicitly in elasticsearch.yml as follows:

@nik9000
Copy link
Member Author

nik9000 commented Sep 13, 2018

I think I need to do some testing work on this before it is ready. I'm seeing a few things I hadn't thought about.

Copy link
Member Author

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rjernst could you have another look at this? Mostly I cleaned up testing things since you last looked.

final Settings settings = builder.build();
final Settings settings = Settings.builder()
.put("cluster.name", randomAlphaOfLength(16))
.put("node.name", randomAlphaOfLength(16))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node.name is always available at this point in the course of normal operation.

}

public void testNoNodeNameInPatternWarning() throws IOException, UserException {
String nodeName = randomAlphaOfLength(16);
LogConfigurator.setNodeName(nodeName);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we automatically add a node name to the pattern we need to configure one or else the NodeNamePatternConverter will refuse to be built.

@@ -43,11 +48,4 @@ protected BufferedReader openReader(Path logFile) throws IOException {
}
});
}

public void testDummy() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad to remove this!

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one nit.

==== The default for `node.name` is now the hostname

`node.name` now defaults to the hostname at the time when Elasticsearch
is started. We hope that this is a more generally useful default than
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would replace the second sentence with:

Previously the default node name was the first eight characters of the node id. It can still be configured explicitly in elasticsearch.yml.

@nik9000 nik9000 merged commit 26c4f1f into elastic:master Sep 19, 2018
@bleskes
Copy link
Contributor

bleskes commented Sep 23, 2018

@nik9000 was it discussed whether we should prevent joins of nodes with a name that clashes with an existing node in the cluster?

@nik9000
Copy link
Member Author

nik9000 commented Sep 24, 2018

@nik9000 was it discussed whether we should prevent joins of nodes with a name that clashes with an existing node in the cluster?

I wasn't around for the discussion. I'm not sure where I come down on the issue. It does feel like a sign of a configuration problem though so I think it'd be fair to prevent such joins.

@nik9000
Copy link
Member Author

nik9000 commented Sep 24, 2018

I think it'd be fair to prevent such joins.

I suppose what I mean is that I think it'd be a good check to do and the argument to prevent joins where the node names match is pretty good. I'm not 100% sure it is worth implementing it but I feel like I could be convinced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking :Core/Infra/Core Core issues without another label v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants