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

Correct documentation of discovery.zen.ping.unicast.hosts regarding support for port ranges #15816

Closed
drekbour opened this issue Jan 7, 2016 · 9 comments
Labels
:Distributed Coordination/Network Http and internode communication implementations >enhancement good first issue low hanging fruit help wanted adoptme

Comments

@drekbour
Copy link

drekbour commented Jan 7, 2016

The documentation of discovery.zen.ping.unicast.hosts explicitly mentions port-ranges but fails to mention that they are not supported and only the first port is checked.
There are several Issues/PRs relating to changing this but none have made it yet so please document that port-range format is allowed but not supported and that if a user wishes to address multiple ports they must do explicitly

server1:9300,server1:9301

https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-discovery-zen.html
#8833 will allow the first two ports to be scanned. Older items relating to this are #99, #7561, #7090, #12999, elastic/elasticsearch-cloud-aws#197

@dadoonet
Copy link
Member

dadoonet commented Jan 7, 2016

@drekbour Thanks for opening this issue. Do you want to contribute a doc PR to fix that?

@dadoonet dadoonet added the >docs General docs changes label Jan 7, 2016
@clintongormley
Copy link
Contributor

We should probably also throw an exception when a user specifies a port range.

@clintongormley clintongormley added >enhancement help wanted adoptme :Distributed Coordination/Network Http and internode communication implementations and removed >docs General docs changes labels Jan 10, 2016
@clintongormley clintongormley added the good first issue low hanging fruit label Mar 1, 2016
clintongormley added a commit that referenced this issue Mar 1, 2016
Remove docs saying that unicast hosts supports port ranges.

Relates to #15816
clintongormley added a commit that referenced this issue Mar 1, 2016
Remove docs saying that unicast hosts supports port ranges.

Relates to #15816
@clintongormley
Copy link
Contributor

I've updated the documentation, but we should still throw an exception when the user specifies a port range.

clintongormley added a commit that referenced this issue Mar 1, 2016
Remove docs saying that unicast hosts supports port ranges.

Relates to #15816
@nikhilchudekar
Copy link

@clintongormley I'm just getting started with elasticsearch and would to work on this one as my first PR. Can you assign it to me please?

@clintongormley
Copy link
Contributor

@nikhilchudekar I can't assign to people outside the organisation, but you're welcome to work on it and to submit a PR referencing this issue eg closes #15816 in the description

@nikhilchudekar
Copy link

will do, thanks.

@nikhilchudekar
Copy link

nikhilchudekar commented Feb 27, 2018

I see a PR that's already addressing this (though it is way back in April 2017). Looks like it has been reviewed, and all that is left is pushing the fix to the repo? @clintongormley I'll move on to something else.

@Tim-Brooks
Copy link
Contributor

We are going to open a separate issue for the "throwing an exception". This issue shows that there are related PRs that have been merged. But non of those PRs appear to have been merged into the elastic/elasticsearch repository.

@DaveCTurner
Copy link
Contributor

The docs are long-fixed, resolving the headline part of this issue, and I've opened #40786 for the remaining part.

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 >enhancement good first issue low hanging fruit help wanted adoptme
Projects
None yet
Development

No branches or pull requests

6 participants