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: stabilize ManagedChannelBuilder.usePlaintext() #6158

Merged
merged 4 commits into from Sep 18, 2019

Conversation

dapengzhang0
Copy link
Member

Resolves #1772

*/
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/1772")
@Deprecated
public T usePlaintext(boolean skipNegotiation) {
Copy link
Member

Choose a reason for hiding this comment

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

There's plenty of existing users. We should leave this in-place for a good while (at least a year).

Copy link
Member Author

Choose a reason for hiding this comment

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

We have already deprecated it for one and half year, still need a year?

Copy link
Member

Choose a reason for hiding this comment

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

I think so, because it is heavily used. And it also doesn't hurt us much at all to keep it longer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Do you mind if I also log a WARNING in the method?

Copy link
Member

Choose a reason for hiding this comment

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

Meh. They would already get a warning during compilation. I'd rather us not think about it much and just leave it setting there for a while.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I misunderstood what you said about a year and a half. Yes, that is plenty. We can remove this. Although we've also found some users that need to be migrated internally. We'll remove it once they are migrated.

if (skipNegotiation) {
negotiationType(NegotiationType.PLAINTEXT);
} else {
negotiationType(NegotiationType.PLAINTEXT_UPGRADE);
Copy link
Member

Choose a reason for hiding this comment

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

I'd really like to kill NegotiationType. We may want to introduce a new method for this before deleting it.

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

One small change needed.

@@ -165,7 +165,6 @@
* @return this
* @since 1.0.0
*/
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/1772")
Copy link
Member

Choose a reason for hiding this comment

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

Leave the annotation. That way it is clear it can be deleted, vs a deprecated method that was stable so we can't delete it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@dapengzhang0 dapengzhang0 merged commit 19b0916 into grpc:master Sep 18, 2019
@dapengzhang0 dapengzhang0 deleted the usePlaintext branch September 18, 2019 22:16
@lock lock bot locked as resolved and limited conversation to collaborators Dec 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.

Tracking Issue for Plaintext being Experimental.
2 participants