Skip to content

Transport: Remove parsing of port ranges #7561

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

Conversation

spinscale
Copy link
Contributor

UnicastZenDiscovery allowed you to specify port ranges for hosts to connect to.
However only the first port was used at all, when specifying something like
host[9300-9400]. This commit only accepts host:port pairs and thus makes
the configuration reflect the execution as even in the old setup only a single
host:port pair was pinged at all, as this was very expensive.

Note there is a coming PR for 1.x, which deprecates this setting and emits a warning instead of removing it.

@@ -62,8 +62,6 @@

public static final String ACTION_NAME = "internal:discovery/zen/unicast";

public static final int LIMIT_PORTS_COUNT = 1;
Copy link
Member

Choose a reason for hiding this comment

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

We talked with @kimchy a while ago about increasing LIMIT_PORTS_COUNT to 2.
See thread here: elastic/elasticsearch-cloud-aws#99 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

And the related issue here: #7090

@dadoonet
Copy link
Member

dadoonet commented Sep 3, 2014

I'm -1 for this change because of the following:

Or it means that discovery plugins need to add more than one node in the unicast list of nodes.

WDYT?

@spinscale
Copy link
Contributor Author

it does not matter, if the max setting is 1 or 2. If you allow the user to set an arbitrary port range (i.e. 100 ports) and then try two ports instead one, you can also make him set host:1 and host:2 - which at least has the advantage, that the user knows what happens and does not start to wonder, why the there is no connection initiated, if the node is running on host:3 - the current situation is just confusing from a configuration and usability point of view

@dadoonet
Copy link
Member

dadoonet commented Sep 3, 2014

@spinscale That's indeed what I suggested here: elastic/elasticsearch-cloud-aws#99 (comment)

So we could think of changing the code to something like:

TransportAddress[] addresses = transportService.addressesFromString(address);
// we only limit to 3 addresses, makes no sense to ping 100 ports
for (int i = 0; (i < addresses.length && i < 3); i++) {
       logger.trace("adding {}, address {}, transport_address {}", instance.getInstanceId(), address, addresses[i]);
       discoNodes.add(new DiscoveryNode("#cloud-" + instance.getInstanceId() + "-" + i, addresses[i], Version.CURRENT));
}

But @kimchy preferred to manage that in elasticsearch core code not in plugins if I understood correctly.

public TransportAddress addressFromString(String address) throws Exception {
int index = address.lastIndexOf(':');
if (index == -1) {
return new InetSocketTransportAddress(address, Integer.parseInt(this.port));
Copy link
Member

Choose a reason for hiding this comment

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

this will not work, right? cause the default is 9300-9400, which is good, since we want to try another port on the second instance we start on the same machine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am confused here. This is not about binding to a port when starting, only about connecting to other nodes, as far as I can judge the source codes. Starting up several nodes in parallel on one machines still works as expected

UnicastZenDiscovery allowed you to specify port ranges for hosts to connect to.
However only the first port was used at all, when specifying something like
`host[9300-9400]`. This commit only accepts `host:port` pairs and thus makes
the configuration reflect the execution as even in the old setup only a single
host:port pair was pinged at all, as this was very expensive.
@spinscale spinscale force-pushed the enhancements/port-range-pinging-remove-master branch from 05663b3 to 5e8cf52 Compare September 12, 2014 10:23
@clintongormley
Copy link
Contributor

@spinscale what's happening with this one?

@clintongormley clintongormley added the :Distributed Coordination/Network Http and internode communication implementations label Nov 11, 2014
@spinscale
Copy link
Contributor Author

@clintongormley waiting for feedback here on my comments, cc @kimchy

@spinscale
Copy link
Contributor Author

closing this for now, not an issue anymore in tests and does not seem to be a problem for anyone else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Network Http and internode communication implementations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants