corgy-w opened a new pull request, #12727:
URL: https://github.com/apache/seatunnel/pull/12727
### What
`ClickhouseValueReader.StreamValueReader.hasNext()` now fails the read when
it is interrupted, instead of swallowing the interrupt and polling for batches
forever.
### Why
The batch-wait loop has two branches, and they disagreed about interrupts:
- `ClickhouseValueReader.java:509-519` (`rowQueue.take()`): an
`InterruptedException` is rethrown as
`ClickhouseConnectorException(ROW_BATCH_GET_FAILED, e)`.
- `ClickhouseValueReader.java:520-525` (`Thread.sleep(10)`, the branch taken
while the streaming query has not delivered its first batch yet): `catch
(InterruptedException ignored) {}`.
Swallowing the interrupt has two effects, and both hurt:
1. `Thread.sleep` **clears** the thread's interrupt status when it throws,
so the cancellation signal is lost at exactly the point that is supposed to end
the wait.
2. With `eos` still `false` and an empty queue, the loop simply keeps
polling. A reader that is waiting for its first batch from a slow or stuck
query cannot be cancelled: the task stays in `RUNNING` and never fails, and
`close()` cannot break the loop either (the reader thread reaches `close()`
only after `pollNext()` returns).
The sibling Doris reader had the same defect and it was just fixed the same
way (#12325, merged as `5127a0a108`): restore the flag, then throw
`ROW_BATCH_GET_FAILED`.
### Fix
```java
} else {
try {
Thread.sleep(10);
} catch (InterruptedException e) {
// Report the interrupt instead of swallowing it: clearing the flag
here
// would drop the cancellation signal and keep polling for a batch
forever.
Thread.currentThread().interrupt();
throw new ClickhouseConnectorException(
ClickhouseConnectorErrorCode.ROW_BATCH_GET_FAILED, e);
}
}
```
The `rowQueue.take()` branch gets the same
`Thread.currentThread().interrupt()` for symmetry (it already threw, but it
also lost the flag on the way out).
### How verified
New test in the existing `ClickhouseValueReaderTest` — no new test class:
`testStreamReaderHasNextFailsOnInterruptWhileWaitingForBatch` keeps the
streaming query in flight (so `eos` stays `false` and the polling branch is the
one running), runs `hasNext()` on a helper thread, interrupts it, and asserts
that the read fails with `ClickhouseConnectorException` whose cause is the
`InterruptedException`, and that the interrupt flag is still set afterwards.
- With this change: `Tests run: 12, Failures: 0, Errors: 0, Skipped: 0`
(`mvn -o -pl seatunnel-connectors-v2/connector-clickhouse test
-Dtest=ClickhouseValueReaderTest`).
- Replacing only `ClickhouseValueReader.java` with its `dev` version and
running the new test:
`AssertionFailedError: hasNext() swallowed the interrupt and kept polling
==> expected: <false> but was: <true>` (the helper thread was still spinning
after 30s).
### E2E coverage
`connector-clickhouse-e2e` exists, but this path only triggers while a
streaming query is in flight and the reader is waiting for its first batch;
reproducing it as an IT would require cancelling a live job mid-read and
asserting on the failure. The deterministic unit test above covers the branch
directly, so no IT is added here — happy to add one if you would rather see it
at that level.
### Compatibility
No option name, default value, or public API change. Only the interrupt path
of the streaming reader is affected: a cancelled read now fails with
`ROW_BATCH_GET_FAILED` instead of polling forever. The non-streaming strategies
are untouched.
--
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]
