feat: introduce HttpJsonClientCall, Listeners infrastructure and ServerStreaming support in REST transport #1599

vam-google · 2022-01-12T04:17:57Z

This includes the following changes for HTTP1.1/REST transport:

HttpJsonClientCall class (with HttpJsonClientCall.Listener) mimicking io.grpc.ClientCall functionality. Most of the complexity of this PR is concentrated in HttpJsonClientCallImpl class.
The unary callables are rewritten to be based on HttpJsonClientCall flow (similarly to how it is already done in gRPC unary calls).
Server streaming support for REST transport. The implementation is based on HttpJsonClientCall and HttpJsonClientCall.Listener (introduced in this PR), similarly to how gRPC streaming is based on io.grpc.ClientCall and io.grpc.ClientCall.Listener (implemented in grpc-java library) respectively.

The extreme similarity between HttpJsonClientCall call and io.grpc.ClientCall is intentional and crucial for consistency of the two transports and also intends simplifying creation and maintenance of multi-transport manual wrappers (like google-ads-java).

The server streaming abstractions in gax java are all based on the flow control managed by a ClientCall, so having similar set of abstractions in REST transport is necessary to reuse transport-independent portions of streaming logic in gax and maintain identical user-facing streaming surface.

This PR also builds a foundation for the soon-coming ClientInterceptor-like infrastructure in REST transport. This is specifically required to support REST transport in google-ads-java.

REST-based client-side streaming and bidirectional streaming is not implemented by this PR and most likely will never be due to limitations of the HTTP1.1/REST protocol compared to HTTP2/gRPC.

Most of the java docs in HttpJsonClientCall class is a modified version of the java docs from io.grpc.ClientCall, which is intentional, because HttpJsonClientCall is designed to be as similar to io.grpc.ClientCall in both surface and behavior as possible (while the two classes cannot be a part of the same class hierarchy, because they belong to two independent transport layers).

What server-streaming means in case of REST transport
In REST transport server-streaming methods return a JSON array of response messages (i.e. the array element type is the same one used as a returned type in the corresponding method definition in protobuf). The response is provided as as Chunck-encoded input stream, containing one big JSON array. To parse the json array we rely on JsonReader from gson library, which gax-httpjson already depended on even prior this PR (check ProtoMessageJsonStreamIterator class implementation in this PR for details). Note, we must process elements of the array one-by-one because the size of the full array may be in realm of gigabytes.

Note, ideally I need to split this PR at least in two separate ones: 1) HttpJsonClientCall stuff and unary calls based on it in one PR and then 2) server streaming feature in a second PR. Unfortunately the most reasonable way to test HttpJsonClientCall infrastructure is by doing it from server streaming logic beause most of the complexity introduced in HttpJsonClient call is induced by necessity to support streaming workflow in the first place (and to support call interceptors (not part of this PR) as a secondary goal).

Note, there are a few minor breaking changes in gax-httpjson module (and only there) inroduced in this PR. This should be ok, because unlike gax and gax-grpc, gax-httpjson is not GA yet. The breaking changes are very minor (in the space of HttpJsonCallContext and ManagedHttpJsonChannel) and are backward-compatible with java-compute (the main and only officially supported user of gax-httpjson as of now).

…erStreaming support in REST transport This includes the following changes for HTTP1.1/REST transport: 1) `HttpJsonClientCall` class (with `HttpJsonClientCall.Listener`) mimicking [io.grpc.ClientCall](https://github.com/grpc/grpc-java/blob/master/api/src/main/java/io/grpc/ClientCall.java#L102) functionality. 2) The unary callables are rewritten to be based on `HttpJsonClientCall` flow (similarly to how it is already done in grpc unary calls). 3) Server streaming support for REST transport. The implementation is based on `HttpJsonClientCall` and `HttpJsonClientCall.Listener`, similarly to how grpc streaming is based on `io.grpc.ClientCall` and io.grpc.ClientCall.Listener` respectively. The extreme similarity between `HttpJsonClient` call and `io.grpc.ClientCall` is intentional and crucial for consistency of the two transports and also intends simplifying creation and maintenance of multi-transport manual wrappers (like [google-ads](https://github.com/googleads/google-ads-java/blob/main/google-ads/src/main/java/com/google/ads/googleads/lib/logging/LoggingInterceptor.java#L68)). The server streaming abstractions in gax java are all based on the flow control managed by a ClientCall, so having similar set of abstractions in REST transport is necessary to reuse transport-independent portions of streaming logic in gax and maintain identical user-facing streaming surface. This PR also builds a foundation for the soon-coming [ClientInterceptor](https://github.com/grpc/grpc-java/blob/master/api/src/main/java/io/grpc/ClientInterceptor.java#L42)-like infrastructure in REST transport. REST-based client-side streaming and bidirectional streaming is not implemented by this PR and most likely will never be due to limitations of the HTTP1.1/REST protocol compared to HTTP2/gRPC. Note, ideally I need to split this PR at least in two separate ones: 1) HttpJsonClientCall stuff and unary calls based on it in one PR and then 2) server streaming feature in a second PR. Unfortunately only reasonable way to test `HttpJsonClientCall` infrastructure is by doing it from server streaming logic beause most of the complexity introduced in HttpJsonClient call is induced by necessity to support streaming workflow.

gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonApiExceptionFactory.java

gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java

chanseokoh · 2022-01-12T18:49:55Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonCallOptions.java

+
+    Builder builder = this.toBuilder();
+
+    Instant newDeadline = inputOptions.getDeadline();


I'm a bit confused about the semantics of "merge." I see it's really just forcing the input values except for when the values are null. I wonder if null should be forcible too? For example, I want to clear the deadline (i.e., set it to null), but it's not doable.

If the current implementation is intended, I think it's worth documenting the exact behavior of merge() in Javadoc.

This merge semantics is a pattern in gax java already, a good example would be GrpcCallContext:
https://github.com/googleapis/gax-java/blob/main/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallContext.java#L321

To clear deadline one is expected to clear it explicitly (Builder.setDeadline(null)), it is not responsibility of merge logic.

Also, clearing something is not typical and not idiomatic in gax/grpc - usually if something becomes non-null it never gets
back to null state (null basically means never touched).

chanseokoh · 2022-01-12T18:57:50Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCallImpl.java

+  //
+  // A lock to guard the state of this call (and the response stream).
+  //
+  private final Lock lock = new ReentrantLock();


Looking at the use of lock in this class (single lock and only used for trivial mutual exclusions), I think we can simply use synchronized blocks to greatly simplify the code. Clean code without try ... finally. Even no need for a lock instance.

Yes, you are right, thought I did it intentionally here. I'm not aware of synchronized being preferred to java.util.concurrent locks, if anything it is opposite to the best of my knowledge. Even though more sophisticated features of ReentrantLock are not used now, we may need them in the future. Basically I usually prefer using java.util.concurrent every time there is a potential of needing heavyweight synchronization, and leave synchronized stuff for trivial cases. You are right that here it is used only for mutual exclusions (not the case in earlier versions of this code btw), but there is still quite a lot of careful synchronization needed and many fields need to be syncrhonized propely. I can change it to synchronized if you have a strong opinion on it though. If you don't I would prefer keeping it as is.

Please let me know what you think. Thanks!

Discussed offline and decided to use synchronized.

chanseokoh

Sorry, just throwing comments haphazardly. It's a long PR, so I think I still need a lot of time to look at various things.

chanseokoh · 2022-01-12T22:11:10Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/StatusRuntimeException.java

+ * HTTP status code in RuntimeException form, for propagating status code information via
+ * exceptions.
+ */
+public class StatusRuntimeException extends RuntimeException {


Oh, now I see this is for HTTP status codes. How about renaming this so that it becomes clear? Also the parameter name statusCode to, e.g., httpCode.

This exception mimics the one in grpc: https://github.com/grpc/grpc-java/blob/master/api/src/main/java/io/grpc/StatusRuntimeException.java, thus the same name is used. I can rename it to HttpJsonStatusRuntimeException though to stay consistent with the naming scheme of prefixing stuff with HttpJson and to make it easier and less confusing for multitransport code (which may need to handle both exceptions, and one will have to be using fully-qualified name to disambiguate, which is annoying).

Your call, but having to use fully-qualified name makes me lean toward prefixing HttpJson, which is the current pattern as well, as you said.

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoMessageResponseParser.java

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoMessageJsonStreamIterator.java

chanseokoh · 2022-01-12T23:20:44Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonDirectStreamController.java

+    if (!hasStarted) {
+      numRequested += count;
+    } else {
+      clientCall.request(count);
+    }


Can request() and start() ever run concurrently? If so, this isn't going to work as inteded unless there is a proper synchronization. (For example, on line 76, hasStarted may be false, but the moment you reach line 77, the value can be flipped.) Just to confirm this doesn't happen.

This code, as most parts of this PR mimicks grpc behavior, and this specific logic is from https://github.com/googleapis/gax-java/blob/main/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcDirectStreamController.java#L89, which is not sycnhronized either. The grpc logic has been there for a few years already and no any race conditions problems so far. Basicaly this logic is externally synchronized. I made those variables volatile just in case to make sure thread visibility. Probably I need to revert them back to non-volatile ones to eliminate confusin.

Yeah, maybe revert them back. I don't see them in the gRPC side either, and actually, it was those volatile keywords that made me to think about concurrency concerns in this class. Since you said this is externally synchronized, it should rather be explicit, I think.

This PR depends on googleapis/gax-java#1599

…p detection

meltsufin · 2022-01-19T17:04:24Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCallImpl.java

+import javax.annotation.concurrent.GuardedBy;
+
+/**
+ * This class servers as main implementation of {@link HttpJsonClientCall} for rest transport and is


Suggested change

* This class servers as main implementation of {@link HttpJsonClientCall} for rest transport and is

* This class serves as main implementation of {@link HttpJsonClientCall} for REST transport and is

gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCallImpl.java

gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCall.java

gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonApiExceptionFactory.java

vam-google · 2022-01-21T10:58:02Z

@chanseokoh @meltsufin PTAL

chanseokoh · 2022-01-20T19:35:34Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java

+  private final HttpJsonCallOptions callOptions;
+  @Nullable private final Duration timeout;
+  @Nullable private final Duration streamWaitTimeout;
+  @Nullable private final Duration streamIdleTimeout;
  private final ImmutableMap<String, List<String>> extraHeaders;
  private final ApiCallContextOptions options;
  private final ApiTracer tracer;


tracer is also @Nullable.

meltsufin

LGTM!

🤖 I have created a release *beep* *boop* --- ## [2.10.0](v2.9.0...v2.10.0) (2022-01-21) ### Features * add api key support ([#1436](#1436)) ([5081ec6](5081ec6)) * introduce HttpJsonClientCall, Listeners infrastructure and ServerStreaming support in REST transport ([#1599](#1599)) ([3c97529](3c97529)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

This PR depends on googleapis/gax-java#1599

🤖 I have created a release *beep* *boop* --- ## [2.10.0](googleapis/gax-java@v2.9.0...v2.10.0) (2022-01-21) ### Features * add api key support ([#1436](googleapis/gax-java#1436)) ([e85699a](googleapis/gax-java@e85699a)) * introduce HttpJsonClientCall, Listeners infrastructure and ServerStreaming support in REST transport ([#1599](googleapis/gax-java#1599)) ([7003903](googleapis/gax-java@7003903)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

This PR depends on googleapis/gax-java#1599

vam-google requested review from a team as code owners January 12, 2022 04:17

vam-google requested a review from igorbernstein2 January 12, 2022 04:18

vam-google changed the title ~~feat: Introduce HttpJsonClientCall, Listeners infrastructure and ServerStreaming support in REST transport~~ feat: introduce HttpJsonClientCall, Listeners infrastructure and ServerStreaming support in REST transport Jan 12, 2022

chanseokoh reviewed Jan 12, 2022

View reviewed changes

roollback duplicatesStrategy = 'include' change in build.gradle

c86cf8b

chanseokoh reviewed Jan 12, 2022

View reviewed changes

vam-google added 3 commits January 13, 2022 17:04

address PR feedback

d0faae4

add missing stream timeout configs to HttpJsonCallContext

27748aa

cleanup and rename a few things

2e2da06

vam-google added a commit to vam-google/gapic-generator-java that referenced this pull request Jan 16, 2022

feat: add server streaming support for REST transport

cfaaf87

This PR depends on googleapis/gax-java#1599

vam-google mentioned this pull request Jan 16, 2022

feat: add server streaming support for REST transport googleapis/sdk-platform-java#902

Merged

replace ReentrantLock with synchronized. Simplify active delivery loo…

0f7edc8

…p detection

meltsufin reviewed Jan 19, 2022

View reviewed changes

chanseokoh reviewed Jan 20, 2022

View reviewed changes

gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonApiExceptionFactory.java Show resolved Hide resolved

gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonApiExceptionFactory.java Show resolved Hide resolved

vam-google added 2 commits January 21, 2022 02:54

address PR feedback

912e504

Merge remote-tracking branch 'upstream/main'

ad57564

chanseokoh approved these changes Jan 21, 2022

View reviewed changes

meltsufin approved these changes Jan 21, 2022

View reviewed changes

vam-google merged commit 3c97529 into googleapis:main Jan 21, 2022

release-please bot mentioned this pull request Jan 21, 2022

chore(main): release 2.10.0 #1605

Merged

vam-google added a commit to googleapis/sdk-platform-java that referenced this pull request Jan 22, 2022

feat: add server streaming support for REST transport (#902)

3b2dec6

This PR depends on googleapis/gax-java#1599

suztomo pushed a commit to googleapis/sdk-platform-java that referenced this pull request Dec 16, 2022

feat: add server streaming support for REST transport (#902)

9d3e81b

This PR depends on googleapis/gax-java#1599

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce HttpJsonClientCall, Listeners infrastructure and ServerStreaming support in REST transport #1599

feat: introduce HttpJsonClientCall, Listeners infrastructure and ServerStreaming support in REST transport #1599

vam-google commented Jan 12, 2022 •

edited

chanseokoh Jan 12, 2022

vam-google Jan 14, 2022

chanseokoh Jan 12, 2022

vam-google Jan 14, 2022

chanseokoh Jan 18, 2022 •

edited

chanseokoh left a comment

chanseokoh Jan 12, 2022

vam-google Jan 14, 2022

chanseokoh Jan 18, 2022

chanseokoh Jan 12, 2022

vam-google Jan 14, 2022

chanseokoh Jan 18, 2022

meltsufin Jan 19, 2022

vam-google commented Jan 21, 2022

chanseokoh Jan 20, 2022

meltsufin left a comment


		Builder builder = this.toBuilder();

		Instant newDeadline = inputOptions.getDeadline();

	* This class servers as main implementation of {@link HttpJsonClientCall} for rest transport and is
	* This class serves as main implementation of {@link HttpJsonClientCall} for REST transport and is

feat: introduce HttpJsonClientCall, Listeners infrastructure and ServerStreaming support in REST transport #1599

feat: introduce HttpJsonClientCall, Listeners infrastructure and ServerStreaming support in REST transport #1599

Conversation

vam-google commented Jan 12, 2022 • edited

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

chanseokoh Jan 18, 2022 • edited

Choose a reason for hiding this comment

chanseokoh 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

vam-google commented Jan 21, 2022

Choose a reason for hiding this comment

meltsufin left a comment

Choose a reason for hiding this comment

vam-google commented Jan 12, 2022 •

edited

chanseokoh Jan 18, 2022 •

edited