HttpJsonClientInterceptorTest.testCustomInterceptor is flaky #1322

Activity

1. blakeli0

   added

   type: bugError or flaw in code with unintended results or allowing sub-optimal usage patterns.Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

   priority: p2Moderately-important priority. Fix may not be included in next release.Moderately-important priority. Fix may not be included in next release.

   on Jul 29, 2022
2. meltsufin

   added

   priority: p3Desirable enhancement or fix. May not be included in next release.Desirable enhancement or fix. May not be included in next release.

   and removed

   priority: p2Moderately-important priority. Fix may not be included in next release.Moderately-important priority. Fix may not be included in next release.

   on Sep 28, 2022
3. blakeli0 commented on Oct 27, 2022

   

   blakeli0

   on Oct 27, 2022

   ContributorAuthor

   More actions

   @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.
4. blakeli0

   assigned

   lqiu96

   on Oct 27, 2022
5. blakeli0 commented on Nov 4, 2022

   

   blakeli0

   on Nov 4, 2022

   ContributorAuthor

   More actions

   See this chat thread for detailed discussion.
6. lqiu96 commented on Nov 9, 2022

   

   lqiu96

   on Nov 9, 2022

   Last edited by lqiu96

   Member

   More actions

   @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.
7. lqiu96 commented on Nov 9, 2022

   

   lqiu96

   on Nov 9, 2022

   Member

   More actions

   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.
8. lqiu96 commented on Nov 10, 2022

   

   lqiu96

   on Nov 10, 2022

   Last edited by lqiu96

   Member

   More actions

   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
9. lqiu96 commented on Nov 15, 2022

   

   lqiu96

   on Nov 15, 2022

   Member

   More actions

   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.
10. ddixit14

    transferred this issue fromgoogleapis/gax-javaon Feb 8, 2023
11. ddixit14

    added

    gax

    on Feb 8, 2023
12. lqiu96 commented on Mar 22, 2023

    

    lqiu96

    on Mar 22, 2023

    Member

    More actions

    This has been resolved in: #1162
13. lqiu96

    closed this as completedon Mar 22, 2023