[OSPP]Support Kubernetes ConfigMap for Apollo java, golang client #79
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
WalkthroughThe pull request introduces significant enhancements to the Apollo client, primarily focused on integrating Kubernetes support. A new dependency for the Kubernetes Java client is added, along with the creation of classes and methods to manage configuration data through Kubernetes ConfigMaps. Key additions include the Changes
Possibly related PRs
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: 0
Outside diff range and nitpick comments (7)
apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java (5)
38-48: Improve exception handling.Consider making the following improvements to the exception handling:
- Make the exception message more specific by including the actual exception message or cause.
- Log the error before throwing the exception to aid in debugging.
Apply this diff to implement the suggestions:
@PostConstruct public void initClient() { try { client = Config.defaultClient(); Configuration.setDefaultApiClient(client); coreV1Api = new CoreV1Api(client); } catch (Exception e) { - throw new RuntimeException("k8s client init failed"); + String errorMessage = "Failed to initialize Kubernetes client: " + e.getMessage(); + log.error(errorMessage, e); + throw new RuntimeException(errorMessage, e); } }
50-63: Improve error handling and add Javadoc comments.Consider making the following improvements to the method:
- Throw a custom exception (e.g.,
KubernetesClientException) in case of an error instead of returningnull. This will make it clear to the caller that an error occurred and needs to be handled.- Add Javadoc comments to document the method, including the parameters, return value, and exceptions thrown.
Apply this diff to implement the suggestions:
+/** + * Creates a Kubernetes ConfigMap. + * + * @param configMapNamespace the namespace of the ConfigMap + * @param name the name of the ConfigMap + * @param data the data to be stored in the ConfigMap + * @return the name of the created ConfigMap + * @throws KubernetesClientException if an error occurs while creating the ConfigMap + */ public String createConfigMap(String configMapNamespace, String name, Map<String, String> data) { if (configMapNamespace == null || configMapNamespace == "" || name == null || name == "") { log.error("create config map failed due to null parameter"); - return null; + throw new KubernetesClientException("ConfigMap namespace and name cannot be null or empty"); } V1ConfigMap configMap = new V1ConfigMap().metadata(new V1ObjectMeta().name(name).namespace(configMapNamespace)).data(data); try { coreV1Api.createNamespacedConfigMap(configMapNamespace, configMap, null, null, null,null); return name; } catch (Exception e) { log.error("create config map failed", e); - return null; + throw new KubernetesClientException("Failed to create ConfigMap: " + e.getMessage(), e); } }
65-87: Improve error handling, use constants for log messages, and add Javadoc comments.Consider making the following improvements to the method:
- Throw a custom exception (e.g.,
KubernetesClientException) in case of an error or if the data is not found instead of returningnull. This will make it clear to the caller that an error occurred and needs to be handled.- Use constants for the log messages and avoid string concatenation to improve performance and readability.
- Add Javadoc comments to document the method, including the parameters, return value, and exceptions thrown.
Apply this diff to implement the suggestions:
+private static final String ERROR_PARAMETER_CANNOT_BE_EMPTY = "参数不能为空"; +private static final String ERROR_CONFIG_MAP_NOT_FOUND = "ConfigMap不存在"; +private static final String ERROR_KEY_NOT_FOUND_IN_CONFIG_MAP = "在ConfigMap中未找到指定的键: %s"; +/** + * Loads data from a Kubernetes ConfigMap. + * + * @param configMapNamespace the namespace of the ConfigMap + * @param name the name of the ConfigMap and the key to retrieve the data from + * @return the data associated with the name key in the ConfigMap + * @throws KubernetesClientException if an error occurs while loading the data or if the data is not found + */ public String loadFromConfigMap(String configMapNamespace, String name) { if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty() || name == null || name.isEmpty()) { - log.error("参数不能为空"); + log.error(ERROR_PARAMETER_CANNOT_BE_EMPTY); - return null; + throw new KubernetesClientException(ERROR_PARAMETER_CANNOT_BE_EMPTY); } try { V1ConfigMap configMap = coreV1Api.readNamespacedConfigMap(name, configMapNamespace, null); if (configMap == null) { - log.error("ConfigMap不存在"); + log.error(ERROR_CONFIG_MAP_NOT_FOUND); - return null; + throw new KubernetesClientException(ERROR_CONFIG_MAP_NOT_FOUND); } Map<String, String> data = configMap.getData(); if (data != null && data.containsKey(name)) { return data.get(name); } else { - log.error("在ConfigMap中未找到指定的键: " + name); + log.error(String.format(ERROR_KEY_NOT_FOUND_IN_CONFIG_MAP, name)); - return null; + throw new KubernetesClientException(String.format(ERROR_KEY_NOT_FOUND_IN_CONFIG_MAP, name)); } } catch (Exception e) { log.error("get config map failed", e); - return null; + throw new KubernetesClientException("Failed to load data from ConfigMap: " + e.getMessage(), e); } }
89-109: Improve error handling, use constants for log messages, and add Javadoc comments.Consider making the following improvements to the method:
- Throw a custom exception (e.g.,
KubernetesClientException) in case of an error or if the value is not found instead of returningnull. This will make it clear to the caller that an error occurred and needs to be handled.- Use constants for the log messages and avoid string concatenation to improve performance and readability.
- Add Javadoc comments to document the method, including the parameters, return value, and exceptions thrown.
Apply this diff to implement the suggestions:
+private static final String ERROR_PARAMETER_CANNOT_BE_EMPTY = "参数不能为空"; +private static final String ERROR_CONFIG_MAP_NOT_FOUND_OR_NO_DATA = "ConfigMap不存在或没有数据"; +private static final String ERROR_KEY_NOT_FOUND_IN_CONFIG_MAP = "在ConfigMap中未找到指定的键: %s"; +/** + * Retrieves a specific value from a Kubernetes ConfigMap. + * + * @param configMapNamespace the namespace of the ConfigMap + * @param name the name of the ConfigMap + * @param key the key to retrieve the value from + * @return the value associated with the key in the ConfigMap + * @throws KubernetesClientException if an error occurs while retrieving the value or if the value is not found + */ public String getValueFromConfigMap(String configMapNamespace, String name, String key) { if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty() || key == null || key.isEmpty()) { - log.error("参数不能为空"); + log.error(ERROR_PARAMETER_CANNOT_BE_EMPTY); - return null; + throw new KubernetesClientException(ERROR_PARAMETER_CANNOT_BE_EMPTY); } try { V1ConfigMap configMap = coreV1Api.readNamespacedConfigMap(name, configMapNamespace, null); if (configMap == null || configMap.getData() == null) { - log.error("ConfigMap不存在或没有数据"); + log.error(ERROR_CONFIG_MAP_NOT_FOUND_OR_NO_DATA); - return null; + throw new KubernetesClientException(ERROR_CONFIG_MAP_NOT_FOUND_OR_NO_DATA); } if (!configMap.getData().containsKey(key)) { - log.error("在ConfigMap中未找到指定的键: " + key); + log.error(String.format(ERROR_KEY_NOT_FOUND_IN_CONFIG_MAP, key)); - return null; + throw new KubernetesClientException(String.format(ERROR_KEY_NOT_FOUND_IN_CONFIG_MAP, key)); } return configMap.getData().get(key); } catch (Exception e) { log.error("get config map failed", e); - return null; + throw new KubernetesClientException("Failed to retrieve value from ConfigMap: " + e.getMessage(), e); } }
111-124: Improve error handling, use constants for log messages, and add Javadoc comments.Consider making the following improvements to the method:
- Throw a custom exception (e.g.,
KubernetesClientException) in case of an error instead of returningnull. This will make it clear to the caller that an error occurred and needs to be handled.- Use constants for the log messages and avoid string concatenation to improve performance and readability.
- Add Javadoc comments to document the method, including the parameters, return value, and exceptions thrown.
Apply this diff to implement the suggestions:
+private static final String ERROR_PARAMETER_CANNOT_BE_EMPTY = "参数不能为空"; +/** + * Updates a Kubernetes ConfigMap. + * + * @param configMapNamespace the namespace of the ConfigMap + * @param name the name of the ConfigMap + * @param data the updated data to be stored in the ConfigMap + * @return the name of the updated ConfigMap + * @throws KubernetesClientException if an error occurs while updating the ConfigMap + */ public String updateConfigMap(String configMapNamespace, String name, Map<String, String> data) { if (configMapNamespace == null || configMapName </blockquote></details> <details> <summary>apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java (1)</summary><blockquote> `25-25`: **Avoid wildcard imports.** Importing all classes from a package using the wildcard `*` is generally discouraged as it can lead to naming conflicts and make the code less readable. Please explicitly import only the required classes from the `com.ctrip.framework.apollo.internals` package. </blockquote></details> <details> <summary>apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java (1)</summary><blockquote> `239-255`: **LGTM, but consider adding a TODO comment for the upstream data recovery logic.** The method correctly attempts to synchronize from the upstream repository and updates the `configMapProperties` if successful. It also catches any exceptions that occur during the synchronization process and logs an error message. Consider adding a TODO comment to indicate that the logic for recovering data from the upstream needs to be implemented: ```java // TODO: Implement logic to recover data from the upstream
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- apollo-client/pom.xml (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/enums/ConfigSourceType.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java (4 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (6 hunks)
- apollo-client/src/main/resources/META-INF/additional-spring-configuration-metadata.json (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManagerTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java (1 hunks)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java (2 hunks)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ConfigConsts.java (1 hunks)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/Env.java (1 hunks)
Additional comments not posted (37)
apollo-core/src/main/java/com/ctrip/framework/apollo/core/ConfigConsts.java (1)
27-27: LGTM!The new constant
KUBERNETES_CACHE_CONFIG_MAP_NAMESPACE_DEFAULTis declared correctly with an appropriate name and value. The addition of this constant aligns with the PR objective of supporting Kubernetes ConfigMap for the Apollo client.apollo-client/src/main/java/com/ctrip/framework/apollo/enums/ConfigSourceType.java (1)
25-28: LGTM!The addition of the
CONFIGMAPentry to theConfigSourceTypeenum is a valid change that aligns with the PR objective of supporting Kubernetes ConfigMap as a new persistent storage mechanism for the Apollo client.This change enhances the flexibility of the configuration management system by providing an additional source type for loading configurations from Kubernetes ConfigMaps, enabling better integration with Kubernetes environments.
apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/Env.java (1)
38-38: LGTM!The addition of the
KUBERNETESenum value to theEnvenum is a valid change that aligns with the PR objective of supporting Kubernetes ConfigMap. This is a non-breaking change that extends the enum to accommodate a new environment type without altering any existing logic or control flow. The code change is small, self-contained, and does not require any additional unit tests.apollo-client/pom.xml (1)
101-105: Dependency addition aligns with the PR objective. Verify compatibility.The addition of the Kubernetes Java client library dependency is consistent with the goal of integrating Kubernetes support into the Apollo client. It enables the project to interact with Kubernetes resources programmatically.
Please ensure that version
18.0.0is the latest stable release and is compatible with the existing dependencies in the project. You can run the following script to check for any newer versions and potential compatibility issues:apollo-client/src/main/resources/META-INF/additional-spring-configuration-metadata.json (1)
79-92: LGTM!The new configuration properties for Kubernetes ConfigMap caching are correctly defined with appropriate types, default values, and descriptions. They follow the existing naming convention and are added in the correct alphabetical order within the
propertiesarray. ThesourceTypeis correctly set, and the default values are appropriate for the respective types and use cases. The descriptions provide clear information about the purpose and usage of the properties.Great job on enhancing the configuration options for managing caching behavior in Kubernetes environments!
apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java (2)
76-84: LGTM!The new constants
APOLLO_CONFIGMAP_NAMESPACEandAPOLLO_CONFIGMAP_NAMESPACE_ENVIRONMENT_VARIABLESare well-named, follow the existing naming convention, and have appropriate access modifiers. The comments provide a clear explanation of their purpose related to Kubernetes ConfigMap namespace configuration.
170-178: LGTM!The new constants
APOLLO_KUBERNETES_CACHE_ENABLEandAPOLLO_KUBERNETES_CACHE_ENABLE_ENVIRONMENT_VARIABLESare well-named, follow the existing naming convention, and have appropriate access modifiers. The comments provide a clear explanation of their purpose related to enabling property names cache in Kubernetes environments.apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java (3)
105-108: LGTM!The changes introduce a new configuration option to enable Kubernetes ConfigMap caching and the logic to create the appropriate config repository based on the cache settings is implemented correctly. The TODO comment is a valid suggestion for future enhancement to allow both local and ConfigMap caching simultaneously.
130-143: LGTM!The new
createConfigMapConfigRepositorymethod is implemented correctly to enable creating a Kubernetes ConfigMap repository for a given namespace. The logic to create the repository based on the Kubernetes mode and using the local config repository as fallback when not in Kubernetes mode is a good approach for backward compatibility.
144-144: LGTM!The additional blank line after the
createConfigMapConfigRepositorymethod improves code readability.apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java (2)
246-255: LGTM!The test method
testConfigMapNamespaceWithSystemPropertyis well-structured and correctly verifies the behavior of retrieving the configuration map namespace from a system property. The test sets the system property, creates an instance ofConfigUtil, and asserts that the value returned bygetConfigMapNamespace()matches the expected value.
257-262: LGTM!The test method
testConfigMapNamespaceWithDefaultis well-structured and correctly verifies the behavior of retrieving the default configuration map namespace when no system property is set. The test creates an instance ofConfigUtiland asserts that the value returned bygetConfigMapNamespace()matches the default value defined inConfigConsts.KUBERNETES_CACHE_CONFIG_MAP_NAMESPACE_DEFAULT.apollo-client/src/test/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManagerTest.java (8)
51-69: LGTM!The test method
testCreateConfigMapSuccessis correctly implemented and covers the successful scenario for creating a ConfigMap. It mocks theCoreV1Apiand verifies that thecreateNamespacedConfigMapmethod is called with the correct arguments.
75-88: LGTM!The test method
testCreateConfigMapEmptyNamespaceis correctly implemented and covers the scenario of creating a ConfigMap with an empty namespace. It verifies that thecreateNamespacedConfigMapmethod is never called and the result is null.
93-107: LGTM!The test method
testCreateConfigMapEmptyNameis correctly implemented and covers the scenario of creating a ConfigMap with an empty name. It verifies that thecreateNamespacedConfigMapmethod is never called and the result is null.
112-125: LGTM!The test method
testCreateConfigMapNullDatais correctly implemented and covers the scenario of creating a ConfigMap with null data. It verifies that thecreateNamespacedConfigMapmethod is called once and the result is the same as the input name.
130-144: LGTM!The test method
testLoadFromConfigMapSuccessis correctly implemented and covers the successful scenario for loading data from a ConfigMap. It mocks theCoreV1Apiand verifies that thereadNamespacedConfigMapmethod is called with the correct arguments and the result is the expected value.
149-161: LGTM!The test method
testLoadFromConfigMapFailureis correctly implemented and covers the scenario of loading data from a ConfigMap when an exception is thrown. It mocks theCoreV1Apito throw anApiExceptionand verifies that the result is null.
166-178: LGTM!The test method
testLoadFromConfigMapConfigMapNotFoundis correctly implemented and covers the scenario of loading data from a ConfigMap when the ConfigMap is not found. It mocks theCoreV1Apito return null and verifies that the result is null.
183-199: LGTM!The test method
testGetValueFromConfigMapReturnsValueis correctly implemented and covers the scenario of getting a value from a ConfigMap when the key exists. It mocks theCoreV1Apito return a ConfigMap with the expected key-value pair and verifies that the result is the expected value.apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java (11)
76-78: LGTM!The constructor correctly delegates to another constructor with an additional null parameter.
80-91: LGTM!The constructor correctly initializes the necessary fields and calls the appropriate setter methods.
93-101: LGTM!The method correctly sets the
configMapKeyfield based on the providedclusterandnamespaceparameters.
103-109: LGTM!The method correctly sets the
configMapNamefield, checks its validity, and performs synchronization if necessary.
111-133: LGTM!The method correctly checks the validity of the
configMapName, creates the ConfigMap if it doesn't exist, and uses a transaction to track the creation.
135-143: LGTM!The method correctly returns the
configMapProperties, performs synchronization if necessary, and creates a newPropertiesinstance to avoid modifying the originalconfigMapProperties.
150-161: LGTM!The method correctly sets the
upstreamfield, removes the current instance as a listener from the previousupstream, and adds it as a listener to the newupstream.
172-200: LGTM!The method correctly attempts to synchronize from the upstream repository and falls back to loading the configuration from the Kubernetes ConfigMap if the upstream synchronization fails. It uses a transaction to track the loading process and throws an exception if the loaded
configMapPropertiesis null.
204-237: LGTM!The method correctly retrieves the configuration from the Kubernetes ConfigMap, decodes the Base64-encoded JSON string, converts it to a
Mapusing Gson, and copies the key-value pairs to a newPropertiesinstance. It also handles exceptions and throws anApolloConfigExceptionif an error occurs during the loading process.
263-271: LGTM!The method correctly updates the
configMapPropertieswith the new properties and fires a repository change event. It also avoids unnecessary updates if thenewPropertiesare equal to the currentconfigMapProperties.
282-310: LGTM!The method correctly converts the
propertiesto a Base64-encoded JSON string, creates aMapwith theappIdas the key and the encoded JSON string as the value, and updates the Kubernetes ConfigMap using thekubernetesManager. It also uses a transaction to track the update process and logs an error if an exception occurs.apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (6)
371-379: LGTM!The
getConfigMapNamespacemethod is well-structured and provides a clear way to retrieve the Kubernetes config map namespace. It allows for customization via thegetCustomizedConfigMapNamespacemethod while falling back to a default namespace if no customization is provided.
381-397: LGTM!The
getCustomizedConfigMapNamespacemethod provides a robust way to retrieve a customized Kubernetes config map namespace. It checks multiple sources in a logical order, allowing for flexibility in how the namespace can be customized. The code is clean and easy to understand.
408-415: LGTM!The
isInKubernetesModemethod is a straightforward way to check if the application is running in a Kubernetes environment. It handles exceptions gracefully by catching and ignoring them, returning false in case of any unexpected errors. This ensures the method doesn't break the application flow.
511-513: LGTM!The
isPropertyKubernetesCacheEnabledmethod is a simple and properly named getter for thePropertyKubernetesCacheEnabledfield. It follows the convention for boolean getter methods.
537-541: LGTM!The
initPropertyKubernetesCacheEnabledmethod follows the established pattern for initializing boolean properties in theConfigUtilclass. It utilizes thegetPropertyBooleanmethod to handle the retrieval and parsing of the Kubernetes cache enable property value from various sources. The code is consistent and maintainable.
74-74: LGTM!The
PropertyKubernetesCacheEnabledfield is declared with the appropriate access modifier and naming convention. The default value of false is a sensible choice. The field serves a clear purpose in theConfigUtilclass.
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (4)
apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java (4)
76-108: Translate error messages to English for consistency.The
loadFromConfigMapmethod correctly implements the retrieval of data from a Kubernetes ConfigMap. It performs necessary validations on the input parameters and handles exceptions appropriately.However, the error messages are in Chinese, while the rest of the codebase uses English. Consider translating the error messages to English for consistency.
Apply this diff to translate the error messages:
-log.error("参数不能为空"); +log.error("Parameters cannot be null or empty"); -log.error("ConfigMap不存在"); +log.error("ConfigMap does not exist"); -log.error("在ConfigMap中未找到指定的键: " + name); +log.error("Specified key not found in ConfigMap: " + name);
110-137: Translate error messages to English for consistency.The
getValueFromConfigMapmethod correctly implements the retrieval of a specific value from a Kubernetes ConfigMap. It performs necessary validations on the input parameters and handles cases where the ConfigMap doesn't exist or the specified key is not found by returning null.However, the error messages are in Chinese, while the rest of the codebase uses English. Consider translating the error messages to English for consistency.
Apply this diff to translate the error messages:
-log.error("参数不能为空"); +log.error("Parameters cannot be null or empty"); -log.error("ConfigMap不存在或没有数据"); +log.error("ConfigMap does not exist or has no data"); -log.error("在ConfigMap中未找到指定的键: " + key); +log.error("Specified key not found in ConfigMap: " + key);
139-159: Translate error message to English for consistency.The
updateConfigMapmethod correctly implements the update of a Kubernetes ConfigMap. It performs necessary validations on the input parameters and handles the case where the update fails by returning null.However, the error message is in Chinese, while the rest of the codebase uses English. Consider translating the error message to English for consistency.
Apply this diff to translate the error message:
-log.error("参数不能为空"); +log.error("Parameters cannot be null or empty");
161-179: Translate error message to English for consistency.The
checkConfigMapExistmethod correctly implements the check for the existence of a Kubernetes ConfigMap. It performs necessary validations on the input parameters and handles the case where the ConfigMap doesn't exist or an exception occurs by returning false.However, the error message is in Chinese, while the rest of the codebase uses English. Consider translating the error message to English for consistency.
Apply this diff to translate the error message:
-log.error("参数不能为空"); +log.error("Parameters cannot be null or empty");
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepositoryTest.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java
Additional comments not posted (6)
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepositoryTest.java (4)
73-87: LGTM!The test method correctly verifies the behavior of the
syncmethod when it successfully syncs from the upstream data source.
93-105: LGTM!The test method correctly verifies the behavior of the
syncmethod when it fails to sync from the upstream data source but successfully loads from the Kubernetes ConfigMap.
126-136: LGTM!The test method correctly verifies that an
ApolloConfigExceptionis thrown when theloadFromK8sConfigMapmethod encounters an exception while loading the configuration from the Kubernetes ConfigMap.
142-152: LGTM!The test method correctly verifies that the
persistConfigMapmethod successfully persists the configuration to the Kubernetes ConfigMap by checking that theupdateConfigMapmethod is called once on theKubernetesManagerwith the expected arguments.apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java (2)
38-50: LGTM!The
initClientmethod correctly initializes the Kubernetes client using the default configuration. It also handles exceptions appropriately by logging the error and throwing aRuntimeException.
52-74: LGTM!The
createConfigMapmethod correctly implements the creation of a Kubernetes ConfigMap. It performs necessary validations on the input parameters, handles exceptions appropriately, and provides a clear Javadoc comment.
| public void testLoadFromK8sConfigMapSuccess() throws Throwable { | ||
| // arrange | ||
| when(kubernetesManager.getValueFromConfigMap(anyString(), anyString(), anyString())).thenReturn("encodedConfig"); | ||
|
|
||
| // act | ||
| Properties properties = k8sConfigMapConfigRepository.loadFromK8sConfigMap(); | ||
|
|
||
| // assert | ||
| verify(kubernetesManager, times(1)).getValueFromConfigMap(anyString(), anyString(), anyString()); | ||
| // 这里应该有更具体的断言来验证properties的内容，但由于编码和解码逻辑未给出，此处省略 | ||
| } |
There was a problem hiding this comment.
Add assertions for the loaded properties.
The test method verifies that the getValueFromConfigMap method is called once on the KubernetesManager. However, it's missing assertions to verify the content of the loaded properties.
Please add assertions to verify that the loaded properties match the expected values based on the mocked encodedConfig.
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (6)
apollo-client/src/test/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManagerTest.java (2)
221-242: LGTM with suggestion: Consider adding error scenario tests for updateConfigMap.The
testUpdateConfigMapSuccessmethod effectively tests the successful update of a ConfigMap. It correctly mocks the API response and verifies both the method call and return value.However, to improve test coverage, consider adding tests for error scenarios, such as:
- API throwing an exception
- Updating a non-existent ConfigMap
- Updating with invalid data (if applicable)
These additional tests would ensure robust error handling in the
updateConfigMapmethod.
1-278: Overall, excellent test coverage with minor improvement suggestions.The
KubernetesManagerTestclass is well-structured and provides comprehensive coverage for theKubernetesManagerclass. Key strengths include:
- Thorough testing of all main functionalities
- Inclusion of both positive and negative test cases for most methods
- Appropriate use of mocking and verification techniques
To further enhance the test suite, consider:
- Adding more error scenario tests, particularly for the
updateConfigMapmethod- Ensuring consistent error handling across all methods
- Adding edge case tests, such as testing with empty data maps or very large ConfigMaps
These additions would make an already robust test suite even more comprehensive.
apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java (4)
33-35: Initialize logger as a static final field.It's a best practice to declare the logger as
private static finalto prevent unnecessary instances and to make it class-specific.Apply this change:
-private final Logger log = LoggerFactory.getLogger(this.getClass()); +private static final Logger log = LoggerFactory.getLogger(KubernetesManager.class);
148-148: Provide a meaningfulfieldManagervalue or omit it if unnecessary.In the
updateConfigMapmethod, thefieldManagerparameter is set to"fieldManagerValue", which may not be meaningful. If not needed, it can be set tonull.Update the method call:
-coreV1Api.replaceNamespacedConfigMap(name, configMapNamespace, configMap, null, null, null, "fieldManagerValue"); +coreV1Api.replaceNamespacedConfigMap(name, configMapNamespace, configMap, null, null, null, null);Or provide a meaningful value that reflects your application's identity.
115-116: Improve error logging with all relevant parameters.In
getValueFromConfigMap, the error log message does not include thekeyparameter, which is essential for debugging.Update the log message:
-log.error("Parameters cannot be null or empty: configMapNamespace={}, name={}", configMapNamespace, name); +log.error("Parameters cannot be null or empty: configMapNamespace={}, name={}, key={}", configMapNamespace, name, key);
52-60: Enhance method documentation with parameter descriptions.The JavaDoc comments for
createConfigMapshould include descriptions for each parameter for better clarity.Update the JavaDoc:
* @param configMapNamespace the namespace of the ConfigMap * @param name the name of the ConfigMap * @param data the data to be stored in the ConfigMap * @return the name of the created ConfigMap * @throws RuntimeException if an error occurs while creating the ConfigMapEnsure all methods have complete and consistent JavaDoc comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManagerTest.java (1 hunks)
🔇 Additional comments not posted (11)
apollo-client/src/test/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManagerTest.java (8)
19-34: LGTM: Import statements are appropriate.The import statements are well-organized and include all necessary classes for Kubernetes client, JUnit, and Mockito. No unnecessary imports are present.
36-45: LGTM: Class setup and mock objects are well-defined.The test class is properly set up with
MockitoJUnitRunnerand appropriate mock objects. The use of@InjectMocksforKubernetesManageris correct, allowing for dependency injection of the mockedCoreV1ApiandApiClient.
47-69: LGTM: testCreateConfigMapSuccess is well-implemented.This test method effectively verifies the successful creation of a ConfigMap. It properly mocks the Kubernetes API call, sets up the expected behavior, and verifies both the method call and the return value. The use of Mockito's
verifymethod ensures that the correct API method is called with the expected parameters.
71-109: LGTM: Error case tests for createConfigMap are well-implemented.The test methods
testCreateConfigMapEmptyNamespaceandtestCreateConfigMapEmptyNameeffectively verify the error handling for invalid inputs (empty namespace or name). The use ofassertThrowsto check forIllegalArgumentExceptionis appropriate. The verification that the API method is never called in these cases is a good practice.
129-179: LGTM: Comprehensive tests for loadFromConfigMap method.The test methods
testLoadFromConfigMapSuccess,testLoadFromConfigMapFailure, andtestLoadFromConfigMapConfigMapNotFoundprovide excellent coverage for theloadFromConfigMapfunctionality. They effectively test:
- Successful retrieval of ConfigMap data
- Handling of API exceptions
- Behavior when ConfigMap is not found
The use of appropriate assertions and exception checks ensures robust testing of both positive and negative scenarios.
181-219: LGTM: Effective tests for getValueFromConfigMap method.The test methods
testGetValueFromConfigMapReturnsValueandtestGetValueFromConfigMapKeyNotFoundprovide good coverage for thegetValueFromConfigMapfunctionality. They effectively test:
- Retrieval of an existing key's value
- Behavior when the key doesn't exist (returning null)
The mocking of API responses and assertions are implemented correctly, ensuring the method behaves as expected in both scenarios.
244-276: LGTM: Comprehensive tests for checkConfigMapExist method.The test methods
testCheckConfigMapExistWhenConfigMapExistsandtestCheckConfigMapExistWhenConfigMapDoesNotExistprovide excellent coverage for thecheckConfigMapExistfunctionality. They effectively test:
- The case when the ConfigMap exists (returning true)
- The case when the ConfigMap does not exist (returning false)
The mocking of API responses and assertions are implemented correctly, ensuring the method behaves as expected in both scenarios.
111-127: LGTM with a suggestion: Consider the implications of allowing null data.The
testCreateConfigMapNullDatamethod effectively tests thatcreateConfigMapcan handle null data input. However, it's worth considering whether allowing null data is the intended behavior. If it is, this test is correct. If not, you might want to modify theKubernetesManagerto throw an exception for null data and update this test accordingly.To verify the intended behavior, you can check the implementation of
KubernetesManager.createConfigMap:✅ Verification successful
Verified behavior:
createConfigMapallows null data.The
createConfigMapmethod currently acceptsnullfor thedataparameter without any null checks, which aligns with thetestCreateConfigMapNullDatatest. If allowingnulldata is the intended behavior, then the test is appropriate and the implementation is correct. Otherwise, consider adding null checks for thedataparameter and updating the test accordingly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if KubernetesManager.createConfigMap handles null data rg -n "public String createConfigMap\(" apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.javaLength of output: 235
Script:
#!/bin/bash # Retrieve the implementation of KubernetesManager.createConfigMap ast-grep --lang java --pattern $'public String createConfigMap($_, $_, $_) { $$$ }' apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.javaLength of output: 175
Script:
#!/bin/bash # Retrieve the implementation of KubernetesManager.createConfigMap using rg rg -n -A 20 'public String createConfigMap\(' apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java | sed -n '/public String createConfigMap(/,/^}/p'Length of output: 1340
apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java (3)
31-31: Consider removing@Serviceannotation if not using Spring's dependency injection.The
@Serviceannotation indicates that this class is a Spring service component. If this class is not intended to be managed by Spring's application context, consider removing the annotation to avoid confusion.Is this class intended to be a Spring-managed bean? If not, remove the
@Serviceannotation.
39-50: Ensure Kubernetes client configuration is appropriate for the environment.The
initClientmethod initializes the Kubernetes client using the default configuration. Verify that this configuration is suitable for your deployment environment, especially if running outside of a Kubernetes cluster or if custom configurations are required.Ensure that
Config.defaultClient()correctly configures the client in your environment. If custom configurations are needed, consider modifying the client initialization accordingly.
28-28: Import only necessary packages.Ensure that all imported classes are used in the code to avoid unnecessary dependencies.
Run the following script to check for unused imports:
| if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty()) { | ||
| log.error("create config map failed due to null or empty parameter: configMapNamespace={}, name={}", configMapNamespace, name); | ||
| throw new IllegalArgumentException("ConfigMap namespace and name cannot be null or empty"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate input parameters consistently across methods.
The input validation logic is duplicated in multiple methods. Consider creating a private helper method to validate parameters to reduce code duplication and potential inconsistencies.
Create a helper method for parameter validation:
private void validateParams(String configMapNamespace, String name) {
if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty()) {
log.error("Parameters cannot be null or empty: configMapNamespace={}, name={}", configMapNamespace, name);
throw new IllegalArgumentException("ConfigMap namespace and name cannot be null or empty");
}
}Then, update your methods:
-public String createConfigMap(String configMapNamespace, String name, Map<String, String> data) {
- if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty()) {
- log.error("Parameters cannot be null or empty: configMapNamespace={}, name={}", configMapNamespace, name);
- throw new IllegalArgumentException("ConfigMap namespace and name cannot be null or empty");
- }
+public String createConfigMap(String configMapNamespace, String name, Map<String, String> data) {
+ validateParams(configMapNamespace, name);Repeat this change for other methods where parameters are validated.
| coreV1Api.readNamespacedConfigMap(configMapName, configMapNamespace, null); | ||
| return true; | ||
| } catch (Exception e) { | ||
| log.error("check config map failed", e); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize existence check for ConfigMap.
Using readNamespacedConfigMap to check existence may be inefficient and can generate unnecessary error logs on exceptions. Instead, use listNamespacedConfigMap with field selectors for a cleaner check.
Replace the method's try-catch block with:
try {
V1ConfigMapList configMapList = coreV1Api.listNamespacedConfigMap(
configMapNamespace,
null,
null,
null,
"metadata.name=" + configMapName,
null,
null,
null,
null,
null,
null
);
return !configMapList.getItems().isEmpty();
} catch (ApiException e) {
log.error("Failed to check ConfigMap existence", e);
throw new RuntimeException("Failed to check ConfigMap existence: " + e.getResponseBody(), e);
}This method avoids exceptions for non-existent ConfigMaps and doesn't log errors in normal operation.
| * limitations under the License. | ||
| * | ||
| */ | ||
| package com.ctrip.framework.apollo.Kubernetes; |
There was a problem hiding this comment.
Use lowercase package naming convention.
Java package names should be in lowercase to avoid conflicts with class names and to follow standard conventions. Please rename the package com.ctrip.framework.apollo.Kubernetes to com.ctrip.framework.apollo.kubernetes.
Apply this change:
-package com.ctrip.framework.apollo.Kubernetes;
+package com.ctrip.framework.apollo.kubernetes;📝 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.
| package com.ctrip.framework.apollo.Kubernetes; | |
| package com.ctrip.framework.apollo.kubernetes; |
There was a problem hiding this comment.
package name should use lowercase
There was a problem hiding this comment.
@shoothzj Thank you for acknowledging the package naming convention.
((\ /)/
(='.'=)
(_(")("))
| if (!configMap.getData().containsKey(key)) { | ||
| throw new RuntimeException(String.format("Specified key not found in ConfigMap: %s, configMapNamespace: %s, name: %s", name, configMapNamespace, name)); | ||
| } |
There was a problem hiding this comment.
Include the missing key in the exception message.
In getValueFromConfigMap, when the specified key is not found, the exception message does not include the missing key. This information is valuable for debugging.
Update the exception message to include the missing key:
-throw new RuntimeException(String.format("Specified key not found in ConfigMap: %s, configMapNamespace: %s, name: %s", name, configMapNamespace, name));
+throw new RuntimeException(String.format("Specified key '%s' not found in ConfigMap '%s' in namespace '%s'", key, name, configMapNamespace));📝 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 (!configMap.getData().containsKey(key)) { | |
| throw new RuntimeException(String.format("Specified key not found in ConfigMap: %s, configMapNamespace: %s, name: %s", name, configMapNamespace, name)); | |
| } | |
| if (!configMap.getData().containsKey(key)) { | |
| throw new RuntimeException(String.format("Specified key '%s' not found in ConfigMap '%s' in namespace '%s'", key, name, configMapNamespace)); | |
| } |
| if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty() || data == null || data.isEmpty()) { | ||
| log.error("Parameters can not be null or empty: configMapNamespace={}, name={}", configMapNamespace, name); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent error handling.
Returning null on failure in updateConfigMap may lead to NullPointerException in calling methods. Instead, consider throwing an exception or returning a boolean to indicate success.
Modify the method signature and return type:
-public String updateConfigMap(String configMapNamespace, String name, Map<String, String> data) {
+public boolean updateConfigMap(String configMapNamespace, String name, Map<String, String> data) {Update the return statements accordingly:
-return name;
+return true;In the catch block:
-return null;
+throw new RuntimeException("Failed to update ConfigMap: " + e.getMessage(), e);This approach provides clearer feedback to the caller about the operation's success.
Committable suggestion was skipped due to low confidence.
| V1ConfigMap configMap = coreV1Api.readNamespacedConfigMap(name, configMapNamespace, null); | ||
| if (configMap == null) { | ||
| throw new RuntimeException(String.format("ConfigMap does not exist, configMapNamespace: %s, name: %s", configMapNamespace, name)); | ||
| } |
There was a problem hiding this comment.
Remove unnecessary null check for configMap.
The readNamespacedConfigMap method throws an ApiException if the ConfigMap is not found, so configMap will not be null. The null check is redundant.
Remove the null check:
-V1ConfigMap configMap = coreV1Api.readNamespacedConfigMap(name, configMapNamespace, null);
-if (configMap == null) {
- throw new RuntimeException(String.format("ConfigMap does not exist, configMapNamespace: %s, name: %s", configMapNamespace, name));
-}Handle ApiException properly as suggested in previous comments.
Committable suggestion was skipped due to low confidence.
| coreV1Api.createNamespacedConfigMap(configMapNamespace, configMap, null, null, null, null); | ||
| return name; | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("Failed to create ConfigMap: " + e.getMessage(), e); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle specific Kubernetes API exceptions.
Catching generic Exception makes it difficult to handle different error scenarios appropriately. Consider catching specific exceptions such as ApiException to provide more granular error handling.
Update the catch block to handle ApiException:
-} catch (Exception e) {
+} catch (ApiException e) {Additionally, you can handle different HTTP status codes within ApiException to provide more detailed error messages.
Committable suggestion was skipped due to low confidence.
| return null; | ||
| } |
There was a problem hiding this comment.
Avoid suppressing exceptions and returning null.
In getValueFromConfigMap, returning null when an exception occurs can conceal underlying issues. It's better to handle the exception appropriately or propagate it.
Consider rethrowing the exception or handling it based on the specific case:
-} catch (Exception e) {
- return null;
+} catch (ApiException e) {
+ log.error("Failed to get value from ConfigMap", e);
+ throw new RuntimeException("Failed to get value from ConfigMap: " + e.getResponseBody(), e);This ensures that calling methods are aware that an error has occurred.
Committable suggestion was skipped due to low confidence.
| if (data != null && data.containsKey(name)) { | ||
| return data.get(name); | ||
| } else { | ||
| throw new RuntimeException(String.format("Specified key not found in ConfigMap: %s, configMapNamespace: %s, name: %s", name, configMapNamespace, name)); | ||
| } |
There was a problem hiding this comment.
Correct the key used to retrieve data from ConfigMap.
In loadFromConfigMap, the method checks for the key name within the ConfigMap's data, which may not be correct. Typically, you should retrieve all data or specify a particular key.
If you intend to return all data as a JSON string:
-if (data != null && data.containsKey(name)) {
- return data.get(name);
-} else {
- throw new RuntimeException(String.format("Specified key not found in ConfigMap: %s, configMapNamespace: %s, name: %s", name, configMapNamespace, name));
-}
+if (data != null) {
+ return new ObjectMapper().writeValueAsString(data);
+} else {
+ throw new RuntimeException(String.format("No data found in ConfigMap: %s, configMapNamespace: %s", name, configMapNamespace));
+}Ensure you handle JSON conversion and include necessary imports for ObjectMapper.
Committable suggestion was skipped due to low confidence.
| public boolean checkConfigMapExist(String configMapNamespace, String configMapName) { | ||
| if (configMapNamespace == null || configMapNamespace.isEmpty() || configMapName == null || configMapName.isEmpty()) { | ||
| log.error("Parameters can not be null or empty: configMapNamespace={}, configMapName={}", configMapNamespace, configMapName); | ||
| return false; | ||
| } | ||
| try { | ||
| coreV1Api.readNamespacedConfigMap(configMapName, configMapNamespace, null); | ||
| return true; | ||
| } catch (Exception e) { | ||
| log.error("check config map failed", e); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Standardize method return types and error handling.
The checkConfigMapExist method returns false on failure, which may conflate errors with a non-existent ConfigMap. It's better to distinguish between these cases.
Consider throwing an exception on failure and returning a boolean for actual existence:
- return false;
+ throw new RuntimeException("Failed to check ConfigMap existence: " + e.getMessage(), e);Update the method's documentation to reflect this behavior.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (2)
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepositoryTest.java (1)
68-74: Consider translating comments to English for consistencyThe comments in lines 68-74 are written in Chinese. For consistency and to accommodate all contributors, it's recommended to use English for code comments throughout the project.
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java (1)
91-92: Address the TODO comments to complete the implementation.There are several TODO comments in the code that indicate incomplete implementations or questions:
- Lines 91-92:
- "TODO: How to design the fallback key to avoid conflicts (the cluster initialization has already set levels)."
- Lines 125-126:
- "TODO: Initially understand that only generation is needed here, subsequent update events will write new values."
- Lines 223-224:
- "TODO: To be modified, first retry accessing IDC and then fallback to default."
- Lines 141-144:
- "TODO Testing points:... "
Please address these TODOs to ensure the code is complete and functions as intended. If you need assistance in implementing these parts or converting them into actionable tasks, I can help by providing suggestions or opening GitHub issues to track them.
Also applies to: 125-126, 223-224, 141-144
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepositoryTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepositoryTest.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java
🔇 Additional comments (2)
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepositoryTest.java (1)
67-67: Initialization ofsomePropertieslooks good, but verify impact on existing tests.The addition of
someProperties = new Properties();in thesetUpmethod is a good practice as it ensures a clean state for each test. However, please consider the following:
- Verify that this change doesn't break any existing tests that might have relied on
somePropertiesbeing null initially.- Update the class-level comment for
somePropertiesto reflect that it's now initialized in setUp.To ensure this change doesn't introduce any regressions, please run the following command and verify that all tests pass:
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepositoryTest.java (1)
260-273: Add assertions for the loaded propertiesThe test method verifies that
getValueFromConfigMapis called but doesn't assert the content of the loaded properties. Please add assertions to verify that the properties loaded from the ConfigMap match the expected values based on the mockedencodedConfig.
| public void testSyncSuccessFromUpstream() throws Throwable { | ||
| K8sConfigMapConfigRepository repo = new K8sConfigMapConfigRepository(someNamespace); | ||
|
|
||
| // arrange | ||
| ConfigRepository upstream = mock(ConfigRepository.class); | ||
| Properties upstreamProperties = new Properties(); | ||
| upstreamProperties.setProperty("key", "value"); | ||
| when(upstream.getConfig()).thenReturn(upstreamProperties); | ||
| when(upstream.getSourceType()).thenReturn(ConfigSourceType.REMOTE); | ||
| repo.setUpstreamRepository(upstream); | ||
|
|
||
| // // mock KubernetesManager | ||
| // when(kubernetesManager.createConfigMap(anyString(), anyString(), anyMap())) | ||
| // .thenReturn(true); | ||
| // setField(repo, "kubernetesManager", kubernetesManager); | ||
|
|
||
| // act | ||
| repo.sync(); | ||
|
|
||
| // assert | ||
| verify(upstream, times(1)).getConfig(); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSyncFromUpstreamWithFileStorage() throws Exception { | ||
| K8sConfigMapConfigRepository repo = new K8sConfigMapConfigRepository(someNamespace); | ||
|
|
||
|
|
||
| Properties upstreamProperties = new Properties(); | ||
| upstreamProperties.setProperty("key1", "value1"); | ||
|
|
||
| when(upstreamRepo.getConfig()).thenReturn(upstreamProperties); | ||
| when(upstreamRepo.getSourceType()).thenReturn(ConfigSourceType.LOCAL); | ||
|
|
||
| repo.sync(); | ||
|
|
||
| Properties config = repo.getConfig(); | ||
| assertEquals("value1", config.getProperty("key1")); | ||
| assertEquals(ConfigSourceType.LOCAL, repo.getSourceType()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSyncFromUpstreamWithRemote() throws Exception { | ||
| K8sConfigMapConfigRepository repo = new K8sConfigMapConfigRepository(someNamespace); | ||
|
|
||
| Properties upstreamProperties = new Properties(); | ||
| upstreamProperties.setProperty("key2", "value2"); | ||
|
|
||
| when(upstreamRepo.getConfig()).thenReturn(upstreamProperties); | ||
| when(upstreamRepo.getSourceType()).thenReturn(ConfigSourceType.REMOTE); | ||
|
|
||
| repo.sync(); | ||
|
|
||
| Properties config = repo.getConfig(); | ||
| assertEquals("value2", config.getProperty("key2")); | ||
| assertEquals(ConfigSourceType.REMOTE, repo.getSourceType()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repetitive test code to improve maintainability
Multiple test methods contain similar setup code, such as mocking the upstream repository and setting properties. Consider extracting common setup code into the @Before method or helper methods to reduce duplication and enhance maintainability.
Also applies to: 239-253
| when(kubernetesManager.getValueFromConfigMap(anyString(), anyString(), anyString())) | ||
| .thenReturn(Base64.getEncoder().encodeToString("{\"key3\":\"value3\"}".getBytes())); |
There was a problem hiding this comment.
Add assertions to verify the decoded properties
In testSyncFromK8s, the test mocks the return of an encoded configuration string but doesn't assert that the properties are correctly loaded and decoded. Please add assertions to verify that the loaded properties match the expected values after decoding.
Apply this diff to include assertions for the loaded properties:
when(kubernetesManager.getValueFromConfigMap(anyString(), anyString(), anyString()))
.thenReturn(Base64.getEncoder().encodeToString("{\"key3\":\"value3\"}".getBytes()));
+ Properties expectedProperties = new Properties();
+ expectedProperties.setProperty("key3", "value3");
repo.sync();
Properties config = repo.getConfig();
assertEquals("value3", config.getProperty("key3"));
+ assertEquals(expectedProperties, config);
assertEquals(ConfigSourceType.CONFIGMAP, repo.getSourceType());Committable suggestion was skipped due to low confidence.
| // 创建一个 LocalFileConfigRepository 实例，作为上游仓库 | ||
| LocalFileConfigRepository upstreamRepo = mock(LocalFileConfigRepository.class); | ||
| when(upstreamRepo.getSourceType()).thenReturn(ConfigSourceType.LOCAL); | ||
|
|
||
| // 创建一个模拟的 KubernetesManager | ||
| KubernetesManager mockKubernetesManager = mock(KubernetesManager.class); | ||
| when(mockKubernetesManager.checkConfigMapExist(anyString(), anyString())).thenReturn(true); | ||
| doNothing().when(mockKubernetesManager).createConfigMap(anyString(), anyString(), any()); | ||
|
|
There was a problem hiding this comment.
Avoid redeclaring the upstreamRepo variable locally
In the testOnRepositoryChange method, upstreamRepo is redeclared as a local variable, which shadows the class field upstreamRepo. This can lead to confusion and potential errors. Use the existing class field instead.
Apply this diff to fix the issue:
- LocalFileConfigRepository upstreamRepo = mock(LocalFileConfigRepository.class);
+ when(upstreamRepo.getSourceType()).thenReturn(ConfigSourceType.LOCAL);Committable suggestion was skipped due to low confidence.
| if (configMapProperties == null) { | ||
| sync(); | ||
| } | ||
| Properties result = propertiesFactory.getPropertiesInstance(); | ||
| result.putAll(configMapProperties); | ||
| return result; |
There was a problem hiding this comment.
Declare and initialize propertiesFactory before use.
The propertiesFactory object is used to obtain new Properties instances at lines 151, 234, and 292, but it is never declared or initialized in the class. This will result in a NullPointerException at runtime. You should declare propertiesFactory as a class member and initialize it before using it.
To fix this, add the following declaration to the class fields:
private final PropertiesFactory propertiesFactory;And initialize it in the constructor after configUtil and kubernetesManager are initialized:
this.propertiesFactory = ApolloInjector.getInstance(PropertiesFactory.class);| String jsonConfig = kubernetesManager.getValueFromConfigMap(configMapNamespace, configUtil.getAppId(), configMapKey); | ||
| if (jsonConfig == null) { | ||
| // TODO 待修改，先重试访问idc再default保底 | ||
| jsonConfig = kubernetesManager.getValueFromConfigMap(configMapNamespace, configMapName, Joiner.on(ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR).join(configUtil, namespace)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Correct the Joiner usage by passing the cluster name instead of configUtil.
At line 225, the Joiner is used incorrectly:
jsonConfig = kubernetesManager.getValueFromConfigMap(configMapNamespace, configMapName, Joiner.on(ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR).join(configUtil, namespace));Here, configUtil is passed to join(), which will invoke configUtil.toString(), likely not providing the intended value. Instead, you should pass the cluster name obtained from configUtil.getCluster().
Apply this diff to fix the issue:
- jsonConfig = kubernetesManager.getValueFromConfigMap(configMapNamespace, configMapName, Joiner.on(ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR).join(configUtil, namespace));
+ jsonConfig = kubernetesManager.getValueFromConfigMap(configMapNamespace, configMapName, Joiner.on(ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR).join(configUtil.getCluster(), namespace));This ensures the correct cluster name is used in the key.
| // 确保获取到的配置信息不为空 | ||
| if (jsonConfig != null) { | ||
| // 解码Base64编码的JSON字符串 | ||
| jsonConfig = new String(Base64.getDecoder().decode(jsonConfig)); |
There was a problem hiding this comment.
Specify character encoding when converting bytes to strings.
At lines 230 and 307, byte arrays are converted to strings without specifying the character encoding. This can lead to platform-dependent issues since the default charset may vary.
Apply this diff to explicitly specify StandardCharsets.UTF_8:
At line 230:
- jsonConfig = new String(Base64.getDecoder().decode(jsonConfig));
+ jsonConfig = new String(Base64.getDecoder().decode(jsonConfig), StandardCharsets.UTF_8);At line 307:
- String encodedJsonConfig = Base64.getEncoder().encodeToString(jsonConfig.getBytes());
+ String encodedJsonConfig = Base64.getEncoder().encodeToString(jsonConfig.getBytes(StandardCharsets.UTF_8));This ensures consistent encoding and decoding across different platforms.
Also applies to: 307-307
| // 创建一个新的HashMap, 将编码后的JSON字符串作为值，业务appId作为键，存入data中 | ||
| Map<String, String> data = new HashMap<>(); | ||
| data.put(configUtil.getAppId(), encodedJsonConfig); | ||
|
|
||
| // 更新ConfigMap | ||
| kubernetesManager.updateConfigMap(configUtil.getConfigMapNamespace(), configUtil.getAppId(), data); | ||
| transaction.setStatus(Transaction.SUCCESS); |
There was a problem hiding this comment.
Ensure consistent key usage when storing data in ConfigMap.
In the persistConfigMap() method, there is inconsistency in the keys used for storing and retrieving data:
- At line 310, you use
configUtil.getAppId()as the key in thedatamap. - At line 313, you use
configUtil.getAppId()as theconfigMapNamewhen updating. - However, when loading the config in
loadFromK8sConfigMap(), you retrieve the data usingconfigMapKey.
To maintain consistency and ensure that data is correctly stored and retrieved, update the code as follows:
-
Use
configMapKeyas the key when putting data into thedatamap:- data.put(configUtil.getAppId(), encodedJsonConfig); + data.put(configMapKey, encodedJsonConfig);
-
Use
configMapNamespaceandconfigMapNamein theupdateConfigMap()call:- kubernetesManager.updateConfigMap(configUtil.getConfigMapNamespace(), configUtil.getAppId(), data); + kubernetesManager.updateConfigMap(configMapNamespace, configMapName, data);
This ensures that the data stored in the ConfigMap aligns with the keys used when retrieving it.
📝 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.
| // 创建一个新的HashMap, 将编码后的JSON字符串作为值，业务appId作为键，存入data中 | |
| Map<String, String> data = new HashMap<>(); | |
| data.put(configUtil.getAppId(), encodedJsonConfig); | |
| // 更新ConfigMap | |
| kubernetesManager.updateConfigMap(configUtil.getConfigMapNamespace(), configUtil.getAppId(), data); | |
| transaction.setStatus(Transaction.SUCCESS); | |
| // 创建一个新的HashMap, 将编码后的JSON字符串作为值，业务appId作为键，存入data中 | |
| Map<String, String> data = new HashMap<>(); | |
| data.put(configMapKey, encodedJsonConfig); | |
| // 更新ConfigMap | |
| kubernetesManager.updateConfigMap(configMapNamespace, configMapName, data); | |
| transaction.setStatus(Transaction.SUCCESS); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java (1)
129-137: Add unit tests forcreateConfigMapConfigRepositorymethodAdding unit tests for the new
createConfigMapConfigRepositorymethod will help ensure its correct functionality and prevent future regressions.
| // TODO 本地和configMap允许同时开启 | ||
| if (m_configUtil.isPropertyKubernetesCacheEnabled()) { | ||
| return createConfigMapConfigRepository(namespace); | ||
| }else if (m_configUtil.isPropertyFileCacheEnabled()) { |
There was a problem hiding this comment.
Issue: Logic does not support simultaneous use of local and ConfigMap repositories
The TODO comment indicates an intention to allow both local and ConfigMap caches to be enabled at the same time. However, the current logic in createConfigRepository only allows one repository to be active. If isPropertyKubernetesCacheEnabled() returns true, it immediately returns the ConfigMap repository without checking if the local cache is also enabled.
Would you like assistance in modifying the logic to support both repositories concurrently, or should we open a GitHub issue to track this enhancement?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
changes/changes-2.4.0.md (2)
8-8: Consider refining the feature description for clarity.While the feature description is informative, there are a few points to consider:
- The changelog is for Apollo Java, but it mentions the Golang client as well. This might be confusing for users.
- The formatting of the link could be improved for better readability.
Consider applying these changes:
- * [Feature Support Kubernetes ConfigMap cache for Apollo java, golang client](https://github.com/apolloconfig/apollo-java/pull/79) + * Feature: Support Kubernetes ConfigMap cache for Apollo Java client + - [Pull Request #79](https://github.com/apolloconfig/apollo-java/pull/79) + - Note: Similar support added for Golang clientThis format separates the feature description from the link and clarifies that the primary focus is on the Java client while still mentioning the Golang client.
1-12: Consider enhancing the changelog with additional details.While the current changelog provides essential information, consider adding the following to make it more comprehensive:
- The exact date of the release
- Any breaking changes or deprecations (if applicable)
- Any bug fixes or minor improvements (if any)
Here's a suggested structure to incorporate these details:
Changes by Version ================== Release Notes. Apollo Java 2.4.0 ----------------- Release Date: [Insert Date] ### Features * Support Kubernetes ConfigMap cache for Apollo Java client - [Pull Request #79](https://github.com/apolloconfig/apollo-java/pull/79) - Note: Similar support added for Golang client ### Breaking Changes * [List any breaking changes, if applicable] ### Deprecations * [List any deprecations, if applicable] ### Bug Fixes * [List any bug fixes, if applicable] ### Minor Improvements * [List any minor improvements, if applicable] All issues and pull requests are [here](https://github.com/apolloconfig/apollo-java/milestone/4?closed=1)This structure provides a more detailed overview of the release, which can be particularly helpful for users upgrading from previous versions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- CHANGES.md (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java (3 hunks)
- changes/changes-2.4.0.md (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java
🔇 Additional comments (3)
changes/changes-2.4.0.md (3)
1-4: LGTM: Title and formatting are appropriate.The title "Changes by Version" and the formatting of the changelog header are clear, consistent, and follow standard Markdown conventions.
5-7: LGTM: Version information is clear and well-formatted.The version "Apollo Java 2.4.0" is clearly stated, and the separator line enhances readability.
11-12: LGTM: Comprehensive link to issues and pull requests.The inclusion of a link to all issues and pull requests for this milestone is valuable for users seeking more detailed information. The formatting and placement are appropriate.
CHANGES.md
Outdated
| @@ -5,7 +5,8 @@ Release Notes. | |||
| Apollo Java 2.4.0 | |||
|
|
|||
| ------------------ | |||
| * | |||
| * [Feature Support Kubernetes ConfigMap cache for Apollo java, golang client](https://github.com/apolloconfig/apollo-java/pull/79) | |||
There was a problem hiding this comment.
💡 Codebase verification
Update Changelog to Reflect Actual Changes
The changelog currently mentions support for both Java and Golang clients. However, this PR only includes modifications related to the Java client. Please update the changelog to accurately reflect the supported clients.
🔗 Analysis chain
LGTM! Verify Golang client mention.
The changelog entry is well-formatted and provides clear information about the new feature. Good job on including the link to the pull request for reference.
However, I noticed that the changelog mentions support for both Java and Golang clients, while the PR objectives only discuss the Java client. Could you please verify if the Golang client is also part of this change?
To help verify this, you can run the following script:
This script will help determine if any Golang-related changes were actually made in this PR. If not, consider updating the changelog to mention only the Java client.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Golang-related files were modified in this PR
# Search for Go files that might have been modified
go_files=$(git diff --name-only origin/main | grep '\.go$')
if [ -n "$go_files" ]; then
echo "Golang files modified in this PR:"
echo "$go_files"
else
echo "No Golang files were modified in this PR."
fi
# Search for mentions of Golang or Go in the diff
golang_mentions=$(git diff origin/main | grep -i 'golang\|go client')
if [ -n "$golang_mentions" ]; then
echo "Mentions of Golang or Go client in the diff:"
echo "$golang_mentions"
else
echo "No mentions of Golang or Go client found in the diff."
fi
Length of output: 1426
| repo.setUpstreamRepository(upstream); | ||
|
|
||
| // // mock KubernetesManager | ||
| // when(kubernetesManager.createConfigMap(anyString(), anyString(), anyMap())) |
There was a problem hiding this comment.
please don't commit commented code
| @@ -111,7 +101,11 @@ public ConfigFile createConfigFile(String namespace, ConfigFileFormat configFile | |||
| } | |||
|
|
|||
| ConfigRepository createConfigRepository(String namespace) { | |||
| if (m_configUtil.isPropertyFileCacheEnabled()) { | |||
| // TODO 本地和configMap允许同时开启 | |||
There was a problem hiding this comment.
please don't commit the progress message
| @@ -375,6 +405,15 @@ public boolean isInLocalMode() { | |||
| return false; | |||
| } | |||
|
|
|||
| public boolean isInKubernetesMode() { | |||
There was a problem hiding this comment.
What's the use of this function?
There was a problem hiding this comment.
Indeed, It was not used. I will delete it
| @@ -35,7 +35,7 @@ | |||
| * @author Jason Song(song_s@ctrip.com) | |||
| */ | |||
| public enum Env{ | |||
| LOCAL, DEV, FWS, FAT, UAT, LPT, PRO, TOOLS, UNKNOWN; | |||
| LOCAL, DEV, FWS, FAT, UAT, LPT, PRO, TOOLS, UNKNOWN, KUBERNETES; | |||
There was a problem hiding this comment.
KUBERNETES doesn't seem to me as an environment...
changes/changes-2.4.0.md
Outdated
| Apollo Java 2.4.0 | ||
|
|
||
| ------------------ | ||
| * [Feature Support Kubernetes ConfigMap cache for Apollo java, golang client](https://github.com/apolloconfig/apollo-java/pull/79) |
There was a problem hiding this comment.
There is no need to update the changes/changes-2.4.0.md, as it will be updated after the release of 2.4.0.
apollo-client/pom.xml
Outdated
| <dependency> | ||
| <groupId>io.kubernetes</groupId> | ||
| <artifactId>client-java</artifactId> | ||
| <version>18.0.0</version> | ||
| </dependency> |
There was a problem hiding this comment.
- Manage the version in the parent POM.
- I'm considering marking the dependency as
optional, as it's not required in every case, similar to thespringdependencies mentioned above.
| // 已完成，创建configmapRepo时会同时创建一个fileRepo作为上游，相当于同时开启了，路径若未设置用默认 | ||
| if (m_configUtil.isPropertyKubernetesCacheEnabled()) { | ||
| return createConfigMapConfigRepository(namespace); | ||
| }else if (m_configUtil.isPropertyFileCacheEnabled()) { |
There was a problem hiding this comment.
Please address the formatting issues in accordance with the Contributing Guide.
| }else if (m_configUtil.isPropertyFileCacheEnabled()) { | |
| } else if (m_configUtil.isPropertyFileCacheEnabled()) { |
| public void testConfigMapNamespaceWithSystemProperty() { | ||
| String someConfigMapNamespace = "someConfigMapNamespace"; | ||
|
|
||
| System.setProperty(ApolloClientSystemConsts.APOLLO_CONFIGMAP_NAMESPACE, someConfigMapNamespace); |
There was a problem hiding this comment.
Clear the state in tearDown method.
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (15)
apollo-core/src/main/java/com/ctrip/framework/apollo/core/ConfigConsts.java (1)
27-27: LGTM: New constant for Kubernetes ConfigMap default namespaceThe addition of
KUBERNETES_CACHE_CONFIG_MAP_NAMESPACE_DEFAULTaligns well with the PR objectives of supporting Kubernetes ConfigMap for Apollo clients. The default value of "default" is appropriate for Kubernetes environments.Consider adding a brief comment above this constant to explain its purpose and usage in the context of Kubernetes ConfigMap support. For example:
/** * Default namespace for Kubernetes ConfigMap cache storage. * Used when no specific namespace is provided for ConfigMap operations. */ String KUBERNETES_CACHE_CONFIG_MAP_NAMESPACE_DEFAULT = "default";This will help other developers understand the constant's role in the new Kubernetes integration.
apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java (1)
76-84: LGTM! Consider enhancing the comments.The new constants for Kubernetes ConfigMap namespace configuration are well-named and consistent with the existing code style. They align with the PR objectives of supporting Kubernetes ConfigMap for Apollo clients.
Consider slightly expanding the comments to provide more context:
/** - * kubernetes configmap cache namespace + * Kubernetes ConfigMap cache namespace for Apollo configuration */ public static final String APOLLO_CACHE_KUBERNETES_CONFIGMAP_NAMESPACE = "apollo.cache.kubernetes.configmap-namespace"; /** - * kubernetes configmap cache namespace environment variables + * Environment variable for Kubernetes ConfigMap cache namespace */ public static final String APOLLO_CACHE_KUBERNETES_CONFIGMAP_NAMESPACE_ENVIRONMENT_VARIABLES = "APOLLO_CACHE_KUBERNETES_CONFIGMAP_NAMESPACE";apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java (1)
258-263: LGTM: Good test for default ConfigMap namespace.This test method effectively verifies that
ConfigUtilreturns the default Kubernetes ConfigMap namespace when no custom value is set. The test is well-structured and consistent with the existing coding style.For improved clarity, consider adding a comment explaining the expected default value:
@Test public void testConfigMapNamespaceWithDefault() { // When no custom namespace is set, it should return the default value ConfigUtil configUtil = new ConfigUtil(); assertEquals(ConfigConsts.KUBERNETES_CACHE_CONFIG_MAP_NAMESPACE_DEFAULT, configUtil.getConfigMapNamespace()); }apollo-client/src/test/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManagerTest.java (4)
78-91: LGTM with suggestion: Consider additional assertionThe test case effectively checks the behavior of
createConfigMapwith null data. It verifies that the method executes without throwing an exception and that the API is called with the correct parameters.To enhance the test, consider adding an assertion to verify that the created ConfigMap has the expected structure when data is null. This could be done by capturing the
V1ConfigMapargument passed tocreateNamespacedConfigMapand asserting its properties.
131-143: LGTM with suggestion: Consider using ApiException for not found scenarioThe test case effectively verifies the behavior of
loadFromConfigMapwhen the ConfigMap is not found. However, the current implementation simulates this by returning null from the API call.To more accurately represent the Kubernetes API behavior, consider throwing an
ApiExceptionwith a 404 status code instead of returning null. This would better simulate the actual API response when a resource is not found.
54-73: Consider implementing or uncommenting additional test casesThere are several commented-out test methods in this file that appear to cover important scenarios:
testCreateConfigMapSuccesstestLoadFromConfigMapSuccesstestGetValueFromConfigMapReturnsValuetestUpdateConfigMapSuccesstestCheckConfigMapExistWhenConfigMapExistsThese tests seem crucial for ensuring comprehensive coverage of the
KubernetesManagerfunctionality. Consider implementing or uncommenting these tests to improve the overall test suite. If there are reasons these tests are not currently active (e.g., pending implementation details), it would be helpful to add TODO comments explaining why they are commented out and when they should be addressed.Also applies to: 96-110, 148-166, 190-208, 213-229
1-264: Overall well-structured test class with room for improvementThe
KubernetesManagerTestclass is well-structured and covers several important test scenarios for theKubernetesManagerclass. The use of Mockito for dependency injection and the clear setup method are commendable.However, there are a few areas for improvement:
- Implement or uncomment the additional test methods to ensure comprehensive coverage of all
KubernetesManagerfunctionalities.- Consider adding more assertions in some test cases, such as verifying the structure of created ConfigMaps.
- Ensure that the simulated API responses accurately represent real Kubernetes API behavior, especially for error cases.
Addressing these points will significantly enhance the robustness and completeness of the test suite.
apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (3)
74-74: Consider renaming the field to follow Java naming conventions.The field name
PropertyKubernetesCacheEnabledstarts with a capital letter, which is not typical for field names in Java. Consider renaming it topropertyKubernetesCacheEnabledto follow the standard camelCase convention for field names.
371-379: LGTM: Well-structured method for retrieving ConfigMap namespace.The
getConfigMapNamespace()method is well-implemented, providing a way to get a custom namespace or fall back to a default value. This approach allows for flexibility in configuration.Consider adding a brief Javadoc comment to explain the method's purpose and return value, which would improve the API documentation.
502-504: LGTM: Well-implemented methods for Kubernetes cache property.The
isPropertyKubernetesCacheEnabled()andinitPropertyKubernetesCacheEnabled()methods are well-implemented and consistent with the class's overall design. The initialization process checks multiple sources, providing flexibility in configuration.As mentioned earlier, consider renaming the
PropertyKubernetesCacheEnabledfield topropertyKubernetesCacheEnabledto follow Java naming conventions. This change should be applied consistently to these methods as well.Also applies to: 528-532
apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java (4)
181-181: ReplaceSystem.err.printlnwith logger for consistencyUsing
System.err.printlnfor error logging is inconsistent with the rest of the class, which useslogfor logging. Replace it withlog.errorfor consistency and better control over logging output.Apply this change:
- System.err.println("Error updating ConfigMap: " + e.getMessage()); + log.error("Error updating ConfigMap: {}", e.getMessage(), e);
129-131: Include all parameters in error logsThe error log in
getValueFromConfigMapdoes not include thekeyparameter, even though it is being validated. Including all relevant parameters aids in debugging.Apply this change:
- log.error("Parameters cannot be null or empty: configMapNamespace={}, name={}", configMapNamespace, name); + log.error("Parameters cannot be null or empty: configMapNamespace={}, name={}, key={}", configMapNamespace, name, key);
152-152: Use English for code comments in an international codebaseFor consistency and to accommodate all contributors and users, consider writing code comments in English.
Update the comment to:
- // TODO 使用client自带的retry机制，设置重试次数,CAS + // TODO Use client's built-in retry mechanism, set retry count, CAS
115-117: Include original exception when rethrowingRuntimeExceptionIn
loadFromConfigMap, when catching an exception and rethrowing aRuntimeException, include the original exception as the cause. This preserves the stack trace and aids in debugging.Apply this change:
} catch (Exception e) { throw new RuntimeException(String - .format("get config map failed, configMapNamespace: %s, name: %s", configMapNamespace, name)); + .format("get config map failed, configMapNamespace: %s, name: %s", configMapNamespace, name), e); }apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java (1)
217-219: Reuse Gson instance to improve performanceMultiple instances of
Gsonare created in the methodsloadFromK8sConfigMapandpersistConfigMap. Consider reusing a singleGsoninstance to improve performance and reduce memory overhead.Apply this diff to declare a single
Gsoninstance:+ private static final Gson GSON = new Gson(); ... // In loadFromK8sConfigMap() - Gson gson = new Gson(); ... - Map<String, String> configMap = gson.fromJson(jsonConfig, type); + Map<String, String> configMap = GSON.fromJson(jsonConfig, type); ... // In persistConfigMap() - Gson gson = new Gson(); - String jsonConfig = gson.toJson(properties); + String jsonConfig = GSON.toJson(properties);Also applies to: 281-281
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- apollo-client/pom.xml (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (5 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManagerTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepositoryTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java (2 hunks)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java (2 hunks)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ConfigConsts.java (1 hunks)
- pom.xml (1 hunks)
🧰 Additional context used
🔇 Additional comments (16)
apollo-core/src/main/java/com/ctrip/framework/apollo/core/ConfigConsts.java (1)
22-22:⚠️ Potential issueBreaking change: CLUSTER_NAMESPACE_SEPARATOR value modified
The change of
CLUSTER_NAMESPACE_SEPARATORfrom "+" to "-" is a breaking change that could affect existing systems using this separator. While "-" is more aligned with Kubernetes naming conventions, this modification requires careful consideration.Please provide justification for this change and outline the migration strategy for existing users. Consider the following:
- Is this change absolutely necessary?
- How will existing configurations using "+" be handled?
- Is there a migration path or backward compatibility mechanism?
To assess the impact, let's search for usages of this constant:
✅ Verification successful
Change to
CLUSTER_NAMESPACE_SEPARATORConfirmed SafeThe modification of
CLUSTER_NAMESPACE_SEPARATORfrom "+" to "-" is confined to internal implementations and test cases. There are no external usages or API dependencies affected by this change. Ensure that all relevant documentation is updated to reflect this new separator.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of CLUSTER_NAMESPACE_SEPARATOR rg --type java "CLUSTER_NAMESPACE_SEPARATOR" -C 3Length of output: 9806
apollo-client/pom.xml (1)
101-105: 🛠️ Refactor suggestionConsider marking the Kubernetes client dependency as optional and manage its version in the parent POM.
The addition of the Kubernetes Java client dependency is appropriate for supporting Kubernetes ConfigMap. However, there are a few considerations:
As suggested in the past review comments, consider marking this dependency as
optional. This is because not all users of the Apollo client will need Kubernetes support, similar to how Spring dependencies are treated in this POM.The version is currently specified using a placeholder
${client-java.version}. Ensure that this version is properly defined in the parent POM for centralized version management, as suggested in the previous review.Consider explicitly defining the scope of this dependency if it's not intended to be a compile-time dependency.
Here's a suggested modification:
<dependency> <groupId>io.kubernetes</groupId> <artifactId>client-java</artifactId> <optional>true</optional> </dependency>Also, ensure that
<client-java.version>is defined in the parent POM.To verify the version management, please run the following command:
This will help ensure that the version is properly managed in the parent POM.
✅ Verification successful
Mark the Kubernetes client dependency as optional.
The version of the Kubernetes Java client is already managed within the current
pom.xml(<client-java.version>18.0.0</client-java.version>), so no changes to the parent POM are necessary.However, it's recommended to mark this dependency as
optionalto ensure that Kubernetes support is only included when needed.<dependency> <groupId>io.kubernetes</groupId> <artifactId>client-java</artifactId> <optional>true</optional> </dependency>🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if client-java.version is defined in the parent POM grep -n "<client-java.version>" ../pom.xmlLength of output: 88
Script:
#!/bin/bash # Search the entire repository for the definition of client-java.version rg '<client-java.version>' --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**'Length of output: 153
apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java (1)
Line range hint
1-179: Overall, the changes look good. Verify Kubernetes integration.The new constants added for Kubernetes ConfigMap integration are well-structured and consistent with the existing codebase. They provide the necessary configuration options to support the PR objectives.
To ensure the changes are properly integrated, please run the following verification script:
This script will help identify where the new constants are being used and if there are any remaining TODOs related to the Kubernetes integration.
✅ Verification successful
Kubernetes Integration Verified
The new Kubernetes-related constants are properly utilized throughout the codebase, and no outstanding TODOs related to Kubernetes integration were found. The changes are correctly implemented and integrated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new Kubernetes-related constants in the codebase. # Test: Search for usages of the new constants echo "Searching for usages of new Kubernetes-related constants:" rg --type java "APOLLO_CACHE_KUBERNETES_CONFIGMAP_NAMESPACE|APOLLO_CACHE_KUBERNETES_CONFIGMAP_NAMESPACE_ENVIRONMENT_VARIABLES|APOLLO_KUBERNETES_CACHE_ENABLE|APOLLO_KUBERNETES_CACHE_ENABLE_ENVIRONMENT_VARIABLES" # Test: Check for any TODOs related to Kubernetes integration echo "Checking for TODOs related to Kubernetes integration:" rg --type java "TODO.*Kubernetes"Length of output: 2881
apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java (3)
50-50: LGTM: Properly clearing the new system property.The addition of clearing
ApolloClientSystemConsts.APOLLO_CACHE_KUBERNETES_CONFIGMAP_NAMESPACEin thetearDownmethod ensures a clean state for subsequent tests. This change is consistent with the new tests being added and addresses the previous review comment about clearing the state in thetearDownmethod.
247-256: LGTM: Well-structured test for custom ConfigMap namespace.This test method effectively verifies that
ConfigUtilcorrectly retrieves the custom Kubernetes ConfigMap namespace when set as a system property. The test follows the Arrange-Act-Assert pattern and is consistent with the existing coding style.
Line range hint
1-264: Summary: Excellent additions to test coverage.The changes in this file effectively enhance the test coverage for the
ConfigUtilclass, specifically for the Kubernetes ConfigMap namespace functionality. The new tests cover both custom and default namespace scenarios, ensuring robust verification of thegetConfigMapNamespace()method. The code quality is high, following existing patterns and styles, and the changes align well with the PR objectives.Great job on improving the test suite!
apollo-client/src/test/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManagerTest.java (5)
38-48: LGTM: Well-structured test class setupThe test class is well-structured with proper use of Mockito for dependency injection and a clear setup method. This approach ensures that each test starts with a clean, controlled environment.
115-126: LGTM: Effective error handling testThis test case effectively verifies the error handling of the
loadFromConfigMapmethod. It correctly checks that aRuntimeExceptionis thrown with the expected error message when the Kubernetes API encounters an exception.
171-185: LGTM: Effective negative test caseThis test case effectively verifies the behavior of
getValueFromConfigMapwhen the requested key is not found in the ConfigMap. It correctly checks that the method returns null in this scenario, which is an important negative test case.
234-246: LGTM: Proper handling of non-existent ConfigMapThis test case effectively verifies the behavior of
checkConfigMapExistwhen the ConfigMap does not exist. It correctly simulates the API exception that would occur in this scenario and ensures that the method returns false as expected.
251-262: LGTM: Good boundary condition testThis test case effectively checks the behavior of
checkConfigMapExistwhen both namespace and name are empty strings. It's a good boundary condition test that ensures the method handles invalid input correctly by returning false.apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (3)
90-90: LGTM: Proper initialization in the constructor.The addition of
initPropertyKubernetesCacheEnabled()in the constructor ensures that the Kubernetes cache property is initialized when a ConfigUtil object is created. This is a good practice for maintaining the object's state.
381-397: LGTM: Well-implemented method for retrieving customized ConfigMap namespace.The
getCustomizedConfigMapNamespace()method is well-structured and follows a clear precedence order for retrieving the namespace from various sources. This implementation provides flexibility in configuration and follows the principle of least surprise.
Line range hint
1-550: Overall assessment: Well-implemented Kubernetes ConfigMap supportThe changes to
ConfigUtil.javasuccessfully introduce support for Kubernetes ConfigMap caching. The implementation is well-structured, consistent with the existing code, and provides flexibility in configuration. The new functionality aligns well with the PR objectives.Main points:
- Proper initialization of Kubernetes-related properties.
- Well-implemented methods for retrieving and managing ConfigMap namespace.
- Consistent use of existing patterns for property initialization and retrieval.
Minor improvements suggested:
- Adjust naming convention for the
PropertyKubernetesCacheEnabledfield and related methods.- Add Javadoc comments to new public methods for better API documentation.
Overall, these changes enhance the Apollo client's functionality in Kubernetes environments as intended.
apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java (2)
128-135: MethodcreateConfigMapConfigRepositoryis correctly implementedThe method properly creates a
K8sConfigMapConfigRepositorywith the provided namespace and local config repository. This adheres to best practices and ensures proper initialization of the repository.
104-106:⚠️ Potential issueRevisit the logic to support simultaneous use of local and ConfigMap repositories
As noted in previous reviews, the current logic only allows one repository to be active at a time. If
isPropertyKubernetesCacheEnabled()returns true, it immediately returns the ConfigMap repository without checking if the local cache is also enabled. Please consider modifying the logic to support both repositories concurrently if that is the intended behavior.
| /** | ||
| * enable property names cache | ||
| */ | ||
| public static final String APOLLO_KUBERNETES_CACHE_ENABLE = "apollo.cache.kubernetes.enable"; | ||
|
|
||
| /** | ||
| * enable property names cache environment variables | ||
| */ | ||
| public static final String APOLLO_KUBERNETES_CACHE_ENABLE_ENVIRONMENT_VARIABLES = "APOLLO_KUBERNETES_CACHE_ENABLE"; | ||
|
|
There was a problem hiding this comment.
Update comments for new Kubernetes cache enable constants.
The new constants for enabling Kubernetes cache are well-named and consistent with the existing code style. They align with the PR objectives of supporting Kubernetes ConfigMap for Apollo clients.
However, the comments for these new constants appear to be copied from a different constant and need to be updated to accurately reflect their purpose.
Please update the comments as follows:
/**
- * enable property names cache
+ * Enable Kubernetes cache for Apollo configuration
*/
public static final String APOLLO_KUBERNETES_CACHE_ENABLE = "apollo.cache.kubernetes.enable";
/**
- * enable property names cache environment variables
+ * Environment variable to enable Kubernetes cache for Apollo configuration
*/
public static final String APOLLO_KUBERNETES_CACHE_ENABLE_ENVIRONMENT_VARIABLES = "APOLLO_KUBERNETES_CACHE_ENABLE";📝 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.
| /** | |
| * enable property names cache | |
| */ | |
| public static final String APOLLO_KUBERNETES_CACHE_ENABLE = "apollo.cache.kubernetes.enable"; | |
| /** | |
| * enable property names cache environment variables | |
| */ | |
| public static final String APOLLO_KUBERNETES_CACHE_ENABLE_ENVIRONMENT_VARIABLES = "APOLLO_KUBERNETES_CACHE_ENABLE"; | |
| /** | |
| * Enable Kubernetes cache for Apollo configuration | |
| */ | |
| public static final String APOLLO_KUBERNETES_CACHE_ENABLE = "apollo.cache.kubernetes.enable"; | |
| /** | |
| * Environment variable to enable Kubernetes cache for Apollo configuration | |
| */ | |
| public static final String APOLLO_KUBERNETES_CACHE_ENABLE_ENVIRONMENT_VARIABLES = "APOLLO_KUBERNETES_CACHE_ENABLE"; |
pom.xml
Outdated
| @@ -65,6 +65,7 @@ | |||
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | |||
| <spring-boot.version>2.7.18</spring-boot.version> | |||
| <cat.client.version>3.1.0</cat.client.version> | |||
| <client-java.version>18.0.0</client-java.version> | |||
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Unused Property and Missing CHANGES File
The new property <client-java.version>18.0.0</client-java.version> has been added to pom.xml but is not utilized in any dependency declaration. Additionally, the CHANGES file is missing from the repository.
Please address the following:
- Utilize the
client-java.versionProperty:- Add a dependency in
pom.xmlthat references${client-java.version}to ensure the property is actively used.
- Add a dependency in
- Update the CHANGES Log:
- Create or update the
CHANGESfile to document the addition of the new property, aligning with the PR objectives.
- Create or update the
🔗 Analysis chain
LGTM: New property added for Java client version
The addition of <client-java.version>18.0.0</client-java.version> is a good preparatory step for implementing Kubernetes ConfigMap support. This property will allow for consistent version management across the project.
Please ensure that:
- There's a corresponding dependency in the POM file using this property. If not present in the visible part of the file, you may want to add it.
- The CHANGES log is updated to reflect this addition, as mentioned in the PR objectives.
To verify the usage of this property, you can run:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the property is used in a dependency declaration
grep -n "<version>\${client-java.version}</version>" pom.xml
# Check if CHANGES file exists and has been updated
if [ -f CHANGES ]; then
grep -n "client-java.version" CHANGES
else
echo "CHANGES file not found"
fi
Length of output: 137
| if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty()) { | ||
| log.error("create configmap failed due to null or empty parameter: configMapNamespace={}, name={}", configMapNamespace, name); | ||
| } |
There was a problem hiding this comment.
Ensure input validation halts execution upon failure
The input validation checks for null or empty parameters log an error but do not halt execution. This can lead to exceptions or unintended behavior later in the code. Consider throwing an exception to prevent the methods from proceeding with invalid input.
Apply the following changes to the methods:
-
In
createConfigMap:if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty()) { log.error("create configmap failed due to null or empty parameter: configMapNamespace={}, name={}", configMapNamespace, name); + throw new IllegalArgumentException("ConfigMap namespace and name cannot be null or empty"); } -
In
loadFromConfigMap:if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty()) { log.error("load configmap failed due to null or empty parameter: configMapNamespace={}, name={}", configMapNamespace, name); + throw new IllegalArgumentException("ConfigMap namespace and name cannot be null or empty"); } -
In
getValueFromConfigMap:if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty() || key == null || key.isEmpty()) { log.error("Parameters cannot be null or empty: configMapNamespace={}, name={}", configMapNamespace, name); + throw new IllegalArgumentException("ConfigMap namespace, name, and key cannot be null or empty"); } -
In
updateConfigMap:if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty() || data == null || data.isEmpty()) { log.error("Parameters cannot be null or empty: configMapNamespace={}, name={}", configMapNamespace, name); + return false; }
Also applies to: 99-101, 129-131, 154-156
| * @param data new data | ||
| * @return config map name | ||
| */ | ||
| // TODO 使用client自带的retry机制，设置重试次数,CAS |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address the TODO: Use client's built-in retry mechanism
The TODO comment indicates that the client's built-in retry mechanism should be used instead of custom retry logic. Implementing this can simplify the code and adhere to best practices.
Would you like assistance in implementing the client's built-in retry mechanism for updating the ConfigMap? This can improve reliability and reduce code complexity.
| * @param configMapNamespace configmap namespace | ||
| * @param name config map name (appId) | ||
| * @param data new data | ||
| * @return config map name |
There was a problem hiding this comment.
Update method documentation to reflect correct return type
The Javadoc comment states that the method returns the config map name, but the method actually returns a boolean indicating success.
Update the @return tag to reflect the correct return type:
- * @return config map name
+ * @return true if the update is successful, false otherwise📝 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.
| * @return config map name | |
| * @return true if the update is successful, false otherwise |
| } | ||
| Properties result = propertiesFactory.getPropertiesInstance(); | ||
| result.putAll(configMapProperties); | ||
| logger.info("configmap值：{}", configMapProperties); |
There was a problem hiding this comment.
Avoid logging potentially sensitive configuration properties
Logging the entire configMapProperties object at INFO level may expose sensitive information in the logs. Consider removing this log statement or adjusting it to log only non-sensitive information.
Apply this diff to remove or modify the log statement:
- logger.info("configmap值：{}", configMapProperties);
+ // logger.info("Configuration properties loaded successfully.");📝 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.
| logger.info("configmap值：{}", configMapProperties); | |
| // logger.info("Configuration properties loaded successfully."); |
| if (newProperties.equals(configMapProperties)) { | ||
| return; |
There was a problem hiding this comment.
Add null check to avoid NullPointerException in onRepositoryChange
When newProperties is null, calling newProperties.equals(configMapProperties) will throw a NullPointerException. Ensure that newProperties is not null before comparison.
Apply this diff:
public void onRepositoryChange(String namespace, Properties newProperties) {
+ if (newProperties == null || newProperties.equals(configMapProperties)) {
return;
}
Properties newFileProperties = propertiesFactory.getPropertiesInstance();
newFileProperties.putAll(newProperties);
updateConfigMapProperties(newFileProperties, upstream.getSourceType());
this.fireRepositoryChange(namespace, newProperties);
}📝 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 (newProperties.equals(configMapProperties)) { | |
| return; | |
| if (newProperties == null || newProperties.equals(configMapProperties)) { | |
| return; |
| if (newProperties.equals(configMapProperties)) { | ||
| return; |
There was a problem hiding this comment.
Prevent NullPointerException when comparing properties
In updateConfigMapProperties, comparing newProperties.equals(configMapProperties) without null checks can lead to a NullPointerException if newProperties is null.
Apply this diff to safely compare the properties:
private synchronized void updateConfigMapProperties(Properties newProperties, ConfigSourceType sourceType) {
this.sourceType = sourceType;
+ if (newProperties == null || newProperties.equals(configMapProperties)) {
return;
}
this.configMapProperties = newProperties;
persistConfigMap(configMapProperties);
}📝 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 (newProperties.equals(configMapProperties)) { | |
| return; | |
| if (newProperties == null || newProperties.equals(configMapProperties)) { | |
| return; |
| if (upstreamConfigRepository == null) { | ||
| return; | ||
| } | ||
| //clear previous listener | ||
| if (upstream != null) { | ||
| upstream.removeChangeListener(this); | ||
| } |
There was a problem hiding this comment.
Ensure previous listeners are removed when setting upstream to null
In the setUpstreamRepository method, if upstreamConfigRepository is null, the previous listener is not removed, which may lead to unintended behavior or memory leaks.
Apply this diff to always remove the previous listener:
public void setUpstreamRepository(ConfigRepository upstreamConfigRepository) {
// Clear previous listener if any
if (upstream != null) {
upstream.removeChangeListener(this);
}
if (upstreamConfigRepository == null) {
upstream = null;
return;
}
upstream = upstreamConfigRepository;
upstream.addChangeListener(this);
}Committable suggestion was skipped due to low confidence.
| try { | ||
| logger.info("Start sync from the upstream data source, upstream.getConfig:{}, upstream.getSourceType():{}", upstream.getConfig(), upstream.getSourceType()); | ||
| updateConfigMapProperties(upstream.getConfig(), upstream.getSourceType()); | ||
| return true; | ||
| } catch (Throwable ex) { | ||
| Tracer.logError(ex); | ||
| logger.warn("Sync config from upstream repository {} failed, reason: {}", upstream.getClass(), |
There was a problem hiding this comment.
Handle null properties returned from upstream
In trySyncFromUpstream, if upstream.getConfig() returns null, passing it to updateConfigMapProperties can cause issues. Add a null check to handle this scenario.
Apply this diff to check for null properties:
try {
Properties upstreamProperties = upstream.getConfig();
if (upstreamProperties == null) {
logger.warn("Upstream returned null properties.");
return false;
}
logger.info("Start sync from the upstream data source, upstream.getSourceType(): {}", upstream.getSourceType());
updateConfigMapProperties(upstreamProperties, upstream.getSourceType());
return true;
} catch (Throwable ex) {
Tracer.logError(ex);
logger.warn("Sync config from upstream repository {} failed, reason: {}",
upstream.getClass(), ExceptionUtil.getDetailMessage(ex));
}📝 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.
| try { | |
| logger.info("Start sync from the upstream data source, upstream.getConfig:{}, upstream.getSourceType():{}", upstream.getConfig(), upstream.getSourceType()); | |
| updateConfigMapProperties(upstream.getConfig(), upstream.getSourceType()); | |
| return true; | |
| } catch (Throwable ex) { | |
| Tracer.logError(ex); | |
| logger.warn("Sync config from upstream repository {} failed, reason: {}", upstream.getClass(), | |
| try { | |
| Properties upstreamProperties = upstream.getConfig(); | |
| if (upstreamProperties == null) { | |
| logger.warn("Upstream returned null properties."); | |
| return false; | |
| } | |
| logger.info("Start sync from the upstream data source, upstream.getSourceType(): {}", upstream.getSourceType()); | |
| updateConfigMapProperties(upstreamProperties, upstream.getSourceType()); | |
| return true; | |
| } catch (Throwable ex) { | |
| Tracer.logError(ex); | |
| logger.warn("Sync config from upstream repository {} failed, reason: {}", | |
| upstream.getClass(), ExceptionUtil.getDetailMessage(ex)); |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
apollo-client/src/main/resources/META-INF/additional-spring-configuration-metadata.json (1)
122-128: LGTM with a minor suggestion: Enhance the description forapollo.cache.kubernetes.namespace.The new property for specifying the Kubernetes ConfigMap namespace is well-defined:
- The naming convention is consistent with other Apollo properties.
- The type (String) is appropriate for a namespace.
- The default value ("default") aligns with Kubernetes conventions.
However, the description could be more informative. Consider updating it to:
"Specifies the Kubernetes namespace where the ConfigMap for Apollo caching will be stored."apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java (1)
258-263: LGTM with a suggestion: Consider clearing the system property explicitly.This test method effectively verifies the default behavior of
ConfigUtil.getConfigMapNamespace()when no custom namespace is set. It's concise and focused on a single behavior, which is good.However, to make the test more robust, consider explicitly clearing the system property at the beginning of this test method:
@Test public void testConfigMapNamespaceWithDefault() { System.clearProperty(ApolloClientSystemConsts.APOLLO_CACHE_KUBERNETES_CONFIGMAP_NAMESPACE); ConfigUtil configUtil = new ConfigUtil(); assertEquals(ConfigConsts.KUBERNETES_CACHE_CONFIG_MAP_NAMESPACE_DEFAULT, configUtil.getConfigMapNamespace()); }This ensures that the test is not affected by any leftover system property from previous tests, even if the
tearDownmethod fails to run for some reason.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- CHANGES.md (1 hunks)
- apollo-client/pom.xml (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (5 hunks)
- apollo-client/src/main/resources/META-INF/additional-spring-configuration-metadata.json (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java (2 hunks)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java (2 hunks)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ConfigConsts.java (1 hunks)
- pom.xml (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apollo-client/pom.xml
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ConfigConsts.java
- pom.xml
🧰 Additional context used
🔇 Additional comments (9)
CHANGES.md (1)
8-9:⚠️ Potential issueRefine the changelog to accurately reflect the PR scope
The current changelog entry includes multiple changes that don't align with the PR objectives. Please consider the following adjustments:
- Remove the mention of the Golang client unless it's explicitly part of this PR. The PR objectives only discuss the Java client.
- Focus on the Kubernetes ConfigMap cache support, which is the main feature of this PR.
- Remove or separate the unrelated changes (items 2-4) if they're not part of this specific PR.
Here's a suggested revision for the changelog:
* [Feature] Support Kubernetes ConfigMap cache for Apollo Java client (https://github.com/apolloconfig/apollo-java/pull/79)If the other items are part of this PR, please clarify their relevance in the PR description.
apollo-client/src/main/resources/META-INF/additional-spring-configuration-metadata.json (2)
115-121: LGTM: New propertyapollo.cache.kubernetes.enablelooks good.The new property for enabling Kubernetes ConfigMap cache is well-defined:
- The naming convention is consistent with other Apollo properties.
- The type (Boolean) is appropriate for a feature flag.
- The description is clear and concise.
- The default value (false) ensures opt-in behavior, which is a safe approach.
115-128: Summary: Kubernetes ConfigMap caching support added successfully.The two new properties,
apollo.cache.kubernetes.enableandapollo.cache.kubernetes.namespace, have been successfully added to the configuration metadata. These additions:
- Enable users to opt-in to Kubernetes ConfigMap caching.
- Allow specification of the Kubernetes namespace for the ConfigMap.
These changes align well with the PR objectives of enhancing Apollo client's functionality in Kubernetes environments. The new configuration options provide the necessary flexibility for users to leverage Kubernetes ConfigMaps as a persistent storage option for Apollo configurations.
apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java (2)
50-50: LGTM: Proper cleanup of the new system property.The addition of clearing the
APOLLO_CACHE_KUBERNETES_CONFIGMAP_NAMESPACEsystem property in thetearDownmethod ensures a clean state for subsequent tests. This is a good practice and aligns well with the new tests being added.
247-256: LGTM: Well-structured test for custom ConfigMap namespace.This test method effectively verifies that
ConfigUtilcorrectly retrieves the custom ConfigMap namespace when set as a system property. The test follows the Arrange-Act-Assert pattern and is focused on a single behavior, which are good practices in unit testing.apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (4)
381-389: New method 'getConfigMapNamespace()' correctly retrieves the configuration map namespaceThe method is well-implemented and handles default values appropriately.
391-407: 'getCustomizedConfigMapNamespace()' method effectively fetches the namespace from multiple sourcesThe method follows the correct order of precedence when retrieving configuration settings from system properties, environment variables, server properties, and application properties.
512-514: Accessor method 'isPropertyKubernetesCacheEnabled()' correctly exposes the property's stateThe method provides a proper way to check if the Kubernetes cache is enabled.
542-546: Initialization method 'initPropertyKubernetesCacheEnabled()' properly initializes the propertyThe method correctly retrieves the property's value using
getPropertyBooleanand sets the field appropriately.
| @@ -71,6 +71,7 @@ public class ConfigUtil { | |||
| private boolean propertyNamesCacheEnabled = false; | |||
| private boolean propertyFileCacheEnabled = true; | |||
| private boolean overrideSystemProperties = true; | |||
| private boolean PropertyKubernetesCacheEnabled = false; | |||
There was a problem hiding this comment.
Rename 'PropertyKubernetesCacheEnabled' to 'propertyKubernetesCacheEnabled' to follow Java naming conventions
In Java, field names should start with a lowercase letter and use camelCase. Renaming this field enhances code consistency and readability.
Apply this diff to rename the field:
-private boolean PropertyKubernetesCacheEnabled = false;
+private boolean propertyKubernetesCacheEnabled = false;📝 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 boolean PropertyKubernetesCacheEnabled = false; | |
| private boolean propertyKubernetesCacheEnabled = false; |
| @@ -479,6 +509,10 @@ | |||
| return propertyFileCacheEnabled; | |||
| } | |||
|
|
|||
| public boolean isPropertyKubernetesCacheEnabled() { | |||
| return PropertyKubernetesCacheEnabled; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update reference to 'PropertyKubernetesCacheEnabled' after renaming
After renaming the field, update the return statement in isPropertyKubernetesCacheEnabled() accordingly.
Apply this diff:
-public boolean isPropertyKubernetesCacheEnabled() {
- return PropertyKubernetesCacheEnabled;
+public boolean isPropertyKubernetesCacheEnabled() {
+ return propertyKubernetesCacheEnabled;Committable suggestion was skipped due to low confidence.
| PropertyKubernetesCacheEnabled = getPropertyBoolean(ApolloClientSystemConsts.APOLLO_KUBERNETES_CACHE_ENABLE, | ||
| ApolloClientSystemConsts.APOLLO_KUBERNETES_CACHE_ENABLE_ENVIRONMENT_VARIABLES, | ||
| PropertyKubernetesCacheEnabled); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update assignment in 'initPropertyKubernetesCacheEnabled()' after field renaming
Ensure the assignment reflects the new field name to maintain consistency.
Apply this diff:
-PropertyKubernetesCacheEnabled = getPropertyBoolean(ApolloClientSystemConsts.APOLLO_KUBERNETES_CACHE_ENABLE,
- ApolloClientSystemConsts.APOLLO_KUBERNETES_CACHE_ENABLE_ENVIRONMENT_VARIABLES,
- PropertyKubernetesCacheEnabled);
+propertyKubernetesCacheEnabled = getPropertyBoolean(ApolloClientSystemConsts.APOLLO_KUBERNETES_CACHE_ENABLE,
+ ApolloClientSystemConsts.APOLLO_KUBERNETES_CACHE_ENABLE_ENVIRONMENT_VARIABLES,
+ propertyKubernetesCacheEnabled);📝 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.
| PropertyKubernetesCacheEnabled = getPropertyBoolean(ApolloClientSystemConsts.APOLLO_KUBERNETES_CACHE_ENABLE, | |
| ApolloClientSystemConsts.APOLLO_KUBERNETES_CACHE_ENABLE_ENVIRONMENT_VARIABLES, | |
| PropertyKubernetesCacheEnabled); | |
| propertyKubernetesCacheEnabled = getPropertyBoolean(ApolloClientSystemConsts.APOLLO_KUBERNETES_CACHE_ENABLE, | |
| ApolloClientSystemConsts.APOLLO_KUBERNETES_CACHE_ENABLE_ENVIRONMENT_VARIABLES, | |
| propertyKubernetesCacheEnabled); |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
apollo-client/src/test/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManagerTest.java (3)
47-69: LGTM: Comprehensive test for ConfigMap creation.The test method
testCreateConfigMapSuccesseffectively verifies the successful creation of a ConfigMap, including the correct API call and return value validation.Consider adding an assertion to verify the content of the created ConfigMap:
ArgumentCaptor<V1ConfigMap> configMapCaptor = ArgumentCaptor.forClass(V1ConfigMap.class); verify(coreV1Api, times(1)).createNamespacedConfigMap(eq(namespace), configMapCaptor.capture(), isNull(), isNull(), isNull(), isNull()); assertEquals(data, configMapCaptor.getValue().getData());This would ensure that the ConfigMap is created with the correct data.
89-128: LGTM: Comprehensive tests for value retrieval from ConfigMap.The
testGetValueFromConfigMapReturnsValueandtestGetValueFromConfigMapKeyNotFoundmethods effectively cover the scenarios of retrieving existing and non-existing keys from a ConfigMap.Consider adding a test case for when the ConfigMap itself doesn't exist:
@Test public void testGetValueFromNonExistentConfigMap() throws Exception { String namespace = "default"; String name = "nonExistentConfigMap"; String key = "someKey"; when(coreV1Api.readNamespacedConfigMap(name, namespace, null)).thenThrow(new ApiException("Not Found")); String actualValue = kubernetesManager.getValueFromConfigMap(namespace, name, key); assertNull(actualValue); }This would ensure that the method handles the case of a non-existent ConfigMap gracefully.
1-208: Overall: Well-structured and comprehensive test suite.The
KubernetesManagerTestclass provides thorough coverage of theKubernetesManagerfunctionality. The tests are well-organized, make proper use of mocking and assertions, and include clear comments in both English and Chinese.Consider enhancing the test suite with the following:
- Edge case testing: Add tests for scenarios like network failures or API rate limiting.
- Negative testing: Include more tests for invalid inputs or error conditions.
- Parameterized tests: Use JUnit's
@ParameterizedTestfor testing multiple input variations.Example for a parameterized test:
@ParameterizedTest @CsvSource({ "default,configMap1,key1,value1", "kube-system,configMap2,key2,value2" }) void testGetValueFromConfigMapWithMultipleInputs(String namespace, String name, String key, String expectedValue) throws Exception { // Test implementation }These additions would further improve the robustness and coverage of the test suite.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (5 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManagerTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepositoryTest.java (1 hunks)
- pom.xml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepositoryTest.java
- pom.xml
🧰 Additional context used
🔇 Additional comments (2)
apollo-client/src/test/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManagerTest.java (2)
33-45: LGTM: Proper test setup and initialization.The test class is well-structured with appropriate use of JUnit and Mockito for mocking Kubernetes API interactions. The
setUpmethod correctly initializes the mockedCoreV1ApiandKubernetesManagerinstances, which is crucial for isolated unit testing.
154-206: LGTM: Comprehensive tests for ConfigMap existence checks.The
testCheckConfigMapExistWhenConfigMapExists,testCheckConfigMapExistWhenConfigMapDoesNotExist, andtestCheckConfigMapExistWithEmptyNamespaceAndNamemethods effectively cover all scenarios for checking ConfigMap existence. The tests verify the correct behavior when:
- The ConfigMap exists
- The ConfigMap doesn't exist
- The namespace and name are empty
This comprehensive coverage ensures that the
checkConfigMapExistmethod behaves correctly under various conditions.
| /** | ||
| * 测试 createConfigMap 传入 null 作为数据，正常执行 | ||
| */ | ||
| @Test | ||
| public void testCreateConfigMapNullData() throws Exception { | ||
| // arrange | ||
| String namespace = "default"; | ||
| String name = "testConfigMap"; | ||
| Map<String, String> data = null; | ||
|
|
||
| // act | ||
| String result = kubernetesManager.createConfigMap(namespace, name, data); | ||
|
|
||
| // assert | ||
| verify(coreV1Api, times(1)).createNamespacedConfigMap(eq(namespace), any(V1ConfigMap.class), isNull(), isNull(), isNull(),isNull()); | ||
| assert name.equals(result); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance null data handling test.
While the testCreateConfigMapNullData method ensures that no exception is thrown when null data is passed, it could be improved to verify the actual behavior of the createConfigMap method in this scenario.
Consider modifying the test to check if an empty data map is created when null is passed:
ArgumentCaptor<V1ConfigMap> configMapCaptor = ArgumentCaptor.forClass(V1ConfigMap.class);
verify(coreV1Api, times(1)).createNamespacedConfigMap(eq(namespace), configMapCaptor.capture(), isNull(), isNull(), isNull(), isNull());
assertTrue(configMapCaptor.getValue().getData() == null || configMapCaptor.getValue().getData().isEmpty());This would ensure that the method handles null data appropriately by either setting the data to null or creating an empty map.
| /** | ||
| * 测试updateConfigMap成功的情况 | ||
| */ | ||
| @Test | ||
| public void testUpdateConfigMapSuccess() throws Exception { | ||
| // arrange | ||
| String namespace = "default"; | ||
| String name = "testConfigMap"; | ||
| Map<String, String> data = new HashMap<>(); | ||
| data.put("key", "value"); | ||
| V1ConfigMap configMap = new V1ConfigMap(); | ||
| configMap.metadata(new V1ObjectMeta().name(name).namespace(namespace)); | ||
| configMap.data(data); | ||
|
|
||
| when(coreV1Api.readNamespacedConfigMap(name, namespace, null)).thenReturn(configMap); | ||
| when(coreV1Api.replaceNamespacedConfigMap(name, namespace, configMap, null, null, null, null)).thenReturn(configMap); | ||
|
|
||
| // act | ||
| Boolean success = kubernetesManager.updateConfigMap(namespace, name, data); | ||
|
|
||
| // assert | ||
| assertTrue(success); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance ConfigMap update test.
The testUpdateConfigMapSuccess method effectively verifies the API calls and return value for updating a ConfigMap. However, it could be improved to ensure that the ConfigMap is updated with the correct data.
Consider adding assertions to verify the content of the updated ConfigMap:
ArgumentCaptor<V1ConfigMap> configMapCaptor = ArgumentCaptor.forClass(V1ConfigMap.class);
verify(coreV1Api, times(1)).replaceNamespacedConfigMap(eq(name), eq(namespace), configMapCaptor.capture(), isNull(), isNull(), isNull(), isNull());
assertEquals(data, configMapCaptor.getValue().getData());This would ensure that the ConfigMap is updated with the correct data.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@shoothzj PTAL |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (11)
apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java (5)
47-69: Enhance assertion in testCreateConfigMapSuccess.The test case for creating a ConfigMap is well-structured. However, the assertion can be improved for better clarity and completeness. Instead of using
assert, consider using JUnit'sassertEqualsfor a more descriptive failure message if the test fails.Replace:
assert name.equals(result);with:
assertEquals("The returned name should match the input name", name, result);Additionally, you could verify that the created ConfigMap contains the expected data:
verify(coreV1Api, times(1)).createNamespacedConfigMap( eq(namespace), argThat(cm -> cm.getData().equals(data)), isNull(), isNull(), isNull(), isNull() );These changes will make the test more robust and easier to debug if it fails.
71-87: Enhance testCreateConfigMapNullData with additional checks.While this test case correctly verifies that the method executes without errors when null data is passed, it could be more comprehensive. Consider the following improvements:
- Verify that the created ConfigMap has a null or empty data map:
verify(coreV1Api, times(1)).createNamespacedConfigMap( eq(namespace), argThat(cm -> cm.getData() == null || cm.getData().isEmpty()), isNull(), isNull(), isNull(), isNull() );
- Check if the method handles the null data gracefully, perhaps by creating an empty map instead of null:
V1ConfigMap capturedConfigMap = ArgumentCaptor.forClass(V1ConfigMap.class).capture(); verify(coreV1Api).createNamespacedConfigMap(eq(namespace), capturedConfigMap.capture(), isNull(), isNull(), isNull(), isNull()); assertNotNull("Data in ConfigMap should not be null", capturedConfigMap.getValue().getData()); assertTrue("Data in ConfigMap should be empty", capturedConfigMap.getValue().getData().isEmpty());These additions will ensure that the method behaves correctly and consistently when dealing with null input data.
89-128: Consider adding edge cases to ConfigMap value retrieval tests.The current tests for
getValueFromConfigMapcover the basic scenarios of existing and non-existing keys. To increase test coverage and robustness, consider adding the following test cases:
- Test retrieving a value when the ConfigMap exists but has no data:
@Test public void testGetValueFromEmptyConfigMap() throws Exception { String namespace = "default"; String name = "emptyConfigMap"; String key = "anyKey"; V1ConfigMap configMap = new V1ConfigMap(); when(coreV1Api.readNamespacedConfigMap(name, namespace, null)).thenReturn(configMap); String actualValue = kubernetesManager.getValueFromConfigMap(namespace, name, key); assertNull(actualValue); }
- Test behavior when the ConfigMap doesn't exist:
@Test public void testGetValueFromNonExistentConfigMap() throws Exception { String namespace = "default"; String name = "nonExistentConfigMap"; String key = "anyKey"; when(coreV1Api.readNamespacedConfigMap(name, namespace, null)).thenThrow(new ApiException("Not Found")); String actualValue = kubernetesManager.getValueFromConfigMap(namespace, name, key); assertNull(actualValue); }
- Test retrieving a value with an empty key:
@Test public void testGetValueFromConfigMapEmptyKey() throws Exception { String namespace = "default"; String name = "testConfigMap"; String key = ""; V1ConfigMap configMap = new V1ConfigMap(); configMap.putDataItem(key, "emptyKeyValue"); when(coreV1Api.readNamespacedConfigMap(name, namespace, null)).thenReturn(configMap); String actualValue = kubernetesManager.getValueFromConfigMap(namespace, name, key); assertEquals("emptyKeyValue", actualValue); }These additional tests will help ensure that the
getValueFromConfigMapmethod handles various edge cases correctly.
130-152: Enhance testUpdateConfigMapSuccess and add failure scenarios.The current test for updating a ConfigMap covers the success scenario, but it could be more comprehensive. Consider the following improvements:
- Verify that the updated ConfigMap contains the new data:
ArgumentCaptor<V1ConfigMap> configMapCaptor = ArgumentCaptor.forClass(V1ConfigMap.class); verify(coreV1Api).replaceNamespacedConfigMap(eq(name), eq(namespace), configMapCaptor.capture(), isNull(), isNull(), isNull(), isNull()); assertEquals("Updated data should match input data", data, configMapCaptor.getValue().getData());
- Add a test for updating a non-existent ConfigMap:
@Test public void testUpdateConfigMapNonExistent() throws Exception { String namespace = "default"; String name = "nonExistentConfigMap"; Map<String, String> data = new HashMap<>(); data.put("key", "value"); when(coreV1Api.readNamespacedConfigMap(name, namespace, null)).thenThrow(new ApiException("Not Found")); Boolean success = kubernetesManager.updateConfigMap(namespace, name, data); assertFalse(success); }
- Test updating with null or empty data:
@Test public void testUpdateConfigMapWithNullData() throws Exception { String namespace = "default"; String name = "testConfigMap"; Map<String, String> data = null; V1ConfigMap existingConfigMap = new V1ConfigMap(); existingConfigMap.setData(new HashMap<>()); when(coreV1Api.readNamespacedConfigMap(name, namespace, null)).thenReturn(existingConfigMap); Boolean success = kubernetesManager.updateConfigMap(namespace, name, data); assertTrue(success); verify(coreV1Api).replaceNamespacedConfigMap(eq(name), eq(namespace), argThat(cm -> cm.getData() == null || cm.getData().isEmpty()), isNull(), isNull(), isNull(), isNull()); }These additions will provide a more comprehensive test suite for the update functionality.
154-207: Improve error handling and add edge cases for checkConfigMapExist tests.The current tests for
checkConfigMapExistcover the main scenarios, but can be improved:
- In
testCheckConfigMapExistWhenConfigMapDoesNotExist, catch a specificApiExceptioninstead of a generic one:doThrow(new ApiException(404, "Not Found")).when(coreV1Api).readNamespacedConfigMap(name, namespace, null);
- Add a test for handling unexpected exceptions:
@Test public void testCheckConfigMapExistUnexpectedException() throws Exception { String namespace = "default"; String name = "testConfigMap"; doThrow(new RuntimeException("Unexpected error")).when(coreV1Api).readNamespacedConfigMap(name, namespace, null); boolean result = kubernetesManager.checkConfigMapExist(namespace, name); assertFalse(result); // Optionally, verify that the exception is logged }
- Test with null namespace and name:
@Test public void testCheckConfigMapExistWithNullNamespaceAndName() { boolean result = kubernetesManager.checkConfigMapExist(null, null); assertFalse(result); }
- Test with valid namespace but empty name, and vice versa:
@Test public void testCheckConfigMapExistWithEmptyName() { boolean result = kubernetesManager.checkConfigMapExist("default", ""); assertFalse(result); } @Test public void testCheckConfigMapExistWithEmptyNamespace() { boolean result = kubernetesManager.checkConfigMapExist("", "configMap"); assertFalse(result); }These additional tests will ensure that the
checkConfigMapExistmethod handles various edge cases and error conditions correctly.apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java (2)
123-123: Translate TODO comments to EnglishThe TODO comment is in Chinese. For consistency and collaboration, it's best to use English in code comments.
Please translate the TODO comment to English.
35-36: Declare fields as finalSince
clientandcoreV1Apiare initialized once and not modified afterward, declaring them asfinalenhances code clarity and thread safety.- private ApiClient client; - private CoreV1Api coreV1Api; + private final ApiClient client; + private final CoreV1Api coreV1Api;apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java (4)
90-97: Translate non-English comments to English for consistencyThe comments at lines 90-92 are written in Chinese. To maintain consistency and readability for all contributors, please translate the comments into English.
Apply this change:
- // TODO 兜底key怎么设计不会冲突(cluster初始化时已经设置了层级) - // cluster: 用户定义>idc>default,所以已经不需要额外层级设置了 + // TODO: Design a fallback key that avoids conflicts (cluster hierarchy is already set during initialization) + // Cluster hierarchy: user-defined > idc > default, so additional hierarchy settings are unnecessary
110-110: Translate non-English comments to English for consistencyThe comment at line 110 is in Chinese. Please translate it to English for consistency.
Apply this change:
- // 初始化configmap + // Initialize ConfigMap
139-143: Translate non-English comments to English for consistencyThe comments from lines 139 to 143 are written in Chinese. Translating them into English will improve collaboration and maintain consistency in the codebase.
Apply this change:
/** - * TODO 测试点： - * 1. 从上游成功恢复（开启文件存储） - * 2. 从上游成功恢复（没开启文件存储，从remote） - * 3. 从k8s成功恢复 */ +/** + * TODO Test Points: + * 1. Successfully recover from upstream (with file storage enabled) + * 2. Successfully recover from upstream (without file storage, from remote) + * 3. Successfully recover from Kubernetes */
163-163: Translate non-English comments to English for consistencyThe comment at line 163 is in Chinese. Please translate it to English.
Apply this change:
- // 设置上游数据源 + // Set the upstream data source
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepositoryTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepositoryTest.java
🧰 Additional context used
🔇 Additional comments (5)
apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java (2)
1-32: LGTM: File header and imports are appropriate.The file includes the necessary copyright information, license details, and relevant imports for JUnit, Mockito, and the Kubernetes client. This sets up the test class correctly.
1-208: Overall, well-structured test suite with room for improvement.The
KubernetesManagerTestclass provides a comprehensive set of unit tests for theKubernetesManagerclass, covering the main functionalities such as creating, updating, and checking the existence of ConfigMaps. The use of Mockito for mocking dependencies is appropriate and allows for isolated testing of theKubernetesManagerclass.Main strengths:
- Good coverage of basic scenarios for each method.
- Proper use of JUnit annotations and Mockito mocking.
- Clear and descriptive test method names.
Areas for improvement:
- Consider using constructor injection for
KubernetesManagerto improve testability.- Enhance assertions in several test methods to be more specific and descriptive.
- Add more edge cases and error handling scenarios, particularly for null or empty inputs.
- Improve consistency in error handling across different test methods.
By addressing these points, the test suite will become more robust and provide even better coverage of the
KubernetesManagerfunctionality. This will increase confidence in the correctness of the implementation and make future maintenance easier.apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java (3)
220-221: Correct theJoinerusage by passing the cluster name instead ofconfigUtilAt line 220,
configUtilis passed toJoiner.on().join(), which will invokeconfigUtil.toString(), likely not providing the intended value. Instead, you should pass the cluster name obtained fromconfigUtil.getCluster().Apply this diff to fix the issue:
- String fallbackKey = Joiner.on(ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR).join(configUtil, namespace); + String fallbackKey = Joiner.on(ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR).join(configUtil.getCluster(), namespace);This ensures the correct cluster name is used in the key.
273-274: Add null check to avoidNullPointerExceptioninonRepositoryChangeWhen
newPropertiesisnull, callingnewProperties.equals(configMapProperties)will throw aNullPointerException. Ensure thatnewPropertiesis notnullbefore comparison.
110-110: InitializepropertiesFactorybefore useThe
propertiesFactoryobject is used at lines 149, 225, and 276 but is never declared or initialized in the class. This will result in aNullPointerExceptionat runtime.
| public class KubernetesManagerTest { | ||
|
|
||
| private CoreV1Api coreV1Api; | ||
| private KubernetesManager kubernetesManager; | ||
|
|
||
| @Before | ||
| public void setUp() { | ||
| coreV1Api = mock(CoreV1Api.class); | ||
| kubernetesManager = new KubernetesManager(coreV1Api); | ||
|
|
||
| MockInjector.setInstance(KubernetesManager.class, kubernetesManager); | ||
| MockInjector.setInstance(CoreV1Api.class, coreV1Api); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using constructor injection for KubernetesManager.
The setup method correctly initializes the mocks and sets up the MockInjector. However, to improve testability and adhere to dependency injection principles, consider modifying the KubernetesManager class to use constructor injection instead of field injection. This change would eliminate the need for MockInjector in this test class.
Example:
public class KubernetesManager {
private final CoreV1Api coreV1Api;
public KubernetesManager(CoreV1Api coreV1Api) {
this.coreV1Api = coreV1Api;
}
// ... rest of the class
}Then in the test setup:
@Before
public void setUp() {
coreV1Api = mock(CoreV1Api.class);
kubernetesManager = new KubernetesManager(coreV1Api);
}This approach would make the dependencies explicit and improve the overall design.
| // TODO 使用client自带的retry机制，设置重试次数,CAS | ||
| public boolean updateConfigMap(String configMapNamespace, String name, Map<String, String> data) { | ||
| if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty() || data == null || data.isEmpty()) { | ||
| log.error("Parameters can not be null or empty: configMapNamespace={}, name={}", configMapNamespace, name); | ||
| return false; | ||
| } | ||
|
|
||
| // retry | ||
| int maxRetries = 5; | ||
| int retryCount = 0; | ||
| long waitTime = 100; | ||
|
|
||
| while (retryCount < maxRetries) { | ||
| try { | ||
| V1ConfigMap configmap = coreV1Api.readNamespacedConfigMap(name, configMapNamespace, null); | ||
| configmap.setData(data); | ||
| coreV1Api.replaceNamespacedConfigMap(name, configMapNamespace, configmap, null, null, null, null); | ||
| return true; | ||
| } catch (ApiException e) { | ||
| if (e.getCode() == 409) { | ||
| retryCount++; | ||
| log.warn("Conflict occurred, retrying... (" + retryCount + ")"); | ||
| try { | ||
| TimeUnit.MILLISECONDS.sleep(waitTime); | ||
| } catch (InterruptedException ie) { | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
| waitTime = Math.min(waitTime * 2, 1000); | ||
| } else { | ||
| System.err.println("Error updating ConfigMap: " + e.getMessage()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Utilize the client's built-in retry mechanism
Instead of implementing custom retry logic, consider leveraging the Kubernetes client's built-in retry features for better efficiency and reliability.
Explore and implement the client's retry mechanism to simplify the code.
| public class KubernetesManager { | ||
| private ApiClient client; | ||
| private CoreV1Api coreV1Api; | ||
|
|
||
| private final Logger log = LoggerFactory.getLogger(this.getClass()); | ||
|
|
||
| public KubernetesManager() { | ||
| try { | ||
| client = Config.defaultClient(); | ||
| Configuration.setDefaultApiClient(client); | ||
| coreV1Api = new CoreV1Api(client); | ||
| } catch (Exception e) { | ||
| String errorMessage = "Failed to initialize Kubernetes client: " + e.getMessage(); | ||
| log.error(errorMessage, e); | ||
| throw new RuntimeException(errorMessage, e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using dependency injection for Kubernetes client
As a @Service class, it's preferable to inject dependencies rather than creating them internally. This enhances testability and adheres to best practices.
Inject ApiClient or CoreV1Api via constructor or setter injection.
| } else { | ||
| System.err.println("Error updating ConfigMap: " + e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Use logger instead of System.err for error messages
Consistently using the logger for error messages ensures uniform logging and better control over log outputs.
Replace System.err.println with log.error.
} else {
- System.err.println("Error updating ConfigMap: " + e.getMessage());
+ log.error("Error updating ConfigMap: {}", e.getMessage(), 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.
| } else { | |
| System.err.println("Error updating ConfigMap: " + e.getMessage()); | |
| } | |
| } | |
| } else { | |
| log.error("Error updating ConfigMap: {}", e.getMessage(), e); | |
| } | |
| } |
| V1ConfigMap configMap = coreV1Api.readNamespacedConfigMap(name, configMapNamespace, null); | ||
| if (!Objects.requireNonNull(configMap.getData()).containsKey(key)) { | ||
| throw new RuntimeException(String.format("Specified key not found in ConfigMap: %s, configMapNamespace: %s, name: %s", name, configMapNamespace, name)); | ||
| } |
There was a problem hiding this comment.
Potential NullPointerException when accessing ConfigMap data
Using Objects.requireNonNull(configMap.getData()) can throw a NullPointerException if getData() returns null. This might happen if the ConfigMap exists but has no data.
Check if configMap.getData() is null and handle the scenario appropriately.
- if (!Objects.requireNonNull(configMap.getData()).containsKey(key)) {
+ Map<String, String> data = configMap.getData();
+ if (data == null || !data.containsKey(key)) {
throw new RuntimeException(String.format("Specified key not found in ConfigMap: %s, configMapNamespace: %s, name: %s", name, configMapNamespace, name));
}📝 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.
| V1ConfigMap configMap = coreV1Api.readNamespacedConfigMap(name, configMapNamespace, null); | |
| if (!Objects.requireNonNull(configMap.getData()).containsKey(key)) { | |
| throw new RuntimeException(String.format("Specified key not found in ConfigMap: %s, configMapNamespace: %s, name: %s", name, configMapNamespace, name)); | |
| } | |
| V1ConfigMap configMap = coreV1Api.readNamespacedConfigMap(name, configMapNamespace, null); | |
| Map<String, String> data = configMap.getData(); | |
| if (data == null || !data.containsKey(key)) { | |
| throw new RuntimeException(String.format("Specified key not found in ConfigMap: %s, configMapNamespace: %s, name: %s", name, configMapNamespace, name)); | |
| } |
| } catch (Exception e) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Exceptions are swallowed without logging
In getValueFromConfigMap, exceptions are caught but not logged, which can make debugging difficult.
Add logging to capture exception details before returning.
} catch (Exception e) {
+ log.error("Error getting value from ConfigMap: {}", e.getMessage(), e);
return 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.
| } catch (Exception e) { | |
| return null; | |
| } | |
| } catch (Exception e) { | |
| log.error("Error getting value from ConfigMap: {}", e.getMessage(), e); | |
| return null; | |
| } |
| public String createConfigMap(String configMapNamespace, String name, Map<String, String> data) { | ||
| if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty()) { | ||
| log.error("create configmap failed due to null or empty parameter: configMapNamespace={}, name={}", configMapNamespace, name); | ||
| } | ||
| V1ConfigMap configMap = buildConfigMap(name, configMapNamespace, data); |
There was a problem hiding this comment.
Missing error handling after parameter validation
After detecting null or empty parameters in createConfigMap, the method should halt execution to prevent potential NullPointerException or unexpected behavior.
Consider returning early or throwing an exception after logging the error.
if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty()) {
log.error("create configmap failed due to null or empty parameter: configMapNamespace={}, name={}", configMapNamespace, name);
+ return null; // Or throw an exception to prevent further execution
}📝 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 createConfigMap(String configMapNamespace, String name, Map<String, String> data) { | |
| if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty()) { | |
| log.error("create configmap failed due to null or empty parameter: configMapNamespace={}, name={}", configMapNamespace, name); | |
| } | |
| V1ConfigMap configMap = buildConfigMap(name, configMapNamespace, data); | |
| public String createConfigMap(String configMapNamespace, String name, Map<String, String> data) { | |
| if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty()) { | |
| log.error("create configmap failed due to null or empty parameter: configMapNamespace={}, name={}", configMapNamespace, name); | |
| return null; // Or throw an exception to prevent further execution | |
| } | |
| V1ConfigMap configMap = buildConfigMap(name, configMapNamespace, data); |
| try { | ||
| log.info("Check whether ConfigMap exists, configMapName: {}", configMapName); | ||
| coreV1Api.readNamespacedConfigMap(configMapName, configMapNamespace, null); | ||
| return true; | ||
| } catch (Exception e) { | ||
| // configmap not exist | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Differentiate between not found and other exceptions
In checkConfigMapExist, catching all exceptions and assuming the ConfigMap doesn't exist may mask other issues.
Catch ApiException and check the status code to distinguish between a missing ConfigMap and other errors.
try {
coreV1Api.readNamespacedConfigMap(configMapName, configMapNamespace, null);
return true;
} catch (ApiException e) {
if (e.getCode() == 404) {
// ConfigMap not found
return false;
} else {
+ log.error("Error checking ConfigMap existence: {}", e.getMessage(), e);
throw new RuntimeException("Error checking ConfigMap existence", e);
}
}Committable suggestion was skipped due to low confidence.
|
|
||
| this.setConfigMapKey(configUtil.getCluster(), namespace); | ||
| this.setConfigMapName(configUtil.getAppId(), false); | ||
| this.setUpstreamRepository(upstream); |
There was a problem hiding this comment.
Initialize or pass the upstream repository in the constructor
In the constructor, this.setUpstreamRepository(upstream); is called, but the upstream variable is not initialized or passed as a parameter. This may lead to unexpected behavior if upstream is null or not intended.
Consider passing upstream as a parameter to the constructor or initializing it before calling setUpstreamRepository(upstream);.
| public K8sConfigMapConfigRepository(String namespace, KubernetesManager kubernetesManager) { | ||
| this.namespace = namespace; | ||
| configUtil = ApolloInjector.getInstance(ConfigUtil.class); | ||
| kubernetesManager = ApolloInjector.getInstance(KubernetesManager.class); |
There was a problem hiding this comment.
Fix the assignment of the kubernetesManager field in the constructor
In the constructor, the parameter kubernetesManager is shadowing the class field kubernetesManager. As a result, the assignment at line 69 does not set the class field but instead reassigns the parameter. This means that the kubernetesManager field remains uninitialized, which can lead to NullPointerException when it is used later in the class.
Apply this diff to correct the assignment:
public K8sConfigMapConfigRepository(String namespace, KubernetesManager kubernetesManager) {
this.namespace = namespace;
configUtil = ApolloInjector.getInstance(ConfigUtil.class);
- kubernetesManager = ApolloInjector.getInstance(KubernetesManager.class);
+ this.kubernetesManager = kubernetesManager != null ? kubernetesManager : ApolloInjector.getInstance(KubernetesManager.class);
configMapNamespace = configUtil.getConfigMapNamespace();
this.setConfigMapKey(configUtil.getCluster(), namespace);
this.setConfigMapName(configUtil.getAppId(), false);
// Ensure that the upstream repository is set appropriately if needed
}📝 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 K8sConfigMapConfigRepository(String namespace, KubernetesManager kubernetesManager) { | |
| this.namespace = namespace; | |
| configUtil = ApolloInjector.getInstance(ConfigUtil.class); | |
| kubernetesManager = ApolloInjector.getInstance(KubernetesManager.class); | |
| public K8sConfigMapConfigRepository(String namespace, KubernetesManager kubernetesManager) { | |
| this.namespace = namespace; | |
| configUtil = ApolloInjector.getInstance(ConfigUtil.class); | |
| this.kubernetesManager = kubernetesManager != null ? kubernetesManager : ApolloInjector.getInstance(KubernetesManager.class); |
What's the purpose of this PR
解决Apollo客户端在Kubernetes环境下因服务端宕机或Pod重启导致配置信息文件丢失的问题。通过使用Kubernetes ConfigMap作为新的持久化存储方案，提高配置信息的可靠性和容错性。
Solve the problem of Apollo client configuration information files being lost due to server downtime or Pod restart in the Kubernetes environment. By using Kubernetes ConfigMap as a new persistent storage solution, the reliability and fault tolerance of configuration information are improved.
discussion apolloconfig/apollo#5210
Brief changelog
XXXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.Summary by CodeRabbit
New Features
Bug Fixes
Tests
K8sConfigMapConfigRepositoryandKubernetesManagerto ensure functionality and reliability.Documentation