feat: reduce conflicts when update configmap in k8s #89 #93
Conversation
WalkthroughThe pull request introduces modifications to the Apollo Java client's Kubernetes configuration management, focusing on reducing potential write conflicts when updating ConfigMaps. The primary change is in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
@nobodyiam @shoothzj Clould you review the idea of code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java (1)
48-48: Consider makingpropertyKubernetesMaxWritePodsfinalTo ensure immutability after initialization, consider declaring
propertyKubernetesMaxWritePodsasfinal.Apply this diff:
-private int propertyKubernetesMaxWritePods; +private final int propertyKubernetesMaxWritePods;apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (1)
417-433: Variable naming ingetK8sConfigPropertiesmethodThe variable
k8sNamespaceis used to store generic property values, which may not always represent a namespace. To improve code readability and maintainability, consider renaming the variable to a more generic name likepropertyValue.Apply this diff:
- String k8sNamespace = System.getProperty(key); - if (Strings.isNullOrEmpty(k8sNamespace)) { + String propertyValue = System.getProperty(key); + if (Strings.isNullOrEmpty(propertyValue)) { // ... - k8sNamespace = System.getenv(environmentKey); + propertyValue = System.getenv(environmentKey); // ... - return k8sNamespace; + return propertyValue;
📜 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(4 hunks)apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java(4 hunks)apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java(2 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java(1 hunks)
🔇 Additional comments (1)
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java (1)
144-146:
Verify the return value when skipping ConfigMap update
In the updateConfigMap method, if the current pod is not a write pod, the method returns true. Please verify that this behavior correctly indicates that the update is not necessary. Returning true might imply that the ConfigMap was successfully updated, which could be misleading.
Run the following script to check how updateConfigMap is used and how its return value is handled:
✅ Verification successful
Let me gather more context about the isWritePod method and its usage to better understand the logic.
Let me check the complete implementation of the updateConfigMap method to understand the full context.
Let me try a different pattern to get the method implementation.
Based on the implementation I can now generate the final response.
Return value in updateConfigMap is correct for non-write pods
The return value of true when the pod is not a write pod is correct and not misleading. Here's why:
- The method returns
trueto indicate a successful outcome, not specifically a successful update - For non-write pods, skipping the update is the expected behavior and represents a successful execution of the method
- This aligns with the method's contract where
trueindicates the operation completed as intended, whether that means updating (for write pods) or intentionally skipping (for non-write pods) - The actual update logic is only attempted for write pods, maintaining the proper separation of responsibilities in a multi-pod environment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of `updateConfigMap` and inspect return value handling.
rg -A 3 'updateConfigMap\('
Length of output: 2898
Script:
#!/bin/bash
# Find the implementation and usage of isWritePod
ast-grep --pattern 'isWritePod($_) {
$$$
}'
# Also search for any documentation or comments about write pod behavior
rg -B 2 -A 2 'isWritePod'
Length of output: 1259
Script:
#!/bin/bash
# Get the complete implementation of updateConfigMap method
ast-grep --pattern 'updateConfigMap($_, $_, $_) {
$$$
}'
# Also get any related tests that verify this behavior
rg -B 2 -A 10 'void test.*updateConfigMap'
Length of output: 105
Script:
#!/bin/bash
# Get the complete implementation using a more specific context
rg -B 5 -A 20 'public boolean updateConfigMap\(' apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
# Get test cases with different pattern
rg -B 5 -A 15 'updateConfigMap.*assert' apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java
Length of output: 1323
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
apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java
Outdated
Show resolved
Hide resolved
apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java
Outdated
Show resolved
Hide resolved
24kpure
force-pushed
the
feature/reduce_conflicts_when_update_configmap_in_k8s
branch
from
45d2cce to
b56e717
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (2)
415-433: Consider adding input validation for the default value.The
getK8sConfigPropertiesmethod accepts a default value parameter but doesn't validate it. For properties that expect specific formats or ranges, consider adding validation to ensure the default value meets the requirements.private String getK8sConfigProperties(String key, String environmentKey, String defaultValue) { + if (defaultValue == null) { + throw new IllegalArgumentException("Default value cannot be null"); + } // 1. Get from System Property String k8sNamespace = System.getProperty(key); // ... rest of the method
401-433: Consider adding documentation for the new configuration property.The new Kubernetes max write pods configuration would benefit from documentation explaining its purpose, acceptable values, and impact on the system. This helps users understand how to properly configure this value.
Add JavaDoc comments:
+ /** + * Initializes the maximum number of pods that can write to the Kubernetes ConfigMap. + * This helps reduce conflicts and pressure on the Kubernetes service when multiple + * pods attempt to update configurations simultaneously. + */ private void initPropertyKubernetesMaxWritePods() { // ... implementation } + /** + * Retrieves configuration properties from various sources in the following order: + * 1. System Property + * 2. Environment Variable + * 3. Server Properties + * 4. Application Properties + * 5. Default Value + * + * @param key The system property key + * @param environmentKey The environment variable key + * @param defaultValue The default value if not found in any source + * @return The configuration value + */ private String getK8sConfigProperties(String key, String environmentKey, String defaultValue) { // ... implementation }apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java (3)
49-57: Add validation for propertyKubernetesMaxWritePodsThe max write pods value should be validated to ensure it's positive.
public KubernetesManager() { try { client = Config.defaultClient(); coreV1Api = new CoreV1Api(client); ConfigUtil configUtil = ApolloInjector.getInstance(ConfigUtil.class); propertyKubernetesMaxWritePods = configUtil.getPropertyKubernetesMaxWritePods(); + if (propertyKubernetesMaxWritePods <= 0) { + throw new IllegalArgumentException("Max write pods must be positive"); + } } catch (Exception e) {
149-151: Add debug logging for write pod eligibility checkConsider adding debug logging to help troubleshoot pod selection behavior.
if (!isWritePod(k8sNamespace)) { + logger.debug("Pod {} is not eligible for writing ConfigMap", localPodName); return true; }
243-244: Make app label key configurableThe "app" label key is hardcoded, which reduces flexibility. Consider making it configurable.
- String appName = localMetadata.getLabels().get("app"); - String labelSelector = "app=" + appName; + String appLabelKey = configUtil.getPropertyKubernetesAppLabelKey("app"); + String appName = localMetadata.getLabels().get(appLabelKey); + String labelSelector = appLabelKey + "=" + appName;apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java (2)
155-161: Add test cases for edge cases in pod selectionThe current test only covers the happy path. Consider adding test cases for:
- Multiple pods with different creation timestamps
- Pods without required labels
- Error scenarios in pod listing
+ @Test + public void testUpdateConfigMapWithMultiplePods() throws Exception { + // arrange + String namespace = "default"; + String name = "testConfigMap"; + + // Create multiple pods with different timestamps + V1Pod pod1 = createPodWithTimestamp("pod1", OffsetDateTime.now().minusHours(1)); + V1Pod pod2 = createPodWithTimestamp("pod2", OffsetDateTime.now()); + V1PodList v1PodList = new V1PodList().addItemsItem(pod1).addItemsItem(pod2); + + // Setup mocks and verify behavior + // ... implementation details ... + }
178-180: Add assertions for ConfigMap data integrityThe test should verify that the ConfigMap data is updated correctly when the pod is eligible to write.
HashMap<String, String> updateData = new HashMap<>(existData); updateData.put("newKey","newValue"); boolean success = kubernetesManager.updateConfigMap(namespace, name, updateData); + + // Verify the ConfigMap was updated with the correct data + ArgumentCaptor<V1ConfigMap> configMapCaptor = ArgumentCaptor.forClass(V1ConfigMap.class); + verify(coreV1Api).replaceNamespacedConfigMap(eq(name), eq(namespace), configMapCaptor.capture(), + isNull(), isNull(), isNull(), isNull()); + + Map<String, String> updatedData = configMapCaptor.getValue().getData(); + assertEquals("newValue", updatedData.get("newKey")); + assertEquals("value", updatedData.get("key"));
📜 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(4 hunks)apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java(4 hunks)apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java(5 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java
🔇 Additional comments (6)
apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (5)
75-75: LGTM: Field declaration with reasonable default value.
The default value of 3 for propertyKubernetesMaxWritePods seems reasonable as it provides redundancy while limiting the number of pods that can write to the ConfigMap.
Line range hint 82-98: LGTM: Constructor properly initializes the new field.
The constructor has been updated to include initPropertyKubernetesMaxWritePods() in the initialization sequence, maintaining consistency with other property initializations.
396-399: LGTM: Refactored method improves code reusability.
The getK8sNamespace() method has been refactored to use the new getK8sConfigProperties() helper method, which reduces code duplication and improves maintainability.
543-545: LGTM: Getter method follows Java conventions.
The getter method is properly implemented following Java naming conventions and visibility modifiers.
401-411:
Fix incorrect constant in error logging.
The error message references APOLLO_CACHE_KUBERNETES_NAMESPACE instead of APOLLO_CACHE_KUBERNETES_MAX_WRITE_PODS. This may lead to confusion when diagnosing configuration issues.
Apply this diff to correct the constant:
logger.error("Config for {} is invalid: {}",
- ApolloClientSystemConsts.APOLLO_CACHE_KUBERNETES_NAMESPACE, propertyKubernetesMaxWritePodsStr);
+ ApolloClientSystemConsts.APOLLO_CACHE_KUBERNETES_MAX_WRITE_PODS, propertyKubernetesMaxWritePodsStr);apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java (1)
260-263:
Fix error handling in isWritePod method
Currently, the method returns true on errors, which could lead to multiple pods attempting to write simultaneously. This defeats the purpose of write pod selection.
} catch (Exception e) {
- logger.info("Error determining write pod eligibility:{}", e.getMessage(), e);
- return true;
+ logger.error("Error determining write pod eligibility: {}", e.getMessage(), e);
+ return false;
}Likely invalid or redundant comment.
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
Show resolved
Hide resolved
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
|
@dyx1234 @shoothzj Would you please help to take a look first? |
24kpure
changed the title
|
@dyx1234 @shoothzj Code review please |
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
Show resolved
Hide resolved
| String appName = localMetadata.getLabels().get("app"); | ||
| String labelSelector = "app=" + appName; | ||
|
|
||
| V1PodList v1PodList = coreV1Api.listNamespacedPod(k8sNamespace, null, null, |
There was a problem hiding this comment.
When the number of pod instances is large, the interface return value may be particularly large, and the limit parameter needs to be used
There was a problem hiding this comment.
I don't know what is a large number of instances, so I set it to 1000 . How about your advice?
There was a problem hiding this comment.
I think 50 or 100 is sufficient. 1000 clients pull 1000 pod lists from the k8s server at the same time, which may affect the performance of the k8s server or network.
There was a problem hiding this comment.
I think 50 or 100 is sufficient. 1000 clients pull 1000 pod lists from the k8s server at the same time, which may affect the performance of the k8s server or network.
First of all, I agree with you. According to the test, there is little difference between the returned data size of 1000 and 200.Howerer, in the worst scenario, there will be 1000 pod trying to update the pod, which is not as expected.100 may be a good choice.
It's very nice of you ~
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apollo-client/src/main/resources/META-INF/additional-spring-configuration-metadata.json (1)
129-135: Consider using Integer type and enhancing the descriptionThe new configuration property has a few areas for improvement:
- The type should be
java.lang.Integersince it represents a numeric value- The description could better explain the purpose and impact of this setting
{ "name": "apollo.cache.kubernetes.max-write-pods", - "type": "java.lang.String", + "type": "java.lang.Integer", "sourceType": "com.ctrip.framework.apollo.util.ConfigUtil", - "description": "max number of pods that can write the configmap cache in Kubernetes.", + "description": "Maximum number of pods that can write to the configmap cache in Kubernetes. This helps reduce write conflicts in multi-pod deployments.", "defaultValue": "3" },apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java (1)
56-73: Consider explicit initialization in the default constructorWhile the test constructor properly initializes all fields, the default constructor relies on the field initialization for
localPodName. Consider moving theSystem.getenv("HOSTNAME")call to the constructor for consistency and easier testing.public KubernetesManager() { try { client = Config.defaultClient(); coreV1Api = new CoreV1Api(client); ConfigUtil configUtil = ApolloInjector.getInstance(ConfigUtil.class); propertyKubernetesMaxWritePods = configUtil.getPropertyKubernetesMaxWritePods(); + localPodName = System.getenv("HOSTNAME"); } catch (Exception e) { String errorMessage = "Failed to initialize Kubernetes client: " + e.getMessage(); logger.error(errorMessage, e); throw new RuntimeException(errorMessage, e); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGES.md(1 hunks)apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java(4 hunks)apollo-client/src/main/resources/META-INF/additional-spring-configuration-metadata.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGES.md
🔇 Additional comments (2)
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java (2)
Line range hint 19-50: LGTM! Well-structured class-level changes
The new imports, constants, and fields are properly defined to support the pod write limitation feature. The MAX_SEARCH_NUM constant is appropriately set to 1000 as discussed in the previous review.
Also applies to: 53-54
153-156: LGTM! Effective conflict reduction strategy
The early check using isWritePod effectively reduces potential conflicts by limiting the number of pods that attempt to update the ConfigMap.
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
Show resolved
Hide resolved
|
It reduces the conflict instead of election. What if list pods's cost more than update configmap? cc @nobodyiam |
24kpure
force-pushed
the
feature/reduce_conflicts_when_update_configmap_in_k8s
branch
from
d2d578f to
e677d88
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java (4)
53-55: Consider making MAX_SEARCH_NUM configurableThe hardcoded limit of 1000 pods might not be suitable for all cluster sizes. Consider making this configurable through the ConfigUtil, similar to
propertyKubernetesMaxWritePods.- private static final int MAX_SEARCH_NUM = 1000; + @Value("${apollo.cache.kubernetes.max-search-pods:1000}") + private int maxSearchPods;
66-67: Add parameter validation in the default constructorThe
propertyKubernetesMaxWritePodsvalue should be validated to ensure it's positive.ConfigUtil configUtil = ApolloInjector.getInstance(ConfigUtil.class); propertyKubernetesMaxWritePods = configUtil.getPropertyKubernetesMaxWritePods(); + if (propertyKubernetesMaxWritePods <= 0) { + throw new IllegalArgumentException("propertyKubernetesMaxWritePods must be positive"); + }
159-161: Add logging when skipping non-write podsWhen a pod is determined to be non-writable, the method silently returns true. This might make it difficult to debug issues or understand system behavior.
if (!isWritePod(k8sNamespace)) { + logger.debug("Pod {} is not designated as writable, skipping ConfigMap update", localPodName); return true; }
261-269: Optimize stream operations for better readabilityThe stream operations can be simplified and made more readable by combining filters and adding meaningful variable names.
- return v1PodList.getItems().stream() - .map(V1Pod::getMetadata) - .filter(Objects::nonNull) - .filter(metadata -> metadata.getCreationTimestamp() != null) - .sorted(Comparator.comparing(V1ObjectMeta::getCreationTimestamp)) - .map(V1ObjectMeta::getName) - .limit(propertyKubernetesMaxWritePods) - .anyMatch(localPodName::equals); + List<String> writablePodNames = v1PodList.getItems().stream() + .map(V1Pod::getMetadata) + .filter(metadata -> metadata != null && metadata.getCreationTimestamp() != null) + .sorted(Comparator.comparing(V1ObjectMeta::getCreationTimestamp)) + .map(V1ObjectMeta::getName) + .limit(propertyKubernetesMaxWritePods) + .collect(Collectors.toList()); + boolean isWritable = writablePodNames.contains(localPodName); + logger.debug("Pod {} is{} designated as writable. Writable pods: {}", + localPodName, isWritable ? "" : " not", writablePodNames); + return isWritable;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGES.md(1 hunks)apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java(3 hunks)apollo-client/src/main/resources/META-INF/additional-spring-configuration-metadata.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGES.md
- apollo-client/src/main/resources/META-INF/additional-spring-configuration-metadata.json
🔇 Additional comments (3)
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java (3)
270-273: Improve error handling in isWritePod
The error handling defaults to true and uses INFO level logging, which could mask issues and lead to multiple pods attempting to write.
253-254:
Add validation for the app label
The app label is retrieved and used without checking if it exists, which could lead to NPE or incorrect behavior.
String appName = localMetadata.getLabels().get("app");
+ if (appName == null) {
+ logger.warn("App label not found for pod {}, defaulting to non-write pod", localPodName);
+ return false;
+ }
String labelSelector = "app=" + appName;Likely invalid or redundant comment.
59-60: Enhance pod name retrieval resilience
The HOSTNAME environment variable might not be available in all Kubernetes environments. Consider adding a fallback mechanism using the Kubernetes Downward API.
It's an interesting question that troubles me too. So I conducted a test using get single confimapget single podget 40 podsget 220 podsget 1000 podsget 2500 podsConclusion
|
|
@dyx1234 @shoothzj @arrow1991 Happy new year, cr please . |
24kpure
force-pushed
the
feature/reduce_conflicts_when_update_configmap_in_k8s
branch
from
e677d88 to
52f9de3
Compare
|
Who Could tell me that there may be an issue with this PR, and I can close the PR so that I am not busy dealing with conflicts with the main branch. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java (1)
245-247:⚠️ Potential issueFix default behavior when pod name is missing
Defaulting to
truewhen the pod name is missing is unsafe and could lead to multiple pods attempting to write simultaneously.Apply this fix:
if (Strings.isNullOrEmpty(localPodName)) { - return true; + logger.warn("Local pod name is missing, defaulting to non-write pod"); + return false; }
🧹 Nitpick comments (5)
apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (2)
75-75: Document the default value rationaleThe default value of 3 for
propertyKubernetesMaxWritePodsshould be documented to explain why this specific value was chosen. This helps maintainers understand the reasoning behind this configuration.Add a comment explaining the rationale:
+ /** Default number of writable pods is set to 3 to balance between write availability and conflict probability */ private int propertyKubernetesMaxWritePods = 3;
415-415: Use more descriptive parameter namesThe parameter names in
getK8sConfigPropertiescould be more descriptive to better indicate their purpose in the configuration hierarchy.- private String getK8sConfigProperties(String key, String environmentKey, String defaultValue) { + private String getK8sConfigProperties(String systemPropertyKey, String environmentVariableKey, String defaultValue) {apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java (2)
53-55: Consider making MAX_SEARCH_NUM configurableThe hardcoded limit of 1000 pods might need adjustment based on cluster size and performance requirements. Consider making this configurable through the same mechanism as
propertyKubernetesMaxWritePods.
62-72: Improve error handling in the default constructorThe constructor catches all exceptions and wraps them in RuntimeException. Consider handling specific exceptions (like K8s API exceptions) differently and providing more detailed error messages.
apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java (1)
182-187: Strengthen test assertionsThe test only verifies that the pod listing API was called. Add assertions for:
- The actual data in the updated ConfigMap
- The number of times the update API is called
- The content of the label selector used
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGES.md(1 hunks)apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java(3 hunks)apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java(4 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(5 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGES.md
- apollo-client/src/main/resources/META-INF/additional-spring-configuration-metadata.json
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java
🔇 Additional comments (6)
apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (3)
409-411: Incorrect constant used in error loggingThe error message references
APOLLO_CACHE_KUBERNETES_NAMESPACEinstead ofAPOLLO_CACHE_KUBERNETES_MAX_WRITE_PODS. This may lead to confusion when diagnosing configuration issues.
396-398: LGTM! Well-structured configuration retrievalThe refactored configuration retrieval logic follows a clear hierarchy (System Property → Environment Variable → Server Property → App Property → Default) and effectively reuses code through the new
getK8sConfigPropertiesmethod.Also applies to: 415-433
543-545: LGTM! Clean getter implementationThe getter method follows Java conventions and maintains proper encapsulation.
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java (2)
250-252:⚠️ Potential issueFix unsafe null handling
Similar to the pod name check, defaulting to
truewhen metadata or labels are null is unsafe.Apply this fix:
if (localMetadata == null || localMetadata.getLabels() == null) { - return true; + logger.warn("Pod metadata or labels are null, defaulting to non-write pod"); + return false; }Likely invalid or redundant comment.
271-273:⚠️ Potential issueFix error handling in isWritePod
Defaulting to
trueon exceptions and using INFO level for errors is unsafe.Apply this fix:
- logger.info("Error determining write pod eligibility:{}", e.getMessage(), e); - return true; + logger.error("Error determining write pod eligibility: {}", e.getMessage(), e); + return false;Likely invalid or redundant comment.
apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java (1)
57-57: Add test cases for constructor error handlingThe test suite lacks coverage for the default constructor's error handling paths. Consider adding test cases for:
- ConfigUtil initialization failure
- K8s client initialization failure
Would you like me to generate the additional test cases?
| if (!isWritePod(k8sNamespace)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider return value for non-write pods
Returning true when isWritePod returns false might mask issues by indicating success when no update was attempted. Consider returning a different status or throwing an exception to make it clear that the pod is not eligible to write.
| return v1PodList.getItems().stream() | ||
| .map(V1Pod::getMetadata) | ||
| .filter(Objects::nonNull) | ||
| //Make each node selects the same write nodes by sorting | ||
| .filter(metadata -> metadata.getCreationTimestamp() != null) | ||
| .sorted(Comparator.comparing(V1ObjectMeta::getCreationTimestamp)) | ||
| .map(V1ObjectMeta::getName) | ||
| .limit(propertyKubernetesMaxWritePods) | ||
| .anyMatch(localPodName::equals); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance pod selection criteria
The current pod selection logic only considers the creation timestamp and "Running" status. Consider additional criteria:
- Pod readiness status (Ready condition)
- Pod container statuses
- Pod deletion timestamp (ignore pods being terminated)
Example enhancement:
return v1PodList.getItems().stream()
.map(V1Pod::getMetadata)
.filter(Objects::nonNull)
+ .filter(metadata -> metadata.getDeletionTimestamp() == null)
.filter(metadata -> metadata.getCreationTimestamp() != null)
.sorted(Comparator.comparing(V1ObjectMeta::getCreationTimestamp))
.map(V1ObjectMeta::getName)
.limit(propertyKubernetesMaxWritePods)
.anyMatch(localPodName::equals);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return v1PodList.getItems().stream() | |
| .map(V1Pod::getMetadata) | |
| .filter(Objects::nonNull) | |
| //Make each node selects the same write nodes by sorting | |
| .filter(metadata -> metadata.getCreationTimestamp() != null) | |
| .sorted(Comparator.comparing(V1ObjectMeta::getCreationTimestamp)) | |
| .map(V1ObjectMeta::getName) | |
| .limit(propertyKubernetesMaxWritePods) | |
| .anyMatch(localPodName::equals); | |
| return v1PodList.getItems().stream() | |
| .map(V1Pod::getMetadata) | |
| .filter(Objects::nonNull) | |
| .filter(metadata -> metadata.getDeletionTimestamp() == null) | |
| //Make each node selects the same write nodes by sorting | |
| .filter(metadata -> metadata.getCreationTimestamp() != null) | |
| .sorted(Comparator.comparing(V1ObjectMeta::getCreationTimestamp)) | |
| .map(V1ObjectMeta::getName) | |
| .limit(propertyKubernetesMaxWritePods) | |
| .anyMatch(localPodName::equals); |
|
Thanks for your contribution! Additional, again apologise, you can add my wechat to let me can move this thing forward faster.(QRCode in my profile) |
|
Thanks for your reply. |
|
From my opinion, I might think it's early to add this Or it's a client configuration, easily add and remove. I am not sure if we need to add this config or hard code. That's my only concern with merging this code, and I'd like to see what @nobodyiam has to say. |
|
I believe this PR is helpful in alleviating the pressure on the K8S API Server caused by a large number of instances writing concurrently. However, as @hezhangjian mentioned, the dynamic configuration of propertyKubernetesMaxWritePods can indeed be added later, as this configuration essentially shouldn't concern the user. For users, it only matters that the writes can be successfully configured. |
|
@24kpure Could you please help remove the configuration, and I am happy to preced with this PR :) |
24kpure
force-pushed
the
feature/reduce_conflicts_when_update_configmap_in_k8s
branch
from
52f9de3 to
5db4993
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGES.md(1 hunks)apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java(4 hunks)apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java(1 hunks)apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGES.md
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (8)
🔇 Additional comments (9)
apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java (7)
41-46: LGTM! Clear documentation of required K8s permissions.The class-level documentation clearly specifies the required Kubernetes permissions for both pod operations and ConfigMap operations.
71-75: LGTM! Well-structured test constructor.The constructor is properly annotated with @VisibleForTesting and correctly initializes all fields.
266-269:⚠️ Potential issueFix error handling in isWritePod.
The current error handling has two issues:
- Using INFO level for errors masks critical issues
- Defaulting to
trueon errors could increase conflicts} catch (Exception e) { - logger.info("Error determining write pod eligibility:{}", e.getMessage(), e); - return true; + logger.error("Error determining write pod eligibility: {}", e.getMessage(), e); + return false; }Likely invalid or redundant comment.
246-248:⚠️ Potential issueFix default behavior for missing metadata.
Similarly, defaulting to
truewhen metadata or labels are missing could lead to unnecessary write attempts.if (localMetadata == null || localMetadata.getLabels() == null) { - return true; + logger.warn("Pod metadata or labels are null, defaulting to non-write pod"); + return false; }Likely invalid or redundant comment.
155-157: 🛠️ Refactor suggestionReconsider return value for non-write pods.
Returning
truewhen the pod is not eligible to write could mask issues by indicating success when no update was attempted. Consider:
- Returning
falseto indicate no action was taken- Throwing an exception with a clear message
- Adding a log message explaining why the update was skipped
if (!isWritePod(k8sNamespace)) { - return true; + logger.info("Skipping ConfigMap update as this pod is not eligible to write"); + return false; }Likely invalid or redundant comment.
241-243:⚠️ Potential issueFix default behavior when pod name is missing.
Defaulting to
truewhen pod name is missing could lead to all pods attempting to write, increasing conflicts.if (Strings.isNullOrEmpty(localPodName)) { - return true; + logger.warn("Local pod name is missing, defaulting to non-write pod"); + return false; }Likely invalid or redundant comment.
257-265: 🛠️ Refactor suggestionEnhance pod selection criteria.
The current pod selection logic should consider additional criteria for better reliability:
- Pod deletion timestamp (ignore pods being terminated)
- Pod readiness status
- Container statuses
return v1PodList.getItems().stream() .map(V1Pod::getMetadata) .filter(Objects::nonNull) + .filter(metadata -> metadata.getDeletionTimestamp() == null) .filter(metadata -> metadata.getCreationTimestamp() != null) .sorted(Comparator.comparing(V1ObjectMeta::getCreationTimestamp)) .map(V1ObjectMeta::getName) .limit(propertyKubernetesMaxWritePods) .anyMatch(localPodName::equals);Likely invalid or redundant comment.
apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java (2)
24-25: LGTM! Proper test setup with new dependencies.The test setup correctly initializes the KubernetesManager with the new constructor parameters and includes necessary imports.
Also applies to: 57-57
184-187: Verify pod listing parameters.The test verifies the correct parameters for pod listing:
- Running pods only
- App label selector
- Limit of 1000 (should be updated when MAX_SEARCH_NUM is reduced)
✅ Verification successful
Pod listing parameters are correctly implemented and tested
The test correctly verifies the pod listing parameters:
- The implementation uses MAX_SEARCH_NUM constant (1000)
- The test verifies this exact value
- All parameters including Running pods filter and app label selector are consistent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the limit parameter is consistently used across tests rg -A 2 "listNamespacedPod.*MAX_SEARCH_NUM"Length of output: 45
Script:
#!/bin/bash # Find MAX_SEARCH_NUM constant definition rg "MAX_SEARCH_NUM.*=.*\d+" # Find all listNamespacedPod calls to check limit parameter usage ast-grep --pattern 'listNamespacedPod($$$)'Length of output: 216
Script:
#!/bin/bash # Search for listNamespacedPod calls with context rg -B 2 -A 2 "listNamespacedPod\("Length of output: 2134
| private static final String RUNNING_POD_FIELD_SELECTOR = "status.phase=Running"; | ||
|
|
||
| private static final int MAX_SEARCH_NUM = 1000; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce MAX_SEARCH_NUM value to prevent performance issues.
Setting MAX_SEARCH_NUM to 1000 could impact k8s server performance when multiple clients pull pod lists simultaneously. Based on previous discussions, a value between 50-100 would be sufficient.
- private static final int MAX_SEARCH_NUM = 1000;
+ private static final int MAX_SEARCH_NUM = 50;📝 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 static final String RUNNING_POD_FIELD_SELECTOR = "status.phase=Running"; | |
| private static final int MAX_SEARCH_NUM = 1000; | |
| private static final String RUNNING_POD_FIELD_SELECTOR = "status.phase=Running"; | |
| private static final int MAX_SEARCH_NUM = 50; | |
| V1Pod pod = new V1Pod() | ||
| .metadata( | ||
| new V1ObjectMeta() | ||
| .name("localPodName") | ||
| .creationTimestamp(OffsetDateTime.now()) | ||
| .labels(Collections.singletonMap("app", "app"))); | ||
| V1PodList v1PodList = new V1PodList().addItemsItem(new V1Pod().metadata(pod.getMetadata())); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for pod write eligibility scenarios.
The current test only covers the basic success case. Add test cases for:
- Pod without required labels
- Pod with deletion timestamp
- Multiple pods with different creation timestamps
- Pods in non-running states
Would you like me to generate these additional test cases?
24kpure
force-pushed
the
feature/reduce_conflicts_when_update_configmap_in_k8s
branch
from
5db4993 to
0ce4201
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java (1)
155-161: 🛠️ Refactor suggestionConsider adding test cases for pod eligibility edge cases.
The current test only covers the basic case. Consider adding test cases for:
- Pod without required labels
- Pod with deletion timestamp
- Multiple pods with different creation timestamps
- Pods in non-running states
🧹 Nitpick comments (2)
apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java (1)
57-57: Consider extracting the magic number.The value
3appears to be a configuration parameter. Consider extracting it as a constant with a descriptive name to improve code maintainability and clarity.+ private static final int DEFAULT_MAX_WRITE_PODS = 3; - kubernetesManager = new KubernetesManager(coreV1Api, "localPodName", 3); + kubernetesManager = new KubernetesManager(coreV1Api, "localPodName", DEFAULT_MAX_WRITE_PODS);apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (1)
Line range hint
639-641: Consider adding validation for appId parameter.The method should validate the appId parameter to prevent null pointer exceptions.
public String getAccessKeySecret(String appId){ + if (Strings.isNullOrEmpty(appId)) { + throw new IllegalArgumentException("appId cannot be null or empty"); + } return Foundation.app().getAccessKeySecret(appId); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGES.md(1 hunks)apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java(4 hunks)apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java(1 hunks)apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGES.md
- apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (8)
🔇 Additional comments (2)
apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java (1)
24-25: LGTM!The new imports support testing the pod-based write permission functionality for ConfigMaps.
Also applies to: 28-28, 39-47
apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (1)
Line range hint
620-637: LGTM! Well-structured boolean property handling.The method follows a clear hierarchy for property resolution:
- System properties
- Environment variables
- Application properties
- Default value
| Mockito.verify(coreV1Api, Mockito.times(1)).listNamespacedPod(namespace, null, null, | ||
| null, "status.phase=Running", "app=app", | ||
| 100, null, null | ||
| , null, null); |
There was a problem hiding this comment.
Verify the pod listing parameters.
The verification uses different parameters than the actual call:
- Status phase selector is present in verification but missing in the actual call
- Limit is set to 100 in verification but missing in the actual call
Apply this diff to align the parameters:
- when(coreV1Api.listNamespacedPod(namespace, null, null,
- null, null, "app=app",
- null, null, null
- , null, null)).thenReturn(v1PodList);
+ when(coreV1Api.listNamespacedPod(namespace, null, null,
+ null, "status.phase=Running", "app=app",
+ 100, null, null
+ , null, null)).thenReturn(v1PodList);📝 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.
| Mockito.verify(coreV1Api, Mockito.times(1)).listNamespacedPod(namespace, null, null, | |
| null, "status.phase=Running", "app=app", | |
| 100, null, null | |
| , null, null); | |
| when(coreV1Api.listNamespacedPod(namespace, null, null, | |
| null, "status.phase=Running", "app=app", | |
| 100, null, null | |
| , null, null)).thenReturn(v1PodList); |
| when(coreV1Api.readNamespacedPod("localPodName", namespace, null)).thenReturn(pod); | ||
| when(coreV1Api.listNamespacedPod(namespace, null, null, | ||
| null, null, "app=app", | ||
| null, null, null | ||
| , null, null)).thenReturn(v1PodList); |
There was a problem hiding this comment.
💡 Codebase verification
Test case needs alignment with KubernetesManager implementation
The test case doesn't match the actual implementation in KubernetesManager.java:
- The label selector should be dynamically constructed from pod metadata's "app" label value
- One test case is missing the required RUNNING_POD_FIELD_SELECTOR
- The MAX_SEARCH_NUM limit (100) should be used in the listNamespacedPod calls
🔗 Analysis chain
Verify the listNamespacedPod parameters.
The API call uses hardcoded values that might need adjustment:
- The label selector "app=app" seems generic
- The status phase selector is missing
- The limit parameter is not set
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test files that might define these constants
rg -t java "app=app" .
rg -t java "status\.phase=Running" .
Length of output: 675
Script:
#!/bin/bash
# Search for the implementation and any constants
ast-grep --pattern 'private static final String $_ = "app=$_";'
cat ./apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
Length of output: 11517
|
Thanks for your advice. I have removed the configuration and you can have a look~ |
|
Thanks for your contribution. :) |
What's the purpose of this PR
fix #89
Which issue(s) this PR fixes:
fix #89
Brief changelog
reduce conflicts when update configmap in k8s
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
Improvements
Testing