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

PHOENIX-6723 Update client to use Zookeeper 3.5.7 and Curator 4.2.0 #1434

Merged

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented May 6, 2022

Previous Zookeeper version was broken on JDK 14+ due to the incorrect usage of
InetSocketAddress.toString. For reference see: https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8232369

@wendigo
Copy link
Contributor Author

wendigo commented May 6, 2022

cc @stoty @vincentpoon

This is relevant for the https://github.com/trinodb/trino project as we want to move to JDK17 (which is the LTS)

Copy link
Member

@vincentpoon vincentpoon left a comment

Choose a reason for hiding this comment

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

+1 assuming tests pass

@stoty
Copy link
Contributor

stoty commented May 9, 2022

Please open a JIRA, and add JIRA-XXXXX to the commit message, @wendigo.

@stoty
Copy link
Contributor

stoty commented May 9, 2022

The ZK version doesn't do anything for ZK on master, we're already using 3.5.7 for both 2.3 and 2.4. (set in the hbase profiles)

Please remove the ZK version overrides from the hbase profiles in the pom, as they are not needed with this change.

As for Curator, HBase uses 4.2, not 4.3 on branch 2.3, 2.4, and 2.5. Is there a particular reason for using 4.3, or did you just pick the latest 4.3 release ?

In case you also want this merged into 5.1, we'd also have to check if HBase 2.1 and 2.2 works with ZK 3.5.7 (IIRC it does)

pom.xml Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@gjacoby126
Copy link
Contributor

@vincentpoon @wendigo - I think this PR is currently waiting for changes that have been requested by @stoty and then I think would be ready for commit. Do you have bandwidth to get back to this in time for the Phoenix 5.2 release (probably in the next month or so?)

@wendigo
Copy link
Contributor Author

wendigo commented Jun 2, 2022

@gjacoby126 I will update this PR soon

@wendigo wendigo force-pushed the serafin/bump-zookeeper-dependency branch from c0cf420 to 4e11281 Compare June 2, 2022 20:08
@wendigo wendigo requested review from vincentpoon and stoty June 2, 2022 20:44
@wendigo
Copy link
Contributor Author

wendigo commented Jun 2, 2022

@stoty @gjacoby126 PR updated. I tested it with patched Trino version, and now it works correctly on JDK 17 with HBase 2.3.0

@gjacoby126
Copy link
Contributor

@wendigo - the changes to the pom look good, but could you please change the commit message to refer to a JIRA number. (And if there isn't a JIRA for this yet, could you please create on on issues.apache.org.) Thanks!

@wendigo
Copy link
Contributor Author

wendigo commented Jun 3, 2022

@gjacoby126 yes, will do, I need to create jira account first.

@gjacoby126
Copy link
Contributor

@wendigo - if you're creating a JIRA account for the first time, please let us know the username so we can add you to the contributors list. (This will give you the ability to assign the ticket to yourself.)

Previous Zookeeper version was broken on JDK 14+ due to the incorrect usage of
InetSocketAddress.toString. For reference see: https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8232369
@wendigo wendigo force-pushed the serafin/bump-zookeeper-dependency branch from 4e11281 to 460491c Compare June 3, 2022 17:48
@wendigo wendigo changed the title Upgrade Zookeeper dependency to 3.5.7 and Curator to 4.3.0 PHOENIX-6723 Update client to use Zookeeper 3.5.7 and Curator 4.2.0 Jun 3, 2022
@wendigo
Copy link
Contributor Author

wendigo commented Jun 3, 2022

@gjacoby126 done, JIRA handle is the same as in GitHub (@wendigo). I've updated the commit message too.

@wendigo
Copy link
Contributor Author

wendigo commented Jun 3, 2022

Copy link
Contributor

@gjacoby126 gjacoby126 left a comment

Choose a reason for hiding this comment

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

+1, thanks @wendigo . @stoty , did you want any other changes to this patch before we commit it?

Copy link
Contributor

@stoty stoty left a comment

Choose a reason for hiding this comment

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

+1 LGTM.
Thank you.

@gjacoby126 gjacoby126 merged commit 7be4834 into apache:master Jun 7, 2022
@wendigo wendigo deleted the serafin/bump-zookeeper-dependency branch June 30, 2022 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants