-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Upgrade Guava to 27 #10683
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 Guava to 27 #10683
Conversation
Seems like something is up with the build ENV |
|
Not sure how to proceed here. Things are failing because... SPOTBUGS for an issue I didn't add, not enough test coverage but I didn't add any new functionality, license failures, again, not sure how these changes did that. |
|
@asdf2014 Any thoughts on this? |
| }), | ||
| (AsyncFunction<Task, TaskStatus>) this::runTask | ||
| (AsyncFunction<Task, TaskStatus>) this::runTask, | ||
| Execs.directExecutor() |
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.
Why it is Execs.directExecutor()?
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.
@wangyum
Because upgrading Guava requires an explicit Executor passed to the method. The previous behavior was to implicitly pass directExecutor if no Executor was provided. I tried to pass the guava version and some maven build tool plugin rejected the change and directed me to use this instead.
Hi @belugabehr, we can ignore them if they are truly irrelevant, I haven't looked into details of Spotbugs or Intellij inspection failures though. However, I do see many real test failures in unit tests and integration tests which should be fixed before this PR gets merged. Also, the guava version is important to be compatible to other ecosystems and libraries such as Hadoop or AWS SDK. Integration tests for them are not running on Travis as they require additional setup such as AWS credentials, so you and the reviewers of this PR should run them manually before merge. |
|
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. |
|
This pull request/issue has been closed due to lack of activity. If you think that |
Fixes #XXXX.
Description
Fixed the bug ...
Renamed the class ...
Added a forbidden-apis entry ...
This PR has:
Key changed/added classes in this PR
MyFooOurBarTheirBaz