Attempt at adding GCE support for cluster joining configuration #2313

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
8 participants
@pires

pires commented Apr 17, 2014

Implemented jclouds-based mechanism for discovering potential nodes for a cluster.
Replaced AWS join configuration with aforementioned mechanism.
Implemented Google Compute Engine support, as well.
Refs #2312

Implemented jclouds-based mechanism for discovering potential nodes f…
…or a cluster.

Replaced AWS join configuration with aforementioned mechanism.
Implemented Google Compute Engine support, as well.
Refs #2312
@pveentjer

This comment has been minimized.

Show comment
Hide comment
@pveentjer

pveentjer Apr 17, 2014

Member

verify

Member

pveentjer commented Apr 17, 2014

verify

@pires

This comment has been minimized.

Show comment
Hide comment
@pires

pires Apr 17, 2014

@pveentjer a couple tests will break because I changed the XML schema.
Tried to get a word with @noctarius, but it seems he's sleeping already ;-)
What would you advise?

pires commented Apr 17, 2014

@pveentjer a couple tests will break because I changed the XML schema.
Tried to get a word with @noctarius, but it seems he's sleeping already ;-)
What would you advise?

checkstyle/suppressions.xml
@@ -28,7 +28,7 @@
<suppress checks="" files="com.hazelcast.jca[\\/]"/>
<suppress checks="" files="com.hazelcast.spring[\\/]"/>
<suppress checks="" files="com.hazelcast.web[\\/]"/>
- <suppress checks="" files="com.hazelcast.aws[\\/]"/>
+ <suppress checks="" files="com.hazelcast.cloud[\\/]"/>

This comment has been minimized.

@noctarius

noctarius Apr 18, 2014

Contributor

New modules shouldn't have checkstyle suppression :) So just remove the line and fix the checkstyle problems :)

@noctarius

noctarius Apr 18, 2014

Contributor

New modules shouldn't have checkstyle suppression :) So just remove the line and fix the checkstyle problems :)

This comment has been minimized.

@pires

pires Apr 18, 2014

Will do.

@noctarius

This comment has been minimized.

Show comment
Hide comment
@noctarius

noctarius Apr 18, 2014

Contributor

Does it support client side discovery?

Contributor

noctarius commented Apr 18, 2014

Does it support client side discovery?

@pires

This comment has been minimized.

Show comment
Hide comment
@pires

pires Apr 18, 2014

As far as I'm concerned, this PR only supports the TCP-IP cluster joining feature. But if you open a new issue, I might try and contribute that as well during the next week!

pires commented Apr 18, 2014

As far as I'm concerned, this PR only supports the TCP-IP cluster joining feature. But if you open a new issue, I might try and contribute that as well during the next week!

@pires

This comment has been minimized.

Show comment
Hide comment
@pires

pires Apr 23, 2014

Is there anything else I can do to see this merged?

pires commented Apr 23, 2014

Is there anything else I can do to see this merged?

+ throw new IllegalArgumentException("CloudConfig is required!");
+ }
+ if (config.getProvider() == null) {
+ if (!config.getProvider().contains("aws-ec2")

This comment has been minimized.

@pveentjer

pveentjer Apr 30, 2014

Member

Why should we limit ourselves to only these two?

@pveentjer

pveentjer Apr 30, 2014

Member

Why should we limit ourselves to only these two?

This comment has been minimized.

@pires

pires May 5, 2014

It was the only two I am sure support key-value user metadata.

Ideally, we can support anything jclouds supports already. Still, I didn't feel comfortable assuming my proposal was enough to support other providers.

@pires

pires May 5, 2014

It was the only two I am sure support key-value user metadata.

Ideally, we can support anything jclouds supports already. Still, I didn't feel comfortable assuming my proposal was enough to support other providers.

@@ -0,0 +1,1027 @@
+<?xml version="1.0" encoding="UTF-8"?>

This comment has been minimized.

@pveentjer

pveentjer Apr 30, 2014

Member

How come this big file is new? What happened to the old one?

@pveentjer

pveentjer Apr 30, 2014

Member

How come this big file is new? What happened to the old one?

This comment has been minimized.

@pires

pires May 5, 2014

Probably some formatting issues. Damn :-/

@pires

pires May 5, 2014

Probably some formatting issues. Damn :-/

+ <xs:attribute name="multicast-time-to-live" type="xs:string" use="optional" default="32"/>
+ </xs:complexType>
+
+ <xs:complexType name="cloud">

This comment has been minimized.

@pveentjer

pveentjer Apr 30, 2014

Member

We need to be backwards compatible. So 'm fine with having a cloud section with more options, but we also need to provide the aws one. We can deprecate it and remove it in the future.

@pveentjer

pveentjer Apr 30, 2014

Member

We need to be backwards compatible. So 'm fine with having a cloud section with more options, but we also need to provide the aws one. We can deprecate it and remove it in the future.

This comment has been minimized.

@pires

pires May 5, 2014

I understand. Will try to find time to recover previous behavior.

@pires

pires May 5, 2014

I understand. Will try to find time to recover previous behavior.

*/
-public class AwsConfig {
+public class CloudConfig {

This comment has been minimized.

@pveentjer

pveentjer Apr 30, 2014

Member

Unfortunately we can't rename the aws config. We need to add a parallel config cloudconfig, which will be a big copy and paste from aws config. But we can't rename else we would not be backwards compatible.

@pveentjer

pveentjer Apr 30, 2014

Member

Unfortunately we can't rename the aws config. We need to add a parallel config cloudconfig, which will be a big copy and paste from aws config. But we can't rename else we would not be backwards compatible.

This comment has been minimized.

@pires

pires May 5, 2014

I understand. Will try to find time to recover previous behavior.

@pires

pires May 5, 2014

I understand. Will try to find time to recover previous behavior.

@pveentjer

This comment has been minimized.

Show comment
Hide comment
@pveentjer

pveentjer Apr 30, 2014

Member

I like your work, but a few things need to be done to get this merged.

  1. code isn't backwards compatible. So you are only allowed to add stuff to the public api, not change or remove.

  2. I don't see any integration tests. With jclouds we can easily start up cluster members and see if this whole cloud discovery thing works. Currently there is no test at all, but the implementation works in practice. But new stuff needs to be implemented

  3. there are some new files, but afaik they are just old files with content added (e.g. the spring xsd). So I guess something went wrong in your ide/git. The problem is that we loose history and I can't see the change you made

Member

pveentjer commented Apr 30, 2014

I like your work, but a few things need to be done to get this merged.

  1. code isn't backwards compatible. So you are only allowed to add stuff to the public api, not change or remove.

  2. I don't see any integration tests. With jclouds we can easily start up cluster members and see if this whole cloud discovery thing works. Currently there is no test at all, but the implementation works in practice. But new stuff needs to be implemented

  3. there are some new files, but afaik they are just old files with content added (e.g. the spring xsd). So I guess something went wrong in your ide/git. The problem is that we loose history and I can't see the change you made

@pires

This comment has been minimized.

Show comment
Hide comment
@pires

pires May 5, 2014

Thanks @pveentjer.

  1. I understand. Will try to find time to recover previous behavior.

  2. I gave some thought on how to achieve this, but I'm clueless. How can I do it without proper access to cloud API? Could you help on this as soon as I'm done with 1?

  3. Some XSD and XML files were changed so that schema validation would support the <cloud> tag. Now, it seems my IDE messed up the formatting and I didn't catch it when committing stuff. My bad. See 1.

pires commented May 5, 2014

Thanks @pveentjer.

  1. I understand. Will try to find time to recover previous behavior.

  2. I gave some thought on how to achieve this, but I'm clueless. How can I do it without proper access to cloud API? Could you help on this as soon as I'm done with 1?

  3. Some XSD and XML files were changed so that schema validation would support the <cloud> tag. Now, it seems my IDE messed up the formatting and I didn't catch it when committing stuff. My bad. See 1.

@pveentjer

This comment has been minimized.

Show comment
Hide comment
@pveentjer

pveentjer May 21, 2014

Member

Hi Pires,

sorry for the long delay. Lets have a talk how we can continue. I really would like to have your work in the system. But we need to make sure that we don't break any old code.

Member

pveentjer commented May 21, 2014

Hi Pires,

sorry for the long delay. Lets have a talk how we can continue. I really would like to have your work in the system. But we need to make sure that we don't break any old code.

@pires

This comment has been minimized.

Show comment
Hide comment
@pires

pires May 23, 2014

Hi @pveentjer. Let's talk then! Ask @noctarius for my googletalk/hangouts ID.

pires commented May 23, 2014

Hi @pveentjer. Let's talk then! Ask @noctarius for my googletalk/hangouts ID.

@pires

This comment has been minimized.

Show comment
Hide comment
@pires

pires Jun 30, 2014

@pveentjer would you be happy if I worked to fix this PR and add a new one for integration with #2485?

pires commented Jun 30, 2014

@pveentjer would you be happy if I worked to fix this PR and add a new one for integration with #2485?

@pveentjer

This comment has been minimized.

Show comment
Hide comment
@pveentjer

pveentjer Aug 19, 2014

Member

I'm going to close this PR for the time being. We are in bugfixing only mode for the 3.3 release. As soon as 3.3 is released, we'll reopen this one. If we don't, can you reopen it? More cloud support is good... so I would like to see functionality like you provided in HZ.

Member

pveentjer commented Aug 19, 2014

I'm going to close this PR for the time being. We are in bugfixing only mode for the 3.3 release. As soon as 3.3 is released, we'll reopen this one. If we don't, can you reopen it? More cloud support is good... so I would like to see functionality like you provided in HZ.

@pveentjer pveentjer closed this Aug 19, 2014

@pveentjer pveentjer added this to the 3.3.1 milestone Aug 19, 2014

@sancar

This comment has been minimized.

Show comment
Hide comment
@sancar

sancar Aug 19, 2014

Member

Closed PRs are very likely to lost. I think we should leave it open and just set the milestone.

Member

sancar commented Aug 19, 2014

Closed PRs are very likely to lost. I think we should leave it open and just set the milestone.

@sancar sancar reopened this Aug 19, 2014

@devOpsHazelcast

This comment has been minimized.

Show comment
Hide comment
@devOpsHazelcast

devOpsHazelcast Aug 19, 2014

Contributor

verify

Contributor

devOpsHazelcast commented Aug 19, 2014

verify

@pveentjer pveentjer closed this Aug 19, 2014

@pveentjer

This comment has been minimized.

Show comment
Hide comment
@pveentjer

pveentjer Aug 19, 2014

Member

Mehmet and me have been asked by Greg to keep the PR list as short as possible. Things which are not going to be planned for the 3.3 release are going to be closed (unless it is 3 maintenance or 2 maintenance). Please contact me next time before reopening.

The issue has been tagged with 3.3.1, so we won't loose it.

Member

pveentjer commented Aug 19, 2014

Mehmet and me have been asked by Greg to keep the PR list as short as possible. Things which are not going to be planned for the 3.3 release are going to be closed (unless it is 3 maintenance or 2 maintenance). Please contact me next time before reopening.

The issue has been tagged with 3.3.1, so we won't loose it.

@mdogan

This comment has been minimized.

Show comment
Hide comment
@mdogan

mdogan Aug 19, 2014

Member

I don't remember Greg said us to close pull requests "which are not going to be planned for the 3.3 release". Yes, he said us to handle PRs and current list is too long. But personally I think setting milestone to later release and parking PR as open is better choice. Closing makes them invisible, they'll be forgotten.

Member

mdogan commented Aug 19, 2014

I don't remember Greg said us to close pull requests "which are not going to be planned for the 3.3 release". Yes, he said us to handle PRs and current list is too long. But personally I think setting milestone to later release and parking PR as open is better choice. Closing makes them invisible, they'll be forgotten.

@sancar

This comment has been minimized.

Show comment
Hide comment
@sancar

sancar Aug 19, 2014

Member

I understand that we can look at the closed issues with milestones. But it is one more thing to keep in mind and we can forget it.
And I think it is not nice to close a PR send by someone outside of company just to keep PR list clean. this guy worked and send a PR . I think it demotivates the community. If we close the pull requests they will probably not follow the PR. I mean, they don't know the release dates, how they will track and reopen the ticket.

Member

sancar commented Aug 19, 2014

I understand that we can look at the closed issues with milestones. But it is one more thing to keep in mind and we can forget it.
And I think it is not nice to close a PR send by someone outside of company just to keep PR list clean. this guy worked and send a PR . I think it demotivates the community. If we close the pull requests they will probably not follow the PR. I mean, they don't know the release dates, how they will track and reopen the ticket.

@pveentjer

This comment has been minimized.

Show comment
Hide comment
@pveentjer

pveentjer Aug 19, 2014

Member

Ok. Good comments; you have valid points.

Personally I do think we should be more aggressive with closing them. So either get them merged or reject them. Else we end up with a long list of things lingering.

Member

pveentjer commented Aug 19, 2014

Ok. Good comments; you have valid points.

Personally I do think we should be more aggressive with closing them. So either get them merged or reject them. Else we end up with a long list of things lingering.

@pveentjer pveentjer reopened this Aug 19, 2014

@devOpsHazelcast

This comment has been minimized.

Show comment
Hide comment
@devOpsHazelcast

devOpsHazelcast Aug 19, 2014

Contributor

verify

Contributor

devOpsHazelcast commented Aug 19, 2014

verify

@sancar

This comment has been minimized.

Show comment
Hide comment
@sancar

sancar Aug 19, 2014

Member

How about the rest of the closed ones today ? Are you going to reopen them too? I think we should.

Member

sancar commented Aug 19, 2014

How about the rest of the closed ones today ? Are you going to reopen them too? I think we should.

@mdogan mdogan removed this from the 3.3.1 milestone Sep 23, 2014

@pires

This comment has been minimized.

Show comment
Hide comment
@pires

pires Jan 27, 2015

@sancar you bet it demotivated me. Actually, it's sad that they've closed something that would work but kept #2485 open, which no one seems to care about.

I'm still interested in having this merged, and add discovery support for Kubernetes as well, but I want to be sure it's not going to be closed once again just because.

pires commented Jan 27, 2015

@sancar you bet it demotivated me. Actually, it's sad that they've closed something that would work but kept #2485 open, which no one seems to care about.

I'm still interested in having this merged, and add discovery support for Kubernetes as well, but I want to be sure it's not going to be closed once again just because.

@mesutcelik

This comment has been minimized.

Show comment
Hide comment
@mesutcelik

mesutcelik Jan 27, 2015

Contributor

Hi @pires ,

I am going to take care of it. No worries...

More cloud support is definitely in our radar but it is a matter of planning... This is not in our 3.5 release roadmap to include JClouds support but we will check what we can do for 3.6

keep in touch 👍

Contributor

mesutcelik commented Jan 27, 2015

Hi @pires ,

I am going to take care of it. No worries...

More cloud support is definitely in our radar but it is a matter of planning... This is not in our 3.5 release roadmap to include JClouds support but we will check what we can do for 3.6

keep in touch 👍

@pires

This comment has been minimized.

Show comment
Hide comment
@pires

pires Jan 27, 2015

@mesutcelik so just to be sure, I can forget about having this sorted and merged in favor of an in-house implementation?

pires commented Jan 27, 2015

@mesutcelik so just to be sure, I can forget about having this sorted and merged in favor of an in-house implementation?

@pires pires closed this Jan 27, 2015

@saturnism

This comment has been minimized.

Show comment
Hide comment
@saturnism

saturnism Feb 25, 2015

quick q on the status of this? it seems stale for a while..

quick q on the status of this? it seems stale for a while..

@noctarius

This comment has been minimized.

Show comment
Hide comment
@noctarius

noctarius Feb 26, 2015

Contributor

Hi @pires ,

We thought about how to handle cloud discovery. We just started an Hazelcast Incubator system. Since we want to make adding new cloud providers easier we want to have a clean SPI to integrate those. I added you to the new repo and I would love to see you help on the SPI design.

Sorry for closing this PR for now but I hope you understand the reasoning. Please do not yet delete the branch on your side, we will come back to the integration for the new SPI!

Chris

Contributor

noctarius commented Feb 26, 2015

Hi @pires ,

We thought about how to handle cloud discovery. We just started an Hazelcast Incubator system. Since we want to make adding new cloud providers easier we want to have a clean SPI to integrate those. I added you to the new repo and I would love to see you help on the SPI design.

Sorry for closing this PR for now but I hope you understand the reasoning. Please do not yet delete the branch on your side, we will come back to the integration for the new SPI!

Chris

@pires

This comment has been minimized.

Show comment
Hide comment
@pires

pires Feb 26, 2015

Makes sense. It would probably be a good call to add @saturnism as well, since he implemented Kubernetes discovery.

Thanks

pires commented Feb 26, 2015

Makes sense. It would probably be a good call to add @saturnism as well, since he implemented Kubernetes discovery.

Thanks

@noctarius

This comment has been minimized.

Show comment
Hide comment
@noctarius

noctarius Feb 26, 2015

Contributor

I already did :) See twitter as well! :D

Contributor

noctarius commented Feb 26, 2015

I already did :) See twitter as well! :D

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