-
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
Free Druid source code from Guava by shading #10964
Conversation
…or or need real guava e.g. calcite and jclouds-core
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. |
This pull request fixes 7 alerts when merging 9f55cbf into 0f81ce3 - view on LGTM.com fixed alerts:
|
This looks good to me. @himanshug when are we planning to commit this? |
@mghosh4 needs to get review/approval before it can be merged. |
Looks good, @himadrisingh I wonder if |
yes, thanks for pointing out |
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. |
@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. |
This pull request has been marked as stale due to 60 days of inactivity. |
This pull request/issue has been closed due to lack of activity. If you think that |
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...
shaded-guava/pom.xml
.import com.google.common.X
is replaced byimport org.apache.druid.com.google.guava.X
cloudfiles-extensions
andsql
modules are reverted to useimport com.google.common.X
because their dependenciesjclouds-core
andcalcite
respectively mandate using realimport com.google.common.X
in various methods exposed by those libraries. But this is minimal.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: