bump version to 2.4.0 #5326
bump version to 2.4.0 #5326
Conversation
dosubot
bot
added
the
size:S
WalkthroughThis pull request updates the Java SDK user guide documentation and project configuration files. Key modifications include updating the Java version requirement from 1.7 to 1.8, increasing the Guava library version from 20.0+ to 22.0+, and transitioning project properties in Changes
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
nobodyiam
force-pushed
the
2.4.0-release
branch
from
22e1de4 to
b8eb3e3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/zh/client/java-sdk-user-guide.md (1)
663-663: Fix markdown formatting issuesThe markdown has some minor formatting issues:
- Unordered list indentation should be 2 spaces instead of 4
- The link fragment reference is invalid
Apply these changes to fix the formatting:
- 用户可以根据需求自定义接入Prometheus等监控系统,客户端提供了SPI,详见 [7.3 指标输出到自定义监控系统](#_73-指标输出到自定义监控系统)。 + 用户可以根据需求自定义接入Prometheus等监控系统,客户端提供了SPI,详见[7.3 指标输出到自定义监控系统](#73-指标输出到自定义监控系统)。🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
663-663: Link fragments should be valid
null(MD051, link-fragments)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/en/client/java-sdk-user-guide.md(2 hunks)docs/zh/client/java-sdk-user-guide.md(2 hunks)pom.xml(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: build
pom.xml
[error] Failed to resolve dependency: com.ctrip.framework.apollo:apollo-core:jar:2.4.0 not found in Maven Central repository
🪛 markdownlint-cli2 (0.17.2)
docs/zh/client/java-sdk-user-guide.md
10-10: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
663-663: Link fragments should be valid
null
(MD051, link-fragments)
docs/en/client/java-sdk-user-guide.md
689-689: Link fragments should be valid
null
(MD051, link-fragments)
🔇 Additional comments (3)
pom.xml (1)
63-63: LGTM: Version bump to 2.4.0The version changes from SNAPSHOT to release version 2.4.0 are consistent with the PR objective.
Also applies to: 66-66
🧰 Tools
🪛 GitHub Actions: build
[error] Failed to resolve dependency: com.ctrip.framework.apollo:apollo-core:jar:2.4.0 not found in Maven Central repository
docs/zh/client/java-sdk-user-guide.md (1)
9-10: LGTM: Updated Guava version requirementsThe documentation clearly states the new minimum required version (22.0+) and default version (32) for Guava.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
10-10: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
docs/en/client/java-sdk-user-guide.md (1)
689-689: LGTM! Documentation addition for custom monitoring systems integration.The added line provides a helpful cross-reference to the detailed section about exporting metrics to custom monitoring systems, improving document navigation.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
689-689: Link fragments should be valid
null(MD051, link-fragments)
nobodyiam
force-pushed
the
2.4.0-release
branch
from
b8eb3e3 to
99d81e9
Compare
dosubot
bot
added
size:L
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/grayReleaseRule/GrayReleaseRulesHolder.java (1)
163-181: LGTM! Robust implementation with proper null checks.The implementation correctly handles both IP-based and label-based gray release rules with appropriate null checks. Consider extracting the label check logic into a private method for better readability.
public boolean hasGrayReleaseRule(String clientAppId, String clientIp, String clientLabel, String namespaceName) { // check ip gray rule if (reversedGrayReleaseRuleCache.containsKey(assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, clientIp)) || reversedGrayReleaseRuleCache.containsKey (assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, GrayReleaseRuleItemDTO .ALL_IP))) { return true; } - // check label gray rule - if (!Strings.isNullOrEmpty(clientLabel) && - (reversedGrayReleaseRuleLabelCache.containsKey( - assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, clientLabel)) || - reversedGrayReleaseRuleLabelCache.containsKey( - assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, - GrayReleaseRuleItemDTO.ALL_Label)))) { - return true; - } + return hasLabelGrayReleaseRule(clientAppId, clientLabel, namespaceName); return false; } + +private boolean hasLabelGrayReleaseRule(String clientAppId, String clientLabel, String namespaceName) { + return !Strings.isNullOrEmpty(clientLabel) && + (reversedGrayReleaseRuleLabelCache.containsKey( + assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, clientLabel)) || + reversedGrayReleaseRuleLabelCache.containsKey( + assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, + GrayReleaseRuleItemDTO.ALL_Label))); +}
🛑 Comments failed to post (2)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/registry/configuration/support/ApolloServiceRegistryProperties.java (2)
92-94: 🛠️ Refactor suggestion
Improve URI construction robustness.
The current URI construction has several potential issues:
- No null check for servletContext
- Possible double slashes if context path starts with "/"
- No validation of the constructed URI
Consider this more robust implementation:
- String contextPath = Strings.isNullOrEmpty(this.servletContext.getContextPath()) ? "/" - : this.servletContext.getContextPath(); - String uriString = "http://" + host + ":" + port + contextPath; + String contextPath = "/"; + if (this.servletContext != null) { + String servletContextPath = this.servletContext.getContextPath(); + if (!Strings.isNullOrEmpty(servletContextPath)) { + contextPath = servletContextPath.startsWith("/") ? servletContextPath : "/" + servletContextPath; + } + } + String uriString = String.format("http://%s:%d%s", host, port, contextPath); + try { + this.uri = new URI(uriString); + } catch (URISyntaxException e) { + throw new IllegalStateException("Failed to construct service URI: " + uriString, e); + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.String contextPath = "/"; if (this.servletContext != null) { String servletContextPath = this.servletContext.getContextPath(); if (!Strings.isNullOrEmpty(servletContextPath)) { contextPath = servletContextPath.startsWith("/") ? servletContextPath : "/" + servletContextPath; } } String uriString = String.format("http://%s:%d%s", host, port, contextPath); try { this.uri = new URI(uriString); } catch (URISyntaxException e) { throw new IllegalStateException("Failed to construct service URI: " + uriString, e); }
77-79: 💡 Codebase verification
ServletContext should be optional in non-web environments
The service registry component is designed to work in both web and non-web environments (evidenced by database-centric implementation and lack of web-specific conditions), but the current ServletContext autowiring could cause failures in non-web scenarios.
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/registry/configuration/support/ApolloServiceRegistryProperties.java: Make ServletContext optional and provide a fallback mechanism:@Autowired(required = false) private ServletContext servletContext;🔗 Analysis chain
Consider handling non-web environments gracefully.
The
@Autowired ServletContextcreates a tight coupling to web environments. This could cause issues in:
- Non-web application contexts
- Unit testing scenarios
- Standalone deployments
Consider making the ServletContext optional and providing a fallback:
- @Autowired - private ServletContext servletContext; + @Autowired(required = false) + private ServletContext servletContext;🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find ServletContext usage patterns ast-grep --pattern 'servletContext.$$$' # Find ServletContext related code rg -l "ServletContext" --type java # Look for test files related to ApolloServiceRegistryProperties fd "ApolloServiceRegistryProperties.*Test.*\.java$" # Check for configuration files that might have web/non-web setup fd "application.*\.properties$|application.*\.yml$"Length of output: 5365
Script:
#!/bin/bash # Check ServletContext usage in both files echo "=== ApolloServiceRegistryProperties.java ===" rg -A 5 -B 5 "ServletContext" apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/registry/configuration/support/ApolloServiceRegistryProperties.java echo "=== PrefixPathController.java ===" rg -A 5 -B 5 "ServletContext" apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/PrefixPathController.java # Check if this is part of a web-only component rg -l "ServiceRegistry" --type javaLength of output: 4036
Script:
#!/bin/bash # Check auto-configuration and conditional setup echo "=== Checking auto-configuration ===" rg -A 10 "@Configuration|@ConditionalOn" apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/registry/configuration/ApolloServiceRegistryAutoConfiguration.java # Check if there's any conditional web environment check echo "=== Checking web environment conditions ===" rg -l "@ConditionalOnWebApplication" --type javaLength of output: 1731
nobodyiam
force-pushed
the
2.4.0-release
branch
from
f947161 to
9382e80
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/grayReleaseRule/GrayReleaseRulesHolder.java (2)
163-181: Consider extracting check logic into separate methods.While the implementation is correct, the method could be more maintainable by extracting the IP and label checks into separate methods.
Consider this refactor:
- public boolean hasGrayReleaseRule(String clientAppId, String clientIp, String clientLabel, - String namespaceName) { - // check ip gray rule - if (reversedGrayReleaseRuleCache.containsKey(assembleReversedGrayReleaseRuleKey(clientAppId, - namespaceName, clientIp)) || reversedGrayReleaseRuleCache.containsKey - (assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, GrayReleaseRuleItemDTO - .ALL_IP))) { - return true; - } - // check label gray rule - if (!Strings.isNullOrEmpty(clientLabel) && - (reversedGrayReleaseRuleLabelCache.containsKey( - assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, clientLabel)) || - reversedGrayReleaseRuleLabelCache.containsKey( - assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, - GrayReleaseRuleItemDTO.ALL_Label)))) { - return true; - } - return false; + public boolean hasGrayReleaseRule(String clientAppId, String clientIp, String clientLabel, + String namespaceName) { + return hasIpGrayReleaseRule(clientAppId, clientIp, namespaceName) || + hasLabelGrayReleaseRule(clientAppId, clientLabel, namespaceName); + } + + private boolean hasIpGrayReleaseRule(String clientAppId, String clientIp, String namespaceName) { + return reversedGrayReleaseRuleCache.containsKey( + assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, clientIp)) || + reversedGrayReleaseRuleCache.containsKey( + assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, + GrayReleaseRuleItemDTO.ALL_IP)); + } + + private boolean hasLabelGrayReleaseRule(String clientAppId, String clientLabel, String namespaceName) { + if (Strings.isNullOrEmpty(clientLabel)) { + return false; + } + return reversedGrayReleaseRuleLabelCache.containsKey( + assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, clientLabel)) || + reversedGrayReleaseRuleLabelCache.containsKey( + assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, + GrayReleaseRuleItemDTO.ALL_Label)); + }
310-313: Consider introducing an abstraction for client identifiers.While the parameter rename makes the dual purpose clear, consider introducing an abstraction to handle different types of client identifiers (IP, label) more elegantly.
Consider creating an enum or class to represent different types of client identifiers:
public enum ClientIdentifierType { IP, LABEL } public class ClientIdentifier { private final String value; private final ClientIdentifierType type; // Constructor and getters }Then update the method signature:
- private String assembleReversedGrayReleaseRuleKey(String clientAppId, String clientNamespaceName, String clientIpOrLabel) + private String assembleReversedGrayReleaseRuleKey(String clientAppId, String clientNamespaceName, ClientIdentifier identifier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
doc/images/manage-cluster-entry.pngis excluded by!**/*.png
📒 Files selected for processing (12)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/grayReleaseRule/GrayReleaseRulesHolder.java(6 hunks)apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/registry/configuration/support/ApolloServiceRegistryProperties.java(3 hunks)apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/grayReleaseRule/GrayReleaseRulesHolderTest.java(2 hunks)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigFileController.java(2 hunks)apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigFileControllerTest.java(2 hunks)apollo-portal/src/main/resources/static/scripts/controller/ConfigExportController.js(2 hunks)apollo-portal/src/main/resources/static/scripts/directive/import-namespace-modal-directive.js(1 hunks)docs/en/client/java-sdk-user-guide.md(2 hunks)docs/en/portal/apollo-user-guide.md(1 hunks)docs/zh/client/java-sdk-user-guide.md(2 hunks)docs/zh/portal/apollo-user-guide.md(1 hunks)pom.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- apollo-portal/src/main/resources/static/scripts/directive/import-namespace-modal-directive.js
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/registry/configuration/support/ApolloServiceRegistryProperties.java
- apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigFileControllerTest.java
- apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/grayReleaseRule/GrayReleaseRulesHolderTest.java
- pom.xml
- apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigFileController.java
- docs/zh/client/java-sdk-user-guide.md
- docs/zh/portal/apollo-user-guide.md
- apollo-portal/src/main/resources/static/scripts/controller/ConfigExportController.js
- docs/en/portal/apollo-user-guide.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (8)
🔇 Additional comments (5)
docs/en/client/java-sdk-user-guide.md (2)
11-12: Guava Version Update: Requirements and Default Reference Changed
The changes clearly update the Guava dependency details to “22.0+” and note that the Apollo client will now reference Guava 32 by default. These modifications provide clearer guidance on compatible versions and ensure that users are aware of the minimum version requirements.
689-689: Prometheus Integration Guidance via SPI
This new sentence explicitly informs users that customization for integrating with monitoring systems (such as Prometheus) is available via SPI. Consider mentioning that this feature is applicable for version 2.4.0 and above to maintain consistency with other sections. Overall, the clarification is beneficial.apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/grayReleaseRule/GrayReleaseRulesHolder.java (3)
71-72: LGTM! Well-structured cache addition.The new cache field follows existing patterns and is well-documented.
85-86: LGTM! Thread-safe cache initialization.The initialization follows the established pattern and maintains thread safety.
253-256: LGTM! Consistent cache management implementation.The label cache management follows the same pattern as IP cache management, ensuring consistency and reliability.
Also applies to: 269-272
What's the purpose of this PR
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.Summary by CodeRabbit
Release Notes v2.4.0
Documentation
Version Update
Compatibility
Enhancements