Optimize dynamic config: integrate Zookeeper & Nacos, support interface-level dynamic config #1430
Conversation
|
Hi @Narzisss, welcome to SOFAStack community, Please sign Contributor License Agreement! After you signed CLA, we will automatically sync the status of this pull request in 3 minutes. |
sofastack-cla
bot
added
cla:no
WalkthroughThe recent updates enhance the dynamic configuration management capabilities within the SOFA RPC framework. Key changes include the introduction of new classes for handling dynamic configurations with Nacos and Zookeeper, along with modifications to existing classes for better management of configuration events. Additionally, new unit tests have been added to validate the functionality of these dynamic configurations, ensuring that changes are correctly processed and reflected in consumer services. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ZookeeperDynamicConfigManager
participant NacosDynamicConfigManager
participant ZooKeeper
participant Nacos
User->>ZookeeperDynamicConfigManager: Request Config from ZK
ZookeeperDynamicConfigManager->>ZooKeeper: Fetch Config Data
ZooKeeper-->>ZookeeperDynamicConfigManager: Return Config Data
ZookeeperDynamicConfigManager-->>User: Provide ZK Config
User->>NacosDynamicConfigManager: Request Config from Nacos
NacosDynamicConfigManager->>Nacos: Fetch Config Data
Nacos-->>NacosDynamicConfigManager: Return Config Data
NacosDynamicConfigManager-->>User: Provide Nacos Config
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- all/pom.xml (2 hunks)
- config/config-zk/pom.xml (1 hunks)
- config/config-zk/src/main/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManager.java (1 hunks)
- config/config-zk/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.dynamic.DynamicConfigManager (1 hunks)
- config/config-zk/src/test/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManagerTest.java (1 hunks)
- config/config-zk/src/test/resources/log4j.xml (1 hunks)
- config/pom.xml (1 hunks)
Files skipped from review due to trivial changes (2)
- config/config-zk/src/test/resources/log4j.xml
- config/pom.xml
Additional comments not posted (24)
config/config-zk/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.dynamic.DynamicConfigManager (1)
1-1: LGTM!The service provider configuration for
ZookeeperDynamicConfigManageris correctly mapped.config/config-zk/src/test/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManagerTest.java (5)
1-16: Ensure the license header is up-to-date.The license header is present and appears to be correct. Ensure it is up-to-date with the latest project standards.
17-17: Package declaration is correct.The package declaration aligns with the project's structure.
19-23: Imports are appropriate.The imported classes are relevant for the test cases.
27-28: Logger initialization is correct.The logger is correctly initialized for the test class.
32-36: Test methodgetProviderServicePropertyis well-defined.The test method checks the retrieval of provider service properties and asserts the expected result.
config/config-zk/pom.xml (6)
1-11: POM file header and parent configuration are correct.The POM file header and parent configuration are correctly defined.
13-13: Artifact ID is correct.The artifact ID
sofa-rpc-config-zkaligns with the project structure.
15-47: Dependencies are correctly defined.The dependencies are necessary and correctly defined for the project.
51-71: Resources configuration is correct.The resources configuration is correctly defined for the project.
73-110: Plugins configuration is correctly defined.The plugins configuration includes necessary Maven plugins and is correctly defined.
111-112: POM file footer is correct.The POM file footer is correctly defined.
config/config-zk/src/main/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManager.java (10)
1-16: Ensure compliance with ASF license.The file includes the Apache Software Foundation license header. Ensure that the usage complies with the ASF guidelines.
17-27: Imports look good.The imports are relevant and necessary for the class functionality.
34-35: Class annotation and declaration are appropriate.The
@Extensionannotation is correctly used to indicate that this class is an extension for Zookeeper.
37-41: Class-level variables are well-defined.The class-level variables are correctly declared and initialized.
43-51: Constructor initializes Zookeeper client correctly.The constructor initializes the Zookeeper client with the specified address, retry policy, and namespace.
58-67: Method correctly retrieves provider service property.The method builds the path using
DynamicConfigKeyHelper, retrieves the data from Zookeeper, and handles exceptions appropriately.
69-77: Method correctly retrieves consumer service property.The method builds the path using
DynamicConfigKeyHelper, retrieves the data from Zookeeper, and handles exceptions appropriately.
80-89: Method correctly retrieves provider method property.The method builds the path using
DynamicConfigKeyHelper, retrieves the data from Zookeeper, and handles exceptions appropriately.
91-99: Method correctly retrieves consumer method property.The method builds the path using
DynamicConfigKeyHelper, retrieves the data from Zookeeper, and handles exceptions appropriately.
109-111: Method correctly constructs prefix path.The method constructs the prefix path by concatenating the configuration node, default group, and context separator.
all/pom.xml (2)
313-317: Dependency forsofa-rpc-config-zkis correctly added.The dependency is correctly declared with the appropriate groupId, artifactId, and version.
573-573: Include forsofa-rpc-config-zkis correctly added.The include is correctly declared within the Maven Shade Plugin configuration.
| private final static Logger logger = LoggerFactory | ||
| .getLogger(ZookeeperDynamicConfigManager.class); | ||
|
|
||
| private ZookeeperDynamicConfigManager zookeeperDynamicConfigManager = new ZookeeperDynamicConfigManager("test"); |
There was a problem hiding this comment.
Consider using dependency injection for ZookeeperDynamicConfigManager.
Direct instantiation of ZookeeperDynamicConfigManager may limit test flexibility. Consider using dependency injection for better testability.
There was a problem hiding this comment.
Can use DynamicConfigManagerFactory.getDynamicManager instead.
There was a problem hiding this comment.
@EvenLjj, thank you for the suggestion. Using DynamicConfigManagerFactory.getDynamicManager is indeed a better approach and aligns with the existing code practices. I recommend updating the test code to utilize this factory method for creating instances of ZookeeperDynamicConfigManager.
private ZookeeperDynamicConfigManager zookeeperDynamicConfigManager = (ZookeeperDynamicConfigManager) DynamicConfigManagerFactory.getDynamicManager("test");This change will enhance the testability and maintain consistency with the rest of the codebase.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the definition and usage of `DynamicConfigManagerFactory.getDynamicManager` in the codebase.
rg 'DynamicConfigManagerFactory.getDynamicManager'
Length of output: 627
...onfig-zk/src/test/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManagerTest.java
Outdated
Show resolved
Hide resolved
...onfig-zk/src/test/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManagerTest.java
Outdated
Show resolved
Hide resolved
...onfig-zk/src/test/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManagerTest.java
Outdated
Show resolved
Hide resolved
...ig/config-zk/src/main/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManager.java
Outdated
Show resolved
Hide resolved
...ig/config-zk/src/main/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManager.java
Outdated
Show resolved
Hide resolved
...ig/config-zk/src/main/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManager.java
Show resolved
Hide resolved
| public String getProviderServiceProperty(String service, String key) { | ||
| try { | ||
| String path = getPrefixPath() + DynamicConfigKeyHelper.buildProviderServiceProKey(service, key); | ||
| byte[] bytes = zkClient.getData().forPath(path); |
There was a problem hiding this comment.
This zkClient.getData().forPath method does not have cached. Here should add a cache in conjunction with the listener mentioned above.
| @Override | ||
| public String getConsumerServiceProperty(String service, String key) { | ||
| try { | ||
| String path = getPrefixPath() + DynamicConfigKeyHelper.buildConsumerServiceProKey(service, key); |
There was a problem hiding this comment.
This zkClient.getData().forPath method does not have cached. Here should add a cache in conjunction with the listener mentioned above.
| @Override | ||
| public String getProviderMethodProperty(String service, String method, String key) { | ||
| try { | ||
| String path = getPrefixPath() + DynamicConfigKeyHelper.buildProviderMethodProKey(service, method, key); |
There was a problem hiding this comment.
This zkClient.getData().forPath method does not have cached. Here should add a cache in conjunction with the listener mentioned above.
| @Override | ||
| public String getConsumerMethodProperty(String service, String method, String key) { | ||
| try { | ||
| String path = getPrefixPath() + DynamicConfigKeyHelper.buildConsumerMethodProKey(service, method, key); |
There was a problem hiding this comment.
This zkClient.getData().forPath method does not have cached. Here should add a cache in conjunction with the listener mentioned above.
|
|
There was a problem hiding this comment.
Actionable comments posted: 11
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- all/pom.xml (2 hunks)
- config/config-nacos/pom.xml (1 hunks)
- config/config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NaocsDynamicConfigManager.java (1 hunks)
- config/config-nacos/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.dynamic.DynamicConfigManager (1 hunks)
- config/config-nacos/src/test/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManagerTest.java (1 hunks)
- config/config-nacos/src/test/resources/log4j.xml (1 hunks)
- config/config-zk/pom.xml (1 hunks)
- config/config-zk/src/main/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManager.java (1 hunks)
- config/config-zk/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.dynamic.DynamicConfigManager (1 hunks)
- config/config-zk/src/test/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManagerTest.java (1 hunks)
- config/config-zk/src/test/resources/log4j.xml (1 hunks)
- config/pom.xml (1 hunks)
- core/common/src/main/java/com/alipay/sofa/rpc/common/config/RpcConfigKeys.java (1 hunks)
Files skipped from review due to trivial changes (2)
- config/config-nacos/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.dynamic.DynamicConfigManager
- config/config-nacos/src/test/resources/log4j.xml
Additional comments not posted (24)
config/config-zk/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.dynamic.DynamicConfigManager (1)
1-1: LGTM!The service provider configuration correctly maps the
zookeeperkey to theZookeeperDynamicConfigManagerclass.config/config-zk/src/test/resources/log4j.xml (1)
1-16: LGTM!The Log4j configuration sets up a console appender with a pattern and sets the root logger level to INFO. This is a standard configuration for test environments.
config/config-zk/src/test/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManagerTest.java (1)
32-36: LGTM!The test method
getProviderServicePropertyis implemented correctly and verifies the expected behavior.config/pom.xml (1)
18-19: LGTM!The additions of
config-zkandconfig-nacosmodules are straightforward and align with the PR objectives.config/config-nacos/pom.xml (1)
1-107: LGTM!The dependencies and build configurations for the
config-nacosmodule are standard for a Maven project and align with the PR objectives.config/config-zk/pom.xml (4)
13-13: LGTM!The artifact ID
sofa-rpc-config-zkis appropriate and follows naming conventions.
51-111: LGTM!The build configurations are appropriate and follow best practices.
1-11: Ensure the parent project version is defined.The parent project version is defined using the
${revision}property. Ensure that this property is set correctly in the parent POM or properties file.Verification successful
The parent project version is defined correctly.
The
${revision}property is set to5.13.1-SNAPSHOTin thepom.xmlandbom/pom.xmlfiles. This confirms that the parent project version is defined and has a valid value.
pom.xml:<revision>5.13.1-SNAPSHOT</revision>bom/pom.xml:<revision>5.13.1-SNAPSHOT</revision>Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `${revision}` property is set correctly in the parent POM or properties file. # Test: Search for the `${revision}` property definition. Expect: The property is defined and has a valid value. rg --type xml -A 3 $'<revision>'Length of output: 518
15-47: Ensure dependency versions are managed.The dependencies do not specify versions, which implies that the versions are managed in the parent POM or a dependency management section. Ensure that the versions are defined and managed correctly.
Verification successful
Dependency versions are managed.
The dependencies within the
dependencyManagementsections have versions specified, often using placeholders like${project.parent.version}or${revision}. This indicates that the versions are managed appropriately.
tracer/pom.xmlremoting/pom.xmlregistry/pom.xmlpom.xmlmetrics/pom.xmlfault/pom.xmlcore/pom.xmlcore-impl/pom.xmlconfig/pom.xmlbootstrap/pom.xmlcodec/pom.xmlbom/pom.xmlScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the dependency versions are managed correctly in the parent POM or a dependency management section. # Test: Search for the dependency management section. Expect: The dependencies are defined with versions. rg --type xml -A 3 $'<dependencyManagement>'Length of output: 2280
Script:
#!/bin/bash # Description: Verify that the dependencies within the dependency management sections have versions specified. # List of POM files containing dependencyManagement sections poms=( "tracer/pom.xml" "remoting/pom.xml" "registry/pom.xml" "pom.xml" "metrics/pom.xml" "fault/pom.xml" "core/pom.xml" "core-impl/pom.xml" "config/pom.xml" "bootstrap/pom.xml" "codec/pom.xml" "bom/pom.xml" ) # Check for the presence of version tags within the dependencyManagement sections for pom in "${poms[@]}"; do echo "Checking $pom for dependency versions..." rg --type xml -A 3 '<dependencyManagement>' "$pom" | grep -A 10 '<dependency>' | grep '<version>' doneLength of output: 3140
Script:
#!/bin/bash # Description: Extract and display the entire dependencyManagement sections from the identified POM files to verify the presence of version tags. # List of POM files containing dependencyManagement sections poms=( "tracer/pom.xml" "remoting/pom.xml" "registry/pom.xml" "pom.xml" "metrics/pom.xml" "fault/pom.xml" "core/pom.xml" "core-impl/pom.xml" "config/pom.xml" "bootstrap/pom.xml" "codec/pom.xml" "bom/pom.xml" ) # Extract and display the dependencyManagement sections for pom in "${poms[@]}"; do echo "Extracting dependencyManagement section from $pom..." rg --type xml -A 50 '<dependencyManagement>' "$pom" doneLength of output: 24768
config/config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NaocsDynamicConfigManager.java (3)
1-17: LGTM!The license header and package declaration are present and correct.
19-30: LGTM!The import statements are appropriate and necessary.
67-104: LGTM!The methods are implemented correctly and handle exceptions appropriately.
config/config-zk/src/main/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManager.java (6)
1-17: LGTM!The license header and package declaration are present and correct.
19-30: LGTM!The import statements are appropriate and necessary.
46-54: LGTM!The constructor initializes the
CuratorFrameworkwith Zookeeper configurations appropriately.
56-59: Reminder: Implement the service configuration initialization.The method
initServiceConfigurationis currently a placeholder. Ensure to implement the service configuration initialization in the future.
107-110: Reminder: Implement the service authentication rule retrieval.The method
getServiceAuthRuleis currently a placeholder. Ensure to implement the service authentication rule retrieval in the future.
36-45: Ensure the default address is configurable.The default address
127.0.0.1:2181is hardcoded. UseSofaConfigs.getOrDefaultto make it configurable.- private static final String ADDRESS = "127.0.0.1:2181"; + private static final String ADDRESS = SofaConfigs.getOrCustomDefault(RpcConfigKeys.ZK_ADDRESS, "127.0.0.1:2181");Likely invalid or redundant comment.
core/common/src/main/java/com/alipay/sofa/rpc/common/config/RpcConfigKeys.java (2)
145-151: LGTM!The
ZK_ADDRESSconfiguration key is correctly defined with appropriate parameters.
153-159: LGTM!The
NACOS_ADDRESSconfiguration key is correctly defined with appropriate parameters.all/pom.xml (4)
313-317: LGTM!The
sofa-rpc-config-zkdependency is correctly added under the<dependencies>section.
578-578: LGTM!The
sofa-rpc-config-zkdependency is correctly included in the<includes>section of the configuration block.
318-322: LGTM!The
sofa-rpc-config-nacosdependency is correctly added under the<dependencies>section.
579-579: LGTM!The
sofa-rpc-config-nacosdependency is correctly included in the<includes>section of the configuration block.
| @Test | ||
| public void getConsumerServiceProperty() { | ||
| } | ||
|
|
||
| @Test | ||
| public void getProviderMethodProperty() { | ||
| } | ||
|
|
||
| @Test | ||
| public void getConsumerMethodProperty() { | ||
| } | ||
|
|
||
| @Test | ||
| public void getServiceAuthRule() { | ||
| } |
There was a problem hiding this comment.
Implement the missing test methods.
The following test methods are currently empty and should be implemented to ensure full test coverage:
getConsumerServicePropertygetProviderMethodPropertygetConsumerMethodPropertygetServiceAuthRule
...fig-nacos/src/test/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManagerTest.java
Outdated
Show resolved
Hide resolved
...onfig-zk/src/test/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManagerTest.java
Outdated
Show resolved
Hide resolved
...onfig-zk/src/test/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManagerTest.java
Outdated
Show resolved
Hide resolved
...onfig-zk/src/test/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManagerTest.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public void initServiceConfiguration(String service) { | ||
| //TODO not now | ||
|
|
||
| } |
There was a problem hiding this comment.
Reminder: Implement the service configuration initialization.
The method initServiceConfiguration is currently a placeholder. Ensure to implement the service configuration initialization in the future.
Do you want me to generate the implementation code or open a GitHub issue to track this task?
| public AuthRuleGroup getServiceAuthRule(String service) { | ||
| //TODO 暂不支持 | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Reminder: Implement the service authentication rule retrieval.
The method getServiceAuthRule is currently a placeholder. Ensure to implement the service authentication rule retrieval in the future.
Do you want me to generate the implementation code or open a GitHub issue to track this task?
.../config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NaocsDynamicConfigManager.java
Outdated
Show resolved
Hide resolved
.../config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NaocsDynamicConfigManager.java
Outdated
Show resolved
Hide resolved
| public String getProviderServiceProperty(String service, String key) { | ||
| try { | ||
| String path = getPrefixPath() + DynamicConfigKeyHelper.buildProviderServiceProKey(service, key); | ||
| byte[] bytes = zkClient.getData().forPath(path); | ||
| return bytes != null ? new String(bytes) : DynamicHelper.DEFAULT_DYNAMIC_VALUE; | ||
| } catch (Exception e) { | ||
| return DynamicHelper.DEFAULT_DYNAMIC_VALUE; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public String getConsumerServiceProperty(String service, String key) { | ||
| try { | ||
| String path = getPrefixPath() + DynamicConfigKeyHelper.buildConsumerServiceProKey(service, key); | ||
| byte[] bytes = zkClient.getData().forPath(path); | ||
| return bytes != null ? new String(bytes) : DynamicHelper.DEFAULT_DYNAMIC_VALUE; | ||
| } catch (Exception e) { | ||
| return DynamicHelper.DEFAULT_DYNAMIC_VALUE; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public String getProviderMethodProperty(String service, String method, String key) { | ||
| try { | ||
| String path = getPrefixPath() + DynamicConfigKeyHelper.buildProviderMethodProKey(service, method, key); | ||
| byte[] bytes = zkClient.getData().forPath(path); | ||
| return bytes != null ? new String(bytes) : DynamicHelper.DEFAULT_DYNAMIC_VALUE; | ||
| } catch (Exception e) { | ||
| return DynamicHelper.DEFAULT_DYNAMIC_VALUE; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public String getConsumerMethodProperty(String service, String method, String key) { | ||
| try { | ||
| String path = getPrefixPath() + DynamicConfigKeyHelper.buildConsumerMethodProKey(service, method, key); | ||
| byte[] bytes = zkClient.getData().forPath(path); | ||
| return bytes != null ? new String(bytes) : DynamicHelper.DEFAULT_DYNAMIC_VALUE; | ||
| } catch (Exception e) { | ||
| return DynamicHelper.DEFAULT_DYNAMIC_VALUE; | ||
| } | ||
|
|
There was a problem hiding this comment.
Add caching for Zookeeper data retrieval.
The zkClient.getData().forPath method does not have caching. Consider adding a cache in conjunction with a listener to improve performance.
- byte[] bytes = zkClient.getData().forPath(path);
+ byte[] bytes = getCachedData(path);
private byte[] getCachedData(String path) {
// Implement caching logic here
return zkClient.getData().forPath(path);
}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.
| public String getProviderServiceProperty(String service, String key) { | |
| try { | |
| String path = getPrefixPath() + DynamicConfigKeyHelper.buildProviderServiceProKey(service, key); | |
| byte[] bytes = zkClient.getData().forPath(path); | |
| return bytes != null ? new String(bytes) : DynamicHelper.DEFAULT_DYNAMIC_VALUE; | |
| } catch (Exception e) { | |
| return DynamicHelper.DEFAULT_DYNAMIC_VALUE; | |
| } | |
| } | |
| @Override | |
| public String getConsumerServiceProperty(String service, String key) { | |
| try { | |
| String path = getPrefixPath() + DynamicConfigKeyHelper.buildConsumerServiceProKey(service, key); | |
| byte[] bytes = zkClient.getData().forPath(path); | |
| return bytes != null ? new String(bytes) : DynamicHelper.DEFAULT_DYNAMIC_VALUE; | |
| } catch (Exception e) { | |
| return DynamicHelper.DEFAULT_DYNAMIC_VALUE; | |
| } | |
| } | |
| @Override | |
| public String getProviderMethodProperty(String service, String method, String key) { | |
| try { | |
| String path = getPrefixPath() + DynamicConfigKeyHelper.buildProviderMethodProKey(service, method, key); | |
| byte[] bytes = zkClient.getData().forPath(path); | |
| return bytes != null ? new String(bytes) : DynamicHelper.DEFAULT_DYNAMIC_VALUE; | |
| } catch (Exception e) { | |
| return DynamicHelper.DEFAULT_DYNAMIC_VALUE; | |
| } | |
| } | |
| @Override | |
| public String getConsumerMethodProperty(String service, String method, String key) { | |
| try { | |
| String path = getPrefixPath() + DynamicConfigKeyHelper.buildConsumerMethodProKey(service, method, key); | |
| byte[] bytes = zkClient.getData().forPath(path); | |
| return bytes != null ? new String(bytes) : DynamicHelper.DEFAULT_DYNAMIC_VALUE; | |
| } catch (Exception e) { | |
| return DynamicHelper.DEFAULT_DYNAMIC_VALUE; | |
| } | |
| public String getProviderServiceProperty(String service, String key) { | |
| try { | |
| String path = getPrefixPath() + DynamicConfigKeyHelper.buildProviderServiceProKey(service, key); | |
| byte[] bytes = getCachedData(path); | |
| return bytes != null ? new String(bytes) : DynamicHelper.DEFAULT_DYNAMIC_VALUE; | |
| } catch (Exception e) { | |
| return DynamicHelper.DEFAULT_DYNAMIC_VALUE; | |
| } | |
| } | |
| @Override | |
| public String getConsumerServiceProperty(String service, String key) { | |
| try { | |
| String path = getPrefixPath() + DynamicConfigKeyHelper.buildConsumerServiceProKey(service, key); | |
| byte[] bytes = getCachedData(path); | |
| return bytes != null ? new String(bytes) : DynamicHelper.DEFAULT_DYNAMIC_VALUE; | |
| } catch (Exception e) { | |
| return DynamicHelper.DEFAULT_DYNAMIC_VALUE; | |
| } | |
| } | |
| @Override | |
| public String getProviderMethodProperty(String service, String method, String key) { | |
| try { | |
| String path = getPrefixPath() + DynamicConfigKeyHelper.buildProviderMethodProKey(service, method, key); | |
| byte[] bytes = getCachedData(path); | |
| return bytes != null ? new String(bytes) : DynamicHelper.DEFAULT_DYNAMIC_VALUE; | |
| } catch (Exception e) { | |
| return DynamicHelper.DEFAULT_DYNAMIC_VALUE; | |
| } | |
| } | |
| @Override | |
| public String getConsumerMethodProperty(String service, String method, String key) { | |
| try { | |
| String path = getPrefixPath() + DynamicConfigKeyHelper.buildConsumerMethodProKey(service, method, key); | |
| byte[] bytes = getCachedData(path); | |
| return bytes != null ? new String(bytes) : DynamicHelper.DEFAULT_DYNAMIC_VALUE; | |
| } catch (Exception e) { | |
| return DynamicHelper.DEFAULT_DYNAMIC_VALUE; | |
| } | |
| } | |
| private byte[] getCachedData(String path) { | |
| // Implement caching logic here | |
| return zkClient.getData().forPath(path); | |
| } |
Narzisss
changed the title
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range, codebase verification and nitpick comments (1)
config/config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManager.java (1)
Line range hint
151-157: Handle null case ingetCachedDatamethod.The
getCachedDatamethod should handle the case wherecache.getCurrentData(path)returns null to avoid potentialNullPointerException.- return cache.getCurrentData(path).getData(); + return cache.getCurrentData(path) != null ? cache.getCurrentData(path).getData() : null;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- all/pom.xml (2 hunks)
- config/config-nacos/pom.xml (1 hunks)
- config/config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManager.java (1 hunks)
- config/config-nacos/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.dynamic.DynamicConfigManager (1 hunks)
- config/config-nacos/src/test/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManagerTest.java (1 hunks)
- config/config-nacos/src/test/resources/log4j.xml (1 hunks)
- config/config-zk/pom.xml (1 hunks)
- config/config-zk/src/main/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManager.java (1 hunks)
- config/config-zk/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.dynamic.DynamicConfigManager (1 hunks)
- config/config-zk/src/test/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManagerTest.java (1 hunks)
- config/config-zk/src/test/resources/log4j.xml (1 hunks)
- config/pom.xml (1 hunks)
- core/common/src/main/java/com/alipay/sofa/rpc/common/config/RpcConfigKeys.java (1 hunks)
Files skipped from review due to trivial changes (4)
- config/config-nacos/pom.xml
- config/config-nacos/src/test/resources/log4j.xml
- config/config-zk/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.dynamic.DynamicConfigManager
- config/config-zk/src/test/resources/log4j.xml
Files skipped from review as they are similar to previous changes (6)
- all/pom.xml
- config/config-nacos/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.dynamic.DynamicConfigManager
- config/config-nacos/src/test/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManagerTest.java
- config/config-zk/pom.xml
- config/config-zk/src/test/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManagerTest.java
- config/pom.xml
Additional comments not posted (7)
config/config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManager.java (2)
1-18: License and package declaration are correct.The license and package declaration follow the required format.
19-33: Imports are appropriate.The imports include necessary libraries for Nacos, logging, and configuration management.
config/config-zk/src/main/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManager.java (3)
1-18: License and package declaration are correct.The license and package declaration follow the required format.
19-40: Imports are appropriate.The imports include necessary libraries for Zookeeper, logging, and configuration management.
61-75: Improve exception handling in the constructor.The exception handling in the constructor could be improved by providing more context in the log message.
- LOGGER.error("setupPathChildrenCache error", e); + LOGGER.error("setupPathChildrenCache error for appName: " + appName, e);Likely invalid or redundant comment.
core/common/src/main/java/com/alipay/sofa/rpc/common/config/RpcConfigKeys.java (2)
Line range hint
1-11: License and package declaration are correct.The license and package declaration follow the required format.
Line range hint
12-15: Imports are appropriate.The imports include necessary libraries for configuration management.
| @Override | ||
| public void initServiceConfiguration(String service) { | ||
| //TODO not now | ||
|
|
||
| } |
There was a problem hiding this comment.
Implement service configuration initialization.
The initServiceConfiguration method is currently a placeholder. Ensure to implement the service configuration initialization in the future.
Do you want me to generate the implementation code or open a GitHub issue to track this task?
| public AuthRuleGroup getServiceAuthRule(String service) { | ||
| //TODO 暂不支持 | ||
| return null; |
There was a problem hiding this comment.
Implement service authentication rule retrieval.
The getServiceAuthRule method is currently a placeholder. Ensure to implement the service authentication rule retrieval in the future.
Do you want me to generate the implementation code or open a GitHub issue to track this task?
| @Extension(value = "nacos", override = true) | ||
| public class NacosDynamicConfigManager extends DynamicConfigManager { | ||
|
|
||
| private final static Logger LOGGER = LoggerFactory.getLogger(NacosDynamicConfigManager.class); | ||
| private static final String DEFAULT_NAMESPACE = "sofa-rpc"; | ||
| private static final String ADDRESS = SofaConfigs.getOrDefault(RpcConfigKeys.NACOS_ADDRESS); | ||
| private static final String DEFAULT_GROUP = "sofa-rpc"; | ||
| private static final long DEFAULT_TIMEOUT = 5000; | ||
| private ConfigService configService; | ||
| private Properties nacosConfig = new Properties(); | ||
| private final String appName; |
There was a problem hiding this comment.
Consider using SofaConfigs#getOrDefault for default values.
The DEFAULT_NAMESPACE and DEFAULT_GROUP values are hardcoded. Consider using SofaConfigs#getOrDefault for these values to allow for configuration flexibility.
- private static final String DEFAULT_NAMESPACE = "sofa-rpc";
- private static final String DEFAULT_GROUP = "sofa-rpc";
+ private static final String DEFAULT_NAMESPACE = SofaConfigs.getOrDefault(RpcConfigKeys.NACOS_NAMESPACE, "sofa-rpc");
+ private static final String DEFAULT_GROUP = SofaConfigs.getOrDefault(RpcConfigKeys.NACOS_GROUP, "sofa-rpc");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.
| @Extension(value = "nacos", override = true) | |
| public class NacosDynamicConfigManager extends DynamicConfigManager { | |
| private final static Logger LOGGER = LoggerFactory.getLogger(NacosDynamicConfigManager.class); | |
| private static final String DEFAULT_NAMESPACE = "sofa-rpc"; | |
| private static final String ADDRESS = SofaConfigs.getOrDefault(RpcConfigKeys.NACOS_ADDRESS); | |
| private static final String DEFAULT_GROUP = "sofa-rpc"; | |
| private static final long DEFAULT_TIMEOUT = 5000; | |
| private ConfigService configService; | |
| private Properties nacosConfig = new Properties(); | |
| private final String appName; | |
| private static final String DEFAULT_NAMESPACE = SofaConfigs.getOrDefault(RpcConfigKeys.NACOS_NAMESPACE, "sofa-rpc"); | |
| private static final String DEFAULT_GROUP = SofaConfigs.getOrDefault(RpcConfigKeys.NACOS_GROUP, "sofa-rpc"); |
| protected NacosDynamicConfigManager(String appName) { | ||
| super(appName); | ||
| if (StringUtils.isEmpty(appName)) { | ||
| this.appName = DEFAULT_GROUP; | ||
| } else { | ||
| this.appName = appName; | ||
| } | ||
| try { | ||
| nacosConfig.put(PropertyKeyConst.SERVER_ADDR, ADDRESS); | ||
| nacosConfig.put(PropertyKeyConst.NAMESPACE, DEFAULT_NAMESPACE); | ||
| configService = ConfigFactory.createConfigService(nacosConfig); | ||
|
|
||
| } catch (NacosException e) { | ||
| LOGGER.error("Failed to create ConfigService", e); | ||
| } |
There was a problem hiding this comment.
Improve exception handling in the constructor.
The exception handling in the constructor could be improved by providing more context in the log message.
- LOGGER.error("Failed to create ConfigService", e);
+ LOGGER.error("Failed to create ConfigService for appName: " + appName, 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.
| protected NacosDynamicConfigManager(String appName) { | |
| super(appName); | |
| if (StringUtils.isEmpty(appName)) { | |
| this.appName = DEFAULT_GROUP; | |
| } else { | |
| this.appName = appName; | |
| } | |
| try { | |
| nacosConfig.put(PropertyKeyConst.SERVER_ADDR, ADDRESS); | |
| nacosConfig.put(PropertyKeyConst.NAMESPACE, DEFAULT_NAMESPACE); | |
| configService = ConfigFactory.createConfigService(nacosConfig); | |
| } catch (NacosException e) { | |
| LOGGER.error("Failed to create ConfigService", e); | |
| } | |
| protected NacosDynamicConfigManager(String appName) { | |
| super(appName); | |
| if (StringUtils.isEmpty(appName)) { | |
| this.appName = DEFAULT_GROUP; | |
| } else { | |
| this.appName = appName; | |
| } | |
| try { | |
| nacosConfig.put(PropertyKeyConst.SERVER_ADDR, ADDRESS); | |
| nacosConfig.put(PropertyKeyConst.NAMESPACE, DEFAULT_NAMESPACE); | |
| configService = ConfigFactory.createConfigService(nacosConfig); | |
| } catch (NacosException e) { | |
| LOGGER.error("Failed to create ConfigService for appName: " + appName, e); | |
| } |
| private byte[] getCachedData(String path) throws Exception { | ||
| PathChildrenCache cache = cacheMap.get(rootPath); | ||
| if (cache == null || cache.getCurrentData() == null) { | ||
| setupPathChildrenCache(); | ||
| cache = cacheMap.get(rootPath); | ||
| } | ||
| return cache.getCurrentData(path).getData(); |
There was a problem hiding this comment.
Handle null case in getCachedData method.
The getCachedData method should handle the case where cache.getCurrentData(path) returns null to avoid potential NullPointerException.
- return cache.getCurrentData(path).getData();
+ return cache.getCurrentData(path) != null ? cache.getCurrentData(path).getData() : null;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.
| private byte[] getCachedData(String path) throws Exception { | |
| PathChildrenCache cache = cacheMap.get(rootPath); | |
| if (cache == null || cache.getCurrentData() == null) { | |
| setupPathChildrenCache(); | |
| cache = cacheMap.get(rootPath); | |
| } | |
| return cache.getCurrentData(path).getData(); | |
| private byte[] getCachedData(String path) throws Exception { | |
| PathChildrenCache cache = cacheMap.get(rootPath); | |
| if (cache == null || cache.getCurrentData() == null) { | |
| setupPathChildrenCache(); | |
| cache = cacheMap.get(rootPath); | |
| } | |
| return cache.getCurrentData(path) != null ? cache.getCurrentData(path).getData() : null; |
...ig/config-zk/src/main/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManager.java
Outdated
Show resolved
Hide resolved
| // config center | ||
| public static ConfigKey<String> ZK_ADDRESS = ConfigKey | ||
| .build( | ||
| "sofa.rpc.config.center.address", | ||
| "127.0.0.1:2181", | ||
| false, | ||
| "The address of Zookeeper configuration center.", | ||
| new String[] { "zookeeper_address" }); | ||
|
|
||
| public static ConfigKey<String> NACOS_ADDRESS = ConfigKey | ||
| .build( | ||
| "sofa.rpc.config.center.address", | ||
| "127.0.0.1:8848", | ||
| false, | ||
| "The address of Nacos configuration center.", | ||
| new String[] { "nacos_address" }); |
There was a problem hiding this comment.
Use different key names for NACOS_ADDRESS and ZK_ADDRESS.
The key names for NACOS_ADDRESS and ZK_ADDRESS are the same. Consider using different key names to avoid conflicts.
- "sofa.rpc.config.center.address",
+ "sofa.rpc.config.center.nacos.address",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.
| // config center | |
| public static ConfigKey<String> ZK_ADDRESS = ConfigKey | |
| .build( | |
| "sofa.rpc.config.center.address", | |
| "127.0.0.1:2181", | |
| false, | |
| "The address of Zookeeper configuration center.", | |
| new String[] { "zookeeper_address" }); | |
| public static ConfigKey<String> NACOS_ADDRESS = ConfigKey | |
| .build( | |
| "sofa.rpc.config.center.address", | |
| "127.0.0.1:8848", | |
| false, | |
| "The address of Nacos configuration center.", | |
| new String[] { "nacos_address" }); | |
| // config center | |
| public static ConfigKey<String> ZK_ADDRESS = ConfigKey | |
| .build( | |
| "sofa.rpc.config.center.address", | |
| "127.0.0.1:2181", | |
| false, | |
| "The address of Zookeeper configuration center.", | |
| new String[] { "zookeeper_address" }); | |
| public static ConfigKey<String> NACOS_ADDRESS = ConfigKey | |
| .build( | |
| "sofa.rpc.config.center.nacos.address", | |
| "127.0.0.1:8848", | |
| false, | |
| "The address of Nacos configuration center.", | |
| new String[] { "nacos_address" }); |
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- all/pom.xml (2 hunks)
- config/config-apollo/src/test/java/com/alipay/sofa/rpc/dynamic/apollo/ApolloDynamicConfigManagerTest.java (3 hunks)
- config/config-nacos/pom.xml (1 hunks)
- config/config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManager.java (1 hunks)
- config/config-nacos/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.dynamic.DynamicConfigManager (1 hunks)
- config/config-nacos/src/test/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManagerTest.java (1 hunks)
- config/config-nacos/src/test/resources/log4j.xml (1 hunks)
- config/config-zk/pom.xml (1 hunks)
- config/config-zk/src/main/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManager.java (1 hunks)
- config/config-zk/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.dynamic.DynamicConfigManager (1 hunks)
- config/config-zk/src/test/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManagerTest.java (1 hunks)
- config/config-zk/src/test/resources/log4j.xml (1 hunks)
- config/pom.xml (1 hunks)
- core/common/src/main/java/com/alipay/sofa/rpc/common/config/RpcConfigKeys.java (1 hunks)
Files skipped from review due to trivial changes (4)
- config/config-nacos/pom.xml
- config/config-nacos/src/test/resources/log4j.xml
- config/config-zk/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.dynamic.DynamicConfigManager
- config/config-zk/src/test/resources/log4j.xml
Files skipped from review as they are similar to previous changes (5)
- all/pom.xml
- config/config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManager.java
- config/config-nacos/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.dynamic.DynamicConfigManager
- config/config-zk/pom.xml
- config/pom.xml
Additional comments not posted (10)
config/config-apollo/src/test/java/com/alipay/sofa/rpc/dynamic/apollo/ApolloDynamicConfigManagerTest.java (1)
32-33: Use of Factory Method for Instantiation.The use of
DynamicConfigManagerFactory.getDynamicManagerfor instantiatingapolloDynamicConfigManagerpromotes flexibility and decouples the test from specific implementations. This is a good practice for enhancing configurability.config/config-nacos/src/test/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManagerTest.java (1)
32-33: Consistent Use of Factory Method for Instantiation.The instantiation of
nacosDynamicConfigManagerusingDynamicConfigManagerFactory.getDynamicManageris consistent with best practices, ensuring flexibility and decoupling from specific implementations.config/config-zk/src/test/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManagerTest.java (1)
32-33: Adoption of Factory Pattern for Instantiation.The use of
DynamicConfigManagerFactory.getDynamicManagerfor creatingzookeeperDynamicConfigManagerinstances aligns with the factory pattern, enhancing the test's flexibility and maintainability.config/config-zk/src/main/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManager.java (6)
17-47: Imports and Class Definition Approved.The imports and class definition are appropriate for integrating Zookeeper as a configuration center.
110-118: Method Approved:getProviderServiceProperty.The method is implemented correctly and handles exceptions gracefully.
120-128: Method Approved:getConsumerServiceProperty.The method is implemented correctly and handles exceptions gracefully.
130-138: Method Approved:getProviderMethodProperty.The method is implemented correctly and handles exceptions gracefully.
140-148: Method Approved:getConsumerMethodProperty.The method is implemented correctly and handles exceptions gracefully.
51-51: MakezkClientFinal.The
zkClientfield could be made final to ensure it is not reassigned after initialization.- private final CuratorFramework zkClient; + private final CuratorFramework zkClient;Likely invalid or redundant comment.
core/common/src/main/java/com/alipay/sofa/rpc/common/config/RpcConfigKeys.java (1)
144-159: New Configuration Keys Approved.The new configuration keys
ZK_ADDRESSandNACOS_ADDRESSare well-defined and enhance the framework's configuration flexibility.
| @Override | ||
| public void initServiceConfiguration(String service) { | ||
| //TODO not now | ||
| } |
There was a problem hiding this comment.
Reminder: Implement Service Configuration Initialization.
The initServiceConfiguration method is currently a placeholder. Ensure to implement the service configuration initialization in the future.
Do you want me to generate the implementation code or open a GitHub issue to track this task?
| @Override | ||
| public AuthRuleGroup getServiceAuthRule(String service) { | ||
| //TODO 暂不支持 | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Reminder: Implement Service Authentication Rule Retrieval.
The getServiceAuthRule method is currently a placeholder. Ensure to implement the service authentication rule retrieval in the future.
Do you want me to generate the implementation code or open a GitHub issue to track this task?
| private final CuratorFramework zkClient; | ||
| private static final String ADDRESS = SofaConfigs | ||
| .getOrDefault(RpcConfigKeys.ZK_ADDRESS); | ||
| private static final String DEFAULT_NAMESPACE = "sofa-rpc"; | ||
| private static final String CONFIG_NODE = "config"; | ||
| private static final String DEFAULT_APP = "sofa-rpc"; | ||
| private final String appName; | ||
| private final String rootPath; | ||
| private ConcurrentMap<String, String> configMap = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Consider Using SofaConfigs#getOrDefault for Default Values.
The DEFAULT_NAMESPACE and DEFAULT_APP values are hardcoded. Consider using SofaConfigs#getOrDefault for these values to allow for configuration flexibility.
- private static final String DEFAULT_NAMESPACE = "sofa-rpc";
- private static final String DEFAULT_APP = "sofa-rpc";
+ private static final String DEFAULT_NAMESPACE = SofaConfigs.getOrDefault(RpcConfigKeys.ZK_NAMESPACE, "sofa-rpc");
+ private static final String DEFAULT_APP = SofaConfigs.getOrDefault(RpcConfigKeys.ZK_APP, "sofa-rpc");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.
| private final CuratorFramework zkClient; | |
| private static final String ADDRESS = SofaConfigs | |
| .getOrDefault(RpcConfigKeys.ZK_ADDRESS); | |
| private static final String DEFAULT_NAMESPACE = "sofa-rpc"; | |
| private static final String CONFIG_NODE = "config"; | |
| private static final String DEFAULT_APP = "sofa-rpc"; | |
| private final String appName; | |
| private final String rootPath; | |
| private ConcurrentMap<String, String> configMap = new ConcurrentHashMap<>(); | |
| private final CuratorFramework zkClient; | |
| private static final String ADDRESS = SofaConfigs | |
| .getOrDefault(RpcConfigKeys.ZK_ADDRESS); | |
| private static final String DEFAULT_NAMESPACE = SofaConfigs.getOrDefault(RpcConfigKeys.ZK_NAMESPACE, "sofa-rpc"); | |
| private static final String DEFAULT_APP = SofaConfigs.getOrDefault(RpcConfigKeys.ZK_APP, "sofa-rpc"); | |
| private static final String CONFIG_NODE = "config"; | |
| private static final String DEFAULT_APP = "sofa-rpc"; | |
| private final String appName; | |
| private final String rootPath; | |
| private ConcurrentMap<String, String> configMap = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Actionable comments posted: 18
Outside diff range and nitpick comments (3)
core/api/src/main/java/com/alipay/sofa/rpc/listener/ConfigListener.java (1)
30-37: Update the method documentation to English for consistency.The method documentation is currently in Chinese, which is inconsistent with the rest of the codebase. Please update the documentation to English to maintain consistency and improve readability for non-Chinese speakers.
Apply this diff to update the method documentation:
- /** - * 处理配置变更事件 - * - * @param event 配置变更事件 - */ + /** + * Process the configuration changed event. + * + * @param event the configuration changed event + */config/config-apollo/src/main/java/com/alipay/sofa/rpc/dynamic/apollo/ApolloDynamicConfigManager.java (1)
117-121: Implement Listener Removal MechanismThe
addListenermethod adds listeners for configuration changes but there is no corresponding method to remove them. Without a removal mechanism, this could lead to memory leaks in long-running applications if listeners are not managed properly.Consider adding a
removeListenermethod to allow for the proper cleanup of listeners when they are no longer needed.config/config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManager.java (1)
160-160: Enhance error logs withappNamefor better contextIncluding the application name in the error logs can provide more context during troubleshooting. This small addition can significantly aid in identifying issues in multi-application environments.
Apply this diff to improve the log messages:
- LOGGER.error("Failed to getConfig for key:{}, group:{}", key, group, e); + LOGGER.error("Failed to getConfig for appName:{}, key:{}, group:{}", getAppName(), key, group, e); - LOGGER.error("Failed to add listener for key:{}, group:{}", key, group, e); + LOGGER.error("Failed to add listener for appName:{}, key:{}, group:{}", getAppName(), key, group, e);Also applies to: 173-173
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- bootstrap/bootstrap-api/src/main/java/com/alipay/sofa/rpc/bootstrap/DefaultConsumerBootstrap.java (5 hunks)
- config/config-apollo/src/main/java/com/alipay/sofa/rpc/dynamic/apollo/ApolloDynamicConfigManager.java (3 hunks)
- config/config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManager.java (1 hunks)
- config/config-zk/src/main/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManager.java (1 hunks)
- core-impl/client/src/main/java/com/alipay/sofa/rpc/client/AbstractCluster.java (2 hunks)
- core-impl/client/src/main/java/com/alipay/sofa/rpc/client/lb/AutoLoadBalancer.java (1 hunks)
- core/api/src/main/java/com/alipay/sofa/rpc/dynamic/ConfigChangeType.java (1 hunks)
- core/api/src/main/java/com/alipay/sofa/rpc/dynamic/ConfigChangedEvent.java (1 hunks)
- core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigKeys.java (1 hunks)
- core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigManager.java (3 hunks)
- core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigManagerFactory.java (2 hunks)
- core/api/src/main/java/com/alipay/sofa/rpc/listener/ConfigListener.java (2 hunks)
- core/common/src/main/java/com/alipay/sofa/rpc/common/utils/StringUtils.java (1 hunks)
Additional comments not posted (22)
core/api/src/main/java/com/alipay/sofa/rpc/dynamic/ConfigChangeType.java (1)
24-39: LGTM!The
ConfigChangeTypeenum is well-defined and follows best practices:
- The constant names are clear and self-explanatory.
- Each constant is accompanied by a concise and accurate description.
- The enum provides a structured way to represent different types of configuration changes.
Great job!
core/api/src/main/java/com/alipay/sofa/rpc/dynamic/ConfigChangedEvent.java (5)
27-35: LGTM!The class declaration and properties are well-defined and capture the necessary information for a configuration change event. Extending
EventObjectis appropriate, and declaring the properties asfinalensures immutability.
37-47: LGTM!The constructors are well-designed, offering both convenience and flexibility. Initializing the
superconstructor with a composite string ofkeyandgroupuniquely identifies the event source, which is a good practice.
49-63: LGTM!The getter methods are straightforward and follow the naming convention, allowing access to all property values.
65-73: LGTM!Overriding the
toStringmethod to provide a meaningful string representation of the event, including all property values, aids in debugging and logging.
75-93: LGTM!Overriding the
equalsandhashCodemethods is necessary for proper equality comparison and hash-based collections. Theequalsmethod correctly compares the events based on their property values, and thehashCodemethod generates a consistent hash code.core-impl/client/src/main/java/com/alipay/sofa/rpc/client/lb/AutoLoadBalancer.java (1)
56-56: LGTM!The comment update clarifies the context of the dynamic configuration priority, specifying that it is at the request level. This aligns with the existing code logic and helps in understanding the behavior.
core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigKeys.java (7)
19-20: LGTM!The import statement for
com.alipay.sofa.common.config.ConfigKeyis necessary and looks good.
26-27: LGTM!The constant
DYNAMIC_ALIASis declared correctly and follows the existing naming convention.
28-29: LGTM!The constant
DYNAMIC_URLis declared correctly and follows the existing naming convention.
30-35: LGTM!The constants
CONFIG_NODE,DEFAULT_NAMESPACE, andDEFAULT_GROUPare declared correctly and follow the existing naming conventions. The default values for namespace and group are appropriately set to "sofa-rpc".
36-43: LGTM!The
ConfigKey<String>instanceZK_ADDRESSis created correctly with appropriate parameters. The key name follows the naming convention, and the default value points to a local Zookeeper instance, which is suitable for development environments. The description and alias are provided for clarity and backward compatibility.
44-50: LGTM!The
ConfigKey<String>instanceNACOS_ADDRESSis created correctly with appropriate parameters. The key name follows the naming convention, and the default value points to a local Nacos instance, which is suitable for development environments. The description and alias are provided for clarity and backward compatibility.
51-58: LGTM!The
ConfigKey<String>instanceAPOLLO_ADDRESSis created correctly with appropriate parameters. The key name follows the naming convention, and the default value points to a local Apollo instance, which is suitable for development environments. The description and alias are provided for clarity and backward compatibility.core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigManager.java (3)
21-21: LGTM!The import statement for
ConfigListeneris necessary for the newly addedaddListenermethod.
37-40: LGTM!The addition of the protected
getAppName()method is a good practice, as it provides controlled access to theappNamefield for subclasses.
94-101: LGTM!The addition of the abstract methods
addListenerandgetConfigis a good design choice, as they provide a way to manage configuration listeners and retrieve configuration values. The methods are well-documented and have clear parameters and return types.These methods will need to be implemented by concrete subclasses, such as
ZookeeperDynamicConfigManagerandNacosDynamicConfigManager, to provide the actual functionality for adding listeners and retrieving configurations.Also applies to: 103-109
core/common/src/main/java/com/alipay/sofa/rpc/common/utils/StringUtils.java (1)
42-45: LGTM!The new constant
KEY_SEPARATORis declared correctly and follows the naming convention. It provides a centralized way to reference the separator, making the code more maintainable.core-impl/client/src/main/java/com/alipay/sofa/rpc/client/AbstractCluster.java (1)
Line range hint
689-707: LGTM!The comment update clarifies the scope of the dynamic configuration and the addition of a blank line enhances readability. The changes do not affect the logic or behavior of the method.
core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigManagerFactory.java (2)
101-105: Reevaluate the warning threshold for dynamic managersThe code logs a warning if
ALL_DYNAMICS.size() > 3. This arbitrary threshold may not be suitable for all use cases. In environments where more than three dynamic managers are normal, this could lead to unnecessary warnings.Consider verifying whether this threshold is appropriate for your application's deployment scenarios.
121-123: Avoid catchingThrowableCatching
Throwablecan inadvertently catch serious errors that shouldn't be handled at this level, such asOutOfMemoryError. It's better practice to catchExceptionto handle recoverable exceptions.Update the catch block to handle
Exceptioninstead:- } catch (Throwable e) { + } catch (Exception e) {Likely invalid or redundant comment.
bootstrap/bootstrap-api/src/main/java/com/alipay/sofa/rpc/bootstrap/DefaultConsumerBootstrap.java (1)
31-32: Necessary imports added for dynamic configurationThe imports for
ConfigChangedEventandConfigChangeTypeare appropriately added to support handling dynamic configuration events.
| public static DynamicConfigManager getDynamicManagerWithUrl(String appName, String url) { | ||
|
|
||
| String[] urlValues = url.split("://", 2); |
There was a problem hiding this comment.
Handle potential ArrayIndexOutOfBoundsException when parsing URL
The method getDynamicManagerWithUrl splits the url using url.split("://", 2) but does not validate the result. If the url does not contain "://", accessing urlValues[1] will lead to an ArrayIndexOutOfBoundsException.
Apply this diff to add input validation:
public static DynamicConfigManager getDynamicManagerWithUrl(String appName, String url) {
+ if (url == null || !url.contains("://")) {
+ throw new IllegalArgumentException("Invalid URL format. Expected format: alias://address");
+ }
String[] urlValues = url.split("://", 2);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.
| public static DynamicConfigManager getDynamicManagerWithUrl(String appName, String url) { | |
| String[] urlValues = url.split("://", 2); | |
| public static DynamicConfigManager getDynamicManagerWithUrl(String appName, String url) { | |
| if (url == null || !url.contains("://")) { | |
| throw new IllegalArgumentException("Invalid URL format. Expected format: alias://address"); | |
| } | |
| String[] urlValues = url.split("://", 2); |
| public static DynamicConfigManager getDynamicManagerWithUrl(String appName, String url) { | ||
|
|
||
| String[] urlValues = url.split("://", 2); | ||
|
|
||
| classLock.lock(); | ||
| try { | ||
| if (ALL_DYNAMICS.size() > 3) { | ||
| if (LOGGER.isWarnEnabled()) { | ||
| LOGGER.warn("Size of dynamic manager is greater than 3, Please check it!"); | ||
| } | ||
| } | ||
| DynamicConfigManager registry = ALL_DYNAMICS.get(urlValues[0]); | ||
| if (registry == null) { | ||
| ExtensionClass<DynamicConfigManager> ext = ExtensionLoaderFactory.getExtensionLoader( | ||
| DynamicConfigManager.class) | ||
| .getExtensionClass(urlValues[0]); | ||
| if (ext == null) { | ||
| throw new SofaRpcRuntimeException(LogCodes.getLog(LogCodes.ERROR_LOAD_EXT, "DynamicConfigManager", | ||
| urlValues[0])); | ||
| } | ||
| registry = ext.getExtInstance(new Class[] { String.class, String.class }, new Object[] { appName, | ||
| urlValues[1] }); | ||
| ALL_DYNAMICS.put(urlValues[0], registry); | ||
| } | ||
| return registry; | ||
| } catch (Throwable e) { | ||
| throw new SofaRpcRuntimeException(LogCodes.getLog(LogCodes.ERROR_LOAD_EXT, "DynamicConfigManager", | ||
| urlValues[0]), | ||
| e); | ||
| } finally { | ||
| classLock.unlock(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Optimize synchronization for better concurrency
Currently, the method uses a Lock to ensure thread safety when accessing ALL_DYNAMICS. This can lead to contention and reduced concurrency. Utilizing ConcurrentMap methods like computeIfAbsent can simplify the code and improve performance.
Consider refactoring the method as follows:
public static DynamicConfigManager getDynamicManagerWithUrl(String appName, String url) {
+ if (url == null || !url.contains("://")) {
+ throw new IllegalArgumentException("Invalid URL format. Expected format: alias://address");
+ }
String[] urlValues = url.split("://", 2);
- classLock.lock();
- try {
- if (ALL_DYNAMICS.size() > 3) {
- if (LOGGER.isWarnEnabled()) {
- LOGGER.warn("Size of dynamic manager is greater than 3, Please check it!");
- }
- }
- DynamicConfigManager registry = ALL_DYNAMICS.get(urlValues[0]);
- if (registry == null) {
- ExtensionClass<DynamicConfigManager> ext = ExtensionLoaderFactory.getExtensionLoader(
- DynamicConfigManager.class)
- .getExtensionClass(urlValues[0]);
- if (ext == null) {
- throw new SofaRpcRuntimeException(LogCodes.getLog(LogCodes.ERROR_LOAD_EXT, "DynamicConfigManager",
- urlValues[0]));
- }
- registry = ext.getExtInstance(new Class[] { String.class, String.class }, new Object[] { appName,
- urlValues[1] });
- ALL_DYNAMICS.put(urlValues[0], registry);
- }
- return registry;
- } catch (Throwable e) {
- throw new SofaRpcRuntimeException(LogCodes.getLog(LogCodes.ERROR_LOAD_EXT, "DynamicConfigManager",
- urlValues[0]),
- e);
- } finally {
- classLock.unlock();
- }
+ if (ALL_DYNAMICS.size() > 3 && LOGGER.isWarnEnabled()) {
+ LOGGER.warn("Size of dynamic manager is greater than 3, Please check it!");
+ }
+
+ try {
+ return ALL_DYNAMICS.computeIfAbsent(urlValues[0], key -> {
+ ExtensionClass<DynamicConfigManager> ext = ExtensionLoaderFactory.getExtensionLoader(
+ DynamicConfigManager.class).getExtensionClass(key);
+ if (ext == null) {
+ throw new SofaRpcRuntimeException(LogCodes.getLog(LogCodes.ERROR_LOAD_EXT, "DynamicConfigManager", key));
+ }
+ return ext.getExtInstance(new Class[] { String.class, String.class }, new Object[] { appName, urlValues[1] });
+ });
+ } catch (Exception e) {
+ throw new SofaRpcRuntimeException(LogCodes.getLog(LogCodes.ERROR_LOAD_EXT, "DynamicConfigManager",
+ urlValues[0]), 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.
| public static DynamicConfigManager getDynamicManagerWithUrl(String appName, String url) { | |
| String[] urlValues = url.split("://", 2); | |
| classLock.lock(); | |
| try { | |
| if (ALL_DYNAMICS.size() > 3) { | |
| if (LOGGER.isWarnEnabled()) { | |
| LOGGER.warn("Size of dynamic manager is greater than 3, Please check it!"); | |
| } | |
| } | |
| DynamicConfigManager registry = ALL_DYNAMICS.get(urlValues[0]); | |
| if (registry == null) { | |
| ExtensionClass<DynamicConfigManager> ext = ExtensionLoaderFactory.getExtensionLoader( | |
| DynamicConfigManager.class) | |
| .getExtensionClass(urlValues[0]); | |
| if (ext == null) { | |
| throw new SofaRpcRuntimeException(LogCodes.getLog(LogCodes.ERROR_LOAD_EXT, "DynamicConfigManager", | |
| urlValues[0])); | |
| } | |
| registry = ext.getExtInstance(new Class[] { String.class, String.class }, new Object[] { appName, | |
| urlValues[1] }); | |
| ALL_DYNAMICS.put(urlValues[0], registry); | |
| } | |
| return registry; | |
| } catch (Throwable e) { | |
| throw new SofaRpcRuntimeException(LogCodes.getLog(LogCodes.ERROR_LOAD_EXT, "DynamicConfigManager", | |
| urlValues[0]), | |
| e); | |
| } finally { | |
| classLock.unlock(); | |
| } | |
| } | |
| public static DynamicConfigManager getDynamicManagerWithUrl(String appName, String url) { | |
| if (url == null || !url.contains("://")) { | |
| throw new IllegalArgumentException("Invalid URL format. Expected format: alias://address"); | |
| } | |
| String[] urlValues = url.split("://", 2); | |
| if (ALL_DYNAMICS.size() > 3 && LOGGER.isWarnEnabled()) { | |
| LOGGER.warn("Size of dynamic manager is greater than 3, Please check it!"); | |
| } | |
| try { | |
| return ALL_DYNAMICS.computeIfAbsent(urlValues[0], key -> { | |
| ExtensionClass<DynamicConfigManager> ext = ExtensionLoaderFactory.getExtensionLoader( | |
| DynamicConfigManager.class).getExtensionClass(key); | |
| if (ext == null) { | |
| throw new SofaRpcRuntimeException(LogCodes.getLog(LogCodes.ERROR_LOAD_EXT, "DynamicConfigManager", key)); | |
| } | |
| return ext.getExtInstance(new Class[] { String.class, String.class }, new Object[] { appName, urlValues[1] }); | |
| }); | |
| } catch (Exception e) { | |
| throw new SofaRpcRuntimeException(LogCodes.getLog(LogCodes.ERROR_LOAD_EXT, "DynamicConfigManager", | |
| urlValues[0]), e); | |
| } | |
| } |
| protected ApolloDynamicConfigManager(String appName,String address) { | ||
| super(appName); | ||
| System.setProperty(APOLLO_APPID_KEY, appName); | ||
| System.setProperty(APOLLO_ADDR_KEY, APOLLO_PROTOCOL_PREFIX + address); | ||
| config = ConfigService.getConfig(DynamicConfigKeys.DEFAULT_GROUP); |
There was a problem hiding this comment.
Avoid Using System.setProperty in Constructors
Setting system properties within a constructor can have unintended side effects, as system properties are global to the JVM and affect all parts of the application. This may lead to unpredictable behavior, especially in multi-threaded environments or when multiple instances are created.
Consider refactoring the code to avoid setting system properties in the constructor. Instead, pass the necessary configuration directly to the components that require it.
| public class ApolloListener implements ConfigChangeListener { | ||
|
|
||
| private Set<ConfigListener> listeners = new CopyOnWriteArraySet<>(); | ||
|
|
||
| ApolloListener() {} | ||
|
|
||
| @Override | ||
| public void onChange(com.ctrip.framework.apollo.model.ConfigChangeEvent changeEvent) { | ||
| for (String key : changeEvent.changedKeys()) { | ||
| ConfigChange change = changeEvent.getChange(key); | ||
| if ("".equals(change.getNewValue())) { | ||
| LOGGER.info("an empty rule is received for key: " + key); | ||
| return; | ||
| } | ||
|
|
||
| ConfigChangedEvent event = | ||
| new ConfigChangedEvent(key, change.getNamespace(), change.getNewValue(), getChangeType(change)); | ||
| listeners.forEach(listener -> listener.process(event)); | ||
| } | ||
| } | ||
|
|
||
| private ConfigChangeType getChangeType(ConfigChange change) { | ||
| if (change.getChangeType() == PropertyChangeType.DELETED) { | ||
| return ConfigChangeType.DELETED; | ||
| } | ||
| return ConfigChangeType.MODIFIED; | ||
| } | ||
|
|
||
| void addListener(ConfigListener configListener) { | ||
| this.listeners.add(configListener); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure Thread Safety When Managing Listeners
The listeners set in the ApolloListener class uses CopyOnWriteArraySet for thread safety. While this is suitable for scenarios with infrequent mutations and frequent iterations, it may not be the most efficient choice if listeners are added or removed frequently.
Evaluate whether ConcurrentHashMap or another thread-safe collection would be more appropriate, especially if listener additions and removals occur often.
| if ("".equals(change.getNewValue())) { | ||
| LOGGER.info("an empty rule is received for key: " + key); | ||
| return; |
There was a problem hiding this comment.
Incorrect Use of return Inside Loop
Using return inside the loop in the onChange method will exit the entire method when an empty new value is encountered. This prevents processing of subsequent keys, which may lead to missed configuration changes.
Apply this diff to fix the issue:
- if ("".equals(change.getNewValue())) {
LOGGER.info("an empty rule is received for key: " + key);
- return;
+ continue;
}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.
| if ("".equals(change.getNewValue())) { | |
| LOGGER.info("an empty rule is received for key: " + key); | |
| return; | |
| if ("".equals(change.getNewValue())) { | |
| LOGGER.info("an empty rule is received for key: " + key); | |
| continue; | |
| } |
| //启用接口级别动态配置 | ||
| final String dynamicUrl = consumerConfig.getParameter(DynamicConfigKeys.DYNAMIC_URL); | ||
| if (StringUtils.isNotBlank(dynamicUrl)) { | ||
| final DynamicConfigManager dynamicManager = DynamicConfigManagerFactory.getDynamicManagerWithUrl( | ||
| consumerConfig.getAppName(), dynamicUrl); | ||
|
|
||
| //build listeners for dynamic config | ||
| consumerConfig.setConfigListener(buildConfigListener(this,dynamicManager)); | ||
| } | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Overwriting existing ConfigListener may lead to unintended behavior
In the interface-level dynamic configuration, you set a new ConfigListener on the consumerConfig using:
consumerConfig.setConfigListener(buildConfigListener(this, dynamicManager));Since consumerConfig may already have a ConfigListener set earlier in the code, this operation could overwrite the existing listener, potentially disrupting previously established configurations or listeners.
Consider modifying the implementation to support multiple ConfigListeners or ensure that you're not unintentionally overwriting an existing listener. You might implement a mechanism to aggregate listeners or check if a listener is already set before assigning a new one.
| /** | ||
| * Build ConfigListener for consumer bootstrap with dynamic config. | ||
| * | ||
| * @param bootstrap ConsumerBootstrap | ||
| * @param dynamicManager DynamicConfigManager | ||
| * @return ConfigListener | ||
| */ | ||
| protected ConfigListener buildConfigListener(ConsumerBootstrap bootstrap, DynamicConfigManager dynamicManager) { | ||
| return new ConsumerAttributeListener(dynamicManager); | ||
| } | ||
|
|
There was a problem hiding this comment.
Clarify the usage of overloaded buildConfigListener methods
The introduction of an overloaded buildConfigListener method adds flexibility, but it may cause confusion due to the similar method names and parameters:
protected ConfigListener buildConfigListener(ConsumerBootstrap bootstrap)protected ConfigListener buildConfigListener(ConsumerBootstrap bootstrap, DynamicConfigManager dynamicManager)
To improve code readability and maintainability, consider renaming one of the methods to more clearly reflect its purpose, such as buildDynamicConfigListener. This differentiation helps future developers understand the distinction without examining method signatures.
| public void initWith(String key, DynamicConfigManager dynamicManager) { | ||
| dynamicManager.addListener(key, this); | ||
| // 初始化配置值 | ||
| String rawConfig = dynamicManager.getConfig(key); | ||
| if (!StringUtils.isEmpty(rawConfig)) { | ||
| process(new ConfigChangedEvent(key, "sofa-rpc",rawConfig)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure listeners are unregistered to prevent memory leaks
In the initWith method, the ConsumerAttributeListener registers itself with the DynamicConfigManager:
dynamicManager.addListener(key, this);However, there is no corresponding logic to remove this listener when it is no longer needed. This might lead to memory leaks if the listener remains registered after the consumer is unreferenced.
Consider implementing a mechanism to unregister the listener when the consumer is unreferenced or destroyed. This can be done in the unRefer method by calling a hypothetical removeListener method:
dynamicManager.removeListener(key, this);| @Override | ||
| public void process(ConfigChangedEvent event){ | ||
| if (event.getChangeType().equals(ConfigChangeType.DELETED)) { | ||
| newValueMap = null; | ||
| } else { | ||
| // ADDED or MODIFIED | ||
| String[] lines = event.getContent().split("\n"); | ||
| for (String line : lines) { | ||
| String[] keyValue = line.split("=", 2); | ||
| if (keyValue.length == 2) { | ||
| String key = keyValue[0].trim(); | ||
| String value = keyValue[1].trim(); | ||
| newValueMap.put(key, value); | ||
| } | ||
| } | ||
| } | ||
| attrUpdated(newValueMap); | ||
| } | ||
|
|
There was a problem hiding this comment.
Add error handling for configuration parsing in process method
In the process method, the configuration content is split and parsed without validating the input:
String[] lines = event.getContent().split("\n");
for (String line : lines) {
String[] keyValue = line.split("=", 2);
if (keyValue.length == 2) {
String key = keyValue[0].trim();
String value = keyValue[1].trim();
newValueMap.put(key, value);
}
}If the configuration content is malformed, this could lead to incorrect behavior or missed configurations.
Consider adding error handling to manage malformed lines gracefully. For example, log a warning when a line doesn't conform to the expected key=value format:
for (String line : lines) {
String[] keyValue = line.split("=", 2);
if (keyValue.length == 2) {
String key = keyValue[0].trim();
String value = keyValue[1].trim();
newValueMap.put(key, value);
} else {
LOGGER.warn("Malformed configuration line: {}", line);
}
}| @Override | ||
| public void configChanged(Map newValue) { | ||
| public void configChanged(Map newValueMap) { |
There was a problem hiding this comment.
Unused configChanged method with empty implementation
The method public void configChanged(Map newValueMap) is declared but has an empty body:
public void configChanged(Map newValueMap) {
}Since it doesn't perform any actions and isn't called within this class, it may be unnecessary.
If this method is not required, consider removing it to clean up the code. If it's intended for future use or to fulfill an interface contract, consider adding a comment explaining its purpose.
Narzisss
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
core/api/src/main/java/com/alipay/sofa/rpc/config/AbstractInterfaceConfig.java (2)
932-935: Use a more explicit indicator for resetting to default valuesUsing a single space
" "as an indicator to reset to the default value may not be intuitive and can lead to confusion. Consider defining a constant or using a more descriptive value to improve code readability and maintainability.Define a constant for the reset indicator:
private static final String RESET_TO_DEFAULT = " ";And use it in the condition:
if (RESET_TO_DEFAULT.equals(newValueStr)) { // Reset to default value }Alternatively, use a clearer value like
"DEFAULT":private static final String RESET_TO_DEFAULT = "DEFAULT"; if (RESET_TO_DEFAULT.equals(newValueStr)) { // Reset to default value }
1035-1041: Unnecessary null check fordynamicConfigValueCacheSince
dynamicConfigValueCacheis initialized during declaration and is not set tonullelsewhere, the null checkif (dynamicConfigValueCache != null)is unnecessary and can be removed to simplify the code.Apply this diff to remove the unnecessary null check:
- if (dynamicConfigValueCache != null){ return dynamicConfigValueCache.get(key); - } + return dynamicConfigValueCache.get(key);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- bootstrap/bootstrap-api/src/main/java/com/alipay/sofa/rpc/bootstrap/DefaultConsumerBootstrap.java (8 hunks)
- config/config-apollo/src/main/java/com/alipay/sofa/rpc/dynamic/apollo/ApolloDynamicConfigManager.java (3 hunks)
- config/config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManager.java (1 hunks)
- config/config-zk/src/main/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManager.java (1 hunks)
- core/api/src/main/java/com/alipay/sofa/rpc/config/AbstractInterfaceConfig.java (5 hunks)
- core/api/src/main/java/com/alipay/sofa/rpc/dynamic/ConfigChangedEvent.java (1 hunks)
- core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigKeys.java (1 hunks)
- core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigManager.java (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- bootstrap/bootstrap-api/src/main/java/com/alipay/sofa/rpc/bootstrap/DefaultConsumerBootstrap.java
- config/config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManager.java
- config/config-zk/src/main/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManager.java
- core/api/src/main/java/com/alipay/sofa/rpc/dynamic/ConfigChangedEvent.java
- core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigKeys.java
- core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigManager.java
🔇 Additional comments (8)
config/config-apollo/src/main/java/com/alipay/sofa/rpc/dynamic/apollo/ApolloDynamicConfigManager.java (6)
19-37: LGTM: New imports are appropriate for the changes.The added imports are relevant to the new functionality implemented in the class, including configuration management, logging, and concurrent data structures.
49-49: Specify Access Modifier and Make LOGGER Static FinalThe
LOGGERvariable is declared without an access modifier and is notstaticorfinal. It's a best practice to declare loggers asprivate static finalto ensure they are class-level constants and not modifiable.Apply this diff to correct it:
- Logger LOGGER = LoggerFactory.getLogger(ApolloDynamicConfigManager.class); + private static final Logger LOGGER = LoggerFactory.getLogger(ApolloDynamicConfigManager.class);
51-55: LGTM: Well-defined constants for Apollo configuration.The new constants are appropriately named and provide clear references for Apollo-specific configuration keys and protocol prefix.
59-59: LGTM: Thread-safe map for storing listeners.The
watchListenerMapis correctly declared as aConcurrentMap, which is suitable for managing listeners in a thread-safe manner.
76-80: LGTM: Well-implemented service configuration initialization.The
initServiceConfigurationmethod correctly retrieves the service configuration and triggers the listener if a non-empty configuration is found.
115-120: LGTM: Efficient and thread-safe listener management.The
addListenermethod effectively manages listeners using thewatchListenerMap. It ensures thread-safety by usingcomputeIfAbsentand adds the listener to Apollo's configuration change mechanism.core/api/src/main/java/com/alipay/sofa/rpc/config/AbstractInterfaceConfig.java (2)
218-222: Appropriate addition of dynamic configuration cacheThe new
dynamicConfigValueCachefield is correctly declared and initialized usingConcurrentHashMap, ensuring thread-safe access to dynamic configuration values.
973-977: Correctly updating dynamic configuration cacheThe code appropriately updates
dynamicConfigValueCacheby adding or removing the property based on the new value, ensuring that dynamic configurations are accurately reflected.
| protected ApolloDynamicConfigManager(String appName) { | ||
| super(appName); | ||
| super(appName, SofaConfigs.getOrDefault(DynamicConfigKeys.APOLLO_ADDRESS)); | ||
| System.setProperty(APOLLO_APPID_KEY, appName); | ||
| System.setProperty(APOLLO_ADDR_KEY, APOLLO_PROTOCOL_PREFIX + getAddress()); | ||
| config = ConfigService.getAppConfig(); | ||
| } | ||
|
|
||
| protected ApolloDynamicConfigManager(String appName, String address) { | ||
| super(appName, address); | ||
| System.setProperty(APOLLO_APPID_KEY, appName); | ||
| System.setProperty(APOLLO_ADDR_KEY, APOLLO_PROTOCOL_PREFIX + address); | ||
| config = ConfigService.getAppConfig(); |
There was a problem hiding this comment.
Reconsider setting system properties in constructors
Setting system properties within constructors can lead to unexpected behavior, especially in multi-threaded environments or when multiple instances are created. Consider refactoring to avoid this practice.
Suggestion:
- Create a separate method for initializing Apollo configuration.
- Call this method from the constructors or externally after object creation.
Example refactoring:
private void initApolloConfig(String appName, String address) {
System.setProperty(APOLLO_APPID_KEY, appName);
System.setProperty(APOLLO_ADDR_KEY, APOLLO_PROTOCOL_PREFIX + address);
config = ConfigService.getAppConfig();
}Update constructors to use this method:
protected ApolloDynamicConfigManager(String appName) {
super(appName, SofaConfigs.getOrDefault(DynamicConfigKeys.APOLLO_ADDRESS));
initApolloConfig(appName, getAddress());
}
protected ApolloDynamicConfigManager(String appName, String address) {
super(appName, address);
initApolloConfig(appName, address);
}This approach provides more flexibility and reduces potential side effects.
| public class ApolloListener implements ConfigChangeListener { | ||
|
|
||
| private Set<ConfigListener> listeners = new CopyOnWriteArraySet<>(); | ||
|
|
||
| ApolloListener() { | ||
| } | ||
|
|
||
| @Override | ||
| public void onChange(com.ctrip.framework.apollo.model.ConfigChangeEvent changeEvent) { | ||
| for (String key : changeEvent.changedKeys()) { | ||
| ConfigChange change = changeEvent.getChange(key); | ||
| if ("".equals(change.getNewValue())) { | ||
| LOGGER.info("an empty rule is received for key: " + key); | ||
| return; | ||
| } | ||
|
|
||
| ConfigChangedEvent event = | ||
| new ConfigChangedEvent(key, change.getNewValue(), getChangeType(change)); | ||
| listeners.forEach(listener -> listener.process(event)); | ||
| } | ||
| } | ||
|
|
||
| private ConfigChangeType getChangeType(ConfigChange change) { | ||
| if (change.getChangeType() == PropertyChangeType.DELETED) { | ||
| return ConfigChangeType.DELETED; | ||
| } | ||
| return ConfigChangeType.MODIFIED; | ||
| } | ||
|
|
||
| void addListener(ConfigListener configListener) { | ||
| this.listeners.add(configListener); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix premature return in onChange method
The onChange method in the ApolloListener class has an issue that can lead to missed configuration changes. When an empty rule is encountered, the method returns prematurely, preventing the processing of subsequent keys.
Apply this diff to fix the issue:
@Override
public void onChange(com.ctrip.framework.apollo.model.ConfigChangeEvent changeEvent) {
for (String key : changeEvent.changedKeys()) {
ConfigChange change = changeEvent.getChange(key);
if ("".equals(change.getNewValue())) {
LOGGER.info("an empty rule is received for key: " + key);
- return;
+ continue;
}
ConfigChangedEvent event =
new ConfigChangedEvent(key, change.getNewValue(), getChangeType(change));
listeners.forEach(listener -> listener.process(event));
}
}This change ensures that all changed keys are processed, even if some have empty values.
📝 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.
| public class ApolloListener implements ConfigChangeListener { | |
| private Set<ConfigListener> listeners = new CopyOnWriteArraySet<>(); | |
| ApolloListener() { | |
| } | |
| @Override | |
| public void onChange(com.ctrip.framework.apollo.model.ConfigChangeEvent changeEvent) { | |
| for (String key : changeEvent.changedKeys()) { | |
| ConfigChange change = changeEvent.getChange(key); | |
| if ("".equals(change.getNewValue())) { | |
| LOGGER.info("an empty rule is received for key: " + key); | |
| return; | |
| } | |
| ConfigChangedEvent event = | |
| new ConfigChangedEvent(key, change.getNewValue(), getChangeType(change)); | |
| listeners.forEach(listener -> listener.process(event)); | |
| } | |
| } | |
| private ConfigChangeType getChangeType(ConfigChange change) { | |
| if (change.getChangeType() == PropertyChangeType.DELETED) { | |
| return ConfigChangeType.DELETED; | |
| } | |
| return ConfigChangeType.MODIFIED; | |
| } | |
| void addListener(ConfigListener configListener) { | |
| this.listeners.add(configListener); | |
| } | |
| } | |
| public class ApolloListener implements ConfigChangeListener { | |
| private Set<ConfigListener> listeners = new CopyOnWriteArraySet<>(); | |
| ApolloListener() { | |
| } | |
| @Override | |
| public void onChange(com.ctrip.framework.apollo.model.ConfigChangeEvent changeEvent) { | |
| for (String key : changeEvent.changedKeys()) { | |
| ConfigChange change = changeEvent.getChange(key); | |
| if ("".equals(change.getNewValue())) { | |
| LOGGER.info("an empty rule is received for key: " + key); | |
| continue; | |
| } | |
| ConfigChangedEvent event = | |
| new ConfigChangedEvent(key, change.getNewValue(), getChangeType(change)); | |
| listeners.forEach(listener -> listener.process(event)); | |
| } | |
| } | |
| private ConfigChangeType getChangeType(ConfigChange change) { | |
| if (change.getChangeType() == PropertyChangeType.DELETED) { | |
| return ConfigChangeType.DELETED; | |
| } | |
| return ConfigChangeType.MODIFIED; | |
| } | |
| void addListener(ConfigListener configListener) { | |
| this.listeners.add(configListener); | |
| } | |
| } |
| if(newValueStr.equals(" ")){ | ||
| // 改为默认值 | ||
| newValueStr = configValueCache.get(property) == null ? null : configValueCache.get(property).toString(); | ||
| } |
There was a problem hiding this comment.
Potential NullPointerException when comparing newValueStr
In the condition if(newValueStr.equals(" ")), if newValueStr is null, this will result in a NullPointerException. Consider reversing the comparison or adding a null check to prevent this exception.
Apply this diff to fix the potential NullPointerException:
- if(newValueStr.equals(" ")){
+ if(" ".equals(newValueStr)){Alternatively, add a null check:
+ if(newValueStr != null && newValueStr.equals(" ")){📝 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.
| if(newValueStr.equals(" ")){ | |
| // 改为默认值 | |
| newValueStr = configValueCache.get(property) == null ? null : configValueCache.get(property).toString(); | |
| } | |
| if(" ".equals(newValueStr)){ | |
| // 改为默认值 | |
| newValueStr = configValueCache.get(property) == null ? null : configValueCache.get(property).toString(); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 29
🧹 Outside diff range and nitpick comments (16)
core/api/src/main/java/com/alipay/sofa/rpc/dynamic/ConfigChangedEvent.java (1)
34-43: Constructors are well-implemented, but consider revising the event source.The constructors are well-designed:
- They provide flexibility in creating the event object.
- The use of
this()to call the more specific constructor is a good practice.- All fields are properly initialized.
However, there's a potential improvement:
Consider revising the
super(key)call in the constructor. Using thekeyas the source of the event might not be the most appropriate choice. The source typically represents the object that generated the event, which in this case might be a configuration manager or similar object. Consider adding asourceparameter to the constructor and using it in thesuper()call.Example:
public ConfigChangedEvent(Object source, String key, String content, ConfigChangeType changeType) { super(source); this.key = key; this.content = content; this.changeType = changeType; }test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/base/BaseZkTest.java (1)
29-34: Enhance the class Javadoc comment.While the Javadoc comment is present, it could be more informative. Consider adding details about the purpose of this base class and how it should be used in integration tests.
Here's a suggested improvement:
/** * Base class for ZooKeeper integration tests. * This class provides setup and teardown methods for a TestingServer instance, * which simulates a ZooKeeper server for testing purposes. * * @author Narziss * @version BaseZkTest.java, v 0.1 2024年10月08日 10:20 Narziss */test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/NacosDynamicConfigTest.java (3)
19-31: Consider adding blank lines between import groups for better readability.While the imports are correct and logically grouped, adding blank lines between different package imports can improve readability. Consider reorganizing the imports as follows:
import com.alipay.sofa.rpc.config.ApplicationConfig; import com.alipay.sofa.rpc.config.ConsumerConfig; import com.alipay.sofa.rpc.dynamic.DynamicConfigKeys; import com.alipay.sofa.rpc.dynamic.DynamicConfigManager; import com.alipay.sofa.rpc.dynamic.DynamicConfigManagerFactory; import com.alipay.sofa.rpc.dynamic.nacos.NacosDynamicConfigManager; import com.alipay.sofa.rpc.test.HelloService; import org.junit.Assert; import org.junit.Test; import java.lang.reflect.Field; import java.util.Map;
33-37: Enhance the class Javadoc for better documentation.The current Javadoc is minimal. Consider expanding it to provide more context about the purpose of this test class and what aspects of Nacos dynamic configuration it's testing. For example:
/** * Integration test class for Nacos dynamic configuration in SOFA RPC. * This class tests the functionality of dynamic configuration updates * using Nacos as the configuration center. * * @author Narziss * @version NacosDynamicConfigTest.java, v 0.1 2024年09月28日 12:11 Narziss */
1-72: Overall assessment: Functional but needs refinement.This new test file for Nacos dynamic configuration provides valuable coverage for the feature. However, several areas need attention to improve its robustness and maintainability:
- Enhance documentation with more detailed class and method Javadocs.
- Improve test setup and teardown to ensure isolation and proper resource management.
- Reduce coupling with implementation details by avoiding reflection and considering the use of mocking frameworks.
- Break down the large test method into smaller, more focused tests.
- Add more comprehensive assertions and error handling.
Addressing these points will significantly improve the quality and reliability of this test class.
core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigKeys.java (2)
28-29: LGTM: CONFIG_NODE constant added correctly.The
CONFIG_NODEconstant is appropriately named and valued. However, consider adding a brief comment to explain its purpose for better code readability.Consider adding a comment like this:
/** * Constant representing the configuration node identifier. */ public static final String CONFIG_NODE = "config";
30-31: LGTM: DEFAULT_NAMESPACE constant added correctly.The
DEFAULT_NAMESPACEconstant is appropriately named and valued. However, consider adding a brief comment to explain its purpose and usage for better code readability.Consider adding a comment like this:
/** * Constant representing the default namespace for SOFA RPC configurations. */ public static final String DEFAULT_NAMESPACE = "sofa-rpc";test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/ZookeeperDynamicConfigTest.java (1)
57-59: Consider alternatives to reflection for accessing private fields.While reflection is sometimes necessary in tests, it can make the test brittle if the internal structure of
ZookeeperDynamicConfigManagerchanges. Consider if there's a way to expose thezkClientfor testing purposes, perhaps through a protected method in the base test class.test/test-integration/pom.xml (3)
282-287: LGTM. Consider documenting multiple configuration options.The addition of ZooKeeper as another configuration management option is good for comprehensive testing. However, with multiple options (Nacos, Apollo, ZooKeeper), it would be helpful to document the purpose and usage of each in the project's README or a separate testing guide.
Could you add a comment in the POM file or update the project documentation to explain the usage of these multiple configuration options in the test scope?
288-293: LGTM. Consider using property for version management.The addition of
curator-testaligns well with the ZooKeeper configuration support. However, to maintain consistency and ease future updates, consider defining the version in a property. This approach is already used for other dependencies in this POM file.Suggest adding a property for the Curator version:
<properties> <!-- other properties --> <curator.version>4.3.0</curator.version> </properties> <!-- In the dependency declaration --> <dependency> <groupId>org.apache.curator</groupId> <artifactId>curator-test</artifactId> <version>${curator.version}</version> <scope>test</scope> </dependency>
Line range hint
209-293: Overall LGTM with suggestions for improvement.The changes to the POM file indicate a shift towards supporting multiple configuration management options (Nacos, Apollo, ZooKeeper) for comprehensive testing. This is a good approach for ensuring compatibility and flexibility. However, there are a few areas that could be improved:
- Clarify the purpose of having both Nacos and Apollo dependencies, as this might lead to confusion.
- Consider adding documentation (either in the POM file or project README) explaining the usage and purpose of each configuration management option in the test scope.
- Implement consistent version management by using properties for all dependency versions, including the newly added
curator-test.These improvements will enhance the maintainability and clarity of the project's dependency management.
config/config-apollo/src/main/java/com/alipay/sofa/rpc/dynamic/apollo/ApolloDynamicConfigManager.java (3)
90-91: Use English for code comments and handle unimplemented methods appropriatelyThe TODO comment
// TODO 暂不支持is in Chinese. For consistency and readability among all contributors, please use English for code comments.Additionally, since the method
initServiceConfiguration(String service)is not yet supported, consider throwing anUnsupportedOperationExceptionto explicitly indicate that this method is unimplemented.Apply this diff to update the comment and handle the unimplemented method:
@Override public void initServiceConfiguration(String service) { - // TODO 暂不支持 + // TODO Not supported yet + throw new UnsupportedOperationException("initServiceConfiguration(String service) is not supported yet."); }
130-132: Use English for code comments and handle unimplemented methods appropriatelyThe TODO comment
//TODO 暂不支持is in Chinese. Please use English for code comments to ensure clarity for all contributors.Since the method
getServiceAuthRule(String service)is not implemented, consider throwing anUnsupportedOperationExceptionto indicate that this functionality is not currently supported.Apply this diff to update the comment and handle the unimplemented method:
@Override public AuthRuleGroup getServiceAuthRule(String service) { - //TODO 暂不支持 + // TODO Not supported yet + throw new UnsupportedOperationException("getServiceAuthRule(String service) is not supported yet."); }
142-142: Specify access modifier and declare the field as finalThe
listenersfield does not have an explicit access modifier and is not declared asfinal. For better encapsulation and thread safety, consider declaring it asprivate final.Apply this diff to update the field declaration:
- Set<ConfigListener> listeners = new CopyOnWriteArraySet<>(); + private final Set<ConfigListener> listeners = new CopyOnWriteArraySet<>();config/config-zk/src/main/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManager.java (2)
201-205: Handle Exception Consistently When StartingNodeCacheIn the constructor of
ZookeeperConfigListener, when starting thenodeCache, exceptions are caught and logged. Ensure that the exception message provides clear context by including additional information, such as the path or reason for failure.Consider enhancing the log message:
} catch (Exception e) { - LOGGER.error("Failed to add listener for path:{}", pathKey, e); + LOGGER.error("Failed to start NodeCache for path [{}]: {}", pathKey, e.getMessage(), e); }
55-55: Consider DeclaringconfigMapasfinalThe
configMapis aConcurrentMapthat is initialized once and its reference does not change. Declaring it asfinalcan make the code clearer by indicating that the reference will not be reassigned.Apply the following change:
- private ConcurrentMap<String, String> configMap = new ConcurrentHashMap<>(); + private final ConcurrentMap<String, String> configMap = new ConcurrentHashMap<>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- bootstrap/bootstrap-api/src/main/java/com/alipay/sofa/rpc/bootstrap/DefaultConsumerBootstrap.java (7 hunks)
- config/config-apollo/src/main/java/com/alipay/sofa/rpc/dynamic/apollo/ApolloDynamicConfigManager.java (3 hunks)
- config/config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManager.java (1 hunks)
- config/config-zk/src/main/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManager.java (1 hunks)
- core/api/src/main/java/com/alipay/sofa/rpc/config/AbstractInterfaceConfig.java (5 hunks)
- core/api/src/main/java/com/alipay/sofa/rpc/dynamic/ConfigChangedEvent.java (1 hunks)
- core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigKeys.java (1 hunks)
- core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigManager.java (4 hunks)
- test/test-integration/pom.xml (2 hunks)
- test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/ApolloDynamicConfigTest.java (1 hunks)
- test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/NacosDynamicConfigTest.java (1 hunks)
- test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/ZookeeperDynamicConfigTest.java (1 hunks)
- test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/base/BaseZkTest.java (1 hunks)
🧰 Additional context used
🔇 Additional comments (30)
core/api/src/main/java/com/alipay/sofa/rpc/dynamic/ConfigChangedEvent.java (3)
26-32: LGTM: Well-structured event class with immutable fields.The
ConfigChangedEventclass is well-designed:
- It extends
EventObject, which is appropriate for an event class.- The use of private final fields (
key,content, andchangeType) ensures immutability, which is a good practice for event objects.This structure will provide a robust and thread-safe representation of configuration change events.
45-55: LGTM: Getter methods are correctly implemented.The getter methods
getKey(),getContent(), andgetChangeType()are well-implemented:
- They provide access to the private fields, which is necessary for the event to be useful.
- The methods are simple and straightforward, returning the corresponding field values.
This implementation maintains encapsulation while allowing necessary access to the event data.
1-56: 🛠️ Refactor suggestionConsider adding toString() method and serialVersionUID for improved robustness.
The overall implementation of
ConfigChangedEventis solid. However, consider the following improvements:
- Implement a
toString()method for easier debugging and logging.- Add a
serialVersionUIDas the class is indirectlySerializablethroughEventObject.Here's an example implementation:
private static final long serialVersionUID = 1L; @Override public String toString() { return "ConfigChangedEvent{" + "key='" + key + '\'' + ", content='" + content + '\'' + ", changeType=" + changeType + '}'; }Also, please ensure that the
ConfigChangeTypeenum is properly defined in the project. Run the following command to verify its existence:✅ Verification successful
Recommendations for Enhancements in ConfigChangedEvent Class
The
ConfigChangedEventclass is well-implemented. To further enhance its robustness and debuggability, consider the following improvements:
Implement
toString()Method
- Facilitates easier debugging and logging by providing a string representation of the object.
Add
serialVersionUID
- Since the class is indirectly
SerializablethroughEventObject, defining aserialVersionUIDensures version compatibility during serialization.private static final long serialVersionUID = 1L; @Override public String toString() { return "ConfigChangedEvent{" + "key='" + key + '\'' + ", content='" + content + '\'' + ", changeType=" + changeType + '}'; }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence of ConfigChangeType enum rg --type java "enum\s+ConfigChangeType" -g '!ConfigChangedEvent.java'Length of output: 176
test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/base/BaseZkTest.java (2)
17-27: LGTM: Package declaration and imports are appropriate.The package name follows Java conventions, and the imports are relevant to the class's functionality. Good job on keeping the imports clean and focused.
1-62: Overall assessment: Good foundation with room for improvement.The
BaseZkTestclass provides a solid foundation for ZooKeeper-based integration tests. It correctly sets up and tears down the test environment, which is crucial for reliable testing. However, there are opportunities to enhance its robustness and maintainability:
- Improve Javadoc comments for better documentation.
- Enhance error handling and logging in both setup and teardown methods.
- Consider using a random available port for the TestingServer to avoid potential conflicts.
- Ensure proper resource cleanup using try-with-resources or similar patterns.
Implementing these suggestions will make the class more resilient and easier to maintain in the long run.
test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/NacosDynamicConfigTest.java (1)
1-17: LGTM: Proper license header and package declaration.The file includes the correct Apache 2.0 license header and the package is properly declared.
core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigKeys.java (5)
19-20: LGTM: Import statement added correctly.The import for
com.alipay.sofa.common.config.ConfigKeyis necessary for the newly added ConfigKey variables and is correctly placed at the beginning of the file.
32-38: LGTM: DYNAMIC_URL ConfigKey added correctly, but clarify default value.The
DYNAMIC_URLConfigKey is well-defined with a clear description. However, the default value is set to a single space character (" "). Is this intentional, or should it be an empty string or null?Could you please clarify the reasoning behind using a space as the default value for
DYNAMIC_URL? If it's not intentional, consider changing it to an empty string ("") or null, whichever is more appropriate for your use case.
40-46: LGTM: ZK_ADDRESS ConfigKey added correctly.The
ZK_ADDRESSConfigKey is well-defined with a clear description and a sensible default value for a local Zookeeper instance. The key follows a consistent naming convention with the other configuration centers.
48-54: LGTM: NACOS_ADDRESS ConfigKey added correctly.The
NACOS_ADDRESSConfigKey is well-defined with a clear description and a sensible default value for a local Nacos instance. The key follows a consistent naming convention with the other configuration centers.
55-61: LGTM: APOLLO_ADDRESS ConfigKey added correctly, but clarify scope.The
APOLLO_ADDRESSConfigKey is well-defined with a clear description and a sensible default value for a local Apollo instance. The key follows a consistent naming convention with the other configuration centers.However, the PR objectives mentioned only Zookeeper and Nacos integration. Could you please clarify if Apollo integration is also part of this PR's scope? If so, consider updating the PR description to include this information.
Please confirm if Apollo integration is intended to be part of this PR. If it is, update the PR description to reflect this addition. If not, consider moving this ConfigKey to a separate PR focused on Apollo integration.
test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/ZookeeperDynamicConfigTest.java (2)
1-37: LGTM: File structure and imports are well-organized.The file has an appropriate license header, relevant imports, and proper class documentation. The structure follows good practices for test files.
40-79: LGTM: Overall test structure is well-designed.The test method effectively sets up the environment, performs the necessary operations, and verifies the expected outcome. It covers the main functionality of Zookeeper dynamic configuration integration.
test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/ApolloDynamicConfigTest.java (3)
1-17: LGTM: File structure and package declaration are correct.The file includes the appropriate license header and follows standard Java conventions for package declaration.
36-43: LGTM: Class and test method declarations are correct.The
ApolloDynamicConfigTestclass andtestApolloDynamicConfigmethod are properly declared following JUnit conventions.
19-34: Consider removing unused imports.Some of the imported classes might not be used in the current implementation. It's a good practice to remove unused imports to keep the code clean and improve readability.
To verify unused imports, run the following command:
test/test-integration/pom.xml (1)
276-281: Clarify the need for both Nacos and Apollo dependencies.Earlier, the Apollo dependency was replaced with Nacos. However, Apollo is now added back as a test dependency. This might lead to confusion. Could you please clarify the reason for including both Nacos and Apollo? If both are needed for different test scenarios, consider adding a comment explaining this.
#!/bin/bash # Check for usage of both Nacos and Apollo in test files echo "Files using Nacos:" rg --type java "import.*com\.alibaba\.nacos" test/ echo "Files using Apollo:" rg --type java "import.*com\.ctrip\.framework\.apollo" test/core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigManager.java (2)
67-75: Overloaded 'initServiceConfiguration' method enhances flexibilityAdding the new method with
ConfigListenerparameter allows for more flexible service configuration initialization, enabling listeners to react to configuration changes.
121-129: Addition of 'addListener' method improves configuration managementThe new
addListenermethod allows registration of configuration listeners based on a specified key, enhancing the dynamic configuration capabilities of the system.config/config-apollo/src/main/java/com/alipay/sofa/rpc/dynamic/apollo/ApolloDynamicConfigManager.java (3)
62-66: Reconsider setting system properties in constructorsSetting system properties within constructors can lead to unexpected behavior, especially in multi-threaded environments or when multiple instances are created. This practice can cause side effects that are hard to track and debug.
Consider refactoring the code to avoid setting system properties in the constructor. Instead, initialize the necessary configurations within the components that require them or use a dedicated initialization method.
72-81: Reconsider setting system properties in constructorsSimilar to the above, setting system properties in the constructor can introduce global side effects. It's advisable to refactor this code to prevent potential issues arising from the modification of system properties during object creation.
157-162: Handle allPropertyChangeTypecases explicitlyIn the
getChangeTypemethod, onlyDELETEDandMODIFIEDchange types are handled, with all other types defaulting toMODIFIED. If thePropertyChangeTypeenum includes other values likeADDED, consider handling them explicitly to avoid unexpected behavior.Run the following script to check if there are other
PropertyChangeTypevalues that need to be handled:config/config-zk/src/main/java/com/alipay/sofa/rpc/dynamic/zk/ZookeeperDynamicConfigManager.java (1)
51-51: Remove RedundantfinalKeywordThe field
zkClientis already declared asfinal. This might be a typographical error in the code provided.bootstrap/bootstrap-api/src/main/java/com/alipay/sofa/rpc/bootstrap/DefaultConsumerBootstrap.java (4)
150-151: Overwriting existingConfigListenermay lead to unintended behavior
161-167: MissingConfigListenerfor request-level dynamic configuration
464-488: Good addition of error handling for malformed configuration linesThe added logic in the
processmethod to handle malformed configuration lines enhances the robustness of the system by logging warnings when a line doesn't conform to the expectedkey=valueformat.
491-491: UnusedconfigChangedmethod with empty implementationcore/api/src/main/java/com/alipay/sofa/rpc/config/AbstractInterfaceConfig.java (3)
218-222: Addition ofdynamicConfigValueCachefor dynamic configurationThe new field
dynamicConfigValueCacheis appropriately added and initialized with aConcurrentHashMap<>. This ensures thread-safe operations when handling dynamic configurations.
Line range hint
952-977: Proper management of method configurations in dynamic cacheThe code correctly updates
dynamicConfigValueCachewhen modifying method configurations. This ensures that dynamic changes are accurately reflected and retrieved.
1035-1044: Efficient retrieval of method configuration valuesThe method
getMethodConfigValuenow prioritizesdynamicConfigValueCacheoverconfigValueCache, which enhances the dynamic configuration retrieval process. The implementation is sound.
| @BeforeClass | ||
| public static void adBeforeClass() { | ||
| RpcRunningState.setUnitTestMode(true); | ||
|
|
||
| try { | ||
| server = new TestingServer(2181, true); | ||
| server.start(); | ||
| } catch (Exception e) { | ||
| e.printStackTrace(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve robustness and error handling in adBeforeClass method.
While the method sets up the testing environment correctly, there are a few areas for improvement:
- The hardcoded port 2181 might cause issues if it's already in use. Consider using a random available port or making it configurable.
- The exception handling could be more informative. Instead of just printing the stack trace, consider logging a meaningful error message.
Here's a suggested improvement:
@BeforeClass
public static void adBeforeClass() throws Exception {
RpcRunningState.setUnitTestMode(true);
try {
server = new TestingServer(0, true); // Use a random available port
server.start();
System.out.println("ZooKeeper TestingServer started on port: " + server.getPort());
} catch (Exception e) {
System.err.println("Failed to start ZooKeeper TestingServer: " + e.getMessage());
throw e; // Rethrow to fail the test setup
}
}This version uses a random available port, provides more informative logging, and rethrows the exception to ensure test setup failure is properly handled.
| @AfterClass | ||
| public static void adAfterClass() { | ||
| RpcRuntimeContext.destroy(); | ||
| RpcInternalContext.removeContext(); | ||
| RpcInvokeContext.removeContext(); | ||
|
|
||
| if (server != null) { | ||
| try { | ||
| server.stop(); | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance cleanup and error handling in adAfterClass method.
The method does a good job of cleaning up resources, but there are opportunities for improvement:
- Consider using try-with-resources for closing the TestingServer.
- Improve error handling and logging for better diagnostics.
Here's a suggested improvement:
@AfterClass
public static void adAfterClass() {
try {
RpcRuntimeContext.destroy();
RpcInternalContext.removeContext();
RpcInvokeContext.removeContext();
if (server != null) {
try {
server.close();
System.out.println("ZooKeeper TestingServer stopped successfully");
} catch (IOException e) {
System.err.println("Error stopping ZooKeeper TestingServer: " + e.getMessage());
}
}
} finally {
server = null; // Ensure the server reference is cleared even if an exception occurs
}
}This version provides more informative logging, uses close() instead of stop() (as close() is the preferred method in newer versions of TestingServer), and ensures the server reference is always cleared.
| @Test | ||
| public void testNacosDynamicConfig() throws Exception { | ||
| System.setProperty(DynamicConfigKeys.DYNAMIC_URL.getKey(), "nacos://127.0.0.1:8848"); | ||
| ApplicationConfig clientApplication = new ApplicationConfig(); | ||
| clientApplication.setAppName("demo"); | ||
|
|
||
| ConsumerConfig<HelloService> consumerConfig = new ConsumerConfig<HelloService>() | ||
| .setInterfaceId(HelloService.class.getName()) | ||
| .setProtocol("bolt") | ||
| .setDirectUrl("bolt://127.0.0.1:12200") | ||
| .setConnectTimeout(10 * 1000) | ||
| .setApplication(clientApplication); | ||
|
|
||
| consumerConfig.refer(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving test setup and configuration.
While the basic setup is correct, there are a few areas for improvement:
-
Setting system properties in a test method can affect other tests. Consider using
@Beforeand@Aftermethods to set up and clean up the system property. -
The hard-coded Nacos server address (127.0.0.1:8848) might not work in all environments. Consider making this configurable or using a mock server for testing.
-
Ensure proper cleanup after the test. Add a method to reset the system property:
@After
public void cleanup() {
System.clearProperty(DynamicConfigKeys.DYNAMIC_URL.getKey());
}- Consider using
@BeforeClassto set up shared resources if they're used across multiple test methods in this class.
| // 获取接口对应的动态配置监听器 | ||
| DynamicConfigManager dynamicConfigManager = DynamicConfigManagerFactory.getDynamicManagerWithUrl | ||
| (clientApplication.getAppName(), System.getProperty(DynamicConfigKeys.DYNAMIC_URL.getKey())); | ||
| Field field = NacosDynamicConfigManager.class.getDeclaredField("watchListenerMap"); | ||
| field.setAccessible(true); | ||
| Map<String, NacosDynamicConfigManager.NacosConfigListener> watchListenerMap = (Map<String, NacosDynamicConfigManager.NacosConfigListener>) field | ||
| .get(dynamicConfigManager); | ||
| NacosDynamicConfigManager.NacosConfigListener nacosConfigListener = watchListenerMap.get(consumerConfig | ||
| .getInterfaceId()); | ||
|
|
||
| // 测试配置更新 | ||
| String configValue = "timeout=5000"; | ||
| nacosConfigListener.innerReceive(consumerConfig.getInterfaceId(), consumerConfig.getAppName(), configValue); | ||
| Assert.assertEquals(5000, consumerConfig.getMethodTimeout("sayHello")); | ||
| configValue = "timeout=5000\n.sayHello.timeout=6000"; | ||
| nacosConfigListener.innerReceive(consumerConfig.getInterfaceId(), consumerConfig.getAppName(), configValue); | ||
| Assert.assertEquals(6000, consumerConfig.getMethodTimeout("sayHello")); | ||
| } |
There was a problem hiding this comment.
Refactor test to reduce coupling and improve robustness.
While the test logic is functional, there are several areas for improvement:
-
Avoid using reflection to access private fields. This creates tight coupling with the implementation and can break if the internal structure changes. Consider exposing necessary methods for testing or use dependency injection to provide a testable configuration.
-
Add error handling for reflection operations. If you must use reflection, ensure you handle potential exceptions:
try {
Field field = NacosDynamicConfigManager.class.getDeclaredField("watchListenerMap");
field.setAccessible(true);
// ... rest of the reflection code
} catch (NoSuchFieldException | IllegalAccessException e) {
Assert.fail("Failed to access watchListenerMap: " + e.getMessage());
}-
Consider using a mocking framework like Mockito to simulate the behavior of NacosDynamicConfigManager instead of relying on its internal implementation.
-
Add more assertions to verify the state before and after each configuration change.
-
Consider breaking this large test method into smaller, more focused test methods for each aspect of the functionality you're testing.
| try { | ||
| Thread.sleep(3000); | ||
| } catch (InterruptedException e) { | ||
| e.printStackTrace(); | ||
| } |
There was a problem hiding this comment.
Replace hard-coded sleep with a more robust waiting mechanism.
Using a fixed sleep time can lead to flaky tests if the configuration update takes longer than expected. Consider using a polling mechanism with a timeout, or if available, a callback or event system to detect when the configuration has been updated.
Here's a potential improvement:
public void waitForConfigUpdate(ConsumerConfig<?> config, String methodName, int expectedTimeout, long maxWaitTime) {
long startTime = System.currentTimeMillis();
while (System.currentTimeMillis() - startTime < maxWaitTime) {
if (config.getMethodTimeout(methodName) == expectedTimeout) {
return;
}
try {
Thread.sleep(100);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new RuntimeException("Interrupted while waiting for config update", e);
}
}
throw new AssertionError("Config was not updated within the expected time");
}
// Usage
waitForConfigUpdate(consumerConfig, "sayHello", 5000, 10000);| public void nodeChanged() throws Exception { | ||
| ChildData childData = nodeCache.getCurrentData(); | ||
| String content = null; | ||
| ConfigChangeType changeType; | ||
| if (childData == null) { | ||
| changeType = ConfigChangeType.DELETED; | ||
|
|
||
| } else if (childData.getStat().getVersion() == 0) { | ||
| content = new String(childData.getData(), RpcConstants.DEFAULT_CHARSET); | ||
| changeType = ConfigChangeType.ADDED; | ||
| } else { | ||
| content = new String(childData.getData(), RpcConstants.DEFAULT_CHARSET); | ||
| changeType = ConfigChangeType.MODIFIED; | ||
| } | ||
| ConfigChangedEvent configChangeEvent = new ConfigChangedEvent(pathKey, (String) content, changeType); |
There was a problem hiding this comment.
Potential NullPointerException in nodeChanged Method
In the nodeChanged() method, childData.getData() may return null, which would cause a NullPointerException when passed to new String(...). Additionally, the condition childData.getStat().getVersion() == 0 may not reliably indicate that a node was added. Consider adjusting the logic to handle null values safely and to accurately determine the change type.
Apply the following change:
@Override
public void nodeChanged() throws Exception {
ChildData childData = nodeCache.getCurrentData();
String content = null;
ConfigChangeType changeType;
if (childData == null) {
changeType = ConfigChangeType.DELETED;
} else {
+ byte[] data = childData.getData();
+ content = data != null ? new String(data, RpcConstants.DEFAULT_CHARSET) : null;
+ int version = childData.getStat().getVersion();
+ if (version == 0) {
changeType = ConfigChangeType.ADDED;
} else {
changeType = ConfigChangeType.MODIFIED;
}
}
ConfigChangedEvent configChangeEvent = new ConfigChangedEvent(pathKey, content, changeType);
listeners.forEach(listener -> listener.process(configChangeEvent));
}This change ensures that childData.getData() is checked for null and handles it appropriately.
📝 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.
| public void nodeChanged() throws Exception { | |
| ChildData childData = nodeCache.getCurrentData(); | |
| String content = null; | |
| ConfigChangeType changeType; | |
| if (childData == null) { | |
| changeType = ConfigChangeType.DELETED; | |
| } else if (childData.getStat().getVersion() == 0) { | |
| content = new String(childData.getData(), RpcConstants.DEFAULT_CHARSET); | |
| changeType = ConfigChangeType.ADDED; | |
| } else { | |
| content = new String(childData.getData(), RpcConstants.DEFAULT_CHARSET); | |
| changeType = ConfigChangeType.MODIFIED; | |
| } | |
| ConfigChangedEvent configChangeEvent = new ConfigChangedEvent(pathKey, (String) content, changeType); | |
| public void nodeChanged() throws Exception { | |
| ChildData childData = nodeCache.getCurrentData(); | |
| String content = null; | |
| ConfigChangeType changeType; | |
| if (childData == null) { | |
| changeType = ConfigChangeType.DELETED; | |
| } else { | |
| byte[] data = childData.getData(); | |
| content = data != null ? new String(data, RpcConstants.DEFAULT_CHARSET) : null; | |
| int version = childData.getStat().getVersion(); | |
| if (version == 0) { | |
| changeType = ConfigChangeType.ADDED; | |
| } else { | |
| changeType = ConfigChangeType.MODIFIED; | |
| } | |
| } | |
| ConfigChangedEvent configChangeEvent = new ConfigChangedEvent(pathKey, content, changeType); | |
| listeners.forEach(listener -> listener.process(configChangeEvent)); | |
| } |
| @Override | ||
| public String getProviderServiceProperty(String service, String key) { | ||
| try { | ||
| String configValue = configMap.get(DynamicConfigKeyHelper.buildProviderServiceProKey(service, key)); | ||
| return configValue != null ? configValue : DynamicHelper.DEFAULT_DYNAMIC_VALUE; | ||
| } catch (Exception e) { | ||
| return DynamicHelper.DEFAULT_DYNAMIC_VALUE; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public String getConsumerServiceProperty(String service, String key) { | ||
| try { | ||
| String configValue = configMap.get(DynamicConfigKeyHelper.buildConsumerServiceProKey(service, key)); | ||
| return configValue != null ? configValue : DynamicHelper.DEFAULT_DYNAMIC_VALUE; | ||
| } catch (Exception e) { | ||
| return DynamicHelper.DEFAULT_DYNAMIC_VALUE; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public String getProviderMethodProperty(String service, String method, String key) { | ||
| try { | ||
| String configValue = configMap.get(DynamicConfigKeyHelper.buildProviderMethodProKey(service, method, key)); | ||
| return configValue != null ? configValue : DynamicHelper.DEFAULT_DYNAMIC_VALUE; | ||
| } catch (Exception e) { | ||
| return DynamicHelper.DEFAULT_DYNAMIC_VALUE; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public String getConsumerMethodProperty(String service, String method, String key) { | ||
| try { | ||
| String configValue = configMap.get(DynamicConfigKeyHelper.buildConsumerMethodProKey(service, method, key)); | ||
| return configValue != null ? configValue : DynamicHelper.DEFAULT_DYNAMIC_VALUE; | ||
| } catch (Exception e) { | ||
| return DynamicHelper.DEFAULT_DYNAMIC_VALUE; | ||
| } |
There was a problem hiding this comment.
Log Exceptions in Configuration Retrieval Methods
In methods like getProviderServiceProperty, exceptions are caught, and a default value is returned without logging. Not logging exceptions can make troubleshooting difficult. Consider logging the exceptions to aid in diagnosing problems.
Apply the following change to each method:
} catch (Exception e) {
+ LOGGER.error("Error retrieving provider service property for service: " + service + ", key: " + key, e);
return DynamicHelper.DEFAULT_DYNAMIC_VALUE;
}Apply similar changes to:
getConsumerServiceProperty(String service, String key)getProviderMethodProperty(String service, String method, String key)getConsumerMethodProperty(String service, String method, String key)
Committable suggestion was skipped due to low confidence.
| for (String key : newValueMap.keySet()) { | ||
| newValueMap.put(key, ""); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using clear() to reset newValueMap
In the process method, you're resetting all values in newValueMap to empty strings:
for (String key : newValueMap.keySet()) {
newValueMap.put(key, "");
}If the intention is to clear the map, using newValueMap.clear(); is more efficient and expresses the intent more clearly. If you need to retain the keys with empty values, please add a comment to explain this behavior.
Apply this diff to clear the map:
-for (String key : newValueMap.keySet()) {
- newValueMap.put(key, "");
-}
+newValueMap.clear();| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.*; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using wildcard imports; import specific classes instead
Using wildcard imports like import java.util.*; can lead to namespace pollution and may introduce ambiguity between class names. It's best practice to import only the specific classes you need.
Apply this diff to import specific classes:
-import java.util.*;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;Committable suggestion was skipped due to low confidence.
| if(StringUtils.isBlank(newValueStr)){ | ||
| // 改为默认值 | ||
| newValueStr = configValueCache.get(property) == null ? null : configValueCache.get(property).toString(); | ||
| } |
There was a problem hiding this comment.
Potential NullPointerException when accessing configValueCache
In line 934, configValueCache.get(property) is called without checking if configValueCache is initialized. If configValueCache is null, this will result in a NullPointerException.
Consider adding a null check before accessing configValueCache:
+ if (configValueCache != null && configValueCache.get(property) != null) {
newValueStr = configValueCache.get(property).toString();
+ } else {
newValueStr = null;
+ }Alternatively, ensure that configValueCache is initialized before it's accessed in this method.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (9)
core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigManager.java (2)
40-58: LGTM with a minor suggestion: Add null check for remainUrl.The updated constructor and new
parseRemainUrlmethod effectively handle the URL parsing, addressing previous concerns about URL decoding and improving code organization. However, consider adding an explicit null check forremainUrlin the constructor to prevent potential NullPointerExceptions.Consider adding this null check at the beginning of the constructor:
if (remainUrl == null) { throw new IllegalArgumentException("remainUrl must not be null"); }
60-69: LGTM with a suggestion: Return an unmodifiable view of params.The new getter methods appropriately provide access to the private fields. However, to prevent potential external modification of the internal state, consider returning an unmodifiable view of the
paramsMap.Modify the
getParamsmethod as follows:protected Map<String, String> getParams() { return Collections.unmodifiableMap(params); }Don't forget to add the necessary import:
import java.util.Collections;test/test-integration/pom.xml (1)
270-281: LGTM. Consider using a property for Curator version.The addition of ZooKeeper and Curator dependencies aligns with the PR objectives. However, to maintain consistency and ease future updates, consider defining the Curator version in a property.
Apply this diff to use a property for the Curator version:
+ <properties> + <curator.version>4.3.0</curator.version> + <!-- ... other properties ... --> + </properties> <!-- ... other dependencies ... --> <dependency> <groupId>org.apache.curator</groupId> <artifactId>curator-test</artifactId> - <version>4.3.0</version> + <version>${curator.version}</version> <scope>test</scope> </dependency>bootstrap/bootstrap-api/src/main/java/com/alipay/sofa/rpc/bootstrap/DefaultConsumerBootstrap.java (3)
492-492: Remove unusedconfigChangedmethodThe
configChangedmethod is empty and doesn't seem to be used. Consider removing it to improve code cleanliness.Apply this diff to remove the unused method:
- @Override - public void configChanged(Map newValueMap) { - - }
Line range hint
496-571: Enhance error handling and logging inattrUpdatedmethodThe
attrUpdatedmethod handles configuration changes well, but could benefit from improved error handling and logging.Consider the following improvements:
- Use try-with-resources for managing resources that need to be closed.
- Add more detailed logging, especially for the re-referencing process.
- Consider using a custom exception for configuration change errors to make error handling more specific.
Here's a partial example of how you might implement these suggestions:
@Override public synchronized void attrUpdated(Map newValueMap) { String appName = consumerConfig.getAppName(); Map<String, String> newValues = (Map<String, String>) newValueMap; Map<String, String> oldValues = new HashMap<>(); boolean rerefer = false; try { rerefer = compareAndUpdateValues(newValues, oldValues); } catch (Exception e) { LOGGER.errorWithApp(appName, "Error comparing consumer attributes: {}", e.getMessage(), e); return; } if (rerefer) { try { rereferConsumer(newValues, oldValues); } catch (ConfigurationChangeException e) { LOGGER.errorWithApp(appName, "Error during consumer re-referencing: {}", e.getMessage(), e); rollbackChanges(oldValues); } } } private void rereferConsumer(Map<String, String> newValues, Map<String, String> oldValues) throws ConfigurationChangeException { String appName = consumerConfig.getAppName(); try { LOGGER.infoWithApp(appName, "Starting consumer re-referencing process"); unSubscribe(); updateAttributes(newValues); switchCluster(); LOGGER.infoWithApp(appName, "Consumer re-referencing completed successfully"); } catch (Exception e) { throw new ConfigurationChangeException("Failed to re-refer consumer", e); } } private void rollbackChanges(Map<String, String> oldValues) { for (Map.Entry<String, String> entry : oldValues.entrySet()) { consumerConfig.updateAttribute(entry.getKey(), entry.getValue(), true); } subscribe(); }This refactoring improves error handling, adds more detailed logging, and makes the code more modular and easier to maintain.
Line range hint
1-624: Improve code organization and documentationWhile the changes implement dynamic configuration management effectively, there are opportunities to improve code organization and documentation:
- Consider grouping related methods together (e.g., all methods related to dynamic configuration).
- Add JavaDoc comments to new methods and fields, explaining their purpose and usage.
- Update the class-level JavaDoc to mention the new dynamic configuration capabilities.
Here's an example of how you might improve the documentation:
/** * Default consumer bootstrap. * This class now supports dynamic configuration management, including interface-level * and request-level dynamic configurations. * * @author <a href="mailto:[email protected]">GengZhang</a> */ @Extension("sofa") public class DefaultConsumerBootstrap<T> extends ConsumerBootstrap<T> { // ... existing code ... /** * Builds a ConfigListener for handling dynamic configuration changes. * * @param bootstrap ConsumerBootstrap * @return ConfigListener */ protected ConfigListener buildConfigListener(ConsumerBootstrap bootstrap) { return new ConsumerAttributeListener(); } // ... other methods ... /** * Inner class for handling consumer attribute changes in dynamic configuration. */ private class ConsumerAttributeListener implements ConfigListener { /** * Stores new configuration values during processing. */ private Map<String, String> newValueMap = new HashMap<>(); /** * List of configuration keys that can be dynamically updated. */ private static final List<String> DYNAMIC_CONFIG_KEYS = Arrays.asList( RpcConstants.CONFIG_KEY_TIMEOUT, RpcConstants.CONFIG_KEY_RETRIES, RpcConstants.CONFIG_KEY_LOADBALANCER ); // ... existing methods ... /** * Processes configuration change events. * * @param event The configuration change event */ @Override public void process(ConfigChangedEvent event) { // ... implementation ... } // ... other methods ... } }These improvements in documentation will make the code more maintainable and easier for other developers to understand and work with.
test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/ZookeeperDynamicConfigTest.java (1)
42-42: Declare logger asprivate static finalTo adhere to standard Java conventions and ensure thread safety, declare the logger as
private static final.Apply this diff:
- Logger logger = LoggerFactory.getLogger(ZookeeperDynamicConfigTest.class); + private static final Logger logger = LoggerFactory.getLogger(ZookeeperDynamicConfigTest.class);config/config-apollo/src/main/java/com/alipay/sofa/rpc/dynamic/apollo/ApolloDynamicConfigManager.java (2)
135-135: Adjust Access Modifier for Inner ClassApolloListenerThe inner class
ApolloListeneris declared aspublic. Since it's intended for internal use withinApolloDynamicConfigManager, consider making itprivateor package-private to encapsulate it and prevent unintended access from external classes.Apply this diff to adjust the access modifier:
-public class ApolloListener implements ConfigChangeListener { +class ApolloListener implements ConfigChangeListener {
73-74: Check for Null or BlankappNameBefore Setting System PropertyIn the constructor,
appNameis used without checking if it's null or blank before setting the system property. This could lead to setting an invalid system property value.Consider adding a null or blank check:
System.setProperty(APOLLO_APPID_KEY, appName); +if (StringUtils.isNotBlank(appName)) { + System.setProperty(APOLLO_APPID_KEY, appName); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- bootstrap/bootstrap-api/src/main/java/com/alipay/sofa/rpc/bootstrap/DefaultConsumerBootstrap.java (7 hunks)
- config/config-apollo/src/main/java/com/alipay/sofa/rpc/dynamic/apollo/ApolloDynamicConfigManager.java (3 hunks)
- config/config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManager.java (1 hunks)
- core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigManager.java (4 hunks)
- test/test-integration/pom.xml (2 hunks)
- test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/ApolloDynamicConfigTest.java (1 hunks)
- test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/NacosDynamicConfigTest.java (1 hunks)
- test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/ZookeeperDynamicConfigTest.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- config/config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManager.java
- test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/ApolloDynamicConfigTest.java
- test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/NacosDynamicConfigTest.java
🧰 Additional context used
🔇 Additional comments (11)
core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigManager.java (5)
21-24: LGTM: Import statements are appropriate.The added import statements for
ConfigListenerand Java collections are necessary for the new functionality introduced in the class.
36-38: LGTM: New fields added and initialized properly.The new private fields
addressandparamsare appropriately added to store parsed information from theremainUrl. The initialization ofparamsas an empty HashMap addresses the previous concern about potential NullPointerExceptions.
80-87: LGTM: New initServiceConfiguration method adds flexibility.The new overloaded
initServiceConfigurationmethod with aConfigListenerparameter provides additional flexibility for service configuration initialization. The method is well-documented with appropriate JavaDoc.
135-141: LGTM: New addListener method enhances configurability.The new abstract
addListenermethod is a valuable addition, allowing for the registration of configuration listeners. The method is well-documented with appropriate JavaDoc.
Line range hint
1-142: Overall LGTM: Significant improvements to DynamicConfigManager.The changes to the
DynamicConfigManagerclass significantly enhance its functionality and flexibility. The addition of URL parsing, new fields, getter methods, and listener support improves the class's capability to manage dynamic configurations. The modifications address previous concerns and generally follow good coding practices.Minor suggestions for improvement have been made in the individual review comments, including:
- Adding a null check for
remainUrlin the constructor.- Returning an unmodifiable view of the
paramsMap in thegetParamsmethod.These changes, once implemented, will further improve the robustness and safety of the class.
bootstrap/bootstrap-api/src/main/java/com/alipay/sofa/rpc/bootstrap/DefaultConsumerBootstrap.java (3)
32-33: LGTM: New imports for dynamic configuration managementThe addition of
ConfigChangedEventandConfigChangeTypeimports is consistent with the implementation of dynamic configuration features in this class.
150-151: LGTM: Implementation of ConfigListenerThe addition of a
ConfigListenerbuilt usingbuildConfigListenermethod and setting it toconsumerConfigis a good practice for handling configuration changes.
169-177: LGTM: Implementation of interface-level dynamic configurationThe addition of interface-level dynamic configuration is a good enhancement. It checks for a dynamic URL, initializes a
DynamicConfigManager, and sets up a listener for the interface.To ensure that the dynamic configuration is properly set up, let's verify the usage of
DynamicConfigKeys.DYNAMIC_URL:✅ Verification successful
Dynamic Configuration Setup Verified Successfully
The usage of
DynamicConfigKeys.DYNAMIC_URLin theDefaultConsumerBootstrap.javaand the accompanying test cases confirms that the dynamic configuration is properly set up.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of DynamicConfigKeys.DYNAMIC_URL rg --type java "DynamicConfigKeys.DYNAMIC_URL"Length of output: 1914
config/config-apollo/src/main/java/com/alipay/sofa/rpc/dynamic/apollo/ApolloDynamicConfigManager.java (3)
63-67: Avoid UsingSystem.setPropertyin ConstructorsSetting system properties within constructors can have unintended side effects since system properties are global and affect the entire JVM. This may lead to unpredictable behavior, especially in multi-threaded environments or when multiple instances are created.
160-161: Ensure Thread Safety When Managing ListenersThe
listenersset usesCopyOnWriteArraySet, which is suitable for scenarios with infrequent mutations and frequent iterations. If listeners are added or removed frequently, consider using a different thread-safe collection likeConcurrentHashMapto improve performance.
137-137:⚠️ Potential issueConsider Using
Set<ConfigListener>Instead of Raw TypeThe
listenersset is declared without specifying the generic type, which can lead to type safety issues.Apply this diff to specify the generic type:
-private Set listeners = new CopyOnWriteArraySet<>(); +private Set<ConfigListener> listeners = new CopyOnWriteArraySet<>();Likely invalid or redundant comment.
| <dependency> | ||
| <groupId>com.alipay.sofa</groupId> | ||
| <artifactId>sofa-rpc-config-apollo</artifactId> | ||
| <version>${project.parent.version}</version> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
Clarify the need for Apollo dependency.
The addition of the sofa-rpc-config-apollo dependency seems to contradict the PR objective of replacing Apollo with Nacos. If this is no longer needed, consider removing it to complete the transition to Nacos.
If this dependency is still required for certain tests, please add a comment explaining its necessity. Otherwise, consider removing it with the following diff:
- <dependency>
- <groupId>com.alipay.sofa</groupId>
- <artifactId>sofa-rpc-config-apollo</artifactId>
- <version>${project.parent.version}</version>
- <scope>test</scope>
- </dependency>📝 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.
| <dependency> | |
| <groupId>com.alipay.sofa</groupId> | |
| <artifactId>sofa-rpc-config-apollo</artifactId> | |
| <version>${project.parent.version}</version> | |
| <scope>test</scope> | |
| </dependency> |
| <artifactId>sofa-rpc-config-nacos</artifactId> | ||
| <version>${project.parent.version}</version> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
💡 Codebase verification
Apollo dependencies still present in the codebase.
The grep results indicate that sofa-rpc-config-apollo dependencies remain in the following files:
all/pom.xmlconfig/config-apollo/pom.xmltest/test-integration/pom.xml
Please ensure that all Apollo dependencies are fully removed to align with the PR objectives.
🔗 Analysis chain
LGTM. Verify complete removal of Apollo dependencies.
The replacement of Apollo with Nacos for configuration management is correctly implemented here. This change aligns with the PR objectives.
To ensure a complete transition, let's verify that all Apollo dependencies have been removed:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining Apollo dependencies
grep -R "sofa-rpc-config-apollo" .
Length of output: 391
| public void process(ConfigChangedEvent event) { | ||
| // 清除上次的赋值，并保留赋值过的key | ||
| for (String key : newValueMap.keySet()) { | ||
| newValueMap.put(key, ""); | ||
| } | ||
| if (!event.getChangeType().equals(ConfigChangeType.DELETED)) { | ||
| // ADDED or MODIFIED | ||
| String[] lines = event.getContent().split("\n"); | ||
| for (String line : lines) { | ||
| String[] keyValue = line.split("=", 2); | ||
| if (keyValue.length == 2) { | ||
| String key = keyValue[0].trim(); | ||
| String value = keyValue[1].trim(); | ||
| for (String dynamicConfigKey : dynamicConfigKeys) { | ||
| if (key.equals(dynamicConfigKey) || key.endsWith("." + dynamicConfigKey)) { | ||
| newValueMap.put(key, value); | ||
| break; | ||
| } | ||
| } | ||
| } else { | ||
| LOGGER.warn("Malformed configuration line: {}", line); | ||
| } | ||
| } | ||
| } | ||
| attrUpdated(newValueMap); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the process method for better readability
The process method is quite long and could benefit from some refactoring to improve readability and maintainability.
Consider extracting the logic for parsing configuration lines into a separate method. Here's a suggested refactoring:
private void parseConfigurationLines(String content) {
String[] lines = content.split("\n");
for (String line : lines) {
String[] keyValue = line.split("=", 2);
if (keyValue.length == 2) {
String key = keyValue[0].trim();
String value = keyValue[1].trim();
for (String dynamicConfigKey : DYNAMIC_CONFIG_KEYS) {
if (key.equals(dynamicConfigKey) || key.endsWith("." + dynamicConfigKey)) {
newValueMap.put(key, value);
break;
}
}
} else {
LOGGER.warn("Malformed configuration line: {}", line);
}
}
}
@Override
public void process(ConfigChangedEvent event) {
newValueMap.clear();
if (!event.getChangeType().equals(ConfigChangeType.DELETED)) {
parseConfigurationLines(event.getContent());
}
attrUpdated(newValueMap);
}This refactoring improves readability and separates concerns, making the code easier to maintain and test.
| private Map<String, String> newValueMap = new HashMap<>(); | ||
|
|
||
| // 动态配置项 | ||
| private List<String> dynamicConfigKeys = Arrays.asList(RpcConstants.CONFIG_KEY_TIMEOUT, | ||
| RpcConstants.CONFIG_KEY_RETRIES, RpcConstants.CONFIG_KEY_LOADBALANCER); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a static final list for dynamicConfigKeys
The dynamicConfigKeys list is initialized with constant values. Consider making it a static final list to improve performance and reduce memory usage.
Apply this diff to make dynamicConfigKeys a static final list:
- private List<String> dynamicConfigKeys = Arrays.asList(RpcConstants.CONFIG_KEY_TIMEOUT,
- RpcConstants.CONFIG_KEY_RETRIES, RpcConstants.CONFIG_KEY_LOADBALANCER);
+ private static final List<String> DYNAMIC_CONFIG_KEYS = Arrays.asList(RpcConstants.CONFIG_KEY_TIMEOUT,
+ RpcConstants.CONFIG_KEY_RETRIES, RpcConstants.CONFIG_KEY_LOADBALANCER);📝 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.
| private Map<String, String> newValueMap = new HashMap<>(); | |
| // 动态配置项 | |
| private List<String> dynamicConfigKeys = Arrays.asList(RpcConstants.CONFIG_KEY_TIMEOUT, | |
| RpcConstants.CONFIG_KEY_RETRIES, RpcConstants.CONFIG_KEY_LOADBALANCER); | |
| private Map<String, String> newValueMap = new HashMap<>(); | |
| // 动态配置项 | |
| private static final List<String> DYNAMIC_CONFIG_KEYS = Arrays.asList(RpcConstants.CONFIG_KEY_TIMEOUT, | |
| RpcConstants.CONFIG_KEY_RETRIES, RpcConstants.CONFIG_KEY_LOADBALANCER); |
| Field field = ZookeeperDynamicConfigManager.class.getDeclaredField("zkClient"); | ||
| field.setAccessible(true); | ||
| CuratorFramework zkClient = (CuratorFramework) field.get(dynamicConfigManager); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using reflection to access private fields
Accessing the private field zkClient via reflection breaks encapsulation and may lead to brittle tests if the internal implementation changes.
Consider one of the following approaches:
- Modify
ZookeeperDynamicConfigManagerto provide a public accessor method forzkClient. - Use existing public APIs to achieve the desired functionality without direct access to internal fields.
- Mock the necessary components if you're aiming to simulate specific behaviors in your test.
This will enhance maintainability and reduce dependency on internal implementation details.
| if (zkClient.checkExists().forPath("/config/demo/com.alipay.sofa.rpc.test.HelloService") == null) { | ||
| zkClient.create() | ||
| .creatingParentsIfNeeded() | ||
| .withMode(CreateMode.PERSISTENT) | ||
| .forPath("/config/demo/com.alipay.sofa.rpc.test.HelloService", "timeout=5000".getBytes()); | ||
| } else { | ||
| zkClient.setData().forPath("/sofa-rpc/config/demo/com.alipay.sofa.rpc.test.HelloService", | ||
| "timeout=5000".getBytes()); | ||
| } |
There was a problem hiding this comment.
Fix inconsistent Zookeeper node paths
There is an inconsistency in the Zookeeper node paths between the creation and update operations:
-
Creation path (lines 66-70):
"/config/demo/com.alipay.sofa.rpc.test.HelloService" -
Update path (lines 72-73):
"/sofa-rpc/config/demo/com.alipay.sofa.rpc.test.HelloService"
This discrepancy may cause the update operation to affect a different node than intended, leading to unexpected behavior during the test.
Please ensure that the paths are consistent across creation, update, and deletion operations.
Apply this diff to correct the update path:
zkClient.setData().forPath(
- "/sofa-rpc/config/demo/com.alipay.sofa.rpc.test.HelloService",
+ "/config/demo/com.alipay.sofa.rpc.test.HelloService",
"timeout=5000".getBytes());| // TODO 暂不支持 | ||
| } |
There was a problem hiding this comment.
Implement initServiceConfiguration(String service) Method
The method initServiceConfiguration(String service) contains a TODO comment indicating it's currently not supported. Leaving it unimplemented might lead to AbstractMethodError if called. Consider implementing the method or throwing an UnsupportedOperationException to handle this scenario gracefully.
Would you like assistance in implementing this method or should we update it to throw an exception? I can help draft the implementation or open a GitHub issue to track this task.
Example of throwing an exception:
@Override
public void initServiceConfiguration(String service) {
- // TODO 暂不支持
+ throw new UnsupportedOperationException("initServiceConfiguration is not supported yet.");
}📝 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.
| // TODO 暂不支持 | |
| } | |
| @Override | |
| public void initServiceConfiguration(String service) { | |
| throw new UnsupportedOperationException("initServiceConfiguration is not supported yet."); | |
| } |
| private ConfigChangeType getChangeType(ConfigChange change) { | ||
| if (change.getChangeType() == PropertyChangeType.DELETED) { | ||
| return ConfigChangeType.DELETED; | ||
| } | ||
| return ConfigChangeType.MODIFIED; | ||
| } |
There was a problem hiding this comment.
Handle All Possible Property Change Types
The getChangeType method currently handles only DELETED and defaults all other change types to MODIFIED. However, there may be other change types like ADDED. Explicitly handling all possible PropertyChangeType values ensures correct behavior and easier maintenance.
Update the method to handle additional change types:
private ConfigChangeType getChangeType(ConfigChange change) {
if (change.getChangeType() == PropertyChangeType.DELETED) {
return ConfigChangeType.DELETED;
+ } else if (change.getChangeType() == PropertyChangeType.ADDED) {
+ return ConfigChangeType.ADDED;
}
return ConfigChangeType.MODIFIED;
}📝 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.
| private ConfigChangeType getChangeType(ConfigChange change) { | |
| if (change.getChangeType() == PropertyChangeType.DELETED) { | |
| return ConfigChangeType.DELETED; | |
| } | |
| return ConfigChangeType.MODIFIED; | |
| } | |
| private ConfigChangeType getChangeType(ConfigChange change) { | |
| if (change.getChangeType() == PropertyChangeType.DELETED) { | |
| return ConfigChangeType.DELETED; | |
| } else if (change.getChangeType() == PropertyChangeType.ADDED) { | |
| return ConfigChangeType.ADDED; | |
| } | |
| return ConfigChangeType.MODIFIED; | |
| } |
| if (params != null && params.containsKey("cluster")) { | ||
| String clusterValue = (String)params.get("cluster"); | ||
| System.setProperty(APOLLO_CLUSTER_KEY, clusterValue); |
There was a problem hiding this comment.
Specify Generic Types for Map Usage
The params variable is declared without generic types, which can lead to unchecked type warnings and potential ClassCastExceptions. It's good practice to specify the generic types for collections to ensure type safety.
Apply this diff to specify the generic types:
- Map params = getParams();
+ Map<String, Object> params = getParams();Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigManager.java (5)
36-38: LGTM: New fields added for storing parsed URL information.The addition of
addressandparamsfields enhances the class's ability to manage configuration details. Good practice initializingparamsto prevent NullPointerExceptions.Consider adding a comment explaining the purpose of these fields for better code documentation.
40-43: LGTM: Constructor updated to handle remainUrl parameter.The addition of the
remainUrlparameter and the call toparseRemainUrlmethod improves the initialization process of the DynamicConfigManager.Consider adding parameter validation for
remainUrlto ensure it's not null or empty before parsing.
45-58: Improve robustness and security in parseRemainUrl method.The method effectively parses the remainUrl to populate address and params fields. However, consider the following improvements:
- Add null check for remainUrl at the beginning of the method.
- Use java.net.URL for more robust URL parsing.
- Implement URL decoding for parameter keys and values to handle encoded characters correctly.
- Consider using a more robust query string parser to handle edge cases (e.g., parameters without values).
Example improvement:
protected void parseRemainUrl(String remainUrl) { if (remainUrl == null || remainUrl.isEmpty()) { throw new IllegalArgumentException("remainUrl must not be null or empty"); } try { URL url = new URL(remainUrl); this.address = url.getProtocol() + "://" + url.getAuthority() + url.getPath(); String query = url.getQuery(); if (query != null && !query.isEmpty()) { for (String param : query.split("&")) { String[] pair = param.split("="); String key = URLDecoder.decode(pair[0], "UTF-8"); String value = pair.length > 1 ? URLDecoder.decode(pair[1], "UTF-8") : ""; this.params.put(key, value); } } } catch (MalformedURLException | UnsupportedEncodingException e) { throw new IllegalArgumentException("Invalid URL: " + remainUrl, e); } }This implementation is more robust and secure, handling various edge cases and properly decoding URL-encoded parameters.
60-69: LGTM: Getter methods added for encapsulation.The addition of getter methods for
appName,address, andparamsprovides proper encapsulation. However, consider returning an unmodifiable view of the params Map to prevent external modification:protected Map<String, String> getParams() { return Collections.unmodifiableMap(params); }This ensures that the internal state of the
paramsMap cannot be modified by external code, maintaining the integrity of the parsed parameters.
135-141: LGTM: New addListener method for dynamic listener registration.The addition of the abstract
addListenermethod is a good design choice, allowing for dynamic registration of configuration listeners. This enhances the flexibility of the configuration management system.Consider adding a corresponding
removeListenermethod to allow for listener deregistration, which would provide a more complete API for listener management.config/config-apollo/src/main/java/com/alipay/sofa/rpc/dynamic/apollo/ApolloDynamicConfigManager.java (1)
137-137: DeclarelistenersasfinalThe
listenersset in theApolloListenerclass is not declared asfinal. Since this set is initialized once and should not be reassigned, marking it asfinalenhances code clarity and thread safety.Apply this diff to declare the field as
final:- private Set<ConfigListener> listeners = new CopyOnWriteArraySet<>(); + private final Set<ConfigListener> listeners = new CopyOnWriteArraySet<>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- bootstrap/bootstrap-api/src/main/java/com/alipay/sofa/rpc/bootstrap/DefaultConsumerBootstrap.java (7 hunks)
- config/config-apollo/src/main/java/com/alipay/sofa/rpc/dynamic/apollo/ApolloDynamicConfigManager.java (3 hunks)
- config/config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManager.java (1 hunks)
- core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigManager.java (4 hunks)
- test/test-integration/pom.xml (2 hunks)
- test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/ApolloDynamicConfigTest.java (1 hunks)
- test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/NacosDynamicConfigTest.java (1 hunks)
- test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/ZookeeperDynamicConfigTest.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- bootstrap/bootstrap-api/src/main/java/com/alipay/sofa/rpc/bootstrap/DefaultConsumerBootstrap.java
- config/config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManager.java
- test/test-integration/pom.xml
- test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/ApolloDynamicConfigTest.java
- test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/NacosDynamicConfigTest.java
- test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/ZookeeperDynamicConfigTest.java
🧰 Additional context used
🔇 Additional comments (5)
core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigManager.java (2)
21-24: LGTM: New imports added for enhanced functionality.The addition of
ConfigListenerandjava.util.Mapimports aligns with the new features implemented in this class.
80-87: LGTM: New initServiceConfiguration method with listener support.The addition of this overloaded abstract method enhances the flexibility of service configuration initialization by allowing the registration of a ConfigListener. This is a good design choice that maintains consistency with the abstract nature of the class while providing extended functionality.
config/config-apollo/src/main/java/com/alipay/sofa/rpc/dynamic/apollo/ApolloDynamicConfigManager.java (3)
63-67: Also applies to: 73-79
75-75: Specify Generic Types forMapUsageThe
paramsvariable is declared without generic types, which can lead to unchecked type warnings and potentialClassCastExceptions. It's good practice to specify generic types for collections to ensure type safety.
126-129:⚠️ Potential issueConsider Throwing
UnsupportedOperationExceptionThe method
getServiceAuthRule(String service)returnsnullwith a TODO comment indicating it's not yet supported. Returningnullmight lead toNullPointerExceptions if the calling code does not handle it properly. Consider throwing anUnsupportedOperationExceptionto make the unsupported status explicit.Apply this diff to update the method:
@Override public AuthRuleGroup getServiceAuthRule(String service) { // TODO 暂不支持 - return null; + throw new UnsupportedOperationException("getServiceAuthRule is not supported yet."); }Likely invalid or redundant comment.
| private ConfigChangeType getChangeType(ConfigChange change) { | ||
| if (change.getChangeType() == PropertyChangeType.DELETED) { | ||
| return ConfigChangeType.DELETED; | ||
| } | ||
| if (change.getChangeType() == PropertyChangeType.ADDED) { | ||
| return ConfigChangeType.ADDED; | ||
| } | ||
| return ConfigChangeType.MODIFIED; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle All Possible Change Types Explicitly
In the getChangeType method, all unhandled PropertyChangeTypes default to MODIFIED. If new change types are introduced in the future, this could lead to unexpected behavior.
Consider modifying the method to explicitly handle all possible change types or throw an exception for unknown types:
private ConfigChangeType getChangeType(ConfigChange change) {
if (change.getChangeType() == PropertyChangeType.DELETED) {
return ConfigChangeType.DELETED;
}
if (change.getChangeType() == PropertyChangeType.ADDED) {
return ConfigChangeType.ADDED;
}
+ if (change.getChangeType() == PropertyChangeType.MODIFIED) {
+ return ConfigChangeType.MODIFIED;
+ }
+ throw new IllegalArgumentException("Unknown change type: " + change.getChangeType());
}📝 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.
| private ConfigChangeType getChangeType(ConfigChange change) { | |
| if (change.getChangeType() == PropertyChangeType.DELETED) { | |
| return ConfigChangeType.DELETED; | |
| } | |
| if (change.getChangeType() == PropertyChangeType.ADDED) { | |
| return ConfigChangeType.ADDED; | |
| } | |
| return ConfigChangeType.MODIFIED; | |
| } | |
| private ConfigChangeType getChangeType(ConfigChange change) { | |
| if (change.getChangeType() == PropertyChangeType.DELETED) { | |
| return ConfigChangeType.DELETED; | |
| } | |
| if (change.getChangeType() == PropertyChangeType.ADDED) { | |
| return ConfigChangeType.ADDED; | |
| } | |
| if (change.getChangeType() == PropertyChangeType.MODIFIED) { | |
| return ConfigChangeType.MODIFIED; | |
| } | |
| throw new IllegalArgumentException("Unknown change type: " + change.getChangeType()); | |
| } |
| public void onChange(com.ctrip.framework.apollo.model.ConfigChangeEvent changeEvent) { | ||
| for (String key : changeEvent.changedKeys()) { | ||
| ConfigChange change = changeEvent.getChange(key); | ||
| ConfigChangedEvent event = | ||
| new ConfigChangedEvent(key, change.getNewValue(), getChangeType(change)); | ||
| listeners.forEach(listener -> listener.process(event)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Prevent Concurrent Modification Exceptions
Modifying the listeners set while iterating over it in the onChange method could lead to ConcurrentModificationException. Although CopyOnWriteArraySet is thread-safe for concurrent reads and writes, it's safer to iterate over a snapshot of the set.
Consider iterating over a snapshot of the listeners:
public void onChange(com.ctrip.framework.apollo.model.ConfigChangeEvent changeEvent) {
for (String key : changeEvent.changedKeys()) {
ConfigChange change = changeEvent.getChange(key);
ConfigChangedEvent event =
new ConfigChangedEvent(key, change.getNewValue(), getChangeType(change));
- listeners.forEach(listener -> listener.process(event));
+ new ArrayList<>(listeners).forEach(listener -> listener.process(event));
}
}📝 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.
| public void onChange(com.ctrip.framework.apollo.model.ConfigChangeEvent changeEvent) { | |
| for (String key : changeEvent.changedKeys()) { | |
| ConfigChange change = changeEvent.getChange(key); | |
| ConfigChangedEvent event = | |
| new ConfigChangedEvent(key, change.getNewValue(), getChangeType(change)); | |
| listeners.forEach(listener -> listener.process(event)); | |
| } | |
| } | |
| public void onChange(com.ctrip.framework.apollo.model.ConfigChangeEvent changeEvent) { | |
| for (String key : changeEvent.changedKeys()) { | |
| ConfigChange change = changeEvent.getChange(key); | |
| ConfigChangedEvent event = | |
| new ConfigChangedEvent(key, change.getNewValue(), getChangeType(change)); | |
| new ArrayList<>(listeners).forEach(listener -> listener.process(event)); | |
| } | |
| } |
| public void addListener(String key, ConfigListener listener) { | ||
| ApolloListener apolloListener = watchListenerMap.computeIfAbsent(key, k -> new ApolloListener()); | ||
| apolloListener.addListener(listener); | ||
| config.addChangeListener(apolloListener, Collections.singleton(key)); | ||
| } |
There was a problem hiding this comment.
Ensure Listeners Are Not Added Multiple Times
In the addListener method, adding the same ApolloListener to config multiple times for the same key could result in duplicate notifications. Ensure that the listener is registered only once per key to prevent redundant processing.
Consider checking if the listener has already been added before registering it:
public void addListener(String key, ConfigListener listener) {
ApolloListener apolloListener = watchListenerMap.computeIfAbsent(key, k -> new ApolloListener());
apolloListener.addListener(listener);
+ if (!apolloListener.isRegistered()) {
config.addChangeListener(apolloListener, Collections.singleton(key));
+ apolloListener.setRegistered(true);
+ }
}You'll need to add isRegistered and setRegistered methods to the ApolloListener class.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (7)
core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigKeys.java (1)
28-30: Consider adding Javadoc for the new constants.While the constant names are self-explanatory, adding Javadoc comments would help developers understand:
- The purpose and usage of these configuration nodes
- Where these values are expected to be used in the configuration hierarchy
- Any constraints or format requirements
+ /** + * The configuration node path used in the configuration center. + */ public static final String CONFIG_NODE = "config"; + /** + * The default namespace for SOFA RPC configurations in the configuration center. + */ public static final String DEFAULT_NAMESPACE = "sofa-rpc";test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/NacosDynamicConfigTest.java (1)
33-37: Enhance test documentation and categorization.Consider adding:
- A detailed class-level JavaDoc explaining the test scenarios and prerequisites.
- Test category annotations (e.g.,
@Category(IntegrationTest.class)) to properly categorize this as an integration test./** + * Integration test for Nacos dynamic configuration functionality. + * Prerequisites: + * - Nacos server running on localhost:8848 + * - Nacos server configured with default credentials (nacos/nacos) + * * @author Narziss * @version NacosDynamicConfigTest.java, v 0.1 2024年09月28日 12:11 Narziss */ +@Category(IntegrationTest.class) public class NacosDynamicConfigTest {core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigManager.java (2)
80-88: Enhance method documentation for configuration listeners.The documentation for these methods should include:
- Lifecycle of the listeners
- Threading model for listener callbacks
- Error handling expectations
- Example usage
Add the following to the JavaDoc:
/** * Init service's governance related configuration with a configuration change listener. * * @param service target service * @param listener config listener that will be notified when configuration changes. * The listener should handle its own error cases and must be thread-safe. * Callbacks will be made on the configuration center's event dispatch thread. * @throws IllegalArgumentException if service or listener is null */ public abstract void initServiceConfiguration(String service, ConfigListener listener); /** * Add listener for configuration changes. * * @param key config key to monitor for changes * @param listener config listener that will be notified when the specified key's value changes. * The listener should handle its own error cases and must be thread-safe. * Callbacks will be made on the configuration center's event dispatch thread. * @throws IllegalArgumentException if key or listener is null */ public abstract void addListener(String key, ConfigListener listener);Also applies to: 135-141
Line range hint
21-142: Consider thread safety implications of the dynamic configuration design.The current implementation handles configuration updates through listeners, but there are some architectural considerations:
- The
paramsmap should be thread-safe if it can be modified after initialization- Consider using immutable configurations or copy-on-write patterns for thread safety
- Document the thread safety guarantees for implementing classes
Consider these options:
- Make
paramsunmodifiable after construction:protected Map<String, String> getParams() { return Collections.unmodifiableMap(params); }
- Use ConcurrentHashMap if runtime modifications are needed:
private final Map<String, String> params = new ConcurrentHashMap<>();
- Add thread safety documentation to the class:
/** * Base class for dynamic configuration management. * Thread Safety: Implementations must ensure thread-safe access to configuration data * as it may be accessed concurrently by multiple threads. */ @Extensible(singleton = true) public abstract class DynamicConfigManager {test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/ApolloDynamicConfigTest.java (1)
36-39: Enhance class documentation.Consider adding more detailed documentation about:
- The purpose of these tests
- Prerequisites for running the tests
- Expected behavior and test scenarios
Example improvement:
/** * Integration tests for Apollo Dynamic Configuration functionality. * Tests the dynamic configuration updates through Apollo configuration center, * including adding, modifying, and deleting configurations. * * Prerequisites: * - Apollo configuration center running at 127.0.0.1:8080 * - Default cluster configuration * * @author Narziss * @version ApolloDynamicConfigTest.java, v 0.1 2024年09月28日 10:46 Narziss */test/test-integration/pom.xml (1)
276-281: Consider using property for Curator test version.While the dependency is correctly added, consider managing the version through a property to align with the project's version management practices. This makes it easier to update versions across the project.
Apply this diff to use a property:
+ <properties> + <!-- existing properties --> + <curator.test.version>4.3.0</curator.test.version> + </properties> <dependencies> <!-- other dependencies --> <dependency> <groupId>org.apache.curator</groupId> <artifactId>curator-test</artifactId> - <version>4.3.0</version> + <version>${curator.test.version}</version> <scope>test</scope> </dependency> </dependencies>config/config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManager.java (1)
76-86: Use generic types for theparamsMap to ensure type safetyThe
paramsMap is used without type parameters, which may lead to unchecked type casting and compiler warnings. Parameterizing the Map enhances type safety and code clarity.Apply this diff to parameterize the Map:
- Map params = getParams(); + Map<String, Object> params = getParams();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- bootstrap/bootstrap-api/src/main/java/com/alipay/sofa/rpc/bootstrap/DefaultConsumerBootstrap.java (7 hunks)
- config/config-apollo/src/main/java/com/alipay/sofa/rpc/dynamic/apollo/ApolloDynamicConfigManager.java (3 hunks)
- config/config-nacos/src/main/java/com/alipay/sofa/rpc/dynamic/nacos/NacosDynamicConfigManager.java (1 hunks)
- core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigKeys.java (1 hunks)
- core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigManager.java (4 hunks)
- test/test-integration/pom.xml (2 hunks)
- test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/ApolloDynamicConfigTest.java (1 hunks)
- test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/NacosDynamicConfigTest.java (1 hunks)
- test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/ZookeeperDynamicConfigTest.java (1 hunks)
🔇 Additional comments (15)
core/api/src/main/java/com/alipay/sofa/rpc/dynamic/DynamicConfigKeys.java (3)
19-20: LGTM!The import is correctly added for the ConfigKey usage.
32-38:⚠️ Potential issueReview the default value for CENTER_ADDRESS.
The default value is set to a single space
" ", which seems unusual. Consider:
- Using an empty string
""if no default is intended- Using a meaningful default value if one is expected
- Adding a comment explaining why a space is used (if intentional)
32-61: Consider adding address format validation.The configuration keys accept any string value without validation. Consider:
- Adding validation logic in the configuration manager implementations
- Documenting the expected address format for each configuration center
- Adding format examples in the configuration key descriptions
For example:
- Zookeeper:
host1:port1,host2:port2- Nacos:
host:port- Apollo:
http(s)://host:porttest/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/NacosDynamicConfigTest.java (1)
65-77:⚠️ Potential issueAdd validation for configuration changes in Nacos.
The test only verifies the local state changes but doesn't validate if the configurations were actually persisted in Nacos.
Add verification of the actual Nacos state:
// 测试配置新增 String configValue = "timeout=5000"; nacosConfigListener.innerReceive(consumerConfig.getInterfaceId(), consumerConfig.getAppName(), configValue); + // Verify configuration in Nacos + String actualConfig = configService.getConfig(consumerConfig.getInterfaceId(), consumerConfig.getAppName(), 3000); + Assert.assertEquals(configValue, actualConfig); Assert.assertEquals(5000, consumerConfig.getMethodTimeout("sayHello"));test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/ZookeeperDynamicConfigTest.java (2)
1-43: LGTM! Class structure and imports are well-organized.The class follows proper structure with license header, necessary imports, and appropriate base class extension.
59-64: LGTM with existing concerns.Note: Please address the existing review comment about avoiding reflection to access private fields.
test/test-integration/src/test/java/com/alipay/sofa/rpc/test/config/ApolloDynamicConfigTest.java (1)
42-56: Previous review comments are still applicable.The test setup could be improved as per the previous review suggestions:
- Add
@Afterfor cleanup- Add error handling
- Verify initial state
test/test-integration/pom.xml (3)
204-207: LGTM!The addition of the Nacos configuration dependency aligns with the PR objectives and follows the project's dependency management practices.
264-269: Skip comment about Apollo dependency.This issue has already been addressed in a previous review comment.
270-275: LGTM!The addition of the Zookeeper configuration dependency aligns with the PR objectives and follows the project's dependency management practices.
config/config-apollo/src/main/java/com/alipay/sofa/rpc/dynamic/apollo/ApolloDynamicConfigManager.java (2)
90-93: Service configuration initialization is correctly implementedThe method
initServiceConfiguration(String service, ConfigListener listener)properly retrieves the raw configuration and notifies the listener when the configuration is available, ensuring that dynamic configurations are correctly initialized for the service.
152-160:⚠️ Potential issueHandle potential unknown
PropertyChangeTypevalues explicitlyThe
getChangeTypemethod handlesDELETEDandADDEDtypes and defaults toMODIFIEDfor all other cases. If newPropertyChangeTypevalues are introduced in the future, they will be incorrectly classified asMODIFIED.Consider updating the method to explicitly handle all possible
PropertyChangeTypevalues or throw an exception for unknown types to prevent unexpected behavior.Apply this diff to enhance robustness:
private ConfigChangeType getChangeType(ConfigChange change) { if (change.getChangeType() == PropertyChangeType.DELETED) { return ConfigChangeType.DELETED; } if (change.getChangeType() == PropertyChangeType.ADDED) { return ConfigChangeType.ADDED; } + if (change.getChangeType() == PropertyChangeType.MODIFIED) { + return ConfigChangeType.MODIFIED; + } + throw new IllegalArgumentException("Unknown change type: " + change.getChangeType()); }Likely invalid or redundant comment.
bootstrap/bootstrap-api/src/main/java/com/alipay/sofa/rpc/bootstrap/DefaultConsumerBootstrap.java (3)
166-170: [Duplicate Comment]: MissingConfigListenerfor request-level dynamic configurationThe previous issue regarding the missing
ConfigListenerfor request-level dynamic configuration remains applicable. Without setting aConfigListener, the consumer may not respond to configuration changes at the request level.
Line range hint
182-219: [Duplicate Comment]: Ensure listeners are unregistered to prevent memory leaksThe issue previously raised about unregistering listeners from the
DynamicConfigManagerin theunRefermethod is still valid. Consider implementing a mechanism to remove the listener when the consumer is unreferenced to prevent potential memory leaks.
460-467: [Duplicate Comment]: Consider makingdynamicConfigKeysa static final fieldThe prior suggestion to make
dynamicConfigKeysa static final field remains relevant. This would improve performance and reduce memory usage.
| ConsumerConfig<HelloService> consumerConfig = new ConsumerConfig<HelloService>() | ||
| .setInterfaceId(HelloService.class.getName()) | ||
| .setProtocol("bolt") | ||
| .setDirectUrl("bolt://127.0.0.1:12200") | ||
| .setConnectTimeout(10 * 1000) | ||
| .setApplication(clientApplication); | ||
|
|
||
| consumerConfig.refer(); |
There was a problem hiding this comment.
Add timeout validation for consumer initialization.
The consumer configuration and reference operation could potentially hang if the Nacos server is not available.
Add timeout handling:
ConsumerConfig<HelloService> consumerConfig = new ConsumerConfig<HelloService>()
.setInterfaceId(HelloService.class.getName())
.setProtocol("bolt")
.setDirectUrl("bolt://127.0.0.1:12200")
.setConnectTimeout(10 * 1000)
.setApplication(clientApplication);
+ long startTime = System.currentTimeMillis();
consumerConfig.refer();
+ Assert.assertTrue("Consumer initialization took too long",
+ System.currentTimeMillis() - startTime < 30000);📝 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.
| ConsumerConfig<HelloService> consumerConfig = new ConsumerConfig<HelloService>() | |
| .setInterfaceId(HelloService.class.getName()) | |
| .setProtocol("bolt") | |
| .setDirectUrl("bolt://127.0.0.1:12200") | |
| .setConnectTimeout(10 * 1000) | |
| .setApplication(clientApplication); | |
| consumerConfig.refer(); | |
| ConsumerConfig<HelloService> consumerConfig = new ConsumerConfig<HelloService>() | |
| .setInterfaceId(HelloService.class.getName()) | |
| .setProtocol("bolt") | |
| .setDirectUrl("bolt://127.0.0.1:12200") | |
| .setConnectTimeout(10 * 1000) | |
| .setApplication(clientApplication); | |
| long startTime = System.currentTimeMillis(); | |
| consumerConfig.refer(); | |
| Assert.assertTrue("Consumer initialization took too long", | |
| System.currentTimeMillis() - startTime < 30000); |
| System.clearProperty(DynamicConfigKeys.CENTER_ADDRESS.getKey()); | ||
| } |
There was a problem hiding this comment.
Ensure proper cleanup of system properties.
The cleanup should restore the original property value if it existed.
- System.clearProperty(DynamicConfigKeys.CENTER_ADDRESS.getKey());
+ } finally {
+ if (originalValue != null) {
+ System.setProperty(DynamicConfigKeys.CENTER_ADDRESS.getKey(), originalValue);
+ } else {
+ System.clearProperty(DynamicConfigKeys.CENTER_ADDRESS.getKey());
+ }
+ }Committable suggestion was skipped due to low confidence.
| public void testNacosDynamicConfig() throws Exception { | ||
| System.setProperty(DynamicConfigKeys.CENTER_ADDRESS.getKey(), | ||
| "nacos://127.0.0.1:8848?username=nacos&password=nacos"); | ||
| ApplicationConfig clientApplication = new ApplicationConfig(); |
There was a problem hiding this comment.
Add error handling for system property configuration.
The system property setup should be wrapped in a try-finally block to ensure cleanup even if the test fails.
@Test
public void testNacosDynamicConfig() throws Exception {
- System.setProperty(DynamicConfigKeys.CENTER_ADDRESS.getKey(),
- "nacos://127.0.0.1:8848?username=nacos&password=nacos");
+ String originalValue = System.getProperty(DynamicConfigKeys.CENTER_ADDRESS.getKey());
+ try {
+ System.setProperty(DynamicConfigKeys.CENTER_ADDRESS.getKey(),
+ "nacos://127.0.0.1:8848?username=nacos&password=nacos");Committable suggestion was skipped due to low confidence.
| @Test | ||
| public void testZookeeperDynamicConfig() throws Exception { | ||
| System.setProperty(DynamicConfigKeys.CENTER_ADDRESS.getKey(), "zookeeper://127.0.0.1:2181"); | ||
| ApplicationConfig clientApplication = new ApplicationConfig(); | ||
| clientApplication.setAppName("demo"); | ||
|
|
||
| ConsumerConfig<HelloService> consumerConfig = new ConsumerConfig<HelloService>() | ||
| .setInterfaceId(HelloService.class.getName()) | ||
| .setProtocol("bolt") | ||
| .setDirectUrl("bolt://127.0.0.1:12200") | ||
| .setConnectTimeout(10 * 1000) | ||
| .setApplication(clientApplication); | ||
|
|
||
| consumerConfig.refer(); | ||
|
|
There was a problem hiding this comment.
Add proper test cleanup using @after or try-with-resources.
The test sets system properties and creates resources but doesn't ensure cleanup in case of test failure.
Add the following cleanup method:
@After
public void cleanup() {
System.clearProperty(DynamicConfigKeys.CENTER_ADDRESS.getKey());
if (consumerConfig != null) {
consumerConfig.unRefer();
}
}| // 新增或修改配置节点 | ||
| if (zkClient.checkExists().forPath("/config/demo/com.alipay.sofa.rpc.test.HelloService") == null) { | ||
| zkClient.create() | ||
| .creatingParentsIfNeeded() | ||
| .withMode(CreateMode.PERSISTENT) | ||
| .forPath("/config/demo/com.alipay.sofa.rpc.test.HelloService", "timeout=5000".getBytes()); | ||
| } else { | ||
| zkClient.setData().forPath("/config/demo/com.alipay.sofa.rpc.test.HelloService", | ||
| "timeout=5000".getBytes()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve code maintainability and error handling.
Consider the following improvements:
- Extract the Zookeeper path to a constant to avoid duplication and potential inconsistencies
- Use try-with-resources for better error handling
Here's the suggested implementation:
private static final String CONFIG_PATH = "/config/demo/com.alipay.sofa.rpc.test.HelloService";
// Usage
if (zkClient.checkExists().forPath(CONFIG_PATH) == null) {
try (var transaction = zkClient.inTransaction()) {
transaction.create()
.creatingParentsIfNeeded()
.withMode(CreateMode.PERSISTENT)
.forPath(CONFIG_PATH, "timeout=5000".getBytes())
.commit();
}
} else {
try (var transaction = zkClient.inTransaction()) {
transaction.setData()
.forPath(CONFIG_PATH, "timeout=5000".getBytes())
.commit();
}
}| // 测试配置新增 | ||
| String configValue = "timeout=5000\n"; | ||
| ConfigChange configChange = new ConfigChange("application", consumerConfig.getInterfaceId(), null, configValue, PropertyChangeType.ADDED); | ||
| Map<String, ConfigChange> changes= new HashMap<>(); | ||
| changes.put(configChange.getPropertyName(), configChange); | ||
| ConfigChangeEvent event = new ConfigChangeEvent("application",changes); | ||
| apolloConfigListener.onChange(event); | ||
| Assert.assertEquals(5000, consumerConfig.getMethodTimeout("sayHello")); | ||
|
|
||
| // 测试配置修改 | ||
| String oldValue = configValue; | ||
| configValue = "timeout=5000\n.sayHello.timeout=6000"; | ||
| configChange = new ConfigChange("application", consumerConfig.getInterfaceId(), oldValue, configValue, PropertyChangeType.MODIFIED); | ||
| changes= new HashMap<>(); | ||
| changes.put(configChange.getPropertyName(), configChange); | ||
| event = new ConfigChangeEvent("application",changes); | ||
| apolloConfigListener.onChange(event); | ||
| Assert.assertEquals(6000, consumerConfig.getMethodTimeout("sayHello")); | ||
|
|
||
| // 测试配置删除 | ||
| configChange = new ConfigChange("application", consumerConfig.getInterfaceId(), configValue, null, PropertyChangeType.DELETED); | ||
| changes= new HashMap<>(); | ||
| changes.put(configChange.getPropertyName(), configChange); | ||
| event = new ConfigChangeEvent("application",changes); | ||
| apolloConfigListener.onChange(event); | ||
| Assert.assertEquals(-1, consumerConfig.getMethodTimeout("sayHello")); | ||
|
|
||
| System.clearProperty(DynamicConfigKeys.CENTER_ADDRESS.getKey()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test scenarios and assertions.
The test scenarios could be more robust:
- Extract configuration values as constants
- Add assertions for configuration change events
- Consider potential race conditions
- Add negative test cases
Example improvement:
+ private static final String INITIAL_TIMEOUT = "timeout=5000\n";
+ private static final String MODIFIED_TIMEOUT = "timeout=5000\n.sayHello.timeout=6000";
+ private static final int EXPECTED_INITIAL_TIMEOUT = 5000;
+ private static final int EXPECTED_METHOD_TIMEOUT = 6000;
+ private static final int EXPECTED_DEFAULT_TIMEOUT = -1;
// Test configuration addition
- String configValue = "timeout=5000\n";
+ String configValue = INITIAL_TIMEOUT;
ConfigChange configChange = new ConfigChange("application", consumerConfig.getInterfaceId(),
null, configValue, PropertyChangeType.ADDED);
// ... existing code ...
+ // Verify the change was processed
+ Thread.sleep(100); // Allow time for event processing
Assert.assertEquals(5000, consumerConfig.getMethodTimeout("sayHello"));
+ Assert.assertTrue("Configuration should be marked as changed",
+ consumerConfig.isDynamicConfigurationUpdated());
// Add negative test
+ try {
+ configChange = new ConfigChange("application", consumerConfig.getInterfaceId(),
+ null, "invalid=config", PropertyChangeType.ADDED);
+ apolloConfigListener.onChange(event);
+ Assert.fail("Should throw exception for invalid configuration");
+ } catch (IllegalArgumentException expected) {
+ // Expected exception
+ }Committable suggestion was skipped due to low confidence.
| protected ApolloDynamicConfigManager(String appName, String remainUrl) { | ||
| super(appName, remainUrl); | ||
| System.setProperty(APOLLO_APPID_KEY, appName); | ||
| System.setProperty(APOLLO_ADDR_KEY, APOLLO_PROTOCOL_PREFIX + getAddress()); |
There was a problem hiding this comment.
Add a check for empty address before setting apollo.meta
In the constructor, getAddress() may return an empty string. Appending it to APOLLO_PROTOCOL_PREFIX results in apollo.meta being set to "http://", which is likely invalid and could lead to misconfigurations.
Consider adding a check to ensure getAddress() is not blank before setting the system property, similar to the check in the other constructor.
Apply this diff to add the check:
protected ApolloDynamicConfigManager(String appName, String remainUrl) {
super(appName, remainUrl);
System.setProperty(APOLLO_APPID_KEY, appName);
+ if (StringUtils.isNotBlank(getAddress())) {
System.setProperty(APOLLO_ADDR_KEY, APOLLO_PROTOCOL_PREFIX + getAddress());
+ }
Map params = getParams();
if (params != null && params.containsKey("cluster")) {
String clusterValue = (String) params.get("cluster");
System.setProperty(APOLLO_CLUSTER_KEY, clusterValue);
}
config = ConfigService.getAppConfig();
}📝 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.
| System.setProperty(APOLLO_ADDR_KEY, APOLLO_PROTOCOL_PREFIX + getAddress()); | |
| protected ApolloDynamicConfigManager(String appName, String remainUrl) { | |
| super(appName, remainUrl); | |
| System.setProperty(APOLLO_APPID_KEY, appName); | |
| if (StringUtils.isNotBlank(getAddress())) { | |
| System.setProperty(APOLLO_ADDR_KEY, APOLLO_PROTOCOL_PREFIX + getAddress()); | |
| } | |
| Map params = getParams(); | |
| if (params != null && params.containsKey("cluster")) { | |
| String clusterValue = (String) params.get("cluster"); | |
| System.setProperty(APOLLO_CLUSTER_KEY, clusterValue); | |
| } | |
| config = ConfigService.getAppConfig(); | |
| } |
| String configValue = configService.getConfig( | ||
| DynamicConfigKeyHelper.buildProviderServiceProKey(service, key), | ||
| group, DEFAULT_TIMEOUT); | ||
| return configValue != null ? configValue : DynamicHelper.DEFAULT_DYNAMIC_VALUE; | ||
| } catch (NacosException e) { | ||
| return DynamicHelper.DEFAULT_DYNAMIC_VALUE; | ||
| } |
There was a problem hiding this comment.
Log exceptions in catch blocks for better debugging
In the methods getProviderServiceProperty, getConsumerServiceProperty, getProviderMethodProperty, and getConsumerMethodProperty, exceptions are caught but not logged. Logging exceptions can aid in debugging and provide visibility into issues when they occur.
Apply this diff to log the exceptions:
@@ -119,6 +119,7 @@ public String getProviderServiceProperty(String service, String key) {
} catch (NacosException e) {
+ LOGGER.error("Failed to get provider service property for service: {}, key: {}.", service, key, e);
return DynamicHelper.DEFAULT_DYNAMIC_VALUE;
}
}
@@ -133,6 +134,7 @@ public String getConsumerServiceProperty(String service, String key) {
} catch (NacosException e) {
+ LOGGER.error("Failed to get consumer service property for service: {}, key: {}.", service, key, e);
return DynamicHelper.DEFAULT_DYNAMIC_VALUE;
}
}
@@ -145,6 +147,7 @@ public String getProviderMethodProperty(String service, String method, String ke
} catch (NacosException e) {
+ LOGGER.error("Failed to get provider method property for service: {}, method: {}, key: {}.", service, method, key, e);
return DynamicHelper.DEFAULT_DYNAMIC_VALUE;
}
}
@@ -157,6 +160,7 @@ public String getConsumerMethodProperty(String service, String method, String ke
} catch (NacosException e) {
+ LOGGER.error("Failed to get consumer method property for service: {}, method: {}, key: {}.", service, method, key, e);
return DynamicHelper.DEFAULT_DYNAMIC_VALUE;
}
}Also applies to: 127-133, 139-145, 151-157
| private NacosConfigListener createTargetListener(String key) { | ||
| NacosConfigListener configListener = new NacosConfigListener(); | ||
| configListener.fillContext(key, group); | ||
| return configListener; | ||
| } |
There was a problem hiding this comment.
Ensure NacosConfigListener has the fillContext method
In the createTargetListener method, the fillContext method is called on NacosConfigListener. Ensure that NacosConfigListener implements or inherits the fillContext method to prevent potential compilation errors.
If AbstractSharedListener doesn't provide this method, consider adding it to NacosConfigListener:
public class NacosConfigListener extends AbstractSharedListener {
private String dataId;
private String group;
public void fillContext(String dataId, String group) {
this.dataId = dataId;
this.group = group;
}
// Existing code...
}| private Map<String, String> newValueMap = new HashMap<>(); | ||
|
|
||
| // 动态配置项 | ||
| private final Set<String> dynamicConfigKeys = new HashSet<>(); | ||
|
|
||
| ConsumerAttributeListener() { | ||
| dynamicConfigKeys.add(RpcConstants.CONFIG_KEY_TIMEOUT); | ||
| dynamicConfigKeys.add(RpcConstants.CONFIG_KEY_RETRIES); | ||
| dynamicConfigKeys.add(RpcConstants.CONFIG_KEY_LOADBALANCER); | ||
| } | ||
|
|
||
| @Override | ||
| public void process(ConfigChangedEvent event) { | ||
| // 清除上次的赋值，并保留赋值过的key | ||
| newValueMap.replaceAll((k, v) -> ""); | ||
| if (!event.getChangeType().equals(ConfigChangeType.DELETED)) { | ||
| // ADDED or MODIFIED | ||
| parseConfigurationLines(event.getContent()); | ||
| } | ||
| attrUpdated(newValueMap); | ||
| } | ||
|
|
||
| private void parseConfigurationLines(String content) { | ||
| String[] lines = content.split("\n"); | ||
| for (String line : lines) { | ||
| String[] keyValue = line.split("=", 2); | ||
| if (keyValue.length == 2) { | ||
| String key = keyValue[0].trim(); | ||
| String value = keyValue[1].trim(); | ||
| String tempKey = key.lastIndexOf(".") == -1 ? key : key.substring(key.lastIndexOf(".") + 1); | ||
| if (dynamicConfigKeys.contains(tempKey)) { | ||
| newValueMap.put(key, value); | ||
| } | ||
| } else { | ||
| LOGGER.warn("Malformed configuration line: {}", line); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure thread safety of newValueMap in ConsumerAttributeListener
The newValueMap is a HashMap that is accessed and modified in the process and parseConfigurationLines methods. If process can be called concurrently by multiple threads, this could lead to race conditions and inconsistent state. Consider synchronizing the process method or using a thread-safe map implementation, such as ConcurrentHashMap, for newValueMap.
Apply one of the following changes to fix the issue:
Option 1: Use ConcurrentHashMap for newValueMap:
- private Map<String, String> newValueMap = new HashMap<>();
+ private Map<String, String> newValueMap = new ConcurrentHashMap<>();Option 2: Synchronize the process method:
- public void process(ConfigChangedEvent event) {
+ public synchronized void process(ConfigChangedEvent event) {📝 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.
| private Map<String, String> newValueMap = new HashMap<>(); | |
| // 动态配置项 | |
| private final Set<String> dynamicConfigKeys = new HashSet<>(); | |
| ConsumerAttributeListener() { | |
| dynamicConfigKeys.add(RpcConstants.CONFIG_KEY_TIMEOUT); | |
| dynamicConfigKeys.add(RpcConstants.CONFIG_KEY_RETRIES); | |
| dynamicConfigKeys.add(RpcConstants.CONFIG_KEY_LOADBALANCER); | |
| } | |
| @Override | |
| public void process(ConfigChangedEvent event) { | |
| // 清除上次的赋值，并保留赋值过的key | |
| newValueMap.replaceAll((k, v) -> ""); | |
| if (!event.getChangeType().equals(ConfigChangeType.DELETED)) { | |
| // ADDED or MODIFIED | |
| parseConfigurationLines(event.getContent()); | |
| } | |
| attrUpdated(newValueMap); | |
| } | |
| private void parseConfigurationLines(String content) { | |
| String[] lines = content.split("\n"); | |
| for (String line : lines) { | |
| String[] keyValue = line.split("=", 2); | |
| if (keyValue.length == 2) { | |
| String key = keyValue[0].trim(); | |
| String value = keyValue[1].trim(); | |
| String tempKey = key.lastIndexOf(".") == -1 ? key : key.substring(key.lastIndexOf(".") + 1); | |
| if (dynamicConfigKeys.contains(tempKey)) { | |
| newValueMap.put(key, value); | |
| } | |
| } else { | |
| LOGGER.warn("Malformed configuration line: {}", line); | |
| } | |
| } | |
| } | |
| private Map<String, String> newValueMap = new ConcurrentHashMap<>(); | |
| // 动态配置项 | |
| private final Set<String> dynamicConfigKeys = new HashSet<>(); | |
| ConsumerAttributeListener() { | |
| dynamicConfigKeys.add(RpcConstants.CONFIG_KEY_TIMEOUT); | |
| dynamicConfigKeys.add(RpcConstants.CONFIG_KEY_RETRIES); | |
| dynamicConfigKeys.add(RpcConstants.CONFIG_KEY_LOADBALANCER); | |
| } | |
| @Override | |
| public void process(ConfigChangedEvent event) { | |
| // 清除上次的赋值，并保留赋值过的key | |
| newValueMap.replaceAll((k, v) -> ""); | |
| if (!event.getChangeType().equals(ConfigChangeType.DELETED)) { | |
| // ADDED or MODIFIED | |
| parseConfigurationLines(event.getContent()); | |
| } | |
| attrUpdated(newValueMap); | |
| } | |
| private void parseConfigurationLines(String content) { | |
| String[] lines = content.split("\n"); | |
| for (String line : lines) { | |
| String[] keyValue = line.split("=", 2); | |
| if (keyValue.length == 2) { | |
| String key = keyValue[0].trim(); | |
| String value = keyValue[1].trim(); | |
| String tempKey = key.lastIndexOf(".") == -1 ? key : key.substring(key.lastIndexOf(".") + 1); | |
| if (dynamicConfigKeys.contains(tempKey)) { | |
| newValueMap.put(key, value); | |
| } | |
| } else { | |
| LOGGER.warn("Malformed configuration line: {}", line); | |
| } | |
| } | |
| } |
This is a Work In Progress pull request for issue #1428.
Modification:
Integrate Zookeeper as configuration center with ZookeeperDynamicConfigManager.
Integrate Nacos as configuration center with NacosDynamicConfigManager.
Support dynamic config at the interface level.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests
Chores