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

Fix port number in recipes docs #937

Merged
merged 3 commits into from
Apr 1, 2020
Merged

Conversation

hub-cap
Copy link
Contributor

@hub-cap hub-cap commented Mar 24, 2020

No description provided.

@hub-cap hub-cap added :Usability Makes Rally easier to use :Docs Changes to the documentation labels Mar 24, 2020
@hub-cap hub-cap added this to the 1.5.0 milestone Mar 24, 2020
@hub-cap hub-cap self-assigned this Mar 25, 2020
Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

The intention behind using port 9200 for the examples was that people are familiar with that port. Also, from what I could see the examples seem to be consistent right now? Is there any reason to switch to port 39200 instead?

@hub-cap
Copy link
Contributor Author

hub-cap commented Mar 25, 2020

@dliappis had asked me to do this because of some errors he had with a discuss issue. I think he would be best to comment on this.

@dliappis
Copy link
Contributor

There are a number of motivations for switching to 39200:

  1. In the docs and in our default out of the box configuration when --target-host is not defined, we default to port 39200.

  2. It's not uncommon that there's already an instance (e.g. metricstore or left over) occupying 9200 already

I've noticed that both can lead to confusion for users, especially when using esrallyd.

Alternatively, I think we could document in --target-hosts that Rally will automatically derive and use the port defined there as the Elasticsearch http_port (and based on that transport_port+100)

@danielmitterdorfer
Copy link
Member

Alternatively, I think we could document in --target-hosts that Rally will automatically derive and use the port defined there as the Elasticsearch http_port (and based on that transport_port+100)

I don't think we should do that given that we are about to move to the new subcommands where the port needs to be specified explicitly.

It's not uncommon that there's already an instance (e.g. metricstore or left over) occupying 9200 already

I'd argue that when users use --pipeline=benchmark-only they set up the cluster themselves and use the default port (9200)? If they use Rally instead to setup the cluster then this is different but otherwise I think we actually want to point them to 9200 by default?

@dliappis
Copy link
Contributor

I don't think we should do that given that we are about to move to the new subcommands where the port needs to be specified explicitly.

Yeah we are in this in-between state currently; people find and still use the daemon mode and hit issues, so this was the main motivation here esp. since this magic handling of the port isn't currently documented.

I'd argue that when users use --pipeline=benchmark-only they set up the cluster themselves and use the default port (9200)? If they use Rally instead to setup the cluster then this is different but otherwise I think we actually want to point them to 9200 by default?

Fair point; I'd counter argue that e.g. in Elastic Cloud this isn't the out of the box default either, but for self managed clusters one would think in most cases the port is 9200.

@dliappis
Copy link
Contributor

had an offline chat with @danielmitterdorfer re ^^

@hub-cap let's amend this PR to only change 9200 to 39200 for the Benchmarking a remote cluster example which uses Rally to spin up the cluster.

The Distributing the load test driver example assumes an externally provisioned cluster (so that's ok, no changes needed there) and the first example, Benchmarking an existing cluster is also about externally provisioned clusters, so no changes needed there either.

Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@danielmitterdorfer danielmitterdorfer added the enhancement Improves the status quo label Apr 1, 2020
@hub-cap hub-cap merged commit ab290a6 into elastic:master Apr 1, 2020
@hub-cap hub-cap deleted the fix_port_in_recipes branch April 1, 2020 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Docs Changes to the documentation enhancement Improves the status quo :Usability Makes Rally easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants