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

core,stub: Add toString() to ClientCallImpl and GrpcFuture #4401

Merged
merged 3 commits into from
May 1, 2018

Conversation

groakley
Copy link
Contributor

Improves debugging experience with ClientCall instances.

Sent on behalf of @charlesmunger

@groakley
Copy link
Contributor Author

Hmm, looks like this project is being built against Guava version 20.0. pendingToString() was added in 23.0. Are there any plans to bump the Guava version?

@thelinuxfoundation
Copy link

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

Regards,
CLA GitHub bot

@groakley
Copy link
Contributor Author

Updated corp CLA status.

@ejona86
Copy link
Member

ejona86 commented Apr 27, 2018

@groakley, it seems the CLA is still a bit unhappy with you. I think #3961 is a blocker for us going as far as Guava 23. It'll happen, but take a little bit. pendingToString() is just an overridden method, not something you call, so it should be fine to have the method and just not put an @Override. If a user is using a newer Guava, they'll get the better toString.

@charlesmunger
Copy link

@ejona86 #3961 shouldn't be a blocker for adopting newer versions of guava, since guava has two versions - one for java 8, one for android.

https://github.com/google/guava/wiki/Release22#new-android-version

Does grpc publish separate targets for android and not-android?

@ejona86
Copy link
Member

ejona86 commented Apr 30, 2018

@charlesmunger, the Android version of Guava uses JDK 7 bytecode. The number of Java 6 users should be low, but it is a prerequisite.

… method was added in a new version of Guava.
@@ -471,6 +471,11 @@ private void closeObserver(Listener<RespT> observer, Status status, Metadata tra
observer.onClose(status, trailers);
}

@Override
public String toString() {
return super.toString() + "method=[" + method + "]";
Copy link
Member

Choose a reason for hiding this comment

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

We are using Guava MoreObjects.ToStringHelper instead of harding coding =[]

Ditto.

@groakley
Copy link
Contributor Author

groakley commented May 1, 2018

Commenting again with hopes that I've jumped through the CLA hoops properly.

@dapengzhang0 dapengzhang0 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 1, 2018
@kokoro-team kokoro-team removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 1, 2018
Copy link
Member

@dapengzhang0 dapengzhang0 left a comment

Choose a reason for hiding this comment

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

LGTM

@ejona86 ejona86 merged commit fcb48ff into grpc:master May 1, 2018
@ejona86
Copy link
Member

ejona86 commented May 1, 2018

Thank you!

@groakley groakley deleted the client-call-tostring branch May 2, 2018 17:21
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 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.

6 participants