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

Add discovery SPI implementation for AWS #21

Merged
merged 1 commit into from Mar 22, 2017
Merged

Add discovery SPI implementation for AWS #21

merged 1 commit into from Mar 22, 2017

Conversation

mmedenjak
Copy link
Contributor

Previously the AWS module supported only discovering members through the TcpIpJoiner mechanism. This was configured through the AwsConfig class. This mechanism is still available for backwards compatibility and here we will add support for the discovery SPI. By doing so, the user can now configure AWS using the old AwsConfig or the new discovery config and usage in Hazelcast core is simplified for future functionalities based on discovery SPI.

Currently the discovery SPI implementation will transform the discovery configuration into the old AwsConfig and reuse the existing code. In the future both the deprecated TcpIpJoiner and the AwsConfig can be removed.

/**
* Access key of your account on EC2
*/
AccessKey("access-key", STRING, false),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer enum constants to be in capital letters.

Copy link
Contributor

@emrahkocaman emrahkocaman left a comment

Choose a reason for hiding this comment

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

I only have minor comment about enum namings, other than that nice job @mmedenjak

Previously the AWS module supported only discovering members through the TcpIpJoiner mechanism. This was configured through the AwsConfig class. This mechanism is still available for backwards compatibility and here we will add support for the discovery SPI. By doing so, the user can now configure AWS using the old AwsConfig or the new discovery config and usage in Hazelcast core is simplified for future functionalities based on discovery SPI.

Currently the discovery SPI implementation will transform the discovery configuration into the old AwsConfig and reuse the existing code. In the future both the deprecated TcpIpJoiner and the AwsConfig can be removed.
@devOpsHazelcast
Copy link
Contributor

Test PASSed.

Copy link
Contributor

@emrahkocaman emrahkocaman left a comment

Choose a reason for hiding this comment

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

👍

@mesutcelik
Copy link
Contributor

Are we gonna send a cleanup PR to Hazelcast repository?

@mmedenjak
Copy link
Contributor Author

@mesutcelik I am not sure about the release cycles of this plugin and IMDG. When is this plugin going to be released?
If we change/cleanup the IMDG code and switch to using the discovery strategy then once a user upgrades IMDG to 3.9, he must upgrade the AWS plugin as well.
On the other hand, the user must upgrade the plugin if he is going to use WAN with EC2 discovery.

WDYT?

@mesutcelik
Copy link
Contributor

mesutcelik commented Mar 21, 2017

We can't maintain two implementation so cleanup should be applied to hazelcast 3.9.
We do release hazelcast-aws mostly monthly basis but this can be done sooner if needed.

Discovery SPI is major change. We will probably release current implementation as major release: v2.0 so supporting only hazelcast 3.6+ is fine because discovery SPI is introduced with hazelcast 3.6. If someone really needs to use pre-3.6 version of hazelcast, she can still use latest 1.x version from maven....

btw, hazelcast-aws is already supporting hazelcast 3.6+ only
https://github.com/hazelcast/hazelcast-aws#supported-hazelcast-versions

@mmedenjak
Copy link
Contributor Author

A cleanup is definitely in order but I would like to avoid breaking user code. It's great that this plugin is released this often, I was concerned that it would not be released in time for IMDG 3.9.
I just had this scenario in mind :

  • a user has IMDG 3.8 and AWS plugin 1.1.1
  • he upgrades IMDG to 3.9 which has cleaned up the code for AWS join mechanism but keeps the plugin 1.1.1
  • the IMDG fails to start because it cannot find the AWS discovery class

Which is why IMDG 3.9 would have to require AWS plugin 2.0 We can continue this on Slack, I'm merging this PR.

@mmedenjak mmedenjak merged commit d746d4c into hazelcast:master Mar 22, 2017
@mmedenjak mmedenjak deleted the aws-discovery branch March 22, 2017 11:39
@emrahkocaman emrahkocaman added this to the 1.1.2 milestone Apr 21, 2017
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.

None yet

5 participants