search

elastic / elasticsearch-cloud-aws

AWS Cloud Plugin for Elasticsearch
https://github.com/elastic/elasticsearch/tree/master/plugins/discovery-ec2
577 stars 181 forks source link

Move AwsEc2UnicastHostsProvider to Ec2DiscoveryModule #160

Closed dadoonet closed 9 years ago

dadoonet commented 9 years ago

Related to https://github.com/elasticsearch/elasticsearch/pull/9099

dadoonet commented 9 years ago

@imotov Thanks for your comments. Wanna review this?

imotov commented 9 years ago

@dadoonet The change LGTM but is there a simple way to add a test for it?

dadoonet commented 9 years ago

Thanks Igor. I have Ec2DiscoveryITest which simply tries to start a node and make sure that it does not fail. May be I should mock AWS API as I did for Azure?

My plan was actually to test this manually: create 3 ec2 nodes with the SNAPSHOT version of the plugin and check that discovery still works fine.

Any other suggestion?

dadoonet commented 9 years ago

BTW I just tested it on Azure project which has more tests and it seems to work fine so far.

imotov commented 9 years ago

In order to test if it injects any unicast hosts we have to have some instances running, which doesn't seem practical. Mocking it would be a good idea, but it would rather test elasticsearch than the plugin. So, I have no other suggestions, unfortunately.

dadoonet commented 9 years ago

@imotov FYI I added a new commit as injecting AmazonEC2 was not possible (it's not an elasticsearch aws object but a pure AWS SDK object - not instantiated by guice that is). Going to run some tests now on real instances.

dadoonet commented 9 years ago

@imotov I needed also to add some changes in pom.xml and plugin.xml.

Tested on AWS platform with elasticsearch version[2.0.0-SNAPSHOT], pid[26010], build[69f74d7/2015-01-12T18:29:44Z] and this patch. 2 nodes cluster. Discovery went well.

Could you check one more time (last :) ) so I can squash the commits and push the change? Thanks!

imotov commented 9 years ago

LGTM

dadoonet commented 9 years ago

@imotov Actually, I think there is something wrong with this change. We were removing Multicast before the change and we don't remove it anymore. I think we need to fix that. Sadly, it will require a change in elasticsearch core as we don't have access anymore to some of the methods we were using.