[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
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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.
| 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
apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java
Outdated
Show resolved
Hide resolved
apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/Env.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java
Outdated
Show resolved
Hide resolved
apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java
Outdated
Show resolved
Hide resolved
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.
apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java
Show resolved
Hide resolved
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; |
...-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java
Show resolved
Hide resolved
| 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 |
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
Show resolved
Hide resolved
| } | ||
| } | ||
| } | ||
| return retryCount < maxRetries; |
| /** | ||
| * kubernetes configmap cache namespace | ||
| */ | ||
| public static final String APOLLO_CACHE_KUBERNETES_CONFIGMAP_NAMESPACE = "apollo.cache.kubernetes.configmap-namespace"; |
There was a problem hiding this comment.
apollo.cache.kubernetes.namespace, maybe better
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java (1)
247-263: Consider adding test cases for edge scenarios.The current tests effectively cover the basic scenarios for K8s namespace configuration. However, consider adding tests for:
- Invalid namespace values
- Empty or whitespace-only namespace
- Error scenarios
Example test case:
@Test public void testK8sNamespaceWithInvalidValue() { String invalidNamespace = " "; // or null System.setProperty(ApolloClientSystemConsts.APOLLO_CACHE_KUBERNETES_NAMESPACE, invalidNamespace); ConfigUtil configUtil = new ConfigUtil(); // Verify behavior with invalid input - adjust assertion based on expected behavior assertEquals(ConfigConsts.KUBERNETES_CACHE_CONFIG_MAP_NAMESPACE_DEFAULT, configUtil.getK8sNamespace()); }apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (2)
381-389: Add Javadoc for the getK8sNamespace method.The method should be documented to explain:
- Its purpose in retrieving the Kubernetes namespace
- The order of precedence for namespace resolution
- The default value when no custom namespace is configured
Add this documentation:
+ /** + * Get the Kubernetes namespace for ConfigMap storage. + * + * @return the configured namespace from system properties, environment variables, + * server.properties, or app.properties. If not configured, returns the default namespace. + */ public String getK8sNamespace() {
512-514: Add Javadoc for the isPropertyKubernetesCacheEnabled method.Document the purpose and usage of this configuration flag.
Add this documentation:
+ /** + * Check if Kubernetes ConfigMap cache is enabled. + * + * @return true if Kubernetes ConfigMap should be used for caching configuration, + * false otherwise + */ public boolean isPropertyKubernetesCacheEnabled() {apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java (1)
89-91: Translate comments to English for consistencySeveral comments in the code are written in Chinese, such as at lines 89 and 156. For consistency and maintainability, consider translating all comments to English.
Also applies to: 156-158
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- 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/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (5 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)
- 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)
✅ Files skipped from review due to trivial changes (1)
- apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
🚧 Files skipped from review as they are similar to previous changes (3)
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepositoryTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java
🧰 Additional context used
🔇 Additional comments (5)
apollo-core/src/main/java/com/ctrip/framework/apollo/core/ConfigConsts.java (2)
23-23: New constants align well with Kubernetes integrationThe new constants follow good practices:
APOLLO_CONFIG_CACHE: Clear prefix for cache identificationKUBERNETES_CACHE_CONFIG_MAP_NAMESPACE_DEFAULT: Uses standard Kubernetes default namespaceAlso applies to: 28-28
22-22:⚠️ Potential issue
⚠️ Breaking Change: Cluster namespace separator modification requires careful migrationChanging
CLUSTER_NAMESPACE_SEPARATORfrom "+" to "-" is a breaking change that could affect existing cluster namespace references. While "-" is more Kubernetes-friendly (as "+" requires URL encoding), this change requires careful consideration of backward compatibility.Let's verify the impact:
Consider:
- Adding a migration guide in the documentation
- Implementing backward compatibility for existing "+" separators
- Adding this to the CHANGES log as a breaking change
apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java (1)
50-50: LGTM! Good test hygiene.The addition of clearing the Kubernetes namespace system property in tearDown maintains proper test isolation.
apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (2)
74-74: LGTM! Field declaration follows best practices.The field is properly encapsulated (private) and follows Java naming conventions. The default value of
falsemakes the Kubernetes ConfigMap feature opt-in, which is a safe default.
538-542: LGTM! Property initialization follows established patterns.The initialization method correctly uses the existing
getPropertyBooleanhelper and follows the class's established patterns for property initialization.
apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java
Outdated
Show resolved
Hide resolved
| // TODO 这样重试访问idc，default是否正确 | ||
| String retryKey = Joiner.on(ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR).join("default", namespace); | ||
| jsonConfig = kubernetesManager.getValueFromConfigMap(k8sNamespace, configMapName, retryKey); | ||
| } |
There was a problem hiding this comment.
Address the TODO comment regarding retry logic
The code at line 212 contains a TODO comment: "这样重试访问idc，default是否正确" (Is this way of retrying access to idc, default correct?). Please review and confirm if the retry logic when accessing the default cluster is correct.
Would you like assistance in reviewing or improving the retry mechanism?
| 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(), | ||
| ExceptionUtil.getDetailMessage(ex)); |
There was a problem hiding this comment.
Avoid logging sensitive configuration properties and optimize method calls
At line 238, the logger.info statement logs upstream.getConfig(), which may contain sensitive configuration data. Logging these properties can expose sensitive information in logs. Additionally, upstream.getConfig() and upstream.getSourceType() are called multiple times. To prevent logging sensitive data and to optimize method calls, assign the results to local variables and adjust the log statement accordingly.
Apply this diff to fix the issue:
public boolean trySyncFromUpstream() {
if (upstream == null) {
return false;
}
try {
+ Properties upstreamProperties = upstream.getConfig();
+ ConfigSourceType upstreamSourceType = upstream.getSourceType();
+ logger.info("Start sync from the upstream data source, sourceType: {}", upstreamSourceType);
+ updateConfigMapProperties(upstreamProperties, upstreamSourceType);
- 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(),
ExceptionUtil.getDetailMessage(ex));
}
return 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.
| 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(), | |
| ExceptionUtil.getDetailMessage(ex)); | |
| Properties upstreamProperties = upstream.getConfig(); | |
| ConfigSourceType upstreamSourceType = upstream.getSourceType(); | |
| logger.info("Start sync from the upstream data source, sourceType: {}", upstreamSourceType); | |
| updateConfigMapProperties(upstreamProperties, upstreamSourceType); | |
| return true; | |
| } catch (Throwable ex) { | |
| Tracer.logError(ex); | |
| logger.warn("Sync config from upstream repository {} failed, reason: {}", upstream.getClass(), | |
| ExceptionUtil.getDetailMessage(ex)); |
CHANGES.md
Outdated
| @@ -5,6 +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) | |||
| * | ||
| * @param namespace the namespace | ||
| */ | ||
| public K8sConfigMapConfigRepository(String namespace) { |
There was a problem hiding this comment.
This constructor seems unnecessary.
| this(namespace, (ConfigRepository) null); | ||
| } | ||
|
|
||
| public K8sConfigMapConfigRepository(String namespace, KubernetesManager kubernetesManager) { |
There was a problem hiding this comment.
It's not a practice to add a constructor only for test scenario. You could use MockInjector.setInstance method to inject the mocked KubernetesManager instance.
| this.setUpstreamRepository(upstream); | ||
| } | ||
|
|
||
| public String getConfigMapKey() { |
| } | ||
|
|
||
| void setConfigMapKey(String cluster, String namespace) { | ||
| // cluster: 用户定义>idc>default |
| return false; | ||
| } | ||
| try { | ||
| logger.info("Start sync from the upstream data source, upstream.getConfig:{}, upstream.getSourceType():{}", upstream.getConfig(), upstream.getSourceType()); |
There was a problem hiding this comment.
It's not a common practice to log the configurations.
| logger.info("Start sync from the upstream data source, upstream.getConfig:{}, upstream.getSourceType():{}", upstream.getConfig(), upstream.getSourceType()); |
| client = Config.defaultClient(); | ||
| Configuration.setDefaultApiClient(client); |
There was a problem hiding this comment.
@dyx1234 Is this a valid suggestion?
| import com.ctrip.framework.apollo.internals.XmlConfigFile; | ||
| import com.ctrip.framework.apollo.internals.YamlConfigFile; | ||
| import com.ctrip.framework.apollo.internals.YmlConfigFile; | ||
| import com.ctrip.framework.apollo.internals.*; |
| @@ -19,11 +19,13 @@ | |||
| public interface ConfigConsts { | |||
| String NAMESPACE_APPLICATION = "application"; | |||
| String CLUSTER_NAME_DEFAULT = "default"; | |||
| String CLUSTER_NAMESPACE_SEPARATOR = "+"; | |||
| String CLUSTER_NAMESPACE_SEPARATOR = "-"; | |||
There was a problem hiding this comment.
This change is very risky! It would invalidate the current cache since it uses + as the separator.
Please change it back!
There was a problem hiding this comment.
Since I found that "+" is not allowed in configmap keys during my tests, I will change this constant back and use another constant to represent "-" , so as not to affect the existing cache.. Is it okay if I handle it this way
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)
139-146: Enhance method documentation.While the Javadoc is present, it could be more descriptive about the fallback behavior. Consider adding details about:
- The relationship with the local repository
- The fallback mechanism
- When this repository type should be used
Here's a suggested improvement:
/** * Creates a Kubernetes config map repository for a given namespace + * This repository uses Kubernetes ConfigMap for primary storage and falls back to + * a local repository when ConfigMap is unavailable. This ensures configuration + * availability during Kubernetes API server downtime or rate limiting. + * * @param namespace the namespace of the repository * @return the newly created repository for the given namespace */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- CHANGES.md (1 hunks)
- apollo-client/pom.xml (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/kubernetes/KubernetesManager.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java (3 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepositoryTest.java (1 hunks)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ConfigConsts.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- CHANGES.md
- apollo-client/pom.xml
- apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepositoryTest.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ConfigConsts.java
🧰 Additional context used
📓 Learnings (1)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java (1)
Learnt from: nobodyiam PR: apolloconfig/apollo-java#79 File: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java:156-162 Timestamp: 2024-10-26T12:49:07.584Z Learning: In `K8sConfigMapConfigRepository#setUpstreamRepository`, when `upstreamConfigRepository` is `null`, we should return early without setting `upstream` to `null`, so the original upstream remains.
🔇 Additional comments (2)
apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java (2)
36-36: LGTM: Import statement is correctly placed.The import for K8sConfigMapConfigRepository is appropriately placed with other internal imports.
115-117: Consider supporting concurrent repository types.The current implementation forces an exclusive choice between Kubernetes ConfigMap, local file, or remote repository. This might limit flexibility in scenarios where both Kubernetes and local caches could be beneficial (e.g., for redundancy or faster fallback).
Consider implementing a composite repository pattern that could support multiple repository types simultaneously.
Let's verify if there are any existing composite patterns in the codebase:
| Gson gson = new Gson(); | ||
| Type type = new TypeToken<Map<String, String>>() {}.getType(); | ||
| Map<String, String> configMap = gson.fromJson(jsonConfig, type); | ||
| configMap.forEach(properties::setProperty); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reuse Gson instance to improve performance.
Creating multiple Gson instances can be inefficient. Consider defining a single Gson instance at the class level to reuse across methods. This enhances performance and reduces memory footprint.
Apply this diff:
+ private static final Gson GSON = new Gson();
...
public Properties loadFromK8sConfigMap() {
...
- Gson gson = new Gson();
Type type = new TypeToken<Map<String, String>>() {}.getType();
- Map<String, String> configMap = gson.fromJson(jsonConfig, type);
+ Map<String, String> configMap = GSON.fromJson(jsonConfig, type);
...
}
public void persistConfigMap(Properties properties) {
...
- Gson gson = new Gson();
- String jsonConfig = gson.toJson(properties);
+ String jsonConfig = GSON.toJson(properties);
...
}Also applies to: 253-254
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepositoryTest.java (4)
29-29: Validate JSON test data structureThe hardcoded JSON string could potentially be invalid or not match the expected format. Consider using a JSON library to validate the structure during test setup.
ObjectMapper mapper = new ObjectMapper(); String defaultJsonValue = mapper.writeValueAsString(Map.of( "id", 123, "name", "John Doe", "email", "[email protected]" ));
22-25: Maintain consistency in field declarationsSome fields are declared as static while others aren't. For test constants that don't change between tests, consider making them all static final.
- private static String someAppId = "someApp"; - private static String someCluster = "someCluster"; - private String someNamespace = "default"; + private static final String SOME_APP_ID = "someApp"; + private static final String SOME_CLUSTER = "someCluster"; + private static final String SOME_NAMESPACE = "default";
81-82: Translate test method documentation to EnglishFor better international collaboration, please translate the Chinese comments to English:
- * 测试sync方法成功从上游数据源同步 + * Test successful synchronization from upstream data source- * 测试sync方法从上游数据源同步失败，成功从Kubernetes的ConfigMap中加载 + * Test sync method fallback to Kubernetes ConfigMap when upstream sync failsAlso applies to: 97-98
100-112: Enhance error handling test coverageThe test for upstream failure should verify:
- That the fallback mechanism works correctly
- The actual content loaded from ConfigMap matches expectations
- The error from upstream is properly handled
@Test public void testSyncFailFromUpstreamSuccessFromConfigMap() throws Throwable { // arrange ConfigRepository upstream = mock(ConfigRepository.class); when(upstream.getConfig()).thenThrow(new RuntimeException("Upstream sync failed")); k8sConfigMapConfigRepository.setUpstreamRepository(upstream); when(kubernetesManager.getValueFromConfigMap(anyString(), anyString(), anyString())).thenReturn(data.get(defaultKey)); // act k8sConfigMapConfigRepository.sync(); // assert verify(kubernetesManager, times(1)).getValueFromConfigMap(anyString(), anyString(), anyString()); + Properties config = k8sConfigMapConfigRepository.getConfig(); + assertEquals(defaultJsonValue, config.getProperty(defaultKey)); + assertEquals(ConfigSourceType.CONFIGMAP, k8sConfigMapConfigRepository.getSourceType()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- 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
...ent/src/test/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepositoryTest.java
Show resolved
Hide resolved
| import io.kubernetes.client.openapi.ApiClient; | ||
| import io.kubernetes.client.openapi.ApiException; | ||
| import io.kubernetes.client.openapi.apis.CoreV1Api; | ||
| import io.kubernetes.client.openapi.models.*; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java (1)
78-79: Correct spelling from "can not" to "cannot" in log messages
In several log messages, "can not" should be corrected to "cannot" for proper spelling.
Apply this diff to correct the spelling:
logger.error("create configmap failed due to null or empty parameter: k8sNamespace={}, name={}", k8sNamespace, name);
+logger.error("Create ConfigMap failed due to null or empty parameters: k8sNamespace={}, name={}", k8sNamespace, name);
logger.error("Parameters can not be null or empty: k8sNamespace={}, name={}", k8sNamespace, name);
+logger.error("Parameters cannot be null or empty: k8sNamespace={}, name={}", k8sNamespace, name);
logger.error("Parameters can not be null or empty: k8sNamespace={}, name={}", k8sNamespace, name);
+logger.error("Parameters cannot be null or empty: k8sNamespace={}, name={}", k8sNamespace, name);
logger.error("Parameters can not be null or empty: k8sNamespace={}, configMapName={}", k8sNamespace, configMapName);
+logger.error("Parameters cannot be null or empty: k8sNamespace={}, configMapName={}", k8sNamespace, configMapName);Also applies to: 102-103, 128-129, 170-171
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- 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)
🚧 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 (6)
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java (6)
41-50: Constructor initializes Kubernetes client correctly
The default constructor properly initializes the Kubernetes client and handles exceptions appropriately with informative logging.
52-54: Dependency injection constructor enhances flexibility
Providing a constructor that accepts a CoreV1Api instance allows for better testability and promotes loose coupling.
56-66: buildConfigMap method constructs ConfigMap accurately
The buildConfigMap method correctly builds a V1ConfigMap object with the specified metadata and data.
77-91: createConfigMap method handles creation effectively
The createConfigMap method performs parameter validation, builds the ConfigMap, and handles exceptions with appropriate logging.
127-160: updateConfigMap method implements retry logic appropriately
The updateConfigMap method uses a retry mechanism with exponential backoff to handle update conflicts, which is a good practice for ensuring eventual consistency.
169-183: checkConfigMapExist method checks existence correctly
The method attempts to read the ConfigMap to determine its existence and handles exceptions properly, logging any errors encountered.
| @@ -19,11 +19,13 @@ | |||
| public interface ConfigConsts { | |||
| String NAMESPACE_APPLICATION = "application"; | |||
| String CLUSTER_NAME_DEFAULT = "default"; | |||
| String CLUSTER_NAMESPACE_SEPARATOR = "+"; | |||
| String CLUSTER_NAMESPACE_SEPARATOR = "-"; | |||
There was a problem hiding this comment.
Since I found that "+" is not allowed in configmap keys during my tests, I will change this constant back and use another constant to represent "-" , so as not to affect the existing cache.. Is it okay if I handle it this way
| return ConfigConsts.KUBERNETES_CACHE_CONFIG_MAP_NAMESPACE_DEFAULT; | ||
| } | ||
|
|
||
| private String getCustomizedConfigMapNamespace() { |
There was a problem hiding this comment.
cache k8s namespace
apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (3)
apollo-client/src/main/java/com/ctrip/framework/apollo/util/escape/EscapeUtil.java (1)
19-22: Enhance class-level documentation.
The class documentation should explain:
- The purpose of this utility class
- Its relationship with Kubernetes ConfigMap functionality
- The rationale behind the escaping strategy
- Examples of usage
Add comprehensive Javadoc:
/**
+ * Utility class for handling namespace escaping in Kubernetes ConfigMap keys.
+ * <p>
+ * When storing Apollo configurations in Kubernetes ConfigMaps, we need to ensure
+ * that namespace names are properly escaped since ConfigMap keys have restrictions
+ * on special characters. This class provides methods to:
+ * <ul>
+ * <li>Escape underscores in namespace names to avoid conflicts with delimiters</li>
+ * <li>Create ConfigMap keys by combining cluster and namespace information</li>
+ * </ul>
+ *
+ * Example usage:
+ * <pre>
+ * String escapedNs = EscapeUtil.escapeNamespace("my_namespace"); // Returns "my__namespace"
+ * String configKey = EscapeUtil.createConfigMapKey("prod", "my_namespace"); // Returns "prod___my__namespace"
+ * </pre>
+ *
* @author dyx1234
*/apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java (1)
248-275: Consider adding more test cases for better coverage
The current tests cover basic functionality well, but consider adding these test cases for more comprehensive coverage:
-
For
testK8sNamespaceWithSystemProperty:- Test with invalid namespace values
- Test with empty string
-
For
testKubernetesCacheEnabledWithSystemProperty:- Test when property is set to false
- Test default behavior when property is not set
- Test with invalid boolean value
Here's a suggested implementation:
@Test
public void testK8sNamespaceWithInvalidValue() {
System.setProperty(ApolloClientSystemConsts.APOLLO_CACHE_KUBERNETES_NAMESPACE, "");
ConfigUtil configUtil = new ConfigUtil();
assertEquals(ConfigConsts.KUBERNETES_CACHE_CONFIG_MAP_NAMESPACE_DEFAULT, configUtil.getK8sNamespace());
}
@Test
public void testKubernetesCacheEnabledWithDefault() {
ConfigUtil configUtil = new ConfigUtil();
assertFalse(configUtil.isPropertyKubernetesCacheEnabled());
}
@Test
public void testKubernetesCacheEnabledWithInvalidValue() {
System.setProperty(ApolloClientSystemConsts.APOLLO_KUBERNETES_CACHE_ENABLE, "invalid");
ConfigUtil configUtil = new ConfigUtil();
assertFalse(configUtil.isPropertyKubernetesCacheEnabled());
}apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (1)
381-389: Consider using full word "Kubernetes" in method name.
While the method logic is correct, consider renaming getK8sNamespace() to getKubernetesNamespace() for better readability and consistency with other method names in the codebase.
-public String getK8sNamespace() {
+public String getKubernetesNamespace() {📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- 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/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (5 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/escape/EscapeUtil.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/ConfigConsts.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepositoryTest.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ConfigConsts.java
🔇 Additional comments (5)
apollo-client/src/main/java/com/ctrip/framework/apollo/util/escape/EscapeUtil.java (1)
29-33:
Add validation and documentation for ConfigMap key creation.
The method needs input validation and should respect Kubernetes ConfigMap key constraints.
Apply these improvements:
- // Concatenate the cluster and the escaped namespace, using three underscores as delimiters
- public static String createConfigMapKey(String cluster, String namespace) {
- String escapedNamespace = escapeNamespace(namespace);
- return cluster + "___" + escapedNamespace;
- }
+ /**
+ * Creates a Kubernetes ConfigMap key by combining cluster and namespace.
+ * The format is: {cluster}___{escaped_namespace}
+ * Triple underscore is used as a delimiter to avoid conflicts with escaped namespaces.
+ *
+ * @param cluster the cluster name
+ * @param namespace the namespace to be escaped
+ * @return the ConfigMap key
+ * @throws IllegalArgumentException if inputs are invalid or result exceeds length limit
+ */
+ public static String createConfigMapKey(String cluster, String namespace) {
+ if (cluster == null || cluster.isEmpty()) {
+ throw new IllegalArgumentException("Cluster cannot be null or empty");
+ }
+
+ String escapedNamespace = escapeNamespace(namespace);
+ String key = cluster + "___" + escapedNamespace;
+
+ // Kubernetes ConfigMap keys have a 253 character limit
+ if (key.length() > 253) {
+ throw new IllegalArgumentException(
+ "Generated ConfigMap key exceeds 253 characters: " + key.length());
+ }
+
+ return key;
+ }Let's verify if there are any existing tests for this utility class:
apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java (2)
50-51: LGTM: Proper test cleanup implemented
The additions to tearDown properly clean up the new Kubernetes-related system properties, preventing test interference.
248-275: Verify integration with Kubernetes ConfigMap implementation
The tests align well with the PR objectives for Kubernetes ConfigMap support. Let's verify the complete integration:
✅ Verification successful
Tests properly validate Kubernetes ConfigMap integration
The test cases in ConfigUtilTest.java correctly validate the essential configuration aspects of the Kubernetes integration, as evidenced by:
- The tests align with the actual
ConfigUtilimplementation which providesgetK8sNamespace()andisPropertyKubernetesCacheEnabled()methods - The complete implementation shows proper integration:
K8sConfigMapConfigRepositoryhandles ConfigMap operations with proper error handlingKubernetesManagermanages K8s client initialization and API interactionsDefaultConfigFactorycorrectly creates ConfigMap repository when Kubernetes cache is enabled
- The default values and system properties are properly tested against the constants defined in
ConfigConsts
The test coverage is comprehensive and matches the implementation details found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all necessary Kubernetes configuration options are tested
# and that the implementation is complete.
# Test 1: Check if all Kubernetes-related constants are tested
echo "Checking for Kubernetes-related constants..."
rg -l "KUBERNETES|K8S" --type java
# Test 2: Check if ConfigMap implementation exists
echo "Checking for ConfigMap implementation..."
rg -l "ConfigMap" --type java
# Test 3: Check for proper exception handling in Kubernetes implementation
echo "Checking for Kubernetes-related error handling..."
rg "throw.*Exception.*[Kk]ubernetes" --type java
Length of output: 1288
Script:
#!/bin/bash
# Let's analyze the Kubernetes implementation more thoroughly
# Check the ConfigUtil implementation for Kubernetes-related methods
echo "=== ConfigUtil Implementation ==="
rg -A 10 "getK8sNamespace|isPropertyKubernetesCacheEnabled" apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java
# Check K8sConfigMapConfigRepository implementation
echo "=== K8sConfigMapConfigRepository Implementation ==="
ast-grep --pattern 'class K8sConfigMapConfigRepository {
$$$
}'
# Check KubernetesManager implementation
echo "=== KubernetesManager Implementation ==="
ast-grep --pattern 'class KubernetesManager {
$$$
}'
# Check how DefaultConfigFactory handles Kubernetes config
echo "=== DefaultConfigFactory Kubernetes Integration ==="
rg -A 10 "kubernetes|k8s" apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java
Length of output: 1521
Script:
#!/bin/bash
# Let's check the actual implementations with different commands
# Check K8sConfigMapConfigRepository content
echo "=== K8sConfigMapConfigRepository Content ==="
rg -A 20 "class K8sConfigMapConfigRepository" apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java
# Check KubernetesManager content
echo "=== KubernetesManager Content ==="
rg -A 20 "class KubernetesManager" apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
# Check how DefaultConfigFactory creates Kubernetes config
echo "=== DefaultConfigFactory Create Config ==="
rg -A 10 "createConfig.*Repository" apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java
# Check the constants being used
echo "=== Kubernetes Related Constants ==="
rg "KUBERNETES.*=|K8S.*=" apollo-core/src/main/java/com/ctrip/framework/apollo/core/ConfigConsts.java
Length of output: 4520
apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (2)
74-74: LGTM! Field declaration follows best practices.
The field name follows Java naming conventions and uses a safe default value of false, requiring explicit opt-in for Kubernetes ConfigMap support.
512-514: LGTM! Implementation follows established patterns.
The getter method and initialization logic follow the class's established patterns and correctly reuse the getPropertyBoolean helper method.
Also applies to: 538-542
apollo-client/src/main/java/com/ctrip/framework/apollo/util/escape/EscapeUtil.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
Outdated
Show resolved
Hide resolved
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