gkhnelbstn commented on PR #12412:
URL: https://github.com/apache/seatunnel/pull/12412#issuecomment-5888294710
Thanks both, and thanks for the welcome. Taking the points in order, in
095da88.
**1. Hidden lock precondition (@SEZ9, @DanielLeens).** I went with moving
the interval wait into `pollNext`, and made it a plain `Thread.sleep` after the
`synchronized` block instead of a `wait` inside it:
```java
synchronized (output.getCheckpointLock()) {
internalPollNext(output);
}
boolean finished =
Boundedness.BOUNDED.equals(context.getBoundedness()) &&
noMoreElementFlag;
if (!finished && httpParameter.getPollIntervalMillis() > 0) {
Thread.sleep(httpParameter.getPollIntervalMillis());
}
```
`internalPollNext` no longer waits or sleeps, and its `finally` keeps only
the bounded-mode `signalNoMoreElement()`. So it has no monitor precondition,
the lock is never re-fetched, and there is no spurious-wakeup case. The
condition is the same one the old `else` branch used. Airtable and Splunk
inherit `pollNext` unchanged, so they get the fix too.
One behaviour change: the sleep used to sit in the `finally`, so a failing
poll waited one interval before its exception propagated. Now the exception
propagates straight away. Nothing depended on that delay: every caller of
`pollNext` (`SourceFlowLifeCycle.collect`, `ParallelSource`,
`CoordinatedSource`) lets the exception fail the reader, so there is no retry
loop that it was throttling.
**2. GraphQL and Prometheus.** Same change in `GraphQLSourceHttpReader` and
`PrometheusSourceReader`. Their `pollNext`/`internalPollNext` had exactly the
same shape, except the "finished" check is just `BOUNDED`, which I kept.
**3. Regression test.** Added `testPollIntervalDoesNotHoldCheckpointLock` to
`HttpSourceReaderInternalPollNextTest`, following your outline. It sets a 60 s
interval on an unbounded reader and runs `pollNext` on a second thread. A latch
in the mocked `execute` fires while the reader holds the lock. The test then
asserts that a `synchronized (lock)` task gets in within 10 s. There are no
fixed sleeps, and `shutdownNow()` interrupts the poller in `finally`. I checked
it against the old code: with `Thread.sleep` back inside the lock, the barrier
task times out and the test fails.
**4. Pagination walk.** Agreed that it's pre-existing and out of scope here.
Happy to open a follow-up issue for it if that's useful.
**CI.** Thanks for the pointer. I'll enable the workflows on my fork and
push again, so the Build check has a run to find.
Locally, in an `eclipse-temurin:17-jdk` container: `spotless:check` is clean
and `HttpSourceReaderInternalPollNextTest` passes on `connector-http-base`.
`connector-graphql` and `connector-prometheus` build.
--
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]
