SEZ9 commented on PR #11602: URL: https://github.com/apache/seatunnel/pull/11602#issuecomment-5225133640
Thanks @tomatotomata for the quick follow-ups — I re-checked `16308107a1eb` and the direction is right. And thanks @DanielLeens for cross-checking; good to see our reviews converge on the same head. To answer your two questions directly: 1. **Dedicated exception vs. completeness result**: I prefer what you landed. `FinalMetricsCollectionException` propagating through `subPlanDone` fits the existing retry model in `SubPlan` cleanly, keeps the task-group context alive for the next attempt, and avoids threading a completeness flag through every caller. Narrowing the retry predicate from `SeaTunnelEngineException` to the dedicated exception in `1630810` was the right call. Please just add a short comment at the retry predicate explaining *why* this exception is retryable (worker transient unavailability, context intentionally preserved) — future readers won't have this thread. 2. **Timeout config and partial-result contract — this PR or follow-up**: the timeout should be resolved in this PR, since the hardcoded/duplicated 3s constant was one of my two merge blockers. Concretely: hoist it to a single shared constant, expose it as an engine config option (a simple `seatunnel.engine` option with 3s default is fine), and document it. The interrupt/retry shape you have now is good; I don't need further design changes there. The interrupt handling (return-immediately + restore flag) and the `@VisibleForTesting` annotation on `fetchTaskGroupMetrics` both look correct, and the two focused `JobMasterTest` cases cover the right boundaries. Remaining asks before merge: - [ ] Deduplicate the 3s timeout into one constant and make it configurable via an engine option, with docs. - [ ] Document the `getRunningJobMetrics()` semantic change (fail-fast → best-effort/partial) in the Javadoc and user-facing docs — this is a behavior change operators will notice. - [ ] Add the comment on the `SubPlan` retry predicate explaining `FinalMetricsCollectionException` retryability. - [ ] Per Daniel's Issue 4: `getCurrJobMetrics(Map)` Javadoc should state that an interrupted collection returns a truncated list, not just that individual workers may be missing. - [ ] Rebase onto `dev` (you mentioned the branch is two commits behind). No concern about the local build issue — the repository CI is the supported path, so let's let it run the engine module tests once you push. Thanks again for the careful iteration here. <!-- streview-comment:92 --> -- 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]
