SEZ9 commented on PR #12454: URL: https://github.com/apache/seatunnel/pull/12454#issuecomment-5842400071
Thanks @goutamadwant, this is exactly the kind of data that makes the scoping call easy. Running each diff against `dev` at `deb16a3c3` in a fresh JVM with `-XX:ActiveProcessorCount=2` on JDK 11 is a fair reproduction of the flake conditions, and the numbers are clear: - `CheckpointCoordinatorFailoverIT`: `dev` 10/78 failed, this PR 0/78, #12449 0/78. Same injection point (`pendingCounter == 0` under the coordinator lock), but #12449 achieves it without adding a production accessor, which I prefer. - `PayPalClientTest#closeWakesRetryWait`: `dev` 5/50 (20/20 with the forced `IOException`), this PR and #12444 both 0/50 and 0/20. Equivalent fixes. - `TestFilterRowKindIT#testFilterRowKindMultiTable` on Flink 1.15/1.18: `dev` 8/14, `parallelism = 1` 0/6, #12216 0/13. I agree with your recommendation. Since #12449 and #12444 each already cover one of these in isolation and are small, keeping duplicate copies here only invites three-way conflicts on the same files. Concrete asks for this PR: 1. Drop the `runWhenNoCheckpointPending` accessor and the `CheckpointCoordinatorFailoverIT` change; let #12449 land that. 2. Drop the `PayPalClientTest` / `PayPalIT` changes; let #12444 land that. 3. Keep the `TestFilterRowKindIT` `parallelism = 1` conf change as a stopgap, with a one-line comment pointing at #12216 as the real fix for the Assert-sink race across all multi-table confs. 4. Keep the `TaskExecutionService.submitBlockingTask` / `PhysicalVertex` recovery fix and the PostgreSQL CDC LSN backport (`PostgresOffsetContext` / `WalPositionLocator` / `PostgresStreamingChangeEventSource`) as the core of the PR, and update the description so the title and summary reflect that narrower scope. Once the scope is trimmed, please post the resulting diff stat so it can be re-reviewed against the reduced file set. The earlier approval was on head `1ce4248c7ecb`; a new head will need a fresh pass, which should be quick given the two remaining pieces were already traced in detail. One note for whoever picks up the recovery fix review: the CDC LSN part has not been stress-tested in this thread, so if you have a savepoint-restore scenario handy where a commit and the next transaction's first event share a WAL LSN, a before/after run of that would be very welcome. Not a blocker, but it would close the last gap in the evidence here. <!-- streview-comment:1323 --> -- 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]
