adds support for propagating grpc server-side cancellations #844

shamsimam · 2019-07-09T22:08:59Z

Jet PR: twosigma/jet#32

Changes proposed in this PR

adds support for propagating grpc server-side cancellations

Why are we making these changes?

gRPC headers/trailers including errors codes should be propagated correctly to the client.

sradack

Ending this review as I just saw more commits come in.

sradack · 2019-07-10T04:33:02Z

containers/test-apps/courier/src/main/java/com/twosigma/waiter/courier/GrpcClient.java

@@ -148,6 +150,15 @@ public static CourierReply sendPackage(final String host,
            } catch (final StatusRuntimeException e) {
                logFunction.apply("RPC failed, status: " + e.getStatus());
                return null;
+            } catch (ExecutionException e) {


Suggested change

} catch (ExecutionException e) {

} catch (final ExecutionException e) {

sradack · 2019-07-10T04:46:03Z

containers/test-apps/courier/src/main/java/com/twosigma/waiter/courier/GrpcClient.java

@@ -148,6 +150,15 @@ public static CourierReply sendPackage(final String host,
            } catch (final StatusRuntimeException e) {
                logFunction.apply("RPC failed, status: " + e.getStatus());
                return null;
+            } catch (ExecutionException e) {
+                final Status status = Status.fromThrowable(e.getCause());


Is getCause() necessary? Status.fromThrowable() searches recursively through causes.

The snipped was inspired from grpc-java example on error handling:
https://github.com/grpc/grpc-java/blob/d648e8f2df091351c373cb75ad9f47e94aa2fca6/examples/src/main/java/io/grpc/examples/errorhandling/ErrorHandlingClient.java#L103

sradack · 2019-07-10T05:06:19Z

containers/test-apps/courier/src/main/java/com/twosigma/waiter/courier/GrpcClient.java

+            } catch (ExecutionException e) {
+                final Status status = Status.fromThrowable(e.getCause());
+                logFunction.apply("RPC execution failed: " + status);
+                return CourierReply


Can we avoid packing into CourierReply? CourierReply is meant to represent a message from the server.

sradack · 2019-07-10T05:13:39Z

containers/test-apps/courier/src/main/java/com/twosigma/waiter/courier/GrpcClient.java

@@ -214,6 +227,16 @@ public void onError(final Throwable throwable) {
                                logFunction.apply("releasing semaphore after receiving error");
                                lockStep.release();
                            }
+                            if (throwable instanceof StatusRuntimeException) {
+                                final StatusRuntimeException exception = (StatusRuntimeException) throwable;
+                                final CourierSummary response = CourierSummary


Can we avoid packing into CourierSummary? CourierSummary is meant to represent a message from the server.

sradack · 2019-07-10T05:56:23Z

containers/test-apps/courier/src/main/java/com/twosigma/waiter/courier/GrpcClient.java

+                    if (i >= cancelThreshold) {
+                        logFunction.apply("cancelling sending messages");
+                        awaitChannelTermination.set(false);
+                        channel.shutdownNow();


We should also test explicit cancels. I found three ways:

final Context.CancellableContext cancellableContext = Context.current().withCancellation(); cancellableContext.run(() -> { futureStub.collectPackages(new StreamObserver<CourierSummary>() { // ... }); }); // later... cancellableContext.cancel(new RuntimeException("cancelling grpc request"));

ListenableFuture<CourierReply> future = futureStub.sendPackage(request); future.cancel(false);

collector.onError(Status.CANCELLED.asRuntimeException());

https://groups.google.com/forum/#!topic/grpc-io/quToVM4NhdQ
https://groups.google.com/forum/#!topic/grpc-io/MdDADccRpAQ

I added the client-side streaming, but server-side cancellation case.
The above examples are for client-side cancellation, they will be added in the client-side cancellation PR.

sradack · 2019-07-10T06:38:35Z

waiter/src/waiter/process_request.clj

+                (let [trailers-map (async/<! trailers)
+                      modified-trailers (merge grpc-headers trailers-map)]
+                  (log/info "response trailers:" trailers-map)
+                  (when (seq grpc-headers)


Is (seq grpc-headers) always true?

Yes, I added the if (seq grpc-headers) later which makes these seq checks unnecessary.

sradack · 2019-07-10T06:38:56Z

waiter/src/waiter/process_request.clj

+                  (log/info "response trailers:" trailers-map)
+                  (when (seq grpc-headers)
+                    (log/info "attaching grpc headers into trailer:" grpc-headers))
+                  (when (seq modified-trailers)


Is (seq modified-trailers) always true?

Yes, I added the if (seq grpc-headers) later which makes these seq checks unnecessary.

sradack · 2019-07-10T06:55:09Z

waiter/src/waiter/process_request.clj

+      ;; mark the request as successful, grpc failures are reported in the headers
+      (deliver reservation-status-promise :success)
+      ;; stop writing any content in the body
+      (async/close! body)


Isn't body being read by stream-http-response? Can we make this comment more clear?

Added comments, we want to avoid writing data to body in stream-http-response

stream-http-response writes to resp-chan. The body here is the body coming back from the backend.

stream-http-response reads from body, closing body triggers request completion of the request by closing resp-chan.

sradack · 2019-07-10T06:55:50Z

waiter/src/waiter/process_request.clj

+      (log/info "eagerly closing response body as grpc status is" grpc-status)
+      (when abort-ch
+        ;; disallow aborting the request
+        (async/close! abort-ch))


Can we say why we are disallowing aborting the request?

Added comment, we have deemed the request as success and will trigger normal completion by closing the body channel.

sradack · 2019-07-10T06:56:08Z

waiter/src/waiter/process_request.clj

+      (deliver reservation-status-promise :success)
+      ;; stop writing any content in the body
+      (async/close! body)
+      (async/close! error-chan))


Why do we close the error-chan?

Added comments, we want to avoid blocking in stream-http-response

…d asap

introduces separate variant field to determine cancellation operation adds unary server exit case cleans up return values from the grpc client adds assertion on grpc health check response body

…th unary responses

asserts on individual fields on the ping response

sradack · 2019-07-11T00:56:18Z

containers/test-apps/courier/src/main/java/com/twosigma/waiter/courier/GrpcClient.java

+    public static RpcResult<CourierReply> sendPackage(final String host,
+                                                      final int port,
+                                                      final Map<String, Object> headers,
+                                                      final String id,


Can we avoid overloading id with the actions to take? Can we avoid updating the signatures of these functions when we want to make a change on the Clojure side? This request is aspirational and not required for this PR.

Will do in a later PR. Created #854.

sradack · 2019-07-11T00:58:04Z

containers/test-apps/courier/src/main/java/com/twosigma/waiter/courier/GrpcClient.java

+                        }
+
+                        @Override
+                        public void onCompleted() {


Can we confirm onCompleted and onError are mutually exclusive?

Yes, here are logs from two runs where onCompleted () is not called when there is an error:

When there is an error:

INFO: completed sending packages INFO: status received from server:Status{code=CANCELLED, description=Cancelled by server, cause=null} INFO: trailers received from server:Metadata(content-type=application/grpc) INFO: error in aggregating summaries io.grpc.StatusRuntimeException: CANCELLED: Cancelled by server INFO: client result: null INFO: shutting down channel

Successful completion:

INFO: completed sending packages INFO: headers received from server:Metadata(content-type=application/grpc,x-cid=cid-aggregate-packages-success.1562836825027,grpc-encoding=identity,grpc-accept-encoding=gzip) INFO: received response CourierSummary{count=10, length=90} INFO: status received from server:Status{code=OK, description=null, cause=null} INFO: trailers received from server:Metadata() INFO: completed aggregating summaries INFO: client result: num_messages: 10, total_length: 90 INFO: shutting down channel

sradack · 2019-07-11T01:03:43Z

waiter/src/waiter/process_request.clj

+      ;; mark the request as successful, grpc failures are reported in the headers
+      (deliver reservation-status-promise :success)
+      ;; stop writing any content in the body
+      (async/close! body)


stream-http-response writes to resp-chan. The body here is the body coming back from the backend.

avoids warnings due to presence of overloaded methods in grpc client

avoids warnings due to presence of overloaded methods in grpc client adds correlation-id to logs from grpc client

shamsimam · 2019-07-11T11:57:57Z

I have updated the courier client code to avoid race in reporting the error status.

shamsimam · 2019-07-11T13:59:21Z

Fyi, consistently green runs internally since the client race fix.

shamsimam self-assigned this Jul 9, 2019

shamsimam force-pushed the grpc-server-side-cancel branch 2 times, most recently from 4907cf3 to 6dd5f97 Compare July 9, 2019 22:26

shamsimam added the internal-green label Jul 9, 2019

shamsimam requested a review from sradack July 9, 2019 23:03

shamsimam force-pushed the grpc-server-side-cancel branch from 24f8236 to 1162f2e Compare July 10, 2019 06:49

sradack suggested changes Jul 10, 2019

View reviewed changes

shamsimam force-pushed the grpc-server-side-cancel branch 2 times, most recently from f4448b9 to c430a6b Compare July 10, 2019 19:44

shamsimam added internal-green and removed internal-green labels Jul 10, 2019

shamsimam added 7 commits July 10, 2019 15:57

adds support for propagating grpc server-side cancellations

c26e0f2

adds messages to test assertions

37486ab

closes trailers eagerly to allow pure header responses to be publishe…

5684e1b

…d asap

removes unnecessary when clauses

5abe4f3

avoids overloading the id field in CourierRequest

accf7dd

introduces separate variant field to determine cancellation operation adds unary server exit case cleans up return values from the grpc client adds assertion on grpc health check response body

documents reasons for closing the channels

781ab21

adds tests for client-side streaming and server-side cancellations wi…

7aa4a15

…th unary responses

shamsimam force-pushed the grpc-server-side-cancel branch from c430a6b to 542ba84 Compare July 10, 2019 21:05

initializes grpc client result Status to UNKNOWN instead of OK

de02079

asserts on individual fields on the ping response

shamsimam force-pushed the grpc-server-side-cancel branch from 542ba84 to de02079 Compare July 11, 2019 00:44

sradack reviewed Jul 11, 2019

View reviewed changes

shamsimam added 2 commits July 11, 2019 03:59

improves assertion messages

62dc8bf

avoids warnings due to presence of overloaded methods in grpc client

improves assertion messages

1809d61

avoids warnings due to presence of overloaded methods in grpc client adds correlation-id to logs from grpc client

shamsimam force-pushed the grpc-server-side-cancel branch from bc96170 to 1809d61 Compare July 11, 2019 10:41

avoids race in reading status on error path in grpc-client

fde4058

sradack merged commit dd6bd41 into master Jul 11, 2019

sradack deleted the grpc-server-side-cancel branch July 11, 2019 19:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds support for propagating grpc server-side cancellations #844

adds support for propagating grpc server-side cancellations #844

shamsimam commented Jul 9, 2019

sradack left a comment

sradack Jul 10, 2019

shamsimam Jul 10, 2019

sradack Jul 10, 2019

shamsimam Jul 10, 2019

sradack Jul 10, 2019

shamsimam Jul 10, 2019

sradack Jul 10, 2019

shamsimam Jul 10, 2019

sradack Jul 10, 2019

shamsimam Jul 10, 2019

sradack Jul 10, 2019

shamsimam Jul 10, 2019

shamsimam Jul 10, 2019

sradack Jul 10, 2019

shamsimam Jul 10, 2019

shamsimam Jul 10, 2019

sradack Jul 10, 2019

shamsimam Jul 10, 2019

sradack Jul 11, 2019

shamsimam Jul 11, 2019

sradack Jul 10, 2019

shamsimam Jul 10, 2019

sradack Jul 10, 2019

shamsimam Jul 10, 2019

sradack Jul 11, 2019

shamsimam Jul 11, 2019

sradack Jul 11, 2019

shamsimam Jul 11, 2019

sradack Jul 11, 2019

shamsimam commented Jul 11, 2019

shamsimam commented Jul 11, 2019

	} catch (ExecutionException e) {
	} catch (final ExecutionException e) {

adds support for propagating grpc server-side cancellations #844

adds support for propagating grpc server-side cancellations #844

Conversation

shamsimam commented Jul 9, 2019

Changes proposed in this PR

Why are we making these changes?

sradack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamsimam commented Jul 11, 2019

shamsimam commented Jul 11, 2019