Skip to content

Conversation

@shardulm94
Copy link
Contributor

#1068 introduced a bundled-guava module which relocated Guava classes and changed existing imports to use these relocated classes. Unfortunately, new PRs can continue to use these classes resulting in ClassNotFoundErrors at runtime when using iceberg-spark-runtime.

e.g. java.lang.NoClassDefFoundError: org/apache/iceberg/shaded/com/google/common/base/Preconditions

This PR adds a checkstyle rule to ban use of unrelocated guava classes and also changes violating imports

Copy link
Contributor

@rdsr rdsr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm +1 for this change

<message key="import.illegal" value="Use the Java8 version of Guava objects."/>
</module>
<module name="IllegalImport">
<property name="id" value="BanUnrelocatedGuavaClasses"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, the intention with the original PR is that the classpath would be set up in such a way that the other subprojects would not be able to see Guava on their classpath and these imports would give compile errors. @rdblue I guess you didn't manage to get that working? Perhaps #1067 will fix this? So this rule shouldn't be necessary and compile errors would give devs a warning earlier than checkstyle errors. I don't really mind having this as an extra safety net though.

@rdblue
Copy link
Contributor

rdblue commented May 29, 2020

I'm looking into why Guava is in the classpath. I tested it with iceberg-api, but it looks like Guava is in the classpath in iceberg-core and other modules. It is a good idea to exclude it from the classpath where we can and also check that we don't reference it anywhere, since we don't ever want to leak it. I'll merge this.

Thanks @shardulm94!

@rdblue rdblue merged commit 295accd into apache:master May 29, 2020
@shardulm94 shardulm94 deleted the use-relocated-guava branch July 15, 2020 03:51
cmathiesen pushed a commit to ExpediaGroup/iceberg that referenced this pull request Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants