-
Notifications
You must be signed in to change notification settings - Fork 882
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
JAVA-1328: Provide compatibility with Guava 20 #784
Conversation
Is there a compelling reason to target 3.0.7 instead of 3.2.0? I would personally be more comfortable with 3.2.0. |
} | ||
} | ||
|
||
private static class Version18OrLower extends GuavaCompatibility { |
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.
Nit: adding @SuppressWarnings("deprecation")
here would spare us a couple warnings.
private static Manifest loadGuavaManifest() { | ||
InputStream in = null; | ||
try { | ||
Enumeration<URL> resources = Resources.class.getClassLoader() |
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.
Resources
is itself part of Guava, and is marked @Beta
. Why not use GuavaCompatibility.class.getClassLoader()
?
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.
I hadn't noticed it was beta. I think it's better to use a Guava class to make sure it's the classloader that loaded Guava, I've changed it to Preconditions
.
|
||
@Override | ||
public <I, O> ListenableFuture<O> transformAsync(ListenableFuture<I> input, AsyncFunction<? super I, ? extends O> function, Executor executor) { | ||
return Futures.transform(input, function, executor); |
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.
Found one occurrence of this method in com.datastax.driver.core.AsyncQueryTest#connectAndQuery
* They are available in some versions of Java/Guava, but not across all versions ranges supported by the driver, hence | ||
* the custom implementation. | ||
*/ | ||
public class MoreObjects { |
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.
Since it's public, maybe we could add javadocs for methods in this class, even if I concede that their purposes are pretty obvious.
@@ -40,8 +40,8 @@ | |||
public class LimitingLoadBalancingPolicy extends DelegatingLoadBalancingPolicy { |
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.
The import import com.google.common.collect.Sets
is unused.
In addition to my review, we should also
|
Addressed all feedback. |
That's a good point, it doesn't break API compatibility, but is more of a new capability than a hotfix so 3.2.0 does seem more appropriate. |
CI looks happy with guava 16.0.1, 17.0, 18.0, 19.0, 20.0 and 21.0, excellent work! |
Glad to see this approved! Is there any chance for a release in the near future? |
@Xaerxess no timeframe yet, but it shouldn't be long. |
Squashed and rebased on top of 3.x instead of 3.0.x. |
Note to reviewers: I added an extra commit to fix OSGi tests and also to revert the Import-Package instruction that we use to create our bundles. We need to explicitly declare that the driver bundle accepts any Guava version in the range [16.0.1,21) so I modified the instructions to read that. |
Based on original work by @mspangdal.
Based on original work by @mspangdal.