Skip to content

Add zenPings(ImmutableList) again #9099

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

Closed
wants to merge 2 commits into from

Conversation

dadoonet
Copy link
Member

Commit 95c2d84 removed zenPings(ImmutableList<? extends ZenPing> pings) which actually used by discovery plugins.
For example here: https://github.com/elasticsearch/elasticsearch-cloud-aws/blob/master/src/main/java/org/elasticsearch/discovery/ec2/Ec2Discovery.java#L68

This PR add it back.

Commit 95c2d84 removed `zenPings(ImmutableList<? extends ZenPing> pings)` which actually used by discovery plugins.
For example here: https://github.com/elasticsearch/elasticsearch-cloud-aws/blob/master/src/main/java/org/elasticsearch/discovery/ec2/Ec2Discovery.java#L68

This PR add it back.
@@ -68,6 +68,22 @@ public ZenPingService(Settings settings, ThreadPool threadPool, TransportService
return this.zenPings;
}

/**
* This method could be used by discovery plugins
Copy link
Member

Choose a reason for hiding this comment

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

can we explain why it may be useful for discovery plugins? This missing bit is exactly why it got removed in the first place :)

I also wonder if we can find a different way to achieve the same result, avoiding to expose a public method that is used by plugins only. That said I don't know this part of es that much...

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, we are adding a HostsProvider to unicastZenPing. This hosts provider makes AWS/Azure/GCE API calls to get back the list of potential elasticsearch nodes.

See https://github.com/elasticsearch/elasticsearch-cloud-aws/blob/master/src/main/java/org/elasticsearch/discovery/ec2/Ec2Discovery.java#L64-71

Copy link
Member

Choose a reason for hiding this comment

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

By "can we explain" I meant that I would like to see this info in the comment, anything else is going to get lost. Even if I understand it now and I look at the link you provided, this is not going to be useful the next time we wonder why this method is there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I understood your comment as a suggestion to enhance my Java comment. I agree.

I was answering actually to the last part of your comment on how we could do something different in core. :)

@dadoonet
Copy link
Member Author

dadoonet commented Jan 2, 2015

@javanna PR updated. Let me know.

@imotov
Copy link
Contributor

imotov commented Jan 2, 2015

@dadoonet, wouldn't it be easier to register this unicast provider in Ec2DiscoveryModule constructor by calling addUnicastHostProvider method of ZenDiscoveryModule instead of fishing for UnicastZenPing? It seems a bit cleaner and simpler to me. What do you think?

@dadoonet
Copy link
Member Author

dadoonet commented Jan 2, 2015

@imotov Agreed. Something like this? https://github.com/dadoonet/elasticsearch-cloud-aws/compare/pr/ec2-host-provider

I think this should work fine.

@imotov
Copy link
Contributor

imotov commented Jan 2, 2015

@dadoonet I think adding it in bindDiscovery is a bit too late because by this time all unicast providers are already added to multi-binder. I think Ec2DiscoveryModule constructor might be a better place.

@dadoonet
Copy link
Member Author

dadoonet commented Jan 2, 2015

@imotov Closing this one in favor of elastic/elasticsearch-cloud-aws#160

@dadoonet dadoonet closed this Jan 2, 2015
@dadoonet dadoonet deleted the pr/zenpings branch January 2, 2015 23:32
@javanna
Copy link
Member

javanna commented Jan 5, 2015

Happy to see we found another way to do the same, without requiring to add this method back to core. Thanks @imotov and @dadoonet !

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

Successfully merging this pull request may close these issues.

3 participants