Repository navigation
[ISSUE #7311] Decouple registry core from implementations. - #7578
Merged
Merged
Conversation
Aias00
approved these changes
Oct 13, 2026
Aias00
left a comment
Aias00
left a comment
Contributor
There was a problem hiding this comment.
Thanks for this cleanup. Decoupling shenyu-registry-core from the concrete registry implementations (zookeeper/etcd/nacos/eureka/kubernetes) is the right call: it shrinks SDK/classpath surfaces, makes lean distributions possible, and the new registry_dependencies CI gate backed by check-registry-dependencies.py gives us a real regression guard instead of relying on reviewers to eyeball pom.xml.
What I verified before approving (all against the PR head 5d7c38e):
- Factory API surface is real and public.
ShenyuInstanceRegisterRepositoryFactory.loadRepository(...)is the new center of gravity. I confirmed every shenyu-spi API it depends on exists and is public inshenyu-spi'sExtensionLoader:getExtensionLoader(Class, ClassLoader)(ExtensionLoader.java:92),getExtensionClasses1()(ExtensionLoader.java:234), and theLOADERSfield the rewritten test reaches via reflection. TheIllegalArgumentExceptionpath for blank/unknown register types is clear and actionable. - No missing-import compile gap for
commons-lang3. The factory now importsorg.apache.commons.lang3.StringUtils.shenyu-spideclarescommons-lang3directly (shenyu-spi/pom.xml:32), so core still gets it transitively after the impl deps were dropped. shenyu-adminstill resolves the factory. It declaresshenyu-registry-core(shenyu-admin/pom.xml:346), soAbstractDiscoveryProcessor.createDiscoveryswitching from raw SPI toShenyuInstanceRegisterRepositoryFactory.loadRepository(type)compiles.- SDK modules drop
coresafely.shenyu-sdk-core/shenyu-sdk-feignmoving fromshenyu-registry-coretoshenyu-registry-apiis safe — I grepped their sources and found zero references toorg.apache.shenyu.registry.core. The SDK starters (shenyu-spring-boot-starter-sdk,...-sdk-feign) correctly re-addshenyu-registry-core, so the factory remains available to applications. - New CI gate won't self-false-positive. None of the six implementation modules depend on
shenyu-registry-core, so theIMPLEMENTATION_MODULES must not depend on coreassertion incheck-registry-dependencies.pypasses for this very PR. - Distribution preserved.
shenyu-adminre-declares the five provider deps atruntimescope andshenyu-spring-boot-starter-registrykeeps all five, matching the README migration notes and avoiding a classpath regression for existing users.
CI: green. pr_build, rat_check, check-license-header, Analyze (java), and the new registry_dependencies job all pass; the full integrated-test / e2e matrix is green on the head commit. mergeable against master (no conflicts).
Non-blocking suggestions (not required for approval):
loadRepository's error message buildsshenyu-registry-<registerType>, which is misleading for external providers (Consul/Polaris) that aren't namedshenyu-registry-*. The trailing "(or an external registry SPI provider)" caveat already covers it, but branching the hint on whether the key looks like a built-in would read even better.getExtensionClasses1()has an unusual "1" suffix that reads like an internal overload. It's stable public API today, but core now couples to that exact name; agetExtensionClasses()alias in shenyu-spi (or documenting the method) would make the cross-module dependency less surprising.
Approving — this is ready to merge once the branch-protection approval threshold is met.
This was referenced Oct 13, 2026
Closed
Aias00
pushed a commit
that referenced
this pull request
Oct 16, 2026
…#7585) (#7642) PR #7578 removed the five registry implementation dependencies from shenyu-registry-core, which is a runtime-visible behavior change for downstream users rather than a code-organization-only refactor: an application on shenyu-registry-core (or on shenyu-sdk-core / shenyu-sdk-feign, which now sit on shenyu-registry-api) no longer gets the implementations transitively, so an upgrade without an explicit provider declaration fails at startup. The registerType-with-no-provider case also changes from silent to throwing. Adds one entry under Unreleased / Behavior Changes covering that, the new public loadRepository(String) surface, and the compatibility story for shenyu-admin and shenyu-spring-boot-starter-registry. Migration guidance is delegated to shenyu-registry/README.md, added by #7578. Documentation only. Fixes #7585
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
shenyu-registry-corecurrently brings every built-in registry client into Admin and SDK applications. This change limits core to API/SPI dependencies and loads the configured provider from the application's classpath.Fixes #7311.
consul/polariskeys), and starter context/registration tests. Check 13 resolved runtime dependency trees in CI to prevent client or implementation leakage into core/SDK consumers.shenyu-registry/README.md. Both distributions retain clients supplied by their selected modules, so their existing LICENSE/NOTICE entries remain applicable.Validation:
./mvnw -B -ntp -T 4 clean install -DskipTests -Dmaven.javadoc.skip=true -DskipRemoteResources=true -Djacoco.skip=truepassed, including Checkstyle and RAT; standaloneapache-rat:checkalso passed.DefaultDiscoveryProcessorTestandZookeeperClientDiscoveryIntegrationTestpassed (8 tests), including real HTTP client registration, Admin and embedded ZooKeeper discovery/sync.External Consul/Polaris compatibility is exercised through SPI resource fixtures; the external plugin JARs and Docker-based full-stack suites were not run locally.