iamhucong commented on PR #38980:
URL: https://github.com/apache/shardingsphere/pull/38980#issuecomment-4862734173
### Summary
**Review Result: Mergeable**
Reason: The PR directly removes the singleton multi-object `Collection<?>`
cache that can retain transient object graphs, while preserving the
single-object fast path, singleton service reuse, and class-based cache. The
changed behavior is covered by focused unit tests for the important lookup
boundaries, and the touched `infra/spi` module passed the relevant local and
required style/license checks available at review time.
### Evidence
- Root-cause path is addressed in the shared SPI loader:
- The singleton registry now keeps only `singleTypeClassToService` and
`multiTypeClassToServices`; the object-collection cache is no longer present in
`infra/spi/src/main/java/org/apache/shardingsphere/infra/spi/type/ordered/OrderedSPILoader.java:126-130`.
- Multi-object lookup no longer stores caller collections as cache keys.
It builds a method-local class grouping and an independent `HashSet<Class<?>>`,
then rebuilds a fresh object-keyed result map from
`getServicesByClass(typeClasses)` at `OrderedSPILoader.java:187-200`.
- The single-object fast path remains unchanged at
`OrderedSPILoader.java:177-184`.
- Ordering and compatibility are preserved:
- Class-based results are still built in ordered-service order at
`OrderedSPILoader.java:167-174`.
- The object result iterates those class-cache values and then maps the
current input objects at `OrderedSPILoader.java:194-199`.
- Prototype/non-singleton behavior is not changed in the production
implementation.
- Test coverage is focused on the changed behavior:
- Ordered service ordering:
`infra/spi/src/test/java/org/apache/shardingsphere/infra/spi/type/ordered/OrderedSPILoaderTest.java:49-55`.
- Class lookup single/multi/empty/no-match cases:
`OrderedSPILoaderTest.java:58-80`.
- Object lookup
single/multi/empty/exact-runtime-class/same-class/current-object-key/comparator
cases: `OrderedSPILoaderTest.java:82-135`.
- Singleton and non-singleton reuse behavior:
`OrderedSPILoaderTest.java:138-165`.
- Scope is narrow:
- GitHub files changed and local triple-dot diff both contain 11 files,
all under `infra/spi`.
- No JMH source or build-profile file is included in the PR diff.
- No public API, configuration key, distribution metadata, dependency
manifest, protocol, SQL syntax, or user-facing diagnostic output is changed.
- Performance risk is disclosed and bounded:
- The PR body documents the tradeoff for `@SingletonSPI + getServices(...,
multi-object collection)`.
- The implementation keeps `rules.size() == 1` on the direct fast path and
keeps `getServicesByClass(...)` on the class cache path.
- No direct `ConcurrentHashMap#computeIfAbsent` was added in Proxy/JDBC
DML/DQL high-frequency SQL paths.
### Review Details
- Reviewed Scope:
- PR: `apache/shardingsphere#38980`
- Latest PR head SHA: `3e4f9bf0232985a7efd1bab9d7ed460433a3dd1b`
- Base ref: `master`
- Local merge-base SHA: `e4fd5c7784780b56bc8d3ead10436d94662ee326`
- GitHub `/pulls/38980/files` matched the local triple-dot file list.
- Reviewed files:
`infra/spi/src/main/java/org/apache/shardingsphere/infra/spi/type/ordered/OrderedSPILoader.java`,
`OrderedSPILoaderTest.java`, new ordered-SPI test fixtures, and related test
SPI service resources.
- Not Reviewed Scope:
- Full repository behavior and full E2E execution were not locally re-run.
- GitHub CI/E2E checks were still in progress at the time of review; no
completed check failure was observed. The required Spotless, License, and
CheckStyle checks that had completed were passing.
- Verification:
- Local scoped verification on the latest PR head:
- `./mvnw spotless:apply -Pcheck -T1C` -> exit 0
- `./mvnw checkstyle:check -Pcheck -T1C` -> exit 0
- `./mvnw -pl infra/spi -DskipITs -Dspotless.skip=true test` -> exit 0,
29 tests
- `./mvnw -pl infra/spi -Pcheck -DskipTests apache-rat:check` -> exit 0
- Additional static checks confirmed that `multiObjectToServices`,
`classTypeMap.keySet()`, and `Map<Collection<?>>` are not present in the
changed `OrderedSPILoader` path.
- Release Note / User Docs:
- Not required for this PR. The change is an internal SPI cache fix with
no public API, configuration, SQL syntax, protocol, migration, or user-action
change. The PR body already documents the relevant performance tradeoff for
reviewers.
--
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]
