gemini-code-assist[bot] commented on code in PR #38695:
URL: https://github.com/apache/beam/pull/38695#discussion_r3304028579
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/action/HeartbeatRecordAction.java:
##########
@@ -104,6 +104,6 @@ public Optional<ProcessContinuation> run(
return Optional.empty();
}
// no new data, finish reading data
- return cancelQueryOnHeartbeat ? Optional.empty() :
Optional.of(ProcessContinuation.resume());
+ return cancelQueryOnHeartbeat ? Optional.of(ProcessContinuation.resume())
: Optional.empty();
Review Comment:
Inverting the ternary operator here makes the behavior of
`cancelQueryOnHeartbeat` opposite to its name:
- If `cancelQueryOnHeartbeat` is `true`, it now returns
`ProcessContinuation.resume()` (meaning it does *not* cancel the query on
heartbeat).
- If `cancelQueryOnHeartbeat` is `false`, it now returns `Optional.empty()`
(meaning it *does* cancel the query on heartbeat).
This also forced the tests to be inverted in a counter-intuitive way (e.g.,
`testEndTimestampNotReachedOnCancellingAction` now asserts `resume()`, and
`testEndTimestampNotReachedOnAction` now asserts `Optional.empty()`).
If the issue is that the feature is enabled by default when it shouldn't be,
the correct fix is to change the default value of `cancelQueryOnHeartbeat` to
`false` where it is initialized or configured (e.g., in the SpannerIO connector
configuration or builder), rather than inverting the core logic here. Please
revert this change and the test changes, and instead update the default
configuration value of `cancelQueryOnHeartbeat` to `false`.
```suggestion
return cancelQueryOnHeartbeat ? Optional.empty() :
Optional.of(ProcessContinuation.resume());
```
--
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]
