-
Couldn't load subscription status.
- Fork 9.1k
HADOOP-17324. Don't relocate org.bouncycastle in shaded client jars #2411
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
Conversation
|
cc @steveloughran @jojochuang @aajisaka - could you please take a look? thanks! |
|
I'm also looking at shading issues at the same time. Mvn verify is broken in trunk otherwise I would ask you to run... I'm not sure I understand the use case here. Is the MiniYARNCluster simply broken out of box? Or that Spark wants to test using MiniYARNCluster but substitue a custom version of BouncyCastle? Our client jar contains only one IT ITUseMiniCluster and it checks only hdfs and webhdfs clients. How about adding an IT for MiniYARNCluster? |
Yes I'd say it's broken out of box since 3.3.0: there are two issues, this one and HADOOP-17327. Spark doesn't use a custom version but since bouncycastle is relocated and not shaded, there is no way for Spark to introduce the dependency, so this never works ...
This is a good suggestion, thanks. I'll perhaps add it in HADOOP-17327 since without MiniYARNCluster won't work without both being resolved. |
|
makes sense, especially given bouncy-castle's export nature...it's a special module. |
| .build(); | ||
| cluster.waitActive(); | ||
|
|
||
| conf.set("yarn.scheduler.capacity.root.queues", "default"); |
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.
another issue: we currently don't bundle YARN test resources like capacity-scheduler.xml in hadoop-client-minicluster so this is a temporary workaround. I'll open another JIRA for this later.
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.
we leave out all of test/resources to stop log4j files, site configs etc getting onto the classpath of apps downstream -so making it impossible for them to choose their own options
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.
maybe we should include them for test-only purpose? I think hadoop-client-minicluster should always be test scope right?
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 may be test only, but the other test suite may have its own configs that it wants. If we put them in the test JAR, it becomes near impossible to change. People will hate us (more)
|
Thanks. I added a test for this. Note as mentioned previously, this will fail currently because of HADOOP-17327. |
|
🎊 +1 overall
This message was automatically generated. |
|
@jojochuang @steveloughran added a unit test for this - can you take another look? thanks. |
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.
looks good, just a few details of the test case to sort out
| import org.apache.hadoop.hdfs.web.WebHdfsTestUtil; | ||
| import org.apache.hadoop.hdfs.web.WebHdfsConstants; | ||
|
|
||
| import org.apache.hadoop.yarn.server.MiniYARNCluster; |
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.
should go straight after line 45 - no gap
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.
will fix. I was trying to follow the existing style though which separates imports based on package.
|
|
||
| import org.apache.hadoop.yarn.server.MiniYARNCluster; | ||
|
|
||
| import static org.junit.Assert.assertTrue; |
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.
unused, according to checkstyle
| .build(); | ||
| cluster.waitActive(); | ||
|
|
||
| conf.set("yarn.scheduler.capacity.root.queues", "default"); |
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.
we leave out all of test/resources to stop log4j files, site configs etc getting onto the classpath of apps downstream -so making it impossible for them to choose their own options
| if (cluster != null) { | ||
| cluster.close(); | ||
| } | ||
| if (yarnCluster != null) { |
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.
all services are closeable, so use IOUtil.cleanupWithLogger() & let it handle null checks and exceptions
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.
Nice. Didn't know this util method, but seems we can only use it with yarnCluster.
|
🎊 +1 overall
This message was automatically generated. |
|
committed to trunk. Is this needed on branch-3.3 too? |
|
Thanks @steveloughran ! yes please backport it to branch-3.3 as well. |
|
Thanks! Could you update the JIRA |
|
Sure @dongjoon-hyun - updated. I'll backport this to branch-3.3 also later. |
…2411) Contributed by Chao Sun.
|
seen this is merged in. @sunchao -you got a GPG key in KEYS yet? Be good to GPG sign your patches. Github does it for the web ui ones, but we have to do it ourselves on the cherrypicks |
|
@steveloughran you mean in https://dist.apache.org/repos/dist/release/hadoop/common/KEYS ? no I haven't done that yet. Thanks for the info, do you know any doc to walk through this? |
|
My two aliases for showing commit info; Now, if you go through history (e.g. branch-2.8) you'll come across a GPG key I had to revoke; the yubikey keygen turned out to be broken. Git doesn't really handle that well. |
|
Thanks @steveloughran , the commands look better than mine so I'll take them. I can sign my future cherry-picks. Question though: where should I add my public key? should it added into https://dist.apache.org/repos/dist/release/hadoop/common/KEYS? sorry not very familiar with this process. |
In HADOOP-15832 we excluded org.bouncycastle from the shaded client jars but still relocate them to
org.apache.hadoop.shaded. This breaks any downstream application that wants to useMiniYARNClusterfromhadoop-client-miniclustersince there is no way to bring its own org.bouncycastle test dependency.This fixes the issue by excluding org.bouncycastle from relocation.