feat: Update to non-eager FutureListener #1878

lqiu96 · 2022-11-14T20:32:23Z

Works on issue found in googleapis/sdk-platform-java#1322

sonarqubecloud · 2022-11-14T20:35:01Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

lqiu96 · 2022-11-15T15:44:14Z

Running java-compute ITs locally with this build of gax-java:

[INFO] Skipping execution
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Google Compute Engine Parent 1.15.1-SNAPSHOT:
[INFO]
[INFO] Google Compute Engine Parent ....................... SUCCESS [  0.845 s]
[INFO] proto-google-cloud-compute-v1 ...................... SUCCESS [  0.723 s]
[INFO] Google Compute Engine .............................. SUCCESS [06:45 min]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  06:49 min (Wall Clock)
[INFO] Finished at: 2022-11-15T10:41:49-05:00
[INFO] ------------------------------------------------------------------------
Finished Integration Tests for:
java-compute

It passes with the httpjson changes!

lqiu96 · 2022-11-15T15:48:05Z

I'll need to complete part 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)

Potential server-streaming call candidate from aiplatform:

  rpc StreamingReadFeatureValues(StreamingReadFeatureValuesRequest) returns (stream ReadFeatureValuesResponse) {
    option (google.api.http) = {
      post: "/v1/{entity_type=projects/*/locations/*/featurestores/*/entityTypes/*}:streamingReadFeatureValues"
      body: "*"
    };
    option (google.api.method_signature) = "entity_type";
  }

Seems like this proto doesn't have a Http Stub?

lqiu96 · 2022-12-06T16:28:27Z

Taking another look at this PR now. Seems like AIPlatform module is only GRPC based. I can't seem to find any other good candidates for server side streaming RPCs. Going to take a look at some of the handwritten libraries.

Spanner:

  rpc StreamingRead(ReadRequest) returns (stream PartialResultSet) {
    option (google.api.http) = {
      post: "/v1/{session=projects/*/instances/*/databases/*/sessions/*}:streamingRead"
      body: "*"
    };
  }

Trying to create a repro/ sample here: https://github.com/lqiu96/ServerSideStreamingHttpJson/tree/main

lqiu96 · 2022-12-07T16:38:34Z

Ran this with the httpjson module changes in the branch and it works. The modified code is being called and the result is valid.

blakeli0 · 2022-12-15T15:56:49Z

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

    @Override
    public void onMessage(T message) {
-      if (!future.set(message)) {
+      if (isMessageReceived) {


Looks like isMessageReceived and message are always set together, can we just check if message is null?

I think you're right. I wasn't sure if parameter message would ever be passed in as null, but it seems like that won't be the case. I think unknown fields will be ignored and null values will be set to their default values and any invalid JSON would have an exception throw so we should be good. I'll make the change

I gave it more thought and I think we should revert to your previous approach. The logic is fine in onMessage, but in onClose, we can not use if(storedMessage == null) to determine anything because we cannot distinguish if the response itself is null or we haven't received response yet.

Sounds good. I can revert to the previous way.

…e_to_noneager_futurelistener

lqiu96 · 2022-12-15T22:20:10Z

Added a few more tests cases. The tests cover:

Successful Unary Call (200 status code + Message returned)
Error Response from Unary Call (404 status code)
Cases for multiple responses from Unary Call
Null Content Success Response (200 status code + Null returned)
Null Content Error Response (400 status code + Null returned)
Non 4xx Error response

blakeli0 · 2022-12-15T22:33:57Z

gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java

+
+    Field request;
+    Field expectedResponse;
+    request = expectedResponse = createTestMessage(2);


I feel like

Field request = createTestMessage(2); Field expectedResponse = createTestMessage(2);

is clearer.
Also I'm not sure if the name makes sense, if we are asserting actual response to be randomResponse1, then we should probably rename randomResponse1 to expectedResponse.

Sure, I can update.

I wanted the assertions below to show that it wasn't the expected response (given that it's not throwing an exception but just returning the first response back):

// Gax returns the first response for Unary Call assertThat(actualResponse).isEqualTo(randomResponse1); assertThat(actualResponse).isNotEqualTo(expectedResponse);

lqiu96 · 2022-12-16T17:11:45Z

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

      } else if (statusCode < 200 || statusCode >= 400) {
        LOGGER.log(
            Level.WARNING, "Received error for unary call after receiving a successful response");
+      } else {


Confirm: Can this be removed?

lqiu96 · 2022-12-16T17:14:18Z

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

+      if (storedMessage != null) {
        throw new IllegalStateException("More than one value received for unary call");
      }
+      storedMessage = message;


Confirm: Is the future done on here? What if the runnableResult has an exception?

lqiu96 · 2022-12-16T17:27:00Z

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

    @Override
    public void onClose(int statusCode, HttpJsonMetadata trailers) {
-      if (!future.isDone()) {
+      if (storedMessage == null) {


Should the logic for this be done via Status Code? Check for 2xx Status Codes?

sonarqubecloud · 2022-12-16T22:16:26Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

90.9% Coverage
0.0% Duplication

gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java

lqiu96 · 2022-12-19T20:51:02Z

Since we're migrating to the gapic monorepo, I will create a patch file and apply the changes in the monorepo. Once that's been done I will close this issue. I'll reference this PR in that repo.

lqiu96 added 2 commits November 10, 2022 16:31

fix: Use non-eager FutureListener for httpjson

5407004

chore: Fix onClose logic

a082acc

lqiu96 marked this pull request as ready for review November 15, 2022 02:22

lqiu96 requested a review from a team November 15, 2022 02:22

lqiu96 requested review from a team as code owners November 15, 2022 02:22

lqiu96 requested a review from blakeli0 November 15, 2022 02:23

blakeli0 reviewed Dec 15, 2022

View reviewed changes

lqiu96 added 5 commits December 15, 2022 12:01

chore: Check if storedMessage is null

4442eb1

chore: Add test cases for non eager listener

972ec87

chore: Fix tests

f307b6b

Merge branch 'main' of github.com:googleapis/gax-java into main-updat…

52e4468

…e_to_noneager_futurelistener

chore: Fix formatting

8ff7feb

blakeli0 reviewed Dec 15, 2022

View reviewed changes

lqiu96 commented Dec 16, 2022

View reviewed changes

chore: Fix comments

dc0a6fd

blakeli0 reviewed Dec 19, 2022

View reviewed changes

gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java Show resolved Hide resolved

lqiu96 mentioned this pull request Dec 19, 2022

feat: Update to non-eager FutureListener googleapis/sdk-platform-java#1162

Merged

lqiu96 closed this Dec 19, 2022

feat: Update to non-eager FutureListener #1878

feat: Update to non-eager FutureListener #1878

Uh oh!

Conversation

lqiu96 commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud bot commented Nov 14, 2022

Uh oh!

lqiu96 commented Nov 15, 2022

Uh oh!

lqiu96 commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lqiu96 commented Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lqiu96 commented Dec 7, 2022

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!

lqiu96 commented Dec 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lqiu96 Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sonarqubecloud bot commented Dec 16, 2022

Uh oh!

Uh oh!

lqiu96 commented Dec 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lqiu96 commented Nov 14, 2022 •

edited

Loading

lqiu96 commented Nov 15, 2022 •

edited

Loading

lqiu96 commented Dec 6, 2022 •

edited

Loading

lqiu96 Dec 15, 2022 •

edited

Loading