2.7.0.1 jdk8#10
Open
Aias00 wants to merge 2 commits into
Open
Conversation
The split client repository needs to carry MCP registration, beat registration, Spring Boot starters, and discovery registration without inheriting the main repository JDK upgrade constraints. This restores the client-side module surface on the JDK 8 release base, rebases dependencies onto Spring Boot 2/JDK 8 compatible lines, and expands CI coverage through JDK 25 after local compatibility verification. Constraint: Client artifacts must remain JDK 8 compatible while validating JDK 17, 21, 23, and 25 Constraint: The split repository does not define the release Maven profile, so CI uses ./mvnw -B clean test Rejected: Move the client back under the main repository | it would keep client compatibility tied to admin/bootstrap JDK upgrade timing Rejected: Keep JDK 23 and 25 as local-only checks | CI would not catch future toolchain regressions Confidence: medium Scope-risk: broad Directive: Do not upgrade client runtime dependencies without rerunning the JDK 8 compatibility build Tested: ./mvnw -DskipTests compile Tested: ./mvnw test Tested: JAVA_HOME=/Library/Java/JavaVirtualMachines/zulu-8.jdk/Contents/Home ./mvnw -B clean test Tested: JAVA_HOME=/Users/aias/Library/Java/JavaVirtualMachines/corretto-17.0.13/Contents/Home ./mvnw -B clean test -Prelease Tested: JAVA_HOME=/Users/aias/Library/Java/JavaVirtualMachines/openjdk-21.0.2/Contents/Home ./mvnw -B clean test -Prelease Tested: JAVA_HOME=/Users/aias/Library/Java/JavaVirtualMachines/openjdk-25.0.2/Contents/Home ./mvnw -B clean test Tested: JAVA_HOME=/tmp/shenyu-jdks/jdk-23.0.2+7/Contents/Home ./mvnw -B clean test Not-tested: End-to-end registration against matching admin/bootstrap runtime
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 105 out of 105 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (8)
shenyu-registry-api/src/main/java/org/apache/shenyu/registry/api/config/RegisterConfig.java:1
equalsis unsafe: it casts without aninstanceof/class check and dereferencesgetRegisterType()/getServerLists()which can be null (default constructor sets them null), causingNullPointerException. Additionally,hashCode()depends onPropertiesiteration order, which may differ between twoPropertiesinstances that areequals()by content—this can violate the equals/hashCode contract. Makeequalsnull-safe (Objects.equals), add type checks (this == obj,obj instanceof RegisterConfig), and computehashCodein an order-independent way forprops(e.g., sort entries by key before mixing into the hash, or hash a normalized map/entry set).
shenyu-register-client-beat/src/main/java/org/apache/shenyu/register/client/beat/HeartbeatListener.java:1- Two issues here can break heartbeat reporting in production: (1) Caffeine
CacheLoadermust not return null—returning null can triggerInvalidCacheLoadExceptionand prevent caching/refresh semantics from working as intended. Represent absence explicitly (e.g., cacheOptional<String>or cache a sentinel like empty string) and handle it insendHeartbeat. (2) Throwing aRuntimeExceptionfrom the scheduled task can cancel future executions ofscheduleAtFixedRate, stopping all heartbeats after one full-cycle failure; instead, log and continue so the next period retries.
shenyu-register-client-beat/src/main/java/org/apache/shenyu/register/client/beat/HeartbeatListener.java:1 - Two issues here can break heartbeat reporting in production: (1) Caffeine
CacheLoadermust not return null—returning null can triggerInvalidCacheLoadExceptionand prevent caching/refresh semantics from working as intended. Represent absence explicitly (e.g., cacheOptional<String>or cache a sentinel like empty string) and handle it insendHeartbeat. (2) Throwing aRuntimeExceptionfrom the scheduled task can cancel future executions ofscheduleAtFixedRate, stopping all heartbeats after one full-cycle failure; instead, log and continue so the next period retries.
shenyu-spring-boot-starter-client/shenyu-spring-boot-starter-client-springmvc/src/test/java/org/apache/shenyu/springboot/starter/client/springmvc/ShenyuSpringMvcClientConfigurationTest.java:1 - There are two
@BeforeEachmethods both assigningapplicationContextRunner. JUnit will run both before each test (order is not something to rely on), so the configuration used by each test becomes fragile/flaky and the first setup is effectively overwritten. Consider using separateApplicationContextRunnerinstances inside each test, or switch to@Nestedtest classes with distinct@BeforeEachsetups.
shenyu-client-mcp/shenyu-client-mcp-common/src/main/java/org/apache/shenyu/client/mcp/generator/McpToolsRegisterDTOGenerator.java:1 parameterscan be null when the OpenAPI operation has noparametersarray.parameterFormatting(parameters)will then throw aNullPointerException. Guard by defaulting to an emptyJsonArraywhenparametersis null (and consider similar guards forpaths,path, andmethodlookups if the input can be incomplete).
shenyu-registry-api/src/main/java/org/apache/shenyu/registry/api/entity/InstanceEntity.java:1toString()returns a type labelURIRegisterDTO{...}which doesn't match this class (InstanceEntity). This makes logs/debugging misleading. Update the prefix toInstanceEntity{...}(and consider includingstatus/weight/uriif those are meaningful for diagnosis).
shenyu-registry-api/src/main/java/org/apache/shenyu/registry/api/path/InstancePathConstants.java:1DOT_SEPARATORis declared but never used within this class (the full file is shown in the diff). Consider removing it to reduce dead code and avoid confusion.
shenyu-spring-boot-starter-client/shenyu-spring-boot-starter-client-tars/src/test/java/org/apache/shenyu/springboot/starter/client/tars/ShenyuTarsClientConfigurationTest.java:1- The static Mockito mock is manually closed at the end of the test. If an assertion or context initialization throws,
close()is skipped and the static mock can leak into other tests. Use try-with-resources (try (MockedStatic<RegisterUtils> mocked = mockStatic(...)) { ... }) to guarantee cleanup.
Comment on lines
+81
to
+88
| InstanceEntity instance = new InstanceEntity(); | ||
| instance.setStatus(currentInstanceUpstream.getStatus()); | ||
| instance.setWeight(currentInstanceUpstream.getWeight()); | ||
| URI uri = URI.create(currentInstanceUpstream.getProtocol() + currentInstanceUpstream.getUrl()); | ||
| instance.setPort(uri.getPort()); | ||
| instance.setHost(uri.getHost()); | ||
| instance.setAppName(discoveryConfig.getProps().getProperty("name")); | ||
| discoveryService.persistInstance(instance); |
Comment on lines
+54
to
56
| - uses: codecov/codecov-action@v4 | ||
| with: | ||
| token: 2760af6a-3405-4882-9e61-04c5176fecfa |
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.
Make sure that:
./mvnw clean install -Dmaven.javadoc.skip=true.