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 project by incorporating the 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); | |
| } |
| 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?
| 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
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
New Features
Tests
Chores