terrymanu commented on PR #38889:
URL: https://github.com/apache/shardingsphere/pull/38889#issuecomment-4771184775
### Summary
**Review Result: Mergeable**
**Reason:** This PR keeps the metadata revision candidate propagation
behavior intact while simplifying `SchemaMetaDataReviseEngine`: the redundant
two-argument constructor is removed, callers now pass revision candidates
explicitly, and the overly long internal candidate naming is shortened without
changing the revise flow.
### Evidence
-
`infra/common/src/main/java/org/apache/shardingsphere/infra/metadata/database/schema/reviser/schema/SchemaMetaDataReviseEngine.java`
now uses a single required-args constructor and renames the internal candidate
fields/local variables to `revisionCandidateSchemas` and
`revisionCandidateTableMetaDataList`.
- The production caller in `MetaDataReviseEngine` already passes
`material.getSchemaMetaDataRevisionCandidateSchemas()`, so the main metadata
revise path still carries candidate schema metadata explicitly.
-
`infra/common/src/test/java/org/apache/shardingsphere/infra/metadata/database/schema/reviser/schema/SchemaMetaDataReviseEngineTest.java`
was updated to call the three-argument constructor explicitly, and the
existing candidate propagation regression remains covered.
- The PR scope is small and focused: one production class and its direct
test. I did not find unrelated behavior changes, target-specific leakage, or
documentation/release-note impact.
### Review Details
- **Reviewed Scope:** PR `#38889`, latest head
`5dadf69b8c21a563a55d76683c83643d8d187c22`, base `master` at
`ad7a6202ff0a9dbfba2e51272930e3babc8d8dc2`, merge-base
`ad7a6202ff0a9dbfba2e51272930e3babc8d8dc2`. Reviewed files:
-
`infra/common/src/main/java/org/apache/shardingsphere/infra/metadata/database/schema/reviser/schema/SchemaMetaDataReviseEngine.java`
-
`infra/common/src/test/java/org/apache/shardingsphere/infra/metadata/database/schema/reviser/schema/SchemaMetaDataReviseEngineTest.java`
- GitHub file list matched the local triple-dot diff.
- **Not Reviewed Scope:** Full `#38449` / `#37123` sharding index metadata
behavior was not re-reviewed end-to-end in this PR; this review only checks the
small follow-up refactor around `SchemaMetaDataReviseEngine`.
- **Verification:**
- `./mvnw -pl infra/common -am -DskipITs -Dcheckstyle.skip
-Dspotless.check.skip=true -Dsurefire.failIfNoSpecifiedTests=false
-Dtest=SchemaMetaDataReviseEngineTest test` passed: 4 tests, build success.
- `./mvnw -pl infra/common -am -Pcheck -DskipTests -DskipITs
spotless:check checkstyle:check` passed.
- GitHub check-runs for the reviewed head showed no failures at review
time: 81 successful, 2 still in progress.
- **Release Note / User Docs:** Not required. This is an internal refactor
and constructor cleanup, with no user-facing behavior, configuration,
migration, or documentation impact.
--
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]
