-
Notifications
You must be signed in to change notification settings - Fork 9.1k
YARN-11881. Use hbase-shaded-client-byo-hadoop in hadoop-yarn-server-timelineservice-hbase #8046
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
base: trunk
Are you sure you want to change the base?
Conversation
|
Testing on pseudo-distributed cluster not yet done. |
|
💔 -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.
there's a mix of shaded and (provided) unshaded dependencies, plus some exclusions.
Is this for testing?
If so we should explain this in the commit message and the jira text.
Other than that, LGTM
+1
| <dependency> | ||
| <groupId>org.apache.hbase</groupId> | ||
| <artifactId>hbase-common</artifactId> | ||
| <scope>provided</scope> |
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.
do we need this at all, given the shaded one is there? Is it testing related?
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.
No, this is a server side component, and needs the hbase-server to compile.
The shaded one is not there, as we're excluding it form the dependencies to avoid conflicts.
|
|
||
| <dependency> | ||
| <groupId>org.apache.hbase</groupId> | ||
| <artifactId>hbase-client</artifactId> |
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.
same comment -seems superflous.
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.
Same reply, we don't want to mix the shaded and unshaded jars, so we use the unshaded version of everything.
|
This uses the shaded Hbase client for the hbase client side modules: However, we need the unshaded hbase-server JAR for compiling the Hbase coprocessor in hadoop-yarn-server-timelineservice-hbase-server-2, so we use the unshaded Hbase jars there. For simlipicity's sake (and beacuse I'm not sure we do otherwise) we're still using the unshaded hbase jars for testing, as the protobuf 2.5 dependency is not a problem there. Hbase does provide an hbase-shaded-testing-util package, but I am not sure that works with custom coprocessors, and I didn't attempt using that. Mixing the unshaded hbase jars with the shaded client is a bad idea, so we exclude the shaded client when building the coprocessor and in the tests. |
…timelineservice-hbase Replaces the unshaded maven dpendencies with hbase-shaded-client-byo-hadoop in hadoop-yarn-server-timelineservice-hbase-common and hadoop-yarn-server-timelineservice-hbase-client. Building the HBase coprocessor requires the hbase-server dependency, so we keep using the unshaded dependencies for hadoop-yarn-server-timelineservice-hbase-server-2. We're also using the unshaded HBase dependencies in hadoop-yarn-server-timelineservice-hbase-tests because that does not affect the production classpath, and I'm not sure we can even test coprocessors with hbase-shaded-testing-util.
|
I have added the explanation to the commit message. |
|
Only hadoop-yarn-server-timelineservice-hbase-common, hadoop-yarn-server-timelineservice-hbase-client are included in YARN classpath. They are also only using the HBase client API, so we can use hbase-shaded-client... with them without problems. hadoop-yarn-server-timelineservice-hbase-server-2 only needs to be present on the HBase classpath and it is packaged without dependencies, so using the unshaded dependencies there is not a problem. As discussed in YARN-11882, hadoop-yarn-server-timelineservice-hbase-server-2 should not even be added to the Yarn classpath. It can not work outside the HBase server environment, it is dead code in the Yarn processes. |
|
💔 -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.
Makes sense
- we want the shaded stuff on the classpath of all clients
- we want to build stuff to run in the server process against unshaded hbase
- we want to test with unshaded hbase
- we don't want to mix
+1, once you switch it from draft to "ready to review"
Description of PR
Use hbase-shaded-client-byo-hadoop in hadoop-yarn-server-timelineservice-hbase
How was this patch tested?
Test suite and testing on pesudodistributed cluster.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?