SEZ9 commented on PR #11398: URL: https://github.com/apache/seatunnel/pull/11398#issuecomment-6331510758
Re-checked head `db2197412815` against the previously reviewed head `dbd0a75fd6de`: the only new commit is a merge of `dev`, which touches neither of this PR's two files, so the previous findings still stand. Summary of what remains open: **PR11398-F1 (Logic, MEDIUM)** – Edges with a null endpoint are still serialized with a null `inputVertexId` rather than dropped, which hands a dangling edge to the Web UI DAG renderer. Ask: skip the edge (and log it, see F3) instead of serializing a null endpoint. **PR11398-F2 (Robustness, MEDIUM)** – Structural nulls (`pipelineEdges` / `vertexInfoMap` / `jobId`) still render as an empty DAG, and `BaseService` will cache that empty JSON for up to 10 minutes with nothing to diagnose it. Ask: either fail fast on these structural nulls or at minimum emit a WARN with the job id so the empty DAG is traceable. **PR11398-F3 (Logic, MEDIUM)** – Null pipeline keys becoming `""` and null vertices/edges being silently dropped still masks corrupted DAG state. Ask: add logging at each drop/degrade point so an inconsistent DAG in the UI can be traced back to its cause. **PR11398-F4 (Docs, LOW)** – The Javadoc still attributes the nulls to "partially written IMap entries". Ask: reword to the actual sources — the no-arg constructor and `LogicalDag` edges with a null `inputVertexId`. **PR11398-F5 (Docs, LOW)** – The PR description still says "no user-facing change", but the REST job-detail JSON can now carry JSON null for `jobId`, `inputVertexId`, `targetVertexId` and `type` where the documented examples only show strings. Ask: update the description (and the documented examples if you keep the null rendering) so the contract change is explicit. **PR11398-F6 (Test, LOW)** – `testToJsonObjectWithPartialVertexInfo` still identifies vertices by array index, which depends on `HashMap` iteration order. Ask: look vertices up by `vertexId` in the resulting array, or use an order-preserving map in the fixture. F1–F3 all sit in `toJsonObject()`, so resolving them together in one pass should be straightforward. Once those are pushed I'll do another full re-review of the new head. Thanks for the continued work on this. <!-- streview-comment:1936 --> -- 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]
