Skip to content
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

all: Downgrade to Guava 19 #2751

Merged
merged 2 commits into from
Feb 28, 2017
Merged

all: Downgrade to Guava 19 #2751

merged 2 commits into from
Feb 28, 2017

Conversation

ejona86
Copy link
Member

@ejona86 ejona86 commented Feb 24, 2017

Guava 20 introduced some overloading optimizations for Preconditions
that require using Guava 20+ at runtime. Unfortunately, Guava 20 removes
some things that is causing incompatibilities with other libraries, like
Cassandra. While the incompatibility did trigger some of those libraries
to improve compatibility for newer Guavas, we'd like to give the
community more time to work through it. See #2688

At this commit, we appear to be compatible with Guava 18+. It's not
clear if we want to actually "support" 18, but it did compile. Guava 17
doesn't have at least MoreObjects, directExecutor, and firstNotNull.
Guava 21 compiles without warnings, so it should be compatible with
Guava 22 when it is released.

One test method will fail with the upcoming Guava 22, but this won't
impact applications. I made MoreThrowables to avoid using any
known-deprecated Guava methods in our JARs, to reduce pain for those
stuck with old versions of gRPC in the future (July 2018).

In the stand-alone Android apps I removed unnecessary explicit deps
instead of syncing the version used.

Guava 20 introduced some overloading optimizations for Preconditions
that require using Guava 20+ at runtime. Unfortunately, Guava 20 removes
some things that is causing incompatibilities with other libraries, like
Cassandra. While the incompatibility did trigger some of those libraries
to improve compatibility for newer Guavas, we'd like to give the
community more time to work through it. See grpc#2688

At this commit, we appear to be compatible with Guava 18+. It's not
clear if we want to actually "support" 18, but it did compile. Guava 17
doesn't have at least MoreObjects, directExecutor, and firstNotNull.
Guava 21 compiles without warnings, so it should be compatible with
Guava 22 when it is released.

One test method will fail with the upcoming Guava 22, but this won't
impact applications. I made MoreThrowables to avoid using any
known-deprecated Guava methods in our JARs, to reduce pain for those
stuck with old versions of gRPC in the future (July 2018).

In the stand-alone Android apps I removed unnecessary explicit deps
instead of syncing the version used.
@ejona86 ejona86 added the TODO:backport PR needs to be backported. Removed after backport complete label Feb 24, 2017

import com.google.common.base.Preconditions;

/** Utility functions when interacting with {@link Throwables}. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: remove this when upgrading.

@@ -1254,7 +1254,7 @@ protected void assertRemoteAddr(String expectedRemoteAddress) {

HostAndPort remoteAddress = HostAndPort.fromString(serverCallCapture.get().getAttributes()
.get(Grpc.TRANSPORT_ATTR_REMOTE_ADDR).toString());
assertEquals(expectedRemoteAddress, remoteAddress.getHost());
assertEquals(expectedRemoteAddress, remoteAddress.getHostText());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is deprecated, and will make it harder to go back up a version. Do we even need to use it?

@jhspaybar
Copy link
Contributor

This is great and will let us get on the 1.1.x branch while waiting for the cassandra and other fixes. Thank you.

@ejona86
Copy link
Member Author

ejona86 commented Feb 27, 2017

@carl-mastrangelo, PTAL

Copy link
Contributor

@carl-mastrangelo carl-mastrangelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@carl-mastrangelo
Copy link
Contributor

@jhspaybar FYI this will probably not make it into 1.1.x. We plan on cutting 1.2 this week, and releasing it mid march.

@ejona86
Copy link
Member Author

ejona86 commented Feb 28, 2017

retest this please (for Jenkins)

@ejona86 ejona86 merged commit 437fafa into grpc:master Feb 28, 2017
@ejona86 ejona86 deleted the guava19 branch February 28, 2017 17:23
@ejona86
Copy link
Member Author

ejona86 commented Feb 28, 2017

:'( I failed to squash first.

@carl-mastrangelo
Copy link
Contributor

:shame:

@lukaszx0
Copy link
Collaborator

pro tip (something that I've been doing recently): never commit something with "Address review comments" message when addressing review comments hehe, even if it's a single char change. That way, in the case of something like this, the history will look good even if PR was merged in unsquashed.

Anyways, :shame: 😉

@ejona86 ejona86 removed the TODO:backport PR needs to be backported. Removed after backport complete label Jun 29, 2017
@ejona86 ejona86 mentioned this pull request Mar 6, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants