Skip to content

Upgrade curator#4786

Merged
gianm merged 2 commits intoapache:masterfrom
jihoonson:upgrade-curator
Sep 15, 2017
Merged

Upgrade curator#4786
gianm merged 2 commits intoapache:masterfrom
jihoonson:upgrade-curator

Conversation

@jihoonson
Copy link
Copy Markdown
Contributor

@jihoonson jihoonson commented Sep 12, 2017

I talked with @gianm offline and raise this PR because he can't afford to work on #4752 right now.
The reason of the test failures is that curator-test 4.0 is not compatible with zookeeper 3.4.x. It looks to be recommended to use 2.12.0 instead (https://issues.apache.org/jira/browse/CURATOR-428).

Also, I excluded zookeeper dependency from curator-recipes to enable soft-compatibility mode (https://curator.apache.org/zk-compatibility.html).

The expected benefits are


This change is Reviewable

@drcrallen
Copy link
Copy Markdown
Contributor

Can you please add the benefits of a 4.0.0?

Taking on a .0 major release is a bit scary

@jihoonson
Copy link
Copy Markdown
Contributor Author

The benefits are described in the original PR (#4752). I updated the PR description.

@gianm gianm mentioned this pull request Sep 13, 2017
@gianm
Copy link
Copy Markdown
Contributor

gianm commented Sep 13, 2017

@jihoonson could you try merging master once again; #4787 should help with the CI failures.

@jihoonson
Copy link
Copy Markdown
Contributor Author

@gianm thanks. Merged master.

Copy link
Copy Markdown
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

Personally, I am +1 on this, although if any ZK related issues crop up in testing the 0.11 release candidate then we should be ready to roll it back.

I think it's worth the risk to put this out there for the first release candidate, since the update does fix real bugs that people have hit, and the risks at this point are theoretical.

@himanshug
Copy link
Copy Markdown
Contributor

@gianm @jihoonson sounds good to upgrade. it would be good to run this on a test cluster with some load for a day or so that any trivial issues surface immediately.

@drcrallen
Copy link
Copy Markdown
Contributor

@himanshug is that something you are equipped to test?

@himanshug
Copy link
Copy Markdown
Contributor

@drcrallen I would be fine to merge it if @jihoonson has done sanity verification on some cluster with multiple machines. once merged, we can update our metrics cluster to latest master and let it run there .. see if any problems spring up.

@jihoonson
Copy link
Copy Markdown
Contributor Author

@himanshug thanks. I'll let you know after testing on our cluster.

@jihoonson
Copy link
Copy Markdown
Contributor Author

@himanshug this patch works well on our cluster.

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Sep 15, 2017

I'll merge this, based on comments so far and testing from @jihoonson. If anyone notices any issues after more testing, that may be related, then please speak up.

@gianm gianm merged commit d606bd7 into apache:master Sep 15, 2017
@jon-wei jon-wei added this to the 0.11.0 milestone Oct 18, 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.

Another day, another Apache Curator bug Curator leader election breaks in the overlord when zookeeper has issues

5 participants