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

Improved cluster api to include the current leader node #3100

Merged
merged 2 commits into from
Apr 16, 2018

Conversation

aantono
Copy link
Contributor

@aantono aantono commented Mar 28, 2018

Added information to display the current cluster leader node to the API

@emilevauge
Copy link
Member

@aantono hmmm not sure not having a GUID is a good thing.
How about adding a Hostname field that would be always set ?

@aantono
Copy link
Contributor Author

aantono commented Mar 29, 2018

@emilevauge Not having a GUID is already possible today, as one can use the --cluster.node=anything as a config, and in that case the Node identifier will not be a GUID, but whatever the supplied value is anything. So this is just a convenience instead of having to externally derive the hostname and pass it as a value. But I can do that today already via doing --cluster.node=${HOSTNAME} and it works. :)

@emilevauge
Copy link
Member

emilevauge commented Mar 30, 2018

I got it, but we prefer to set this value in a specific cluster.hostname field then, and not reuse cluster.name :) OK for you ?

@aantono aantono changed the base branch from master to v1.6 March 30, 2018 16:00
@aantono
Copy link
Contributor Author

aantono commented Mar 30, 2018

@emilevauge As per our discussion in Slack, I've removed the part about UseHostname, so the cluster.node will have to be set externally or via config file if desired.

@mmatur
Copy link
Member

mmatur commented Apr 9, 2018

Hi @aantono, could you please explain us why you have rebase your PR on branch v1.6 ?

@aantono
Copy link
Contributor Author

aantono commented Apr 9, 2018

That was done per @ldez request to be merged into 1.6 release. Is that no longer needed?

@ldez
Copy link
Contributor

ldez commented Apr 9, 2018

@aantono Are you sure? Where am I asking you this? Are you sure you do not confuse with another PR?

@aantono
Copy link
Contributor Author

aantono commented Apr 9, 2018

Hm... I thought we talked about it on Slack... were we talking about a different PR? Should I rebase this against master then?

@aantono aantono changed the base branch from v1.6 to master April 9, 2018 19:45
@ldez ldez added this to the 1.7 milestone Apr 10, 2018
Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants