wombatu-kun commented on code in PR #19202:
URL: https://github.com/apache/hudi/pull/19202#discussion_r3584846720
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/table/format/cdc/CdcIterators.java:
##########
@@ -135,14 +153,25 @@ public boolean hasNext() {
}
@Override
- public RowData next() {
+ public synchronized RowData next() {
Review Comment:
The CDC case differs from causes (2)/(3). Those Parquet-reader symptoms are
a benign teardown race where a closed reader throwing is the correct production
behavior (a genuine mid-read stream close should fail the job), so they are
tolerated in the test only. Here `close()` nulls
`recordIterator`/`imageManager`/`cdcItr`, which the task thread reads without
synchronization, so it is a real cross-thread visibility race: a closed
iterator should return end-of-stream cleanly rather than NPE. That is why this
one carries a production guard, not just test tolerance.
The test-side tolerance is still required on top of the prod fix:
`BatchRecords` calls `hasNext()` then `next()` as two separate calls, so a
`close()` landing between them produces a deterministic
`NoSuchElementException` under `maxNumberRestartAttempts=0`, which the
production synchronization alone cannot swallow. Hence both layers.
@danny0405 this is also the crux of your caller-side question: I am happy to
move the teardown cleanup onto the task-thread drain path instead if you prefer
that route, but as it stands the iterator-side guard is what releases the
`CdcImageManager` and the file-group readers when a queued batch is never
drained on a forced cancel. Let me know which you would like.
--
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]
