SEZ9 commented on PR #11495: URL: https://github.com/apache/seatunnel/pull/11495#issuecomment-6331445499
Thanks both. @zhangshenghang — thanks for the breakdown of the `Build` failures. Agreed that the `LLMBooleanOutputTest.readsLegacyFactoryWrapperWithValidationDisabled` failure (`serialVersionUID = 7090258061839605137 vs -3241928148724104003`) is inherited: `AbstractMultiCatalogTransform` isn't touched here, and dev's own run at `bab2d089c` ([run 39536327852](https://github.com/apache/seatunnel/actions/runs/39536327852)) shows the same exception. Syncing `dev` into this branch once the upstream fix lands sounds right; no rerun needed in the meantime. The `BackpressureSlowSinkIT` error I'm also happy to treat as the known flaky one. @nzw921rx — thanks for the review. That said, the CI picture doesn't change the open items from my earlier review, which haven't been addressed in the thread yet. Before I can approve, I'd like to see the following resolved or explicitly discussed: - **F1 / F2 (HIGH)**: the restore path still never sends `CleanTaskGroupContextOperation`, so surviving workers keep the failed attempt's `TaskGroupContext`s in `finishedExecutionContexts` and re-report them on the next metrics backup interval; and the IMap clear in `SubPlan.java` races with that periodic report, so stale snapshots can be re-merged before the new attempt deploys. As it stands the fix is probabilistic. Please either send the clean operation to the workers as part of restore, or otherwise explain how the re-report is prevented. - **F3 (MEDIUM)**: the unconditional reset changes metrics semantics for streaming jobs (counters previously carried across restarts now drop to zero on every pipeline retry). Please add a doc / incompatible-changes note, or make the behaviour opt-in. - **F4 (MEDIUM)**: `StateTransitionCleanupTest` currently only verifies that a mock method is invoked. Please add coverage that goes through the real metrics store and the worker re-report path and asserts the restored attempt actually starts from zero. - **F5 / F6 / F7 (LOW)**: the `CALLS_REAL_METHODS` + reflective private-field injection scaffolding, the fully-qualified `java.lang.reflect.*` / `org.mockito.Mockito.*` references, and the duplicated body between `clearPipelineMetricsForRestore` and `removeMetricsContext` (consolidate into one helper). If any of these have already been handled in a push I missed, a short pointer in the thread is enough and I'll re-review. <!-- streview-comment:1935 --> -- 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]
