xds: provide fallback protocol negotiator by sanjaypujare · Pull Request #7040 · grpc/grpc-java

sanjaypujare · 2020-05-13T23:51:03Z

No description provided.

creamsoup · 2020-05-14T16:55:49Z

@@ -129,8 +139,9 @@ public static XdsServerBuilder forPort(int port) {
  @Override
  public Server build() {
    // note: doing it in build() will overwrite any previously set ProtocolNegotiator


this comment is not correct, remove it?

creamsoup · 2020-05-14T16:56:38Z


  private final NettyServerBuilder delegate;
  private final int port;
+  private ProtocolNegotiator fallbackProtocolNegotiator;


do we want to provide default value? if no default value, probably no fallback instead of throwing NPE?

Yes, I was rethinking null check in build() as well. Providing a default for fallback doesn't sound right: they both have similar meaning: fall back to something or default to something. So why provide a default for fallback? In that case no fallback is the way to go - which means failing the connection if the Sds PN fails. Is that right?

yes, i think that is natural behavior or at least better than not able to construct XdsServer.
no fallback PN -> no fallback when SdsPN fails.

creamsoup · 2020-05-14T17:01:25Z


+  public XdsServerBuilder fallbackProtocolNegotiator(
+      ProtocolNegotiator fallbackProtocolNegotiator) {
+    this.fallbackProtocolNegotiator = fallbackProtocolNegotiator;


null check here would be nice.

Hmmm, on second thoughts since we are making fallbackProtocolNegotiator optional (in the builder) if somebody wants to "nullify" the previously set value, shouldn't we allow null here?

that's little bit subjective. but setting and unsetting on the same builder is weird. calling this has intention of setting Fallback PN, so it is okay to not accept null imo.

But I can call it multiple times with different values - the last value wins. It doesn't make sense to disallow null in that case. If I need to clear the previously set value in a different piece of code (over which I have no control) then the only thing I can do is to call this with null assuming it is allowed.

i don't really care which way you use because both has pros and cons. iirc, this topic was mentioned in the api meeting briefly.
make sure this is described in the javadoc.

We normally allow people to "undo" changes to the builder, so if null is okay, allowing null is fine. But I'd just remind you not to send too much effort on this part of the API because it will be replaced.

null is okay so I am going to allow null to override a previous non-null value (and mention so in javadocs)

creamsoup · 2020-05-14T17:20:02Z

  }

+  @Test
+  public void fallbackProtocolNegotiator_expectException() throws IOException, URISyntaxException {


i am not sure what this is testing. it provides fallbackPN that throws "no fallback negotiation" exception?

The test makes sure the fallback PN is invoked in an easy way. I can see the contradiction of the fallback PN throwing exception saying "no fallback exception". I can change the text message. In connection with the other thread: if fallback PN is now optional, I can use the same fallback PN (as internal default) that throws an exception? Or else the HandlerPickerHandler can generate an error when there is no fallback PN. Which is better?

mock PN can be better fit here. verify(mockFallbackPn).newHandler();.
The thrown exception should be able to converted to meaningful Status, probably UNAVAILABLE. the conversion logic is in WriteBufferingAndExceptionHandler#exceptionCaught. you can do it in default FallbackPN or HandlerPickerHandler whichever makes more sense.

Just used null and verified that server is shutdown because of missing fallback PN

ejona86

Looks fair. The server-side builder fallback will change API, but the plumbing inside the ProtocolNegotiator is the important thing for now. We are going to have XdsServerBuilder being the equivalent of xds:// on client-side, and then we'll either pass in XdsCredentials (assuming server credentials exist) or call a method to enable xds credentials (if server credentials don't exist).

sanjaypujare · 2020-05-14T21:21:56Z

Looks fair. The server-side builder fallback will change API, but the plumbing inside the ProtocolNegotiator is the important thing for now. We are going to have XdsServerBuilder being the equivalent of xds:// on client-side, and then we'll either pass in XdsCredentials (assuming server credentials exist) or call a method to enable xds credentials (if server credentials don't exist).

We should also remember to rename XdsServerBuilder to something else. This is NOT building an xDS server (like TD or Pilot) but a regular gRPC server that is "managed" by xDS. So how about XdsManagedServerBuilder? And similar changes to other classes (if needed).

ejona86 · 2020-05-14T23:16:12Z

It's the difference between "(xDS Server) Builder" and "xDS (Server Builder)" (or written in plain English, xDS-Server Builder and xDS Server Builder; not to be confused with xDS, Server Builder!) Given it is a ServerBuilder, I don't feel XdsServerBuilder is inappropriate. This will be a cross-language discussion, because we were planning "XdsServerBuilder" for the wrapped languages as well.

sanjaypujare · 2020-05-14T23:56:16Z

It's the difference between "(xDS Server) Builder" and "xDS (Server Builder)" (or written in plain English, xDS-Server Builder and xDS Server Builder; not to be confused with xDS, Server Builder!) Given it is a ServerBuilder, I don't feel XdsServerBuilder is inappropriate. This will be a cross-language discussion, because we were planning "XdsServerBuilder" for the wrapped languages as well.

Well, naming is subjective so I can't argue with it (and it doesn't affect functionality!) but I suspect many folks would agree that XdsServerBuilder is not appropriate in this case.

creamsoup · 2020-05-15T16:49:47Z

    @VisibleForTesting
-    public ServerSdsProtocolNegotiator(XdsClientWrapperForServerSds xdsClientWrapperForServerSds) {
+    public ServerSdsProtocolNegotiator(XdsClientWrapperForServerSds xdsClientWrapperForServerSds,
+        ProtocolNegotiator fallbackProtocolNegotiator) {


those are @Nullable (anything accept fallbackProtocolNegotiator, there are quite a few of those)

creamsoup · 2020-05-15T17:05:48Z

        new SdsProtocolNegotiators.HandlerPickerHandler(
-            grpcHandler, /* xdsClientWrapperForServerSds= */ null);
+            grpcHandler, /* xdsClientWrapperForServerSds= */ null,
+            new FallbackProtocolNegotiator());


no reply for my previous comment. consider mock here. you can verify newHandler is called or not. this is cleaner and we can remove L316-344.

Your previous comment was for a different file/test XdsSdsClientServerTest#nullFallbackProtocolNegotiator_expectException I renamed the test and also changed it. Also that test verifies the server closes so the clients can't connect any more.

For this one: yes, I will look into using a mock - I think it should work.

creamsoup

LGTM

creamsoup · 2020-05-18T16:53:27Z

        new SdsProtocolNegotiators.HandlerPickerHandler(
-            grpcHandler, /* xdsClientWrapperForServerSds= */ null);
+            grpcHandler, /* xdsClientWrapperForServerSds= */ null,
+            mockProtocolNegotiator);  // new FallbackProtocolNegotiator()


nit: this comment (ed code?) is confusing

oops, good catch. Will fix

xds: provide fallback protocol negotiator

46b93ea

sanjaypujare requested review from creamsoup and ejona86 May 13, 2020 23:51

creamsoup reviewed May 14, 2020

View reviewed changes

ejona86 reviewed May 14, 2020

View reviewed changes

xds: address review comments

2c30db2

creamsoup reviewed May 15, 2020

View reviewed changes

xds: address review comments - 2

77b3e2c

creamsoup approved these changes May 18, 2020

View reviewed changes

xds: remove commented code

3e55360

sanjaypujare merged commit efa9cf6 into grpc:master May 18, 2020

sanjaypujare deleted the next-xds-work branch May 18, 2020 17:30

dfawley pushed a commit to dfawley/grpc-java that referenced this pull request Jan 15, 2021

xds: provide fallback protocol negotiator (grpc#7040)

84eb4c6

github-actions Bot locked as resolved and limited conversation to collaborators Jun 13, 2021

Uh oh!

Conversation

sanjaypujare commented May 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

sanjaypujare commented May 14, 2020

Uh oh!

ejona86 commented May 14, 2020

Uh oh!

sanjaypujare commented May 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

creamsoup left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants