-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Conversation
*/ | ||
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/1772") | ||
@Deprecated | ||
public T usePlaintext(boolean skipNegotiation) { |
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.
There's plenty of existing users. We should leave this in-place for a good while (at least a year).
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.
We have already deprecated it for one and half year, still need a year?
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 think so, because it is heavily used. And it also doesn't hurt us much at all to keep it longer.
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.
Sure. Do you mind if I also log a WARNING in the method?
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.
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.
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.
Done.
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.
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); |
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'd really like to kill NegotiationType. We may want to introduce a new method for this before deleting it.
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.
One small change needed.
@@ -165,7 +165,6 @@ | |||
* @return this | |||
* @since 1.0.0 | |||
*/ | |||
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/1772") |
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.
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.
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.
Done.
Resolves #1772