[SPARK-8015] [flume] Remove Guava dependency from flume-sink. #6555
Conversation
The minimal change would be to disable shading of Guava in the module, and rely on the transitive dependency from other libraries instead. But since Guava's use is so localized, I think it's better to just not use it instead, so I replaced that code and removed all traces of Guava from the module's build.
|
I verified that there are no references to Guava in the generated bytecode and that Guava does not show up in |
|
Test build #33905 has finished for PR 6555 at commit
|
| <artifactId>maven-shade-plugin</artifactId> | ||
| <configuration> | ||
| <relocations combine.self="override" /> | ||
| </configuration> |
There was a problem hiding this comment.
is it worth adding a short comment here explaining why this is necessary? maybe this is obvious to others more versed in maven, but it was a little mysterious to me. Then again we don't doc a lot of the intricacies of the build system. anyway, just a suggestion, I'll leave it up to your judgement.
|
jenkins, retest this please |
|
Test build #33907 has finished for PR 6555 at commit
|
|
+1. LGTM |
|
Test build #33908 has finished for PR 6555 at commit
|
|
Hmm... the build failure doesn't seem related to the changes, but I'll take another look. |
Not sure what's wrong, but running sbt with no profiles enabled causes
an error about not being able to resolve version "${thrift.version}" of
libthrift.
|
Test build #33909 has finished for PR 6555 at commit
|
|
Some of the test code's transitive dependencies seem to pull in |
|
Hmm. Wonder how those tests passed locally. Yeah, I'll add the dependency in test scope. |
|
Test build #33914 has finished for PR 6555 at commit
|
| Flume 1.4.0 depends on is "3.4.0.Final" . | ||
| --> | ||
| <groupId>com.google.guava</groupId> | ||
| <artifactId>guava</artifactId> |
There was a problem hiding this comment.
Why do you need to add this in test?
There was a problem hiding this comment.
I see, I missed the earlier conversation. But instead of randomly adding guava back for test, its better to remove the wrong version of guava that is coming in from somewhere, and allow Flume's guava dependency to be used. At least, it needs to be understood. Could you check the dependency tree to find the wrong guava version?
There was a problem hiding this comment.
There's no wrong version of Guava.
I added this because I removed Guava from compile-scope dependencies, to avoid having people use it in this module. Not because of a conflict.
If instead you'd rather keep it, but at the same version as flume (whatever that is), it could be done also. I kinda prefer avoiding it, though.
As I mention in the PR description, the minimal fix doesn't remove Guava at all, just removes the code to relocate it. I just thought it would be better to avoid another potential dependency mess here since it's not needed.
There was a problem hiding this comment.
Aah, I was misunderstanding the problem. It makes sense now. Can you add comments on why Guava and thrift has been excluded, explaining the problem and mentioning the JIRA.
|
Lgtm pending tests |
|
Test build #33994 has finished for PR 6555 at commit
|
|
LGTM. Merging this, in master and branch 1.4. |
The minimal change would be to disable shading of Guava in the module, and rely on the transitive dependency from other libraries instead. But since Guava's use is so localized, I think it's better to just not use it instead, so I replaced that code and removed all traces of Guava from the module's build. Author: Marcelo Vanzin <[email protected]> Closes #6555 from vanzin/SPARK-8015 and squashes the following commits: c0ceea8 [Marcelo Vanzin] Add comments about dependency management. c38228d [Marcelo Vanzin] Add guava dep in test scope. b7a0349 [Marcelo Vanzin] Add libthrift exclusion. 6e0942d [Marcelo Vanzin] Add comment in pom. 2d79260 [Marcelo Vanzin] [SPARK-8015] [flume] Remove Guava dependency from flume-sink. (cherry picked from commit 0071bd8) Signed-off-by: Tathagata Das <[email protected]>
The minimal change would be to disable shading of Guava in the module, and rely on the transitive dependency from other libraries instead. But since Guava's use is so localized, I think it's better to just not use it instead, so I replaced that code and removed all traces of Guava from the module's build. Author: Marcelo Vanzin <[email protected]> Closes apache#6555 from vanzin/SPARK-8015 and squashes the following commits: c0ceea8 [Marcelo Vanzin] Add comments about dependency management. c38228d [Marcelo Vanzin] Add guava dep in test scope. b7a0349 [Marcelo Vanzin] Add libthrift exclusion. 6e0942d [Marcelo Vanzin] Add comment in pom. 2d79260 [Marcelo Vanzin] [SPARK-8015] [flume] Remove Guava dependency from flume-sink.
The minimal change would be to disable shading of Guava in the module, and rely on the transitive dependency from other libraries instead. But since Guava's use is so localized, I think it's better to just not use it instead, so I replaced that code and removed all traces of Guava from the module's build. Author: Marcelo Vanzin <[email protected]> Closes apache#6555 from vanzin/SPARK-8015 and squashes the following commits: c0ceea8 [Marcelo Vanzin] Add comments about dependency management. c38228d [Marcelo Vanzin] Add guava dep in test scope. b7a0349 [Marcelo Vanzin] Add libthrift exclusion. 6e0942d [Marcelo Vanzin] Add comment in pom. 2d79260 [Marcelo Vanzin] [SPARK-8015] [flume] Remove Guava dependency from flume-sink.
The minimal change would be to disable shading of Guava in the module,
and rely on the transitive dependency from other libraries instead. But
since Guava's use is so localized, I think it's better to just not use
it instead, so I replaced that code and removed all traces of Guava from
the module's build.