RaigorJiang commented on PR #38848:
URL: https://github.com/apache/shardingsphere/pull/38848#issuecomment-4697725583
### Summary
- **Merge Decision: Mergeable**
- **Reason:** The latest PR head fixes the rule-specific `SHOW RULES USED
STORAGE UNIT` case-sensitivity mismatch across all production retrievers,
includes
targeted regression tests and release note coverage, and passed the scoped
verification checks I ran.
### Evidence
- The linked issue #38846 reports ShardingSphere-Proxy 5.5.4-SNAPSHOT
behavior where registered storage units such as `ds_0` and `DS_0` could both
exist, but
`SHOW RULES USED STORAGE UNIT` could not distinguish them.
- The central executor already performs exact storage-unit existence
checking through
`database.getResourceMetaData().getStorageUnits().containsKey(sqlStatement.getStorageUnitName())`,
so the remaining root-cause path was the rule-specific SPI
retrievers under `InUsedStorageUnitRetriever`.
- The PR changes every production `InUsedStorageUnitRetriever`
implementation to exact storage-unit name matching:
-
`features/readwrite-splitting/distsql/handler/src/main/java/org/apache/shardingsphere/readwritesplitting/distsql/handler/query/
InUsedReadwriteSplittingStorageUnitRetriever.java:37`
-
`features/readwrite-splitting/distsql/handler/src/main/java/org/apache/shardingsphere/readwritesplitting/distsql/handler/query/
InUsedReadwriteSplittingStorageUnitRetriever.java:40`
-
`features/sharding/distsql/handler/src/main/java/org/apache/shardingsphere/sharding/distsql/handler/query/InUsedShardingStorageUnitRetriever.java:37`
-
`features/shadow/distsql/handler/src/main/java/org/apache/shardingsphere/shadow/distsql/handler/query/InUsedShadowStorageUnitRetriever.java:36`
-
`features/shadow/distsql/handler/src/main/java/org/apache/shardingsphere/shadow/distsql/handler/query/InUsedShadowStorageUnitRetriever.java:37`
-
`kernel/single/distsql/handler/src/main/java/org/apache/shardingsphere/single/distsql/handler/query/InUsedSingleStorageUnitRetriever.java:42`
- A repository scan found the four changed classes are the only production
implementations of `InUsedStorageUnitRetriever`, so the fix covers the full
current
SPI blast radius for this DistSQL query path.
- The behavior is consistent with the existing user documentation for
`REGISTER STORAGE UNIT`, which documents `storageUnitName` as case-sensitive in
`docs/
document/content/user-manual/shardingsphere-proxy/distsql/syntax/rdl/storage-unit-definition/register-storage-unit.en.md`.
- The regression tests cover exact-match success and different-case
non-match behavior for the affected rule families:
- Readwrite-splitting:
`features/readwrite-splitting/distsql/handler/src/test/java/org/apache/shardingsphere/readwritesplitting/distsql/handler/query/
InUsedReadwriteSplittingStorageUnitRetrieverTest.java:41`,
`features/readwrite-splitting/distsql/handler/src/test/java/org/apache/shardingsphere/
readwritesplitting/distsql/handler/query/InUsedReadwriteSplittingStorageUnitRetrieverTest.java:47`,
`features/readwrite-splitting/distsql/handler/src/test/
java/org/apache/shardingsphere/readwritesplitting/distsql/handler/query/InUsedReadwriteSplittingStorageUnitRetrieverTest.java:53`
- Sharding:
`features/sharding/distsql/handler/src/test/java/org/apache/shardingsphere/sharding/distsql/handler/query/
InUsedShardingStorageUnitRetrieverTest.java:41`,
`features/sharding/distsql/handler/src/test/java/org/apache/shardingsphere/sharding/distsql/handler/query/
InUsedShardingStorageUnitRetrieverTest.java:47`
- Shadow:
`features/shadow/distsql/handler/src/test/java/org/apache/shardingsphere/shadow/distsql/handler/query/
InUsedShadowStorageUnitRetrieverTest.java:41`,
`features/shadow/distsql/handler/src/test/java/org/apache/shardingsphere/shadow/distsql/handler/query/
InUsedShadowStorageUnitRetrieverTest.java:47`,
`features/shadow/distsql/handler/src/test/java/org/apache/shardingsphere/shadow/distsql/handler/query/
InUsedShadowStorageUnitRetrieverTest.java:53`
- Single:
`kernel/single/distsql/handler/src/test/java/org/apache/shardingsphere/single/distsql/handler/query/InUsedSingleStorageUnitRetrieverTest.java:42`,
`kernel/single/distsql/handler/src/test/java/org/apache/shardingsphere/single/distsql/handler/query/InUsedSingleStorageUnitRetrieverTest.java:48`
- The change does not alter SQL grammar, DistSQL syntax, public API/SPI
signatures, metadata storage format, configuration format, dependency
manifests, or
distribution packaging.
- No substantive unrelated changes were found. The release note entry is
scoped to the user-visible DistSQL fix in `RELEASE-NOTES.md:30`.
### Review Details
- **Reviewed Scope:** Latest PR head
`8ef3f0f4f94ae254d0b4025a9a4c4355c80da6f9`; base `apache:master` / merge-base
`c65bed3358a66f71d744cdc95e3e46cbf0c1d0e4`.
The local triple-dot file list matched GitHub `/pulls/38848/files`:
`RELEASE-NOTES.md`, readwrite-splitting/sharding/shadow/single
`InUsedStorageUnitRetriever`
implementations, and their corresponding tests.
- **Not Reviewed Scope:** I did not use GitHub Actions or CI status, and I
did not run a live Proxy reproduction against an actual MySQL instance. I also
did
not review unrelated `CaseInsensitiveSet` usages outside the `SHOW RULES
USED STORAGE UNIT` SPI path except to confirm they are not production
`InUsedStorageUnitRetriever` implementations.
- **Verification:** Reviewer-run commands on the latest PR head all
passed: `./mvnw -pl
features/readwrite-splitting/distsql/handler,features/sharding/distsql/
handler,features/shadow/distsql/handler,kernel/single/distsql/handler -am
-DskipITs -Dspotless.skip=true
-Dtest=InUsedReadwriteSplittingStorageUnitRetrieverTest,InUsedShardingStorageUnitRetrieverTest,InUsedShadowStorageUnitRetrieverTest,InUsedSingleStorageUnitRetr
ieverTest -DfailIfNoTests=false -Dsurefire.failIfNoSpecifiedTests=false
test` exited `0`; `./mvnw -pl
features/readwrite-splitting/distsql/handler,features/
sharding/distsql/handler,features/shadow/distsql/handler,kernel/single/distsql/handler
spotless:check -Pcheck -T1C` exited `0`; `./mvnw -pl features/readwrite-
splitting/distsql/handler,features/sharding/distsql/handler,features/shadow/distsql/handler,kernel/single/distsql/handler
checkstyle:check -Pcheck -T1C` exited
`0`.
- **Release Note / User Docs:** Release note is required for this
user-visible DistSQL bug fix and is present in `RELEASE-NOTES.md:30`.
Additional user
documentation changes are not required because this PR restores the
already documented case-sensitive `storageUnitName` behavior rather than
introducing new
syntax or configuration.
--
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]
