HttpJsonClientInterceptorTest.testCustomInterceptor is flaky

[blakeli0]

See https://github.com/googleapis/gax-java/runs/7581553740?check_suite_focus=true for the failure in gradle build and https://github.com/googleapis/gax-java/runs/7582163011?check_suite_focus=true for failure in maven build.

[blakeli0]

@lqiu96 Can you please fix this as part of the Maven migration? This test is very flaky in Maven so we should fix it before migrating to Maven.

[blakeli0]

See this chat thread for detailed discussion.

[lqiu96]

@blakeli0, I took a look at the original GRPC implementation for Unary calls: googleapis/gax-java#1356. It seems that this was intended to be an eager implementation: https://github.com/googleapis/gax-java/blob/abecb7cfd8680ec1a693accde5b8e08c23c0ffdb/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcClientCalls.java#L169

gRPC ClientCalls and thus gax currently wait for trailers to resolve unary call futures. I believe the original reason for this was to mitigate misconfigured servers where a server endpoint was changed to be server streaming, but the client still expects a unary method. We measured the cost of this safety net to be O(hundreds of millis). For low latency services like Bigtable, this is very high.

The eager implementation seems to be a copy of Grpc-Java's ClientCalls: https://github.com/grpc/grpc-java/blob/eb1e5a11c6397dd9809d3bb4adf142caf0007065/stub/src/main/java/io/grpc/stub/ClientCalls.java#L224 and the original non-eager implementation has the value set in onClose: https://github.com/grpc/grpc-java/blob/eb1e5a11c6397dd9809d3bb4adf142caf0007065/stub/src/main/java/io/grpc/stub/ClientCalls.java#L542

---

Possible path forward (?)

Perhaps we can copy the GRPC logic with an awaitTrailers settings (disabled by default or I guess enabled since that's how it's been running)? https://github.com/googleapis/gax-java/blob/abecb7cfd8680ec1a693accde5b8e08c23c0ffdb/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcDirectCallable.java#L52

We'll have a copy of this function called futureUnaryCall: https://github.com/googleapis/gax-java/blob/main/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java#L67.

We can update the current FutureListener to be EagerFutureListener. The new FutureListener will have the non-eager implementation where it stores the value at onMessage and sets the future onClose.

[lqiu96]

For now, we have decided to busy-wait until the interceptor has received the status code googleapis/gax-java#1851

We're going to explore some potential fixes for the issues above.

[lqiu96]

One approach that we are looking to take for minimal changes:

1. Change FutureListener's implementation to be similar to what we see in Grpc-Java (https://github.com/grpc/grpc-java/blob/eb1e5a11c6397dd9809d3bb4adf142caf0007065/stub/src/main/java/io/grpc/stub/ClientCalls.java#L542). This is the non-eager implementation -- We'll set the future's value onClose() instead of onMessage.
2. Rename eagerFutureUnaryCall to be futureUnaryCall to be true to the implementation details: https://github.com/googleapis/gax-java/blob/6a8150a9c7cb1799cedb49a27808b78346cb7f17/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java#L67. We can add an eager implementation for the services that want it in the future
3. Revert the changes in chore: Busy-Wait for status code in customInterceptorTest gax-java#1851 since we should no longer have issues with Future.get()

Per Vadym, to properly test these changes:

Step 1:
The best real-world test would be running java-compute integration tests with the changes in place. 

Step 2:
Test some of the server-streaming calls on any regapics. Server-side streaming logic relies heavily on listeners and stuff like that, so any messing with them may affect streaming logic. Search for `returns stream` across googleapis repository to find all server-streaming call candidates (Need to make sure that the methods also don't have `stream` in their input parameter as that would be a bidi-stream method)

If both java-compute and server-streaming call on any regapics passes with explicit custom interceptors then we are fine

[lqiu96]

Our preference is to update the code to a non-eager implementation (converting the old eager implementation to non-eager). If there is interest/ support for an eager implementation, we can add it in.

[lqiu96]

This has been resolved in: #1162