terrymanu commented on PR #38888:
URL: https://github.com/apache/shardingsphere/pull/38888#issuecomment-4770954741
### Summary
**Review Result: Mergeable**
Reason: No merge-blocking issue was found in the latest PR version. The
change removes the unused convenience overloads from `IndexReviseEngine`, keeps
the production path on the explicit 4-argument revision context, and the scoped
tests pass.
### Evidence
-
`infra/common/src/main/java/org/apache/shardingsphere/infra/metadata/database/schema/reviser/index/IndexReviseEngine.java:51`
now exposes only the explicit revision method with `originalTableMetaDataList`
and `schemaMetaDataRevisionCandidateTableMetaDataList`.
-
`infra/common/src/main/java/org/apache/shardingsphere/infra/metadata/database/schema/reviser/table/TableMetaDataReviseEngine.java:77`
still passes both metadata collections through the production metadata
revision path, so the index-name recovery context added around #38449 is
preserved.
- Repository reference search found no remaining in-repo callers of the
removed 2-argument or 3-argument overloads.
- Updated tests cover both the direct engine path and the shared sharding
index revision path:
- `IndexReviseEngineTest`
- `ShardingIndexReviserTest`
### Review Details
- Reviewed Scope: PR #38888 latest head
`dd0883324f4eaa33b415d1c1b5faf2fd0653b9cd`, base / merge-base
`cf0af7586852de48cc4534b6b142dea3623678a2`. GitHub `/pulls/38888/files` matched
the local triple-dot diff. Reviewed changed files:
-
`infra/common/src/main/java/org/apache/shardingsphere/infra/metadata/database/schema/reviser/index/IndexReviseEngine.java`
-
`infra/common/src/test/java/org/apache/shardingsphere/infra/metadata/database/schema/reviser/index/IndexReviseEngineTest.java`
-
`features/sharding/core/src/test/java/org/apache/shardingsphere/sharding/metadata/reviser/index/ShardingIndexReviserTest.java`
- Not Reviewed Scope: full E2E/proxy runtime behavior, unchanged modules,
and full logs for public CI jobs still in progress.
- Verification:
- `git diff --name-status
cf0af7586852de48cc4534b6b142dea3623678a2..dd0883324f4eaa33b415d1c1b5faf2fd0653b9cd`:
exit 0, matched the 3 GitHub changed files.
- `./mvnw -pl infra/common,features/sharding/core -am -DskipITs
-Dsurefire.failIfNoSpecifiedTests=false
-Dtest=IndexReviseEngineTest,ShardingIndexReviserTest test`: exit 0,
`IndexReviseEngineTest` 4 tests passed, `ShardingIndexReviserTest` 10 tests
passed, `BUILD SUCCESS`.
- Public checks observed for the latest head: Spotless, CheckStyle, and
License completed successfully; several broader CI/E2E jobs were still in
progress and were not required for this refactor-level judgment.
- Release Note / User Docs: not required. This is an internal metadata
revision refactor with no user-facing configuration, SQL behavior, migration
step, or documentation change.
--
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]
