SEZ9 commented on PR #12209: URL: https://github.com/apache/seatunnel/pull/12209#issuecomment-6622406216
Thanks for the follow-up commits. I went through the three commits since `ca7867c583` (`d8d9ac81ae`, `047b8b9bb2`, `8434902b28`) against the points from my earlier review. Since these are described as comment/Javadoc updates with no runtime behavior change, here is where I think each point stands. **Needs confirmation** - **F3 (unguarded `fromConnectorOptions` in `MultipleTableJobConfigParser`)** – the new `extractLineageDatasets` contract (ordinary extractor failures become an empty list, `VirtualMachineError` is rethrown, the warning logs only the factory id and exception class) describes the intended behavior, and the rationale that extraction still runs when reporting is disabled because the client cannot see the server-side cluster settings makes sense to me. However, I only see the Javadoc in this delta, not the catch/guard itself. Could you point me at the actual try/catch around the extractor call (or let me know if it was already present and my original finding was mistaken)? I'll mark this resolved once I can see it. **Still open as far as I can tell** - **F1 (HIGH, job-env precedence for `openlineage_url` / `openlineage_transport` / `openlineage_enabled` while the bearer token is cluster-scoped)** – I don't see a change in `JobMaster.java` for this. This is the one blocking item for me: a job submitter should not be able to redirect the cluster token to an arbitrary URL. Making the URL/transport cluster-only, or requiring the job to supply its own token when it overrides the URL, would both work. - **F2 (untimed fail-fast heartbeat RPC fan-out to every worker on the lineage pool)** – no change I can see. A per-worker timeout plus tolerating individual worker failures would be enough. - **F4 (heartbeat throttle timestamp advanced before delivery succeeds in `ZetaLineageReporter`)** – no change I can see. Please advance the timestamp only on successful submission/delivery, or reset it on drop/failure. - **F5 (no upper bound on job-controlled `openlineage_timeout_ms` / `openlineage_retry_times`)** – the updated `submitLineageEmission` Javadoc clarifying that `timeoutMs` is a per-phase inactivity limit and that `(retryTimes + 1) * timeoutMs` is not a hard total bound is a useful clarification, but it documents the issue rather than bounding it. Could you clamp these two values to a cluster-configured maximum, or explain why a single job pointed at a black-hole URL can no longer pin both threads of the shared 2-thread pool? - **F6 / F7 (docs)** – `hybrid-cluster-deployment.md` still references a Flink section and a known-limitations section that aren't in this PR's `lineage.md`, and the cluster-level `openlineage` block is still missing from `separated-cluster-deployment.md` (en/zh). Please either add those sections or drop the forward references, and mirror the block into the separated-cluster page. - **F8 (LOW, plain-text `auth_token` in the raw `Map` on the Lombok-generated `EngineConfig`)** – no change I can see. A small typed holder with a masking `toString()` would be consistent with the other `seatunnel.yaml` sections; happy to take this as a follow-up if you prefer, just say so. If any of these were handled in commits I'm not seeing, please point me at them and I'll re-check. Otherwise, F1 is the one I'd need resolved before approving; the rest I'm fine landing as follow-ups if you'd rather keep this PR focused. <!-- streview-comment:2271 --> -- 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]
