-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Use now + 2mins as the end timestamp for change stream read API if the connector endTimestamp is omitted #34967
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
Conversation
4d78dc3
to
6d6b262
Compare
6d6b262
to
953531c
Compare
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
953531c
to
3585ffe
Compare
Checked failed integration test, not relevant to this PR. No need to block the review. |
assign set of reviewers |
Assigning reviewers: R: @m-trieu for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @robertwb for label java. Available commands:
|
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.
LTGM
Test failures appear to be unrelated to this change
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.
Did we run a pipeline to test this?
...in/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/action/QueryChangeStreamAction.java
Outdated
Show resolved
Hide resolved
Yes I ran a pipeline containing this change in order to run a connector without specifying end timestamp. However, I don't record any evidence for that. If you have opinion, I can run again and put evidence somewhere maybe here. |
Just so I understand things correctly:
Was wondering if you could confirm? |
3585ffe
to
33798df
Compare
Please let me know if more details/explanation is needed. Thanks! |
Friendly ping :) |
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.
Lgtm
please fix:
|
connector endTimestamp is omitted.
33798df
to
3d76a66
Compare
V1 change stream can use null end timestamp for the query, however V2 the end timestamp of the query should be NOT NULL, and should be at most 30 mins from the max(now, start_timestamp).
To allow users to still omit the connector endTimestamp field to run the connector forever, but to give a valid endTimestamp when try to query change stream, we set the change stream endTimestamp in this case as now + 2 mins.
This solution works as the Apache beam checkpoints the ReadChangeStreamPartition execution every 5s or 5MB of output data produced.
Moreover the change stream query has a hard 1 min deadline.