Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add logical termination for RunQueryResponse #956

Merged
merged 6 commits into from
May 26, 2022
Merged

feat: add logical termination for RunQueryResponse #956

merged 6 commits into from
May 26, 2022

Conversation

cherylEnkidu
Copy link
Contributor

Add logical termination for RunQueryResponse.

When the SDK side receives either halfClose or RunQueryResponse.done set to true, the SDK will return user requested documents.

@cherylEnkidu cherylEnkidu requested a review from a team as a code owner April 29, 2022 17:27
@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/java-firestore API. label Apr 29, 2022
@cherylEnkidu cherylEnkidu requested a review from wu-hui May 2, 2022 18:11
@cherylEnkidu cherylEnkidu changed the title [DRAFT] feat: add logical termination for RunQueryResponse feat: add logical termination for RunQueryResponse May 2, 2022
wu-hui
wu-hui reviewed May 5, 2022
View reviewed changes
Copy link
Contributor

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting some minor changes.

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java Outdated Show resolved Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java Outdated Show resolved Hide resolved
google-cloud-firestore/src/test/java/com/google/cloud/firestore/QueryTest.java Show resolved Hide resolved
@wu-hui wu-hui assigned cherylEnkidu and unassigned wu-hui May 5, 2022
@cherylEnkidu cherylEnkidu assigned wu-hui and unassigned cherylEnkidu May 7, 2022
wu-hui
wu-hui reviewed May 9, 2022
View reviewed changes
Copy link
Contributor

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are almost done here..

google-cloud-firestore/src/test/java/com/google/cloud/firestore/QueryTest.java Outdated Show resolved Hide resolved
google-cloud-firestore/src/test/java/com/google/cloud/firestore/QueryTest.java Show resolved Hide resolved
google-cloud-firestore/src/test/java/com/google/cloud/firestore/QueryTest.java Show resolved Hide resolved
@wu-hui wu-hui assigned cherylEnkidu and unassigned wu-hui May 12, 2022
@cherylEnkidu cherylEnkidu requested a review from a team as a code owner May 17, 2022 19:21
@cherylEnkidu
Copy link
Contributor Author

@wu-hui
There are two functions using internalStream(); One is stream(@Nonnull final ApiStreamObserver<DocumentSnapshot> responseObserver) and the other one is get(@Nullable ByteString transactionId) . Unfortunately I only know how to create tests for stream() . Please let me know if you have any ideas on unit tests for get() . Otherwise I have to blindly trust it :(

@cherylEnkidu cherylEnkidu assigned wu-hui and unassigned cherylEnkidu May 17, 2022
@wu-hui
Copy link
Contributor

wu-hui commented May 24, 2022

@wu-hui There are two functions using internalStream(); One is stream(@Nonnull final ApiStreamObserver<DocumentSnapshot> responseObserver) and the other one is get(@Nullable ByteString transactionId) . Unfortunately I only know how to create tests for stream() . Please let me know if you have any ideas on unit tests for get() . Otherwise I have to blindly trust it :(

I think this is fine.

wu-hui
wu-hui approved these changes May 24, 2022
View reviewed changes
google-cloud-firestore/src/test/java/com/google/cloud/firestore/QueryTest.java
semaphore.release();
}
});

semaphore.acquire();

Thread.sleep(200);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment to explain why we are doing this

@wu-hui wu-hui assigned cherylEnkidu and unassigned wu-hui May 24, 2022
@cherylEnkidu cherylEnkidu merged commit 1d869c8 into main May 26, 2022
@cherylEnkidu cherylEnkidu deleted the cheryllin/RunQueryResponse branch May 26, 2022 16:10
@release-please release-please bot mentioned this pull request Jul 1, 2022
Merged
gcf-merge-on-green bot pushed a commit that referenced this pull request Jul 2, 2022
@release-please
chore(main): release 3.3.0 (#980)
f46fbe4 
🤖 I have created a release *beep* *boop*
---


## [3.3.0](v3.2.0...v3.3.0) (2022-07-01)


### Features

* add logical termination for RunQueryResponse ([#956](#956)) ([1d869c8](1d869c8))


### Bug Fixes

* add build script for JDK 17 native image testing ([#965](#965)) ([963e384](963e384))


### Performance Improvements

* Change how proto was called in Query.java ([#970](#970)) ([f6f6352](f6f6352))


### Dependencies

* update beam.version to v2.40.0 ([#978](#978)) ([44276f8](44276f8))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v2.13.0 ([#974](#974)) ([6def73d](6def73d))
* update dependency org.graalvm.buildtools:native-maven-plugin to v0.9.12 ([#973](#973)) ([bfb4ec9](bfb4ec9))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
cherylEnkidu pushed a commit that referenced this pull request Dec 11, 2023
@release-please @cherylEnkidu
chore(main): release 3.3.0 (#980)
01b0ae8 
🤖 I have created a release *beep* *boop*
---


## [3.3.0](v3.2.0...v3.3.0) (2022-07-01)


### Features

* add logical termination for RunQueryResponse ([#956](#956)) ([1d869c8](1d869c8))


### Bug Fixes

* add build script for JDK 17 native image testing ([#965](#965)) ([963e384](963e384))


### Performance Improvements

* Change how proto was called in Query.java ([#970](#970)) ([f6f6352](f6f6352))


### Dependencies

* update beam.version to v2.40.0 ([#978](#978)) ([44276f8](44276f8))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v2.13.0 ([#974](#974)) ([6def73d](6def73d))
* update dependency org.graalvm.buildtools:native-maven-plugin to v0.9.12 ([#973](#973)) ([bfb4ec9](bfb4ec9))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wu-hui wu-hui wu-hui approved these changes

Assignees

@cherylEnkidu cherylEnkidu

Labels
api: firestore Issues related to the googleapis/java-firestore API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cherylEnkidu @wu-hui