SEZ9 commented on PR #12081: URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-5986937594
@Rangsh thanks for re-dispatching on `1cbac0086` with no code changes (run 36520988425) and for pulling the `dev` baseline from runs 37177196081, 37135459941, 37119879859 and 36985353318 — that's exactly the comparison I was after. Agreed on all three verdicts: - `BackpressureSlowSinkIT`: same `only observed 2` assertion failing 2 of 5 on `dev`, so pre-existing and unrelated to this PR. - `SavepointBusySourceBarrierIT`: passed on both JDKs, cleared. - `CheckpointCoordinatorFailoverIT#testBatchJobCompletesAfterMasterFailoverDuringCloseHandshake`: 4 of 4 reproductions here vs 5 of 5 passes on `dev` is conclusive — a regression from this PR. Good to see `all-connectors-it-1`, `all-connectors-it-7` and `doris-connector-it` green on the re-run. Your root-cause analysis makes sense to me: with no file map-store in that test the WAL path is out of the picture, and the `notifyCheckpointMonitor` catch from `7c74bf982` swallowing `HazelcastInstanceNotActiveException` on the shutting-down `masterNode1` lets it finish the checkpoint (including `onCheckpointCompleted`) that the new master then redoes — the `2 × 5` table_fast rows line up exactly with `expected: <610> but was: <620>`, and the 7 `continuing coordinator bookkeeping` log lines per JDK confined to that method are good supporting evidence. I'm aligned with the direction of your proposed fix: `notifyCheckpointMonitor` should only isolate the auxiliary monitor-map durability failure (`IMapStorageException` from the fail-loud `FileMapStore`) and let node-shutdown exceptions propagate exactly as `dev` does today, so a dying master stops processing the ack instead of completing the checkpoint. Before I take another look: 1. Please push the fix and re-run `engine-v2-it` on JDK 8 and JDK 11, confirming `CheckpointCoordinatorFailoverIT#testBatchJobCompletesAfterMasterFailoverDuringCloseHandshake` passes on both. 2. The earlier review points are still open from my side: the `RequestFuture.get()` unbounded wait and the untimed/timed semantic change (plus documenting the `TimeoutException` contract on the method itself), `WALWorkHandler` letting the single worker die when `executeResponse()` throws and reusing the writer after a write failure, the sequential full-timeout wait in `batchQueryExecuteFailsStatus`, the missing Mockito test dependency for `HdfsWriterFlushSyncPathTest`, and the ERROR-level stack trace on every timed-out wait in `queryExecuteStatus`. Please either address them in the same push or reply inline to each so we can close them out together. <!-- streview-comment:1516 --> -- 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]
