Skip to content
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

Free Druid source code from Guava by shading #10964

Closed
wants to merge 9 commits into from

Conversation

himanshug
Copy link
Contributor

@himanshug himanshug commented Mar 9, 2021

Fixes #6942

Description

This might look like a large PR but most changes are mechanical i.e. updates to import statements and guava dependency in various pom files.

Here is the meaty part...

  1. A new module "shaded-guava" is introduced that produces classes in "org.apache.druid.com.google.common" package by shading real guava. See shaded-guava/pom.xml .
  2. pom.xml in all modules are made to depend upon shaded guava jar above instead of real guava, that change looks like
-      <groupId>com.google.guava</groupId>
+      <groupId>org.apache.druid.com.google.guava</groupId>
  1. In all Java files, import com.google.common.X is replaced by import org.apache.druid.com.google.guava.X
  2. "Organize Imports" is done to all Java files to correct import ordering or checkstyles fails
  3. Some imports in cloudfiles-extensions and sql modules are reverted to use import com.google.common.X because their dependencies jclouds-core and calcite respectively mandate using real import com.google.common.X in various methods exposed by those libraries. But this is minimal.
  4. some imports that used org.apache.curator.shaded.com.google.common.X , i.e. shaded version of guava used by curator, continue to use same. that is independent.

(this approach was briefly described in #6942 (comment) )

End Result is that Druid source code is free from real guava and can use any version of guava inside shaded-guava module, few exceptions in (5) as described above.
we still ship guava-0.16.1.jar like before, so no external integrations should see any impact. User should be able to switch this with their favorite version of guava jar at runtime and Druid source code be fine because it uses druid-shaded-guava-0.x.y.jar

Existing Unit, Integration Tests and Travis builds should cover all of the changes except for Hadoop Integration that I could not verify due to not having quick access to such setup. However, given the way this patch works and that we continue to ship existing guava jar, Hadoop Integration should see no change.

#10965 should also be done after this PR is accepted.


This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@himanshug
Copy link
Contributor Author

Ah, this PR is very prone to merge conflicts due to the sweeping changes made. However, this PR can be reviewed in the presence of conflicts [with mechanical changes described in PR description]. I will fix all the conflicts when this PR is accepted and ready to be merged.

@lgtm-com
Copy link

lgtm-com bot commented Mar 9, 2021

This pull request fixes 7 alerts when merging 9f55cbf into 0f81ce3 - view on LGTM.com

fixed alerts:

  • 7 for Array index out of bounds

@mghosh4
Copy link
Contributor

mghosh4 commented Mar 22, 2021

This looks good to me. @himanshug when are we planning to commit this?

@himanshug
Copy link
Contributor Author

@mghosh4 needs to get review/approval before it can be merged.

@egor-ryashin
Copy link
Contributor

Looks good, @himadrisingh I wonder if extensions-core/hive-extensions/pom.xml was added by mistake?

@himanshug
Copy link
Contributor Author

himanshug commented May 20, 2021

Looks good, .. I wonder if extensions-core/hive-extensions/pom.xml was added by mistake?

yes, thanks for pointing out

@egor-ryashin
Copy link
Contributor

egor-ryashin commented Mar 20, 2022

We need to merge the ticket, it will solve a bunch of problems and people waiting for it. Please, ping me, if you need help with the merge.

@himanshug
Copy link
Contributor Author

@egor-ryashin Thanks. Unfortunately I will not be able to spend much time on it in the near future. Please feel free to take the commits here , create a new PR with conflicts resolved and any other files that were added later.

@github-actions
Copy link

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Oct 11, 2023
Copy link

github-actions bot commented Nov 8, 2023

This pull request/issue has been closed due to lack of activity. If you think that
is incorrect, or the pull request requires review, you can revive the PR at any time.

@github-actions github-actions bot closed this Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shade Guava manually
3 participants