-
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
Upgrade Guice to 5.1.0 #17578
Upgrade Guice to 5.1.0 #17578
Conversation
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.
Thanks for the changes, @Akshat-Jain !
Most of the stuff looks good. The ExceptionMatcher
is very useful for readability of tests.
I have a couple of questions:
- Do we not need the
guice-multibindings
dependency anymore? Where was it being used? Is it now coming packaged inside the other guice dependencies? - Are the changes made to
DruidCoreInjectorBuilder
required in this PR? I would prefer it if we tackled that improvement separately.
processing/src/test/java/org/apache/druid/query/DruidProcessingConfigTest.java
Outdated
Show resolved
Hide resolved
sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java
Outdated
Show resolved
Hide resolved
@kfaraz Thanks for the review!
Credits to @imply-cheddar for the original changes! 😄
Yeah. Since Guice 4.2, multibindings support has moved to Guice core. Reference: https://github.com/google/guice/wiki/Multibindings
Have reverted those changes. |
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.
+1 (pending tests)
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.
Looks good to me. Small request for a javadoc to describe the new ExceptionMatcher class. Will when passing CI
Description
This PR upgrades Guice to 5.1.0 and fixes the failing tests.
This PR has: