sandynz commented on PR #38734:
URL: https://github.com/apache/shardingsphere/pull/38734#issuecomment-4554800824
### Decision
- **Merge Verdict: Mergeable**
- **Reviewed Scope:** Latest commit `35430cea14c` only;
`kernel/data-pipeline/core/src/main/java/org/apache/shardingsphere/data/pipeline/core/util/PipelineDataSourceConfigurationUtils.java`,
its new unit test, and
migration callback / consistency checker changes plus
`MigrationJobExecutorCallbackTest`.
- **Not Reviewed Scope:** Uncommitted local files
`proxy/bootstrap/src/main/resources/conf/global.yaml` and
`test/e2e/operation/pipeline/src/test/resources/env/e2e-env.properties`; full
E2E runtime with ZooKeeper /
Proxy restart was not rerun in this review round.
- **Need Expert Review:** No.
### Basis
- The root-cause path is addressed directly:
- Migration source storage unit pool size is loaded from pipeline
metadata and applied by storage unit name before rebuilding the restarted job
item context.
- Migration target pool size is loaded from Proxy storage units and
applied to the target `ShardingSpherePipelineDataSourceConfiguration`.
- The utility no longer binds itself to `PipelineContextManager`;
callers now provide the relevant storage unit map or source
`DataSourcePoolProperties`, which keeps the shared utility boundary cleaner.
- Blast-radius scan:
- Production callers of
`PipelineDataSourceConfigurationUtils.transformPipelineDataSourceConfiguration(...)`
in the current open-source tree are limited to migration callback and
migration consistency checker.
- CDC, migration DistSQL handlers, and other data-pipeline modules do
not execute the changed utility path directly.
- Counterexample behavior was checked in tests for null / empty storage
units, unnamed `StandardPipelineDataSourceConfiguration`, absent storage unit,
unchanged pool size, and source-metadata-missing migration source
storage unit.
- Compatibility / regression scan:
- No SQL parser, routing, identifier, or default-schema semantics are
changed.
- No dependency, SPI, or config format change is introduced.
- The removed old utility behavior was context-bound and had no
remaining production callers in the current tree.
- The change is not on Proxy/JDBC high-frequency DML/DQL execution path
and does not introduce `ConcurrentHashMap#computeIfAbsent`.
- Unrelated-change screening:
- The latest commit contains only the 5 pipeline files in scope. The two
dirty config files are local working-tree changes and are not part of the
reviewed commit.
### Pre-Merge Checks
- `./mvnw -pl kernel/data-pipeline/core -am -DskipITs -Dspotless.skip=true
-Dcheckstyle.skip=true -Dsurefire.failIfNoSpecifiedTests=false
-Dtest=PipelineDataSourceConfigurationUtilsTest test` passed.
- `./mvnw -pl kernel/data-pipeline/scenario/migration/core -am -DskipITs
-Dspotless.skip=true -Dcheckstyle.skip=true
-Dsurefire.failIfNoSpecifiedTests=false -Dtest=MigrationJobExecutorCallbackTest
test` passed.
- Earlier scoped formatting and style checks passed:
- `./mvnw -pl
kernel/data-pipeline/core,kernel/data-pipeline/scenario/migration/core
spotless:apply -Pcheck -T1C`
- `./mvnw -pl
kernel/data-pipeline/core,kernel/data-pipeline/scenario/migration/core
checkstyle:check -Pcheck -T1C`
- Note: one parallel `-am` Maven attempt failed with a transient
`NoSuchFileException` from concurrent builds writing shared `target`
directories; the same core test passed when rerun serially.
--
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]
