tianz101 commented on code in PR #37326:
URL: https://github.com/apache/beam/pull/37326#discussion_r2710343613
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/action/QueryChangeStreamAction.java:
##########
@@ -179,10 +178,9 @@ public ProcessContinuation run(
final String token = partition.getPartitionToken();
final Timestamp startTimestamp = tracker.currentRestriction().getFrom();
final Timestamp endTimestamp = partition.getEndTimestamp();
Review Comment:
We would like to get the right behavior agreed through this comment first.
It may need multiple PR's to fix.
Here endTs from the partition can be
1) bounded (i.e., a specific end ts specified by user) or
2) unbounded(i.e., MAX_INCLUSIVE_END_AT).
If bounded,
1.1) For change stream V1, we can use the bounded endTs to query.
1.2) For change stream V2, we can not use the bounded endTs to query as
validation may fail endTs <= max(startTs, now) + 30m and return exception. So
we should use min(endTs, now+2m) to query.
We can unify 1.1) and 1.2) as min(endTs, now+2m) to query.
If unbounded,
2.1) For change stream V1, we can use the unbounded endTs to query.
2.2) For change stream V2 we can not use the unbounded endTs to query as
validation may fail endTs <= max(startTs, now) + 30min and return exception.
So we should use now+2m instead.
We can unify 2.1) and 2.2) as min(endTs, now+2m) to query as well.
So in summary, we do not need to know v1 or v2, and consistently use
min(endTs, now+2m) to query no matter v1 or v2, bounded or unbounded.
Feel free to poke a hole if this comment is not accurate. But I agree that
unify to min(endTs, now+2m) can be risky as it changes V1 existing behaviors
and may need more testing and more follow up PR's.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
