sandynz commented on PR #38834:
URL: https://github.com/apache/shardingsphere/pull/38834#issuecomment-4656047265
### Summary
- **Merge Decision: Mergeable**
- **Reason:** The patch handles the verified transient list-query race at
the per-job info construction point without changing job lifecycle,
configuration format, or operation semantics.
### Evidence
- `PipelineJobManager#getJobInfos` still obtains the ElasticJob brief list
first, but now builds each `PipelineJobInfo` through `createJobInfo` and skips
only `PipelineJobNotFoundException` for that individual job
(`kernel/data-pipeline/core/src/main/java/org/apache/shardingsphere/data/pipeline/core/job/service/PipelineJobManager.java:178`).
- The swallowed exception is narrowly scoped to missing detailed job
configuration: `PipelineJobIdUtils#getElasticJobConfigurationPOJO` throws it
only when the configuration API returns `null` (`kernel/data-
pipeline/core/src/main/java/org/apache/shardingsphere/data/pipeline/core/job/id/PipelineJobIdUtils.java:102`).
- The replacement construction path is semantically equivalent to the old
`PipelineJobConfigurationManager#getJobConfiguration` path, which also only
swaps `getElasticJobConfigurationPOJO(jobId).getJobParameter()`
into a pipeline job configuration
(`kernel/data-pipeline/core/src/main/java/org/apache/shardingsphere/data/pipeline/core/job/service/PipelineJobConfigurationManager.java:49`).
- Regression coverage directly models the original race: one valid brief
job and one brief job whose detailed config lookup throws
`PipelineJobNotFoundException`; the list returns only the valid job info
(`kernel/
data-pipeline/core/src/test/java/org/apache/shardingsphere/data/pipeline/core/job/service/PipelineJobManagerTest.java:49`).
- Shared-path blast radius is acceptable: the changed method is used by
migration and CDC list DistSQL executors, both read-only list paths, and the
patch preserves filtering by transmission job type before fetching
detailed configuration
(`kernel/data-pipeline/scenario/migration/distsql/handler/src/main/java/org/apache/shardingsphere/data/pipeline/scenario/migration/distsql/handler/query/ShowMigrationListExecutor.java:48`,
`kernel/data-pipeline/scenario/cdc/distsql/handler/src/main/java/org/apache/shardingsphere/data/pipeline/cdc/distsql/handler/query/ShowStreamingListExecutor.java:46`).
- No SQL parser behavior, routing precedence, config keys, public API/SPI
contracts, dependencies, packaging, or diagnostics are changed.
### Review Details
- **Reviewed Scope:** Local open-source branch range
`add8ddc056343bf709388de5810f206573b5d7c0^..add8ddc056343bf709388de5810f206573b5d7c0`;
base SHA `32a929182f8d160d1779b2f7db9442ae7932a954`; reviewed files are
`kernel/data-pipeline/core/src/main/java/org/apache/shardingsphere/data/pipeline/core/job/service/PipelineJobManager.java`
and
`kernel/data-pipeline/core/src/test/java/org/apache/shardingsphere/data/pipeline/core/
job/service/PipelineJobManagerTest.java`. GitHub `/pulls/{number}/files`
comparison is not applicable because this review used the user-provided local
commit range rather than a GitHub PR number.
- **Not Reviewed Scope:** Full nightly e2e, registry-center integration
timing under real cluster load, and unrelated pipeline job operation paths
beyond checking callers and the unchanged configuration lookup
semantics.
- **Verification:** JetBrains `get_file_problems` on both changed files:
no problems. `./mvnw -pl kernel/data-pipeline/core -am -DskipITs
-Dspotless.skip=true -Dtest=PipelineJobManagerTest -DfailIfNoTests=false
-Dsurefire.failIfNoSpecifiedTests=false test`: exit 0.
- **Release Note / User Docs:** Not required; this is an internal
robustness fix for transient list-query consistency and does not add
user-facing configuration, syntax, API, migration steps, or changed documented
behavior.
--
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]
