DanielLeens commented on PR #11995: URL: https://github.com/apache/seatunnel/pull/11995#issuecomment-5459114729
Thanks for the quick follow-up on Issue 2, and for adding the "Known limitation" section to the PR description — that's the right home for it. One factual correction on that section though, since it affects how safe the documented limitation actually is: the description currently says "all existing Assert E2E configs use `parallelism = 1`", but that's not accurate. The two configs I cited in Issue 2 are on `dev` right now, unmodified by this PR: - `seatunnel-e2e/seatunnel-connector-v2-e2e/connector-iceberg-e2e/src/test/resources/iceberg/filter_iceberg_source.conf` — line 22 sets `parallelism = 2`, and lines 48-54 configure Assert `row_rules` with `MAX_ROW`/`MIN_ROW` on that same source. - `seatunnel-e2e/seatunnel-connector-v2-e2e/connector-iceberg-e2e/src/test/resources/iceberg/filter_iceberg_source_tables.conf` — same pattern, `parallelism = 2` at line 22, `MAX_ROW`/`MIN_ROW` rules at lines 52-58. I re-checked both directly against `apache/seatunnel:dev` just now (this PR doesn't touch either file) to be sure I wasn't citing something stale. So the residual risk in Issue 2 isn't just a hypothetical single-table-parallelism scenario — it's the exact combination two shipped e2e configs currently run. Could you update the "Known limitation" wording to reflect that (e.g. "existing configs that combine `parallelism > 1` with row-count assertions on a single table, such as `filter_iceberg_source.conf`, are exposed to this pre-existing race — not fixed by this PR, tracked as a follow-up")? I don't want a reviewer or user reading "all existing configs use parallelism = 1" and concluding this is purely theoretical when it isn't. To be clear, I'm not asking you to fix Issue 2 in this PR — I agreed in my last review it's a reasonable follow-up, not a blocker here. This is just about the description being accurate about scope. Separately, Issue 1 (the one-line comment on `close()` explaining why no wait/sync is needed post-sleep-removal) and Issue 3 (class Javadoc on `AssertSinkWriterCloseTest`) are still open — I don't see a new commit on the branch since my last review (head is still `67162eeb`), so there's nothing new to re-verify on those yet. I'll do a full pass again once the fixes land. -- 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]
