-
Notifications
You must be signed in to change notification settings - Fork 994
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-6349 Add and use commons-cli to phoenix-thirdparty #1123
Conversation
💔 -1 overall
This message was automatically generated. |
97c3b7a
to
b77482d
Compare
💔 -1 overall
This message was automatically generated. |
Huh, I'm not sure if this is my local setting issue or something else. With my local HBase setup, I tried 4.14.3 and 4.15 server jars without any issues when I called ./hbase org.apache.phoenix.mapreduce.index.IndexTool @stoty did you verify it locally? I guess the ITs are not using the server patch so it's very hard to catch the issue... |
b77482d
to
defc12f
Compare
No I didn't test it, I didn't have the time set up a 4.x pseudo-distributed cluster yesterday. I just keep forgetting that you need to add everything to the 4.x server jar manually Can you re-test with the updated patch @yanxinyi ? |
Huh, I'm getting the same error. When I compared the 4.x and master server jars, the master branch server jar does have dependent classes but not 4.x. Here is the list |
This is a test version that depends on phoenix-thirdparty 1.1.0-SNAPSHOT
defc12f
to
f118ac7
Compare
The difference is normal. In the 4.x server jar we add the shaded packages manually, and do not relocate. (Don't know why, probably historical reasons) In 4.x HEAD we don't add commons-cli, and use whatever HBase/Hadoop classpatch gives us. In master HEAD we add and shade every dependecy that isn't excluded and provided With this (updated) patch we add the org.apache.phoenix.thirdparty.org.apache.commons.cli classes on either branch. I've just run a test locally on the Phoenix 5 pseudodistributed cluster with this patch, and didn't get any errors. I am quite baffled why the same setup would cause problems on 4.x |
I found the problem. Can you try yet again on 4.x with the now-really-fixed-i-swear patch @yanxinyi ? |
Sure @stoty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works!!!
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 (non-binding), only pending change is dropping -SNAPSHOT after release
💔 -1 overall
This message was automatically generated. |
test version snapshot phoenix-thirdparty