SEZ9 commented on PR #11602: URL: https://github.com/apache/seatunnel/pull/11602#issuecomment-5230720313
Thanks @tomatotomata for the two follow-ups on `16308107a1eb`, and @DanielLeens for cross-checking — glad our reviews converge. To answer the direct questions: **Retry + interrupt shape**: yes, this is the right design. Narrowing the `subPlanDone` retry predicate to `FinalMetricsCollectionException` instead of the broad `SeaTunnelEngineException` is exactly what I wanted, and restoring the interrupt flag while returning immediately on the realtime path is correct. The strict final-collection path with all-or-nothing persistence also matches the boundary we discussed — no concerns there. **Timeout and partial-result contract — this PR or follow-up**: split it. - **In this PR**: dedupe the hard-coded 3000ms into a single named constant (it currently lives in both `CoordinatorService` and `JobMaster`) and promote it to a proper engine option in `seatunnel.yaml` with a documented default of 3s. Since this PR silently changes `getRunningJobMetrics()` from fail-fast to best-effort, that behavior change and the new option need to land together with docs — shipping an undocumented, unconfigurable timeout that alters existing semantics isn't something we can defer. - **Follow-up is fine for**: an explicit completeness field/result on the metrics response. That's an API-surface decision that deserves its own discussion, so please open a tracking issue and link it here. Two smaller remaining asks (echoing Daniel's points, which I agree with): 1. Add a comment on the `SubPlan` retry predicate explaining *why* `FinalMetricsCollectionException` is retryable (task-group context is intentionally left intact for the next attempt). 2. Update the `getCurrJobMetrics(Map)` Javadoc to state that an interrupted collection returns a **truncated** list, not just that individual workers may be missing. On testing: understood re: the sparse checkout blocking the local reactor — CI will exercise the module. The new `JobMasterTest` coverage for the timeout/persistence/cleanup and interrupt boundaries is appreciated; please make sure both pass in CI before we do a final pass. So: config option + docs + the two comment/Javadoc fixes, and I think this is mergeable. <!-- streview-comment:99 --> -- 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]
