Skip to content

[BEAM-22089] Fix query retry in Java FirestoreIO - #22175

Merged
chamikaramj merged 1 commit into
apache:masterfrom
danthev:fix/beam_fs_query_resume
Jul 15, 2022
Merged

[BEAM-22089] Fix query retry in Java FirestoreIO#22175
chamikaramj merged 1 commit into
apache:masterfrom
danthev:fix/beam_fs_query_resume

Conversation

@danthev

@danthevdanthev commented Jul 6, 2022

Copy link
Copy Markdown
Contributor

This fixes a condition where, when a RunQuery request is retried, the cursor inferred from the last received value is set incorrectly, potentially yielding no or skipping some results. This can happen due to nested field paths in the orderBy or when the implicit order (e.g. by name) is not filled correctly. See also #22089.
This PR fixes both root-causes along with unit test cases for each.

Java tests

@danthev

danthev commented Jul 6, 2022

Copy link
Copy Markdown
ContributorAuthor

R: @yixiaoshen
R: @clement

yixiaoshen
yixiaoshen reviewed Jul 6, 2022
View reviewed changes
Comment thread...d-platform/src/test/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreV1FnRunQueryTest.java Outdated
Comment thread...d-platform/src/test/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreV1FnRunQueryTest.java Outdated
yixiaoshen
yixiaoshen reviewed Jul 7, 2022
View reviewed changes
Comment thread.../io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/QueryUtils.java Outdated
Comment thread.../io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/QueryUtils.java Outdated
Comment thread.../io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/QueryUtils.java Outdated
Comment thread.../io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/QueryUtils.java Outdated
Comment thread.../io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/QueryUtils.java Outdated
Comment thread...gle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreV1ReadFn.java Outdated
yixiaoshen
yixiaoshen reviewed Jul 8, 2022
View reviewed changes
Comment thread.../io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/QueryUtils.java Outdated
Comment thread...d-platform/src/test/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreV1FnRunQueryTest.java Outdated
@yixiaoshen

yixiaoshen commented Jul 8, 2022

Copy link
Copy Markdown
Contributor

LGTM. please also get a review from someone else.

@danthev

danthev commented Jul 8, 2022

Copy link
Copy Markdown
ContributorAuthor

R: @johnjcasey
(yixiaoshen@ has approved, review by clem@ still pending)

@johnjcasey

johnjcasey commented Jul 8, 2022

Copy link
Copy Markdown
Contributor

Nothing looks bad to me, but a lot of this is firestore specific, so its hard to judge from a BeamIO perspective

clement
clement suggested changes Jul 8, 2022
View reviewed changes
Comment thread...gle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreV1ReadFn.java Outdated
Comment thread...gle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreV1ReadFn.java Outdated
Comment thread...gle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreV1ReadFn.java Outdated
Comment thread...gle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreV1ReadFn.java Outdated
Comment thread...gle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreV1ReadFn.java Outdated
Comment thread.../io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/QueryUtils.java Outdated
Comment thread.../io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/QueryUtils.java Outdated
Comment thread.../io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/QueryUtils.java Outdated
Comment thread.../io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/QueryUtils.java Outdated
clement
clement reviewed Jul 9, 2022
View reviewed changes
Comment thread.../io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/QueryUtils.java Outdated
Comment thread.../io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/QueryUtils.java Outdated
Comment thread...gle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreV1ReadFn.java Outdated
@danthev

danthev commented Jul 12, 2022

Copy link
Copy Markdown
ContributorAuthor

Code is ready again, fixed some spotless errors after adding all the unit tests, filter sorting etc.

clement
clement suggested changes Jul 12, 2022
View reviewed changes
Comment thread.../io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/QueryUtils.java Outdated
Comment thread.../io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/QueryUtils.java Outdated
Comment thread...google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/firestore/QueryUtilsTest.java Outdated
clement
clement approved these changes Jul 12, 2022
View reviewed changes
Comment thread.../io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/QueryUtils.java Outdated
@danthev
danthevforce-pushed the fix/beam_fs_query_resume branch from 73ed328 to c314775CompareJuly 12, 2022 23:02
@danthev
danthevforce-pushed the fix/beam_fs_query_resume branch from c314775 to ad3af31CompareJuly 12, 2022 23:30
johnjcasey
johnjcasey approved these changes Jul 13, 2022
View reviewed changes

@johnjcaseyjohnjcasey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, though I think a lot of this is very low level encoding work, and should probably be pushed down to the Firestore library itself

@johnjcasey

johnjcasey commented Jul 14, 2022

Copy link
Copy Markdown
Contributor

@pabloem@chamikaramj for final pass & merge

@chamikaramj
chamikaramj merged commit 2b42751 into apache:masterJul 15, 2022
danthev pushed a commit to danthev/beam that referenced this pull request Jul 15, 2022
@chamikaramj@danthev
Merge pull requestapache#22175: [BEAM-22089] Fix query retry in Java…
e5aacb5
… FirestoreIO
@chamikaramjchamikaramj mentioned this pull request Jul 15, 2022
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@johnjcaseyjohnjcaseyjohnjcasey approved these changes
+2 more reviewers
@yixiaoshenyixiaoshenyixiaoshen left review comments
@clementclementclement approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

gcpiojava

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@danthev@yixiaoshen@johnjcasey@clement@chamikaramj