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.

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

Merged
merged 6 commits into from Jun 4, 2021

Conversation

schmidt-sebastian
Copy link
Contributor

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.

@schmidt-sebastian schmidt-sebastian requested a review from a team as a code owner May 27, 2021 16:36
@schmidt-sebastian schmidt-sebastian requested a review from a team May 27, 2021 16:36
@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/java-firestore API. label May 27, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 27, 2021
Copy link
Collaborator

@kolea2 kolea2 left a comment

Choose a reason for hiding this comment

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

@schmidt-sebastian
Copy link
Contributor Author

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

@schmidt-sebastian
Copy link
Contributor Author

Thanks for the pointers. I added unit tests.

@schmidt-sebastian schmidt-sebastian added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 2, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 2, 2021
}

@Test
public void doesNotIssueRpcIfOnlyASinglePartitionIsRequested() throws Exception {
Copy link
Contributor

@crwilcox crwilcox Jun 3, 2021

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@crwilcox crwilcox Jun 3, 2021

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.

schmidt-sebastian and others added 2 commits June 3, 2021 14:19
…e/CollectionGroup.java

Co-authored-by: BenWhitehead <BenWhitehead@users.noreply.github.com>
@schmidt-sebastian schmidt-sebastian merged commit 12d17d1 into master Jun 4, 2021
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/order branch June 4, 2021 03:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/java-firestore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants