-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Update dependencies #4313
Update dependencies #4313
Conversation
Curator should not be upgraded. 2.12.0 has a bug that affects Druid, see #4103 and #3837 (comment). |
Thanks for leaving a comment about Curator, I should have done that in #4103. For the other changes, let's run integration tests and test with hadoop as well (it is notoriously finicky about which versions of things get used). If those look good then I'm +1. |
If you don't have an easy way to test with hadoop, maybe someone else could help, perhaps @nishantmonu51 or @b-slim. We also have a hadoop-gauntlet test suite we use from time to time we could dig up if needed. |
yeah sure will check this out hopefully this week. |
@b-slim thank you! |
ec4ce86
to
e5fac2b
Compare
It passes integration tests in Travis CI. |
Filed #4372 |
IMO we need to make sure that this will not lock out all the hadoop user, it is default way to ingest batch data in druid as far as i know so i am not sure rushing it is a good idea. Can we set this back to 10.2 release timeline in order to run tests? CI tests does not cover hadoop indexing job. 👎 |
@b-slim 0.10.1 will be in RC for at least two weeks, and the first RC will go out optimistically the end of this week. Isn't it enough time to do testing? |
it will take us at least one week to roll this out at the best cases if i make it a number one goal so two weeks can be really tight, also would like to see so other hadoop users like yahoo folks @himanshug and @cheddar roll this out and see if there is any issue. Again i am not against upgrading but would like to release this after testing at scale. |
@b-slim, I can't see the sense in vetoing this patch. We would want you and yahoo (and others as well) to test 0.10.1 no matter what, regardless of whether it includes this patch or not. And I do think that when a Druid release is in the RC stage, it should be a high priority for committers to do what they can to put it through the paces and verify that the release will be solid. Otherwise we risk doing a bad release. If the patch causes issues with hadoop we'll revert it, just like we reverted a curator upgrade in the 0.10.0 cycle since it broke tranquility (#4103). |
@gianm yes we are working on testing the RCs no matter what. Am not vetoing the merge of this PR but i guess releasing it without proper testing is not a good idea either. |
@leventov i am currently testing a home made 10.0.1 RC + this PR. If cutting an RC with this PR will help testing it as part of the RC testing process then lets go for it. |
I'm running a build with this patch against our hadoop version test suite as well. |
<exclusions> | ||
<exclusion> | ||
<groupId>com.metamx</groupId> | ||
<artifactId>java-util</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.
We can't exclude this though. server-metrics
uses the metamx
origin of java-utl
, things in druid.io only use the io.druid
fork
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's to resolve conflict, I added this because some other library needs newer version of java-util than server-metrics depend on, maybe emitter or http-client
</dependency> | ||
<dependency> | ||
<groupId>org.codehaus.jackson</groupId> | ||
<artifactId>jackson-mapper-asl</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.
what are these brought in for?
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.
Specifying the version in dependencyManagement. There are conflicts in subprojects, where different dependencies want different version of this lib.
update on hadoop compatibility with this patch: I downgraded hadoop.compile.version and removed hadoop-aws dependency from druid-hdfs-storage to test indexing tasks against versions < 2.6.0 as a workaround suggested by @b-slim, the indexing tasks all succeeded, so I think this PR is good on that aspect. |
Great thanks @jon-wei LGTM |
@leventov @drcrallen
and fails with following error...
I wonder if this is related to aws sdk change. Has anyone faced this issue ? |
@himanshug internally we had to upgrade to aws-java-sdk 1.11 because of Spark incompatibility as far as I remember... and also to Jackson 2.6, that is not compatible with Hadoop. But we didn't have such problems with aws-java-sdk 1.11 |
@leventov I guess its working for you probably because you're setting up AWS credentials (as described in http://docs.aws.amazon.com/sdk-for-java/v1/developer-guide/credentials.html ) in your environment. |
@leventov I'm gonna try to do a build with |
@himanshug only integration tests that run in Travis, that has nothing to do with AWS S3. Also could try to revert 1.10.77 -> 1.10.56 |
@himanshug the line numbers in the stack trace you posted don't correspond to 1.10.77, but instead look more like 1.7.4. Perhaps that version is getting pulled in somehow for your build. Fwiw, the 0.10.1-rc1 tarball is bundled with:
So if you run out of the tarball, you should be getting 1.10.77 in the io.druid.guice.AWSModule code path. |
@gian here are the jars in my build machine...
ok, i see that error in the stacktrace comes from file ProfilesConfigFileParser which belongs to aws-java-sdk-1.7.4 which is downloaded as a transitive dependency from hadoop-aws-2.7.3 in hdfs-storage module. I believe it is coming from hadoop version change in #4116 . |
@gianm @leventov continuing the thread in #4116 (comment) . |
@himanshug looks like a bug, aws-java-sdk should be excluded from hdfs-storage dependency |
Looks like we endup getting different SDKs and this due to https://github.com/druid-io/druid/pull/4313/files#diff-600376dffeb79835ede4a0b285078036L182. |
This dependency is used by HDFS storage when asked to load data from S3a. |
@b-slim exclusion means that version resolver won't consider 1.7.4. The project may depend on aws-java-sdk-s3:1.10.77 globally. If it is binary-compatible in the functionality that hdfs-storage uses. |
an other way to fix this will be to make profile loading lazy initialized. What you guys think ? |
@b-slim that might work for the problem i posted. but then its not great to have two different jars in the system that provide same functionality or conflict with each other. my understanding is that aws-java-sdk and aws-java-sdk-ec2 are in that boat . are they ? |
@himanshug i have tested it with 1.10.56 against an internal 2.7.3 (patched that is more like 2.8 ish) and it worked, i guess we can go that route of having one version at the top. |
@b-slim ok, let me also test whether that solves the original problem i posted. will update. |
@b-slim if you know that would fix original problem too, then go ahead and send the PR. |
i see you have already this himanshug@9632b0c |
Updated many, but not all library dependencies, mainly updated infrastructure dependencies and skipped testing and logging dependencies.
3.10.4.Final
->3.10.6.Final
inkafka-indexing-service
4.1.6.Final
->4.1.11.Final
in other modules3.4.9
->3.4.10
Apache Curator2.11.0
->2.12.0
9.3.16.v20170120
->9.3.19.v20170502
1.19
->1.19.3
(see also Move from com.sun.jersey to org.glassfish.jersey #4310)com.metamx:emitter:0.4.1
->0.4.5
com.metamx:http-client:1.0.6
->1.1.0
commons-io:commons-io:2.4
->2.5
com.amazonaws:aws-java-sdk-ec2:1.10.56
->1.11.132
1.10.77
, update to 1.11+ is not possible because Druid is stuck on Jackson 2.4. Also depend just onaws-java-sdk-ec2
instead ofaws-java-sdk
, fixing Depend only on needed parts of aws-java-sdk #4382.com.ibm.icu:icu4j:4.8.1
->54.1.1
joda-time:joda-time:2.8.2
->2.9.9
com.lmax:disruptor:3.3.0
->3.3.6
net.spy:spymemcached:2.11.7
->2.12.3
org.apache.httpcomponents:httpclient:4.5.1
->4.5.3
org.apache.httpcomponents:httpcore:4.4.3
->4.4.6
it.unimi.dsi:fastutil:7.0.13
->7.2.0
Also reduced dependency conflicts.
Updated patch or minor dependency versions, in order not to have to change source code, and better compatibility in patch release 0.10.1. However still labelling
Design Review
because of potential compatibility concerns.