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.
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
fix: return results from getPartitions() in order #653
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this example of mocking paged responses help? Maybe we could add something similar here? https://github.com/googleapis/java-bigtable/blob/master/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/admin/v2/BigtableInstanceAdminClientTest.java#L647-L663
@kolea2 It might - I will see if I have some cycles to flush this out. I think we should not block this bug fix on it though, as the code does get executed by the existing integration tests. |
Mocking of PartitionQuery paged responses is present in my beam work https://github.com/BenWhitehead/beam/blob/66c203fced82b938c2251d21fa45e04a6b6e42d9/sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreV1FnPartitionQueryTest.java#L48 |
Thanks for the pointers. I added unit tests. |
google-cloud-firestore/src/main/java/com/google/cloud/firestore/CollectionGroup.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
@Test | ||
public void doesNotIssueRpcIfOnlyASinglePartitionIsRequested() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an interesting optimization. And also an unlikely case? Is it worth special handling in libs or should we just let the backend respond with no partitions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The backend returns an error when 0 partitions are requested. We should catch this ourselves since we request one less than what the user requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already ensured we always returned a minimum of 1 QueryPartition. I just added the short circuit in golang though, since it is the case we don't really need to request anything.
…e/CollectionGroup.java Co-authored-by: BenWhitehead <BenWhitehead@users.noreply.github.com>
🤖 I have created a release \*beep\* \*boop\* --- ### [2.5.1](https://www.github.com/googleapis/java-firestore/compare/v2.5.0...v2.5.1) (2021-06-22) ### Bug Fixes * return results from getPartitions() in order ([#653](https://www.github.com/googleapis/java-firestore/issues/653)) ([12d17d1](https://www.github.com/googleapis/java-firestore/commit/12d17d1ac9d7a1c21eca1469164b079de4476633)) ### Dependencies * update dependency com.google.cloud:google-cloud-conformance-tests to v0.1.1 ([#650](https://www.github.com/googleapis/java-firestore/issues/650)) ([b93ca8a](https://www.github.com/googleapis/java-firestore/commit/b93ca8a2b5751c61b3fbe0ca608056e2c0398575)) * update dependency com.google.cloud:google-cloud-shared-dependencies to v1.3.0 ([#660](https://www.github.com/googleapis/java-firestore/issues/660)) ([0f13fd0](https://www.github.com/googleapis/java-firestore/commit/0f13fd0c0db0208b9f68a57dabcb1e998b4a7b9b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
The Partition RPC does not guarantee that results are returned in order, which means we have to sort the partitions before we can use them as cursors.
Port of googleapis/nodejs-firestore#1521
We don't have unit tests for PartitionQueries as we never figured out how to mock paged respones.