fix: return results from getPartitions() in order#653
fix: return results from getPartitions() in order#653schmidt-sebastian merged 6 commits intomasterfrom
Conversation
product-auto-label
bot
added
the
api: firestore
google-cla
bot
added
the
cla: yes
kolea2
left a comment
kolea2
left a comment
There was a problem hiding this comment.
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. |
schmidt-sebastian
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
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.
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.
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.
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 <[email protected]>
🤖 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).
5 participants
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.