Use Guava Compatible immediate executor service#6815
Conversation
|
Is it worth to make Druid compatible with various Guava version? Why don't we implement our own direct executor (as you did in #5413) or simply copy the latest Guava implementation? |
|
i Agree with @jihoonson i think moving away from Guava by copying pasting code or doing our own stuff is far more worth it than trying to support multiple guava versions. |
|
@jihoonson I tried the "our own implementation" route, and there was too much nuance that I didn't want to deal with. Just fetching the guava stuff correctly was much easier, and puts the maintenance of proper implementation on the Guava libs instead of us. |
|
Then why don't we copy the latest (or whatever with less bugs) Guava implementation? I don't see the point of supporting various Guava versions at run time. |
|
+1 for c/p'ing the latest Guava implementation as-is, like we did with Closer and splitToList. At least then we'll know what we're dealing with. |
|
Not failing on arbitrary guava versions is a problem on our side related to things like gRPC extensions and building Druid libs in with Beam workloads. Making the Druid libs at least support a wide range of versions greatly reduces the surprising library conflicts that have to be resolved, and eliminates the temptation to shade-shade-shade a whole bunch of stuff. |
|
I would love to, for example, be able to have my internal fork of Druid run on the more recent Guava versions to be compatible with whatever extensions we want to use. But such a thing is not a drop-in exercise until issues like this one are resolved. |
|
Copied over the Direct Executor Service. Luckily it isn't doing any package private mumbo jumbo, so the copy was pretty straight forward |
|
I can't see why teamcity is failing |
|
@drcrallen It looks like there's a reference to CallerRunsPolicy in a javadoc, a class that was not copied over. (You should be able to see this stuff by registering for a TeamCity acct.) Probably the right thing to do is edit the javadoc.
|
|
oh! login as guest works well now. |
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
LGTM other than the question about GroupByQueryRunnerTest.java.
| @@ -29,7 +29,6 @@ | |||
| import com.google.common.collect.Lists; | |||
| import com.google.common.collect.Ordering; | |||
| import com.google.common.collect.Sets; | |||
| import com.google.common.util.concurrent.MoreExecutors; | |||
There was a problem hiding this comment.
Were the changes in this file all autoformatter changes? They are hard to follow so I'd rather ask than try to figure that out.
There was a problem hiding this comment.
Either auto-formatter or MoreExecutors.sameThreadExecutor() --> Execs.directExecutor(), yes
|
Ugh, if the import is there, failure because unused import (even though it is used in javadoc). If the import is not there it gets failure in teamcity due to unknown reference. |
|
I have never really found a good way around that particular weirdness with imports + javadocs. I usually deal with it by using fully qualified class names in javadocs (rather than import + short name). Or by avoiding |
|
@drcrallen thanks for doing this! |
|
For future folks: #6948 adds more compat up to 27.0.1 |
5 participants
As a follow on to #5413 this PR copies the DirectExecutorService implementation from more recent Guava versions (18 and later).
I think this is the last change to make the druid code base work for a HUGE swath of Guava versions.