DanielLeens commented on PR #11559: URL: https://github.com/apache/seatunnel/pull/11559#issuecomment-6594296110
Follow-up on Issues 6 and 7 from my [November 1 review](https://github.com/apache/seatunnel/pull/11559#pullrequestreview-5728483513). New head: `949ac4f70763c80d8a4d83fb0384f4cd9fd08a0c` (normal push on top of `bb23b9f2`, no history rewrite). **Issue 6, dimension role bound to the logical source ID: `a4ab739bf0f`.** - Root cause: `ExecutionPlanGenerator.generateExecutionEdges` allocates new action IDs in edge order. `DynamicLookupAction` deliberately keeps the logical fact/dimension IDs; the existing `ExecutionPlanGeneratorTest` asserts this, and `InputPortBinding.upstreamActionId` documents it. `PhysicalPlanGenerator.createDynamicLookupRuntimeVertex` already read the fact source ID from the `FACT_INPUT` edge, but passed `lookupAction.getDimensionSourceActionId()` into `setFlowConfig`. With logical fact=1, dimension=2, lookup=3, the execution IDs are fact=1, lookup=2, dimension=3. As a result, `SourceConfig.dynamicLookupDimension` stayed false, `SourceFlowLifeCycle` never emitted the COMPLETE marker, and the fact gate never opened. - Fix: the new package-private helper `PhysicalPlanGenerator.resolveDynamicLookupSourceActionId(inputEdges, port)` resolves both roles from their port edges. I fixed this in the physical plan rather than remapping the IDs inside `ExecutionPlanGenerator`, so the documented logical-ID contract on `DynamicLookupAction` and `InputPortBinding` stays unchanged. Nothing else in the server reads those logical IDs. - Test: `DynamicLookupPlanValidationTest.resolvesLookupSourceRolesFromRegeneratedExecutionIds` runs the real `LogicalDagGenerator` and `ExecutionPlanGenerator` on logical IDs 1/2/3. It asserts that the dimension source's execution ID is 3, that the lookup still carries logical ID 2, and that the helper returns 3 for the dimension port and 1 for the fact port. If the dimension role is resolved from the lookup's logical ID again, the test fails. **Issue 7, restore assumed one source per pipeline: `949ac4f7076`.** - Root cause: `MultipleTableJobConfigParser.parse` required `sourceConfigs.size() == pipelineCheckpoints.size()`, and `getSourceCheckpoint` read source `i` from `pipelineCheckpoints.get(i)` with `pipelineId == i + 1`. A lookup job's two sources share one pipeline, so `CheckpointService` exports one pipeline record for two source configs, and the restore failed before any source was created. - Fix: - `getSourceCheckpoint` now searches every pipeline record by the source action name (`Source[<index>]-<factory>`, which encodes the config index) and still requires exactly one match. - The count check is replaced by `validatePipelineCheckpointsBelongToSources`, which rejects any pipeline record that contains none of the configured sources. A checkpoint from a different topology therefore still fails fast. - For ordinary one-source-per-pipeline jobs, every source still has to be found exactly once. The difference is that it is matched by name rather than by position, so the `pipelineId == index + 1` check is gone. - `getSourceCheckpoint` is now `protected` (`@VisibleForTesting`) so the test subclass can call it. - Test: `MultipleTableJobConfigParserTest.testDynamicLookupRestoreResolvesBothSourcesFromOnePipelineCheckpoint` restores the lookup fixture from one pipeline record that holds `Source[0]-FakeSource`, `Source[1]-FakeSource` and the lookup state. It asserts the exact enumerator and split bytes each source receives, and that an extra pipeline record holding only `Source[0]-OtherSource` is rejected. On the previous head, the first `parse` fails with "The number of source configurations and pipeline checkpoints must be equal." **Scope and validation.** - This fixes the client parser step of `restoreJobWithCheckpoint`. Whether the rest of the server-side restore and the real E2E pass is not proven here. - No pom.xml changed, and no existing assertion was removed or loosened. - Per the SeaTunnel local-validation policy, only scoped `spotless:apply` ran locally on `seatunnel-engine-core`, `-server` and `-client`. Compile, UT and E2E are left to the fork Build on `949ac4f7`, which has not finished yet, and I am not claiming it is green. -- 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]
