DanielLeens commented on PR #11398:
URL: https://github.com/apache/seatunnel/pull/11398#issuecomment-6310049422
Thanks @SEZ9 for the detailed pass on `dbd0a75fd6de`. Since that commit is
the follow-up I pushed onto this branch, I went through each point against the
code instead of just accepting them. Here is where I land, so @Isha2790 is not
left with conflicting signals.
**Where I agree (small follow-ups, none of them blocking):**
- Issue 2 / Issue 3 (logging): agreed. The structural fallbacks
(`pipelineEdges`, `vertexInfoMap`, `jobId`) are only reachable through the
no-arg constructor or a broken producer, so a single `log.warn` with the job id
and field name is cheap and makes "empty DAG" distinguishable from "corrupt
DAG", especially because `BaseService` caches the rendered JSON for 10 minutes.
This is the same point as my own Issue 1 in the previous review.
- Issue 4 (Javadoc): fair. The "partially written IMap entries" wording in
`JobDAGInfo.java:50-51` is mine, and it is not accurate: a Hazelcast IMap value
is one serialized blob, and `DAGUtils.getJobDAGInfo` always builds a complete
object before `JobHistoryService.storeJobInfo` stores it. The honest sources of
nulls are the Lombok no-arg constructor and defensive handling of hand-built
instances. The wording should be changed to say that.
- Issue 6 (test ordering): agreed, `testToJsonObjectWithPartialVertexInfo`
should use a `LinkedHashMap` (or look vertices up by `vertexId`). I flagged the
same thing as Low in my previous review.
- Issue 5 (docs/PR description): agreed that the description should mention
that missing scalars now render as JSON null; a one-line nullable note in the
`jobDag` section of the REST docs would be a nice touch, but I would keep it
optional.
**Where I read the code differently:**
- Issue 1 (dangling edge for a null endpoint): the premise that a null
`inputVertexId` "happens for real on the logical-DAG path" does not hold in the
current source. `LogicalDagGenerator.createLogicalEdges()` builds `new
LogicalEdge(entry.getKey(), targetId)` from a map keyed by
`inputAction.getId()`, so both ids are non-null. `LogicalEdge.writeData` calls
`out.writeLong(inputVertexId)` and `readData` uses `in.readLong()`, so a
`LogicalEdge` with a null id cannot even be serialized or deserialized (it
would NPE on unboxing). The `e.getInputVertexId() != null` check at
`DAGUtils.java:170` is purely defensive. That means the `{"inputVertexId":
null}` edge is only reachable from a hand-built `Edge`, which is the case the
test pins. I do agree the policy inside the loop is inconsistent (null `Edge`
is skipped, null endpoint is kept), so I am fine with either skipping such an
edge or keeping the JSON null; I would not call it Medium.
- `BaseService.getJobMetrics` (`BaseService.java:597-598`) dereferencing
`vertexInfo.getConnectorType()`: true, but the same reasoning applies, a null
`VertexInfo` value is not producible by `DAGUtils`. If we keep the null-vertex
skip in `toJsonObject()`, the consistent options are a one-line `continue` on a
null `vertexInfo` in those two loops (`BaseService.java:597` and the similar
loop near `:760`) or no skip in the serializer at all. I would rather not widen
this PR beyond the serializer unless the maintainers want it.
**Net:** no new blocker from my side. Suggested follow-up: fold the logging,
Javadoc wording and `LinkedHashMap` changes into one small commit on this
branch. CI on this head is red in jobs unrelated to this diff (see below), so a
rerun is still needed before this can be merged.
CI note for `dbd0a75fd6de` (fork run for this head): the `unit-test` jobs
fail only in `seatunnel-transforms-v2`
(`LLMBooleanOutputTest.readsLegacyFactoryWrapperWithValidationDisabled`,
`InvalidClass`), a module this PR does not touch; `JobDAGInfoTest` is not among
the failures. The remaining red jobs are connector ITs (hbase, mysql-cdc,
all-connectors-it, transform-v2-it).
--
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]
