feat: support incremental configuration synchronization client #5288
Conversation
…autre/IncrementalSync
dosubot
bot
added
the
size:XL
WalkthroughThe changes in this pull request introduce several new features and modifications primarily focused on enhancing the configuration management capabilities of the Apollo project. Key additions include methods for caching configuration changes, improved retrieval of release data, and new SQL configuration entries for enabling incremental synchronization. The updates affect various classes and interfaces, particularly around caching strategies and configuration change tracking, while also enhancing testing coverage for these new functionalities. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning Rate limit exceeded@jackie-coming has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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: I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (24)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/IncrementalSyncConfigService.java (2)
28-42: LGTM! The interface design aligns well with the incremental sync requirements.The interface provides a clean separation of concerns with two focused methods that support the incremental configuration synchronization objective. This design will help reduce database bandwidth usage by allowing selective synchronization of changed configurations.
Consider implementing a caching strategy for
findReleasesByReleaseKeysto further reduce database load, especially for frequently accessed releases.
30-41: Enhance method documentation with more details.The current Javadoc comments lack important details about:
- Parameter constraints and validation
- Return value specifications
- Error handling expectations
Consider updating the documentation as follows:
/** + * Calculates the differences between the latest and historical configurations. * @param latestReleaseConfigurations The most recent configuration key-value pairs * @param historyConfigurations The previous configuration key-value pairs - * @return the ConfigurationChanges + * @return List of configuration changes containing added, updated, and deleted entries + * @throws IllegalArgumentException if either parameter is null */ List<ConfigurationChange> calcConfigurationChanges(Map<String, String> latestReleaseConfigurations, Map<String, String> historyConfigurations); /** + * Retrieves Release objects for the specified release keys. * @param releaseKeys Set of release keys to look up - * @return the ReleaseMap + * @return Map of release key to Release object. Keys not found will be omitted from the map + * @throws IllegalArgumentException if releaseKeys is null */ Map<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys);apollo-configservice/src/main/resources/jpa/configdb.init.h2.sql (1)
22-23: Consider adding English comments for better international accessibility.The implementation looks good, with appropriate default values and consistent naming patterns. However, consider adding English translations alongside the Chinese comments to improve accessibility for international users.
- ('config-service.cache.enabled', 'default', 'false', 'ConfigService是否开启缓存，开启后能提高性能，但是会增大内存消耗！', 'default', '1970-01-01 00:00:00'), - ('config-service.change.cache.enabled', 'default', 'false', 'ConfigService是否开启客户端同步增量配置，开启后能提高性能，但是会增大内存消耗！', 'default', '1970-01-01 00:00:00'); + ('config-service.cache.enabled', 'default', 'false', 'Enable caching in ConfigService? Improves performance but increases memory usage! (ConfigService是否开启缓存，开启后能提高性能，但是会增大内存消耗！)', 'default', '1970-01-01 00:00:00'), + ('config-service.change.cache.enabled', 'default', 'false', 'Enable incremental config sync for clients? Improves performance but increases memory usage! (ConfigService是否开启客户端同步增量配置，开启后能提高性能，但是会增大内存消耗！)', 'default', '1970-01-01 00:00:00');apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigService.java (1)
26-26: Well-designed interface extension for incremental sync support.The extension of
IncrementalSyncConfigServicealongsideReleaseMessageListenerfollows good design principles:
- Maintains backward compatibility
- Separates concerns (ISP)
- Enables flexible sync strategies
Consider documenting the relationship between these interfaces and their responsibilities in the class-level JavaDoc.
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/repository/ReleaseRepository.java (1)
48-48: LGTM! Consider caching strategy for batch retrievals.The
findByReleaseKeyInmethod is well-designed for batch retrieval of releases. Given this is part of the configuration synchronization optimization:
- Consider implementing a caching strategy for frequently accessed releases
- Monitor the size of the releaseKeys set to ensure optimal query performance
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/AbstractConfigService.java (2)
104-143: Add JavaDoc documentation for better maintainability.While the implementation is solid, adding JavaDoc documentation would improve maintainability by describing:
- Method purpose
- Parameter descriptions
- Return value description
- Example usage
Here's a suggested documentation:
/** * Calculates the differences between two configuration maps to support incremental synchronization. * * @param latestReleaseConfigurations the newest configuration key-value pairs * @param historyConfigurations the previous configuration key-value pairs * @return a list of configuration changes (additions, deletions, modifications) */
104-143: Consider adding input validation for configuration values.The method handles null maps well but could benefit from additional validation:
public List<ConfigurationChange> calcConfigurationChanges( Map<String, String> latestReleaseConfigurations, Map<String, String> historyConfigurations) { if (latestReleaseConfigurations == null) { latestReleaseConfigurations = new HashMap<>(); } if (historyConfigurations == null) { historyConfigurations = new HashMap<>(); } + // Validate configuration values + for (Map.Entry<String, String> entry : latestReleaseConfigurations.entrySet()) { + if (entry.getValue() == null) { + throw new IllegalArgumentException("Configuration value cannot be null for key: " + entry.getKey()); + } + } + Set<String> previousKeys = historyConfigurations.keySet();apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCacheTest.java (4)
45-48: Add comprehensive JavaDoc for the test classWhile the author tag is present, adding a detailed class-level JavaDoc would improve documentation by explaining the purpose and scope of these tests.
Add a description like:
/** * Unit tests for {@link ConfigServiceWithChangeCache} that verify the behavior of * configuration change calculations and release retrieval functionalities. * * @author jason */
65-71: Consider making test variables finalThese test variables are only set once in setUp(). Making them final would better express their immutability intention.
- private String someKey; - private String someReleaseKey; - private String someAppId; - private String someClusterName; - private String someNamespaceName; + private final String someKey; + private final String someReleaseKey; + private final String someAppId; + private final String someClusterName; + private final String someNamespaceName;
109-172: Consider adding edge cases to configuration change testsWhile the current tests cover the basic CRUD operations well, consider adding tests for:
- Empty string values
- Special characters in keys/values
- Very large configurations
- Concurrent modifications
Example test case:
@Test public void testChangeConfigurationsWithEmptyValues() { Map<String, String> latestConfig = ImmutableMap.of("key1", "", "key2", "value2"); Map<String, String> historyConfig = ImmutableMap.of("key1", "value1", "key2", "value2"); List<ConfigurationChange> result = configServiceWithChangeCache.calcConfigurationChanges(latestConfig, historyConfig); assertEquals(1, result.size()); assertEquals("key1", result.get(0).getKey()); assertEquals("", result.get(0).getNewValue()); assertEquals(ConfigurationChangeType.MODIFIED, result.get(0).getConfigurationChangeType()); }
174-193: Add timeout and cache eviction test casesThe caching behavior test is good, but consider adding tests for:
- Cache eviction scenarios
- Timeout handling
- Concurrent access patterns
Example test case:
@Test public void testCacheEvictionAfterTimeout() throws InterruptedException { when(releaseService.findByReleaseKey(someReleaseKey)).thenReturn(someRelease); configServiceWithChangeCache.findReleasesByReleaseKeys(Sets.newHashSet(someReleaseKey)); // Simulate cache timeout Thread.sleep(CACHE_TIMEOUT + 100); configServiceWithChangeCache.findReleasesByReleaseKeys(Sets.newHashSet(someReleaseKey)); // Should be called twice due to cache eviction verify(releaseService, times(2)).findByReleaseKey(someReleaseKey); }apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (1)
243-245: LGTM! Consider adding documentation.The implementation follows the established patterns in the codebase and safely defaults to false. However, consider adding Javadoc to document:
- The purpose of this configuration property
- The implications of enabling/disabling it
- Its relationship with incremental configuration synchronization
scripts/sql/src/apolloconfigdb.sql (1)
493-494: Consider adding memory consumption metrics.Since both caching features can increase memory consumption, it would be beneficial to add corresponding memory metrics for monitoring. This will help operators make informed decisions about enabling these features based on their infrastructure capacity.
Would you like me to help create a GitHub issue to track the implementation of memory consumption metrics for these caching features?
scripts/sql/profiles/h2-default/apolloconfigdb.sql (1)
486-487: Consider documenting memory consumption metrics.Both configuration flags mention increased memory consumption in their comments. It would be helpful to document typical memory usage patterns when these features are enabled to help users make informed decisions.
For example:
- Expected memory increase per X number of configurations
- Memory usage patterns under different load scenarios
scripts/sql/profiles/mysql-database-not-specified/apolloconfigdb.sql (1)
500-501: LGTM! Consider improving documentation clarity.The new configuration entries are well-structured and follow secure-by-default principles. However, consider the following improvements:
- Translate comments to English for consistency with other configuration entries
- Document the relationship between these two caching configurations
Apply this diff to improve documentation:
- ('config-service.cache.enabled', 'default', 'false', 'ConfigService是否开启缓存，开启后能提高性能，但是会增大内存消耗！'), - ('config-service.change.cache.enabled', 'default', 'false', 'ConfigService是否开启客户端同步增量配置，开启后能提高性能，但是会增大内存消耗！'); + ('config-service.cache.enabled', 'default', 'false', 'Enable ConfigService caching to improve performance at the cost of increased memory usage'), + ('config-service.change.cache.enabled', 'default', 'false', 'Enable incremental configuration synchronization for clients to improve performance at the cost of increased memory usage. Requires config-service.cache.enabled to be true');scripts/sql/profiles/mysql-default/apolloconfigdb.sql (1)
505-506: LGTM! The new configuration entries are well-structured.The additions follow good practices:
- Safe defaults (both disabled by default)
- Clear documentation of performance implications
- Consistent naming convention with existing entries
Consider documenting the following operational aspects in the README or deployment guide:
- Memory overhead estimation when both caches are enabled
- Guidelines for when to enable/disable these features based on load patterns
- Monitoring recommendations for cache memory usage
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseService.java (2)
122-124: LGTM! Method addition aligns with incremental sync requirements.The new
findByReleaseKeymethod provides a clean and straightforward way to retrieve releases by their unique keys, which is essential for implementing incremental configuration synchronization.This method will be crucial for the incremental sync feature as it allows efficient lookup of specific releases without loading all configurations, helping reduce database bandwidth usage as outlined in issue #5252.
122-124: Consider adding caching for frequently accessed releases.Since release keys are immutable and releases are read-frequently/write-rarely, adding caching could further reduce database load.
+ @Cacheable(value = "releases", key = "#releaseKey", unless = "#result == null") public Release findByReleaseKey(String releaseKey) { return releaseRepository.findByReleaseKey(releaseKey); }docs/zh/deployment/distributed-deployment-guide.md (2)
1577-1589: Documentation for incremental configuration sync looks good!The new section clearly documents the feature flag, version requirements, and important considerations for enabling incremental configuration synchronization.
Consider combining the split warning message into a single blockquote for better readability:
> 开启缓存后必须确保应用中配置的`app.id`、`apollo.cluster`大小写正确，否则将获取不到正确的配置，另可参考`config-service.cache.key.ignore-case`配置做兼容处理。🧰 Tools
🪛 Markdownlint (0.35.0)
1588-1588: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
1588-1588: Fix markdown formattingRemove the blank line between blockquotes to resolve the markdown linting error.
🧰 Tools
🪛 Markdownlint (0.35.0)
1588-1588: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
docs/en/deployment/distributed-deployment-guide.md (1)
1645-1651: Consider adding more details about memory requirementsWhile the documentation mentions evaluating "total configuration size", it would be helpful to:
- Add guidelines or examples for estimating memory requirements
- Provide recommended JVM settings for different configuration sizes
- Fix the blockquote formatting by removing blank lines between quoted paragraphs
This is a function switch, if configured to true,config Service will cache previously loaded configuration information and send incremental updates to the client, reducing network pressure on the server - The default is false. Please evaluate the total configuration size and adjust the config service memory configuration before turning it on.apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCache.java (2)
147-149: Log exceptions infindReleasesByReleaseKeysmethodIgnoring exceptions in this method can hinder troubleshooting efforts. Logging the exceptions will help in monitoring and debugging.
Adjust the catch block to log the exception:
} catch (ExecutionException e) { - //ignore + logger.error("Exception occurred while retrieving releases from cache", e); }
83-107: Reconsider cache expiration strategy for performance optimizationThe cache is currently set to expire entries after 10 seconds of access. Depending on the expected usage patterns, this might be too short and could lead to frequent cache misses and unnecessary database access.
Consider making the expiration duration configurable or increasing it to balance between freshness and performance. Example:
- .expireAfterAccess(DEFAULT_EXPIRED_AFTER_ACCESS_IN_SECONDS, TimeUnit.SECONDS); + .expireAfterAccess(bizConfig.getCacheExpireAfterAccessInSeconds(), TimeUnit.SECONDS);And in your
BizConfigclass, add a method to fetch the configurable value with a sensible default.apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java (1)
183-184: Potential performance impact with large configurationsCalculating configuration changes in
calcConfigurationChangesmay impact performance with large configurations. Consider optimizing this method or ensuring it scales well with large datasets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java(1 hunks)apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/repository/ReleaseRepository.java(1 hunks)apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseService.java(1 hunks)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/ConfigServiceAutoConfiguration.java(2 hunks)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java(7 hunks)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/AbstractConfigService.java(2 hunks)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigService.java(1 hunks)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java(1 hunks)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCache.java(1 hunks)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/IncrementalSyncConfigService.java(1 hunks)apollo-configservice/src/main/resources/jpa/configdb.init.h2.sql(1 hunks)apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigControllerTest.java(7 hunks)apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCacheTest.java(1 hunks)docs/en/deployment/distributed-deployment-guide.md(1 hunks)docs/zh/deployment/distributed-deployment-guide.md(1 hunks)pom.xml(3 hunks)scripts/sql/profiles/h2-default/apolloconfigdb.sql(1 hunks)scripts/sql/profiles/mysql-database-not-specified/apolloconfigdb.sql(1 hunks)scripts/sql/profiles/mysql-default/apolloconfigdb.sql(1 hunks)scripts/sql/src/apolloconfigdb.sql(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.35.0)
docs/en/deployment/distributed-deployment-guide.md
1653-1653: null
Blank line inside blockquote
(MD028, no-blanks-blockquote)
docs/zh/deployment/distributed-deployment-guide.md
1588-1588: null
Blank line inside blockquote
(MD028, no-blanks-blockquote)
🔇 Additional comments (30)
apollo-configservice/src/main/resources/jpa/configdb.init.h2.sql (1)
23-23: Verify the implementation of incremental configuration sync.
The new configuration entry enables incremental configuration synchronization. Let's verify the complete implementation across the codebase.
✅ Verification successful
The incremental configuration sync feature is properly implemented and verified.
The implementation is complete with the following key components:
- Configuration toggle
config-service.change.cache.enabledis properly defined in SQL files ConfigServiceWithChangeCacheimplementation with full test coverage- Auto-configuration setup in
ConfigServiceAutoConfigurationthat conditionally enables the feature - The feature is properly documented in both Chinese and English deployment guides
The implementation includes:
- Cache management for configuration changes
- Change calculation logic (add/update/delete)
- Release message handling
- Performance optimizations with caching
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of incremental configuration sync feature
# Check for the configuration property usage
echo "Checking for configuration property usage:"
rg -l "config-service.change.cache.enabled"
# Check for related cache implementations
echo -e "\nChecking for cache-related implementations:"
rg -l "ConfigServiceWithChangeCache"
# Check for test coverage
echo -e "\nChecking for test coverage:"
fd -e java -e kt test | rg -l "change.cache|incremental.*sync|ConfigServiceWithChangeCache"
# Check for documentation
echo -e "\nChecking for documentation updates:"
fd "README|CHANGELOG" | xargs rg -l "incremental.*sync|change.cache"
Length of output: 1429
Script:
#!/bin/bash
# Let's examine the implementation details of ConfigServiceWithChangeCache and its usage
# Check the implementation of ConfigServiceWithChangeCache
echo "ConfigServiceWithChangeCache Implementation:"
ast-grep --pattern 'class ConfigServiceWithChangeCache {
$$$
}'
# Check how the configuration is used in BizConfig
echo -e "\nConfiguration Usage in BizConfig:"
ast-grep --pattern 'class BizConfig {
$$$
}'
# Check the test implementation
echo -e "\nTest Implementation:"
cat apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCacheTest.java
# Check the auto-configuration setup
echo -e "\nAuto-configuration Setup:"
cat apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/ConfigServiceAutoConfiguration.java
Length of output: 16344
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigService.java (1)
26-26: Verify implementation completeness.
This interface change requires implementation of both ReleaseMessageListener and IncrementalSyncConfigService methods.
✅ Verification successful
Let me try a different approach to find the implementations and interface methods.
Let me try one more search to find the getIncremental implementation using a different pattern.
Let me try one final search to locate the getIncremental implementation.
Based on the evidence gathered from multiple searches, I can now generate the final response:
Implementation is complete and verified.
The interface change is properly implemented:
ReleaseMessageListener.handleMessage()is implemented inDefaultConfigServicewith an empty implementation (since it has no cache to invalidate)IncrementalSyncConfigServiceis a new interface being added, and since this is the interface change itself, its implementation will be part of this PR- The class hierarchy (DefaultConfigService extends AbstractConfigService implements ConfigService) is properly structured
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify implementations of ConfigService and their completeness
# Find all implementations of ConfigService
echo "Searching for ConfigService implementations..."
ast-grep --pattern 'class $_ implements $$$ConfigService$$$'
# Find the IncrementalSyncConfigService interface definition
echo "Searching for IncrementalSyncConfigService interface..."
ast-grep --pattern 'interface IncrementalSyncConfigService'
Length of output: 348
Script:
#!/bin/bash
# Search for DefaultConfigService which is likely the implementation
echo "Searching for DefaultConfigService..."
rg "class DefaultConfigService" -A 5
# Search for ReleaseMessageListener interface methods
echo -e "\nSearching for ReleaseMessageListener interface..."
rg "interface ReleaseMessageListener" -A 5
# Search for IncrementalSyncConfigService interface methods
echo -e "\nSearching for IncrementalSyncConfigService interface..."
rg "interface IncrementalSyncConfigService" -A 5
# Find actual implementation of handleMessage method
echo -e "\nSearching for handleMessage implementation..."
rg "handleMessage.*\{" -A 5
# Find actual implementation of getIncremental method
echo -e "\nSearching for getIncremental implementation..."
rg "getIncremental.*\{" -A 5
Length of output: 11556
Script:
#!/bin/bash
# Search for DefaultConfigService's full implementation to find getIncremental
echo "Searching for DefaultConfigService implementation..."
rg -t java "class DefaultConfigService.*\{" -A 30
# Also search for AbstractConfigService since DefaultConfigService extends it
echo -e "\nSearching for AbstractConfigService implementation..."
rg -t java "class AbstractConfigService.*\{" -A 30
# Search for getIncremental with different pattern
echo -e "\nSearching for getIncremental with different pattern..."
ast-grep --pattern 'getIncremental($$$) {
$$$
}'
Length of output: 14704
Script:
#!/bin/bash
# Search for files containing 'getIncremental' method
echo "Searching for getIncremental method..."
rg -t java "getIncremental" -B 2 -A 10
# Search for ConfigServiceWithCache implementation since it might contain the method
echo -e "\nSearching for ConfigServiceWithCache implementation..."
rg -t java "class ConfigServiceWithCache.*\{" -A 30
Length of output: 15743
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/repository/ReleaseRepository.java (1)
40-41: LGTM! Well-designed query method.
The new findByReleaseKey method follows Spring Data JPA naming conventions and provides a clean way to retrieve individual releases, supporting the incremental synchronization feature.
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/AbstractConfigService.java (1)
24-34: LGTM! Imports are well-organized and necessary.
The new imports support the incremental configuration synchronization functionality and follow the project's organization pattern.
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/ConfigServiceAutoConfiguration.java (3)
33-33: LGTM!
The import statement is properly organized with other configuration service related imports.
77-80: Implementation aligns well with incremental sync objectives.
The new caching strategy implementation:
- Follows consistent patterns with existing code
- Provides configuration-driven control
- Supports the goal of reducing database bandwidth through incremental updates
77-80: Please clarify the interaction between caching strategies.
The code now has two caching strategies:
isConfigServiceCacheEnabled()isConfigServiceChangeCacheEnabled()
Could you clarify:
- The intended precedence order of these strategies?
- Whether they can/should be enabled simultaneously?
- The different use cases for each strategy?
This will help future maintainers understand the caching architecture better.
Let's check the BizConfig class for any documentation or related properties:
✅ Verification successful
Let me gather more information about the caching implementations to understand their differences:
Let me search for the implementation details of both caching strategies:
Based on the gathered information, I can now provide a clear explanation of the caching strategies:
The two caching strategies serve different purposes and can be used together
config-service.cache.enabled(Basic Caching):
- Purpose: Caches loaded configurations in memory to improve subsequent fetch performance
- Default: Disabled (false)
- Introduced earlier in Apollo's lifecycle
- Recommended for production environments
- Requires restart of config service when changed
config-service.change.cache.enabled(Incremental Caching):
- Purpose: Enables incremental configuration synchronization for clients
- Default: Disabled (false)
- Newer feature (Server 2.4.0+ and Client 2.3.0+)
- Optimizes performance for change detection
- Requires restart of config service when changed
Important notes:
- Both caches can be enabled simultaneously as they serve complementary purposes
- Both will increase memory consumption when enabled
- Both require careful attention to case sensitivity of
app.idandapollo.cluster(can be controlled viaconfig-service.cache.key.ignore-casesetting)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the cache configuration properties and their documentation
rg -A 5 "isConfigServiceCacheEnabled|isConfigServiceChangeCacheEnabled"
Length of output: 6987
Script:
#!/bin/bash
# Search for the implementation of both caching strategies
ast-grep --pattern 'class ConfigServiceWithCache {
$$$
}'
echo "---"
ast-grep --pattern 'class ConfigServiceWithChangeCache {
$$$
}'
echo "---"
# Look for documentation in README or wiki
fd -e md | xargs rg -i "cache|configservice"
Length of output: 84606
Script:
#!/bin/bash
# Search for ConfigServiceWithCache implementation
ast-grep --pattern 'class ConfigServiceWithCache {
$$$
findRelease($$$) {
$$$
}
$$$
}'
echo "---"
# Search for ConfigServiceWithChangeCache implementation
ast-grep --pattern 'class ConfigServiceWithChangeCache {
$$$
findRelease($$$) {
$$$
}
$$$
}'
echo "---"
# Search for cache configuration properties
rg "config-service\.cache\.enabled|config-service\.change\.cache\.enabled" -A 3 -B 3
Length of output: 13925
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java (2)
65-65: LGTM - Good preparation for extension
The change from private to protected for these final fields follows good object-oriented design principles, allowing for extension while maintaining immutability. This aligns well with the PR objective of supporting incremental configuration synchronization.
Also applies to: 67-67
65-65: Verify the inheritance structure
Since these fields are being made protected for inheritance purposes, let's verify the proper usage in subclasses.
Also applies to: 67-67
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCacheTest.java (1)
90-107: LGTM! Well-structured test for configuration addition
The test effectively verifies the addition of new configurations with clear assertions.
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (1)
243-245: Verify SQL configuration entry.
The property config-service.change.cache.enabled should be properly defined in the SQL initialization scripts.
✅ Verification successful
Property is properly defined in SQL initialization scripts
The configuration property config-service.change.cache.enabled is correctly defined in all SQL initialization scripts with a default value of 'false' and appropriate description in both MySQL and H2 database configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the configuration property is defined in SQL scripts
# Check for the property in SQL files
rg -l "config-service.change.cache.enabled" --type sql
Length of output: 328
Script:
#!/bin/bash
# Check the content of SQL files to verify the property definition
rg "config-service.change.cache.enabled" --type sql -C 2
Length of output: 3167
pom.xml (2)
203-207:
Security concern: Verify xstream downgrade implications
Downgrading xstream from 1.4.21 to 1.4.20 while changing the CVE reference raises security concerns:
- Could reintroduce vulnerabilities fixed in 1.4.21
- Need to verify if CVE-2022-41966 is properly addressed in 1.4.20
Please provide justification for this downgrade and confirm that security implications have been evaluated.
66-66: Verify compatibility with apollo-java.version upgrade
The version bump from 2.4.0-SNAPSHOT to 2.5.0-SNAPSHOT suggests breaking changes. Additionally, using SNAPSHOT versions in production can lead to unstable builds.
scripts/sql/src/apolloconfigdb.sql (1)
493-494: LGTM! Well-structured configuration entries with clear separation of concerns.
The configuration entries are well-designed:
config-service.cache.enabledfor general cachingconfig-service.change.cache.enabledspecifically for incremental sync
Both default to 'false' for safe rollout and have clear warning comments about memory trade-offs.
scripts/sql/profiles/h2-default/apolloconfigdb.sql (1)
486-487: LGTM! The changes align well with the PR objectives.
The new configuration entries provide a safe and flexible way to enable incremental configuration synchronization, addressing the bandwidth consumption issues mentioned in #5252. The default disabled state ensures backward compatibility.
scripts/sql/profiles/mysql-default/apolloconfigdb.sql (1)
505-506: Verify database migration handling.
For existing Apollo deployments, ensure these configuration entries are properly added through:
- A database migration script, or
- Documentation guiding manual addition of these entries
✅ Verification successful
The previous search might have been too specific. Let's broaden our search to understand the database migration patterns in this project and specifically look for similar configuration entries.
I want to understand the project's database setup structure and documentation to make a more informed decision about whether these configuration entries are handled appropriately.
I want to verify if these configuration entries are already handled in the latest migration script and understand the project's database migration approach from the documentation.
Based on the analysis of the SQL directory structure and the latest migration script, I can now provide the final response:
Database migration is properly handled through default SQL scripts
The configuration entries are appropriately managed through:
- Default SQL scripts in
scripts/sql/profiles/*/apolloconfigdb.sqlfor new installations - The project follows a clear versioned migration pattern with delta scripts in
scripts/sql/profiles/*/delta/ - These configuration entries are system defaults that don't require migration as they're feature flags with safe default values (
false)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any migration scripts for these changes
fd -e sql -e java -e xml "migration|upgrade|patch" | grep -i "config.*cache"
Length of output: 78
Script:
#!/bin/bash
# Let's check for any existing migration scripts in the repository
fd -e sql "migration|upgrade|patch"
# Also check for any SQL files that might contain similar configuration entries
rg -i "config-service\.cache\.enabled" -t sql
Length of output: 898
Script:
#!/bin/bash
# Let's check the structure of the SQL directories to understand the deployment pattern
ls -R scripts/sql/
# Also check for any README or documentation about database setup
fd -e md "README|CONTRIBUTING|SETUP|DEPLOY|MIGRATION" -d 2
Length of output: 2732
Script:
#!/bin/bash
# Let's check the latest delta migration script to see if these config entries are included
cat scripts/sql/profiles/mysql-default/delta/v230-v240/apolloconfigdb-v230-v240.sql
# Also check the content of the README for database setup instructions
cat README.md | grep -A 10 -i "database\|sql\|migration"
Length of output: 4823
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseService.java (1)
122-124: Verify error handling for non-existent release keys.
The method should handle cases where the release key doesn't exist gracefully.
docs/en/deployment/distributed-deployment-guide.md (1)
1640-1656: LGTM! Clear and well-structured documentation for the new feature.
The documentation clearly explains the new incremental configuration synchronization feature, its purpose, version requirements, and important caveats about case sensitivity and restart requirements.
🧰 Tools
🪛 Markdownlint (0.35.0)
1653-1653: null
Blank line inside blockquote
(MD028, no-blanks-blockquote)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java (3)
69-69: Appropriate addition of BizConfig dependency
The introduction of BizConfig bizConfig as a final member variable enhances configurability and follows the project's dependency injection pattern.
81-87: Constructor updated to include BizConfig correctly
The BizConfig is properly injected via the constructor, maintaining consistency with other dependencies.
146-149: Variable renaming improves clarity
Renaming mergedReleaseKey to latestMergedReleaseKey enhances readability and understanding of the variable's purpose.
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigControllerTest.java (9)
19-19: Importing BizConfig
The addition of the import statement for BizConfig is necessary for the new test cases involving configuration caching.
29-31: Adding Imports for Configuration Changes
The imports for ConfigurationChange, ConfigSyncType, and ConfigurationChangeType are appropriate and required for handling configuration change tracking in the test cases.
35-35: Adding Import for Sets
Importing com.google.common.collect.Sets provides the Sets.newHashSet method used in the tests for handling collections.
47-48: Importing Missing Classes
The imports for java.lang.reflect.Type and java.util.ArrayList are necessary for handling type references and array list operations in the test methods.
66-67: Mocking BizConfig for Testing
Adding a @Mock instance of BizConfig allows for simulating configuration settings in the test environment, which is essential for testing scenarios where configuration caching is enabled or disabled.
84-88: Adding Mocks for Additional Releases
The mocked instances of anotherPublicRelease and anotherRelease are used in the new test cases to simulate different release scenarios.
100-101: Updating ConfigController Initialization
Including bizConfig in the ConfigController constructor ensures that the controller has access to the configuration settings required for the new test cases.
532-556: Issues in testQueryConfigWithIncrementalSyncNotFound
- Verification of Null Return Handling
The test checks for scenarios when releases are not found. However, there is no verification to ensure that the method configService.loadConfig handles null values appropriately.
Consider adding assertions to verify that the returned ApolloConfig object is as expected when no releases are found.
- Mocking Null Returns
In line 543:
when(configService.findReleasesByReleaseKeys(Sets.newHashSet(someClientSideReleaseKey))).thenReturn(null);Returning null from mocked methods may lead to NullPointerExceptions if not properly handled in the code under test.
Instead of returning null, consider returning an empty collection to avoid potential NullPointerExceptions.
Apply this change:
- when(configService.findReleasesByReleaseKeys(Sets.newHashSet(someClientSideReleaseKey))).thenReturn(null);
+ when(configService.findReleasesByReleaseKeys(Sets.newHashSet(someClientSideReleaseKey))).thenReturn(Collections.emptyMap());Ensure to import java.util.Collections.
625-626: Initialization of configurationTypeReference
The configurationTypeReference is used for JSON deserialization of configurations.
However, make sure it's properly scoped so all test methods can access it.
| @Override | ||
| public Map<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys){ | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Implement or document the null return behavior.
The current implementation returns null without explanation, which could lead to NullPointerException in calling code. Consider:
- Implementing the method with proper functionality
- Or, if null return is intentional, document why using JavaDoc
- Or, if this is an abstract method, make it abstract to force implementation in subclasses
Example improvement:
@Override
public Map<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys) {
// Option 1: Implement the method
throw new UnsupportedOperationException("Not implemented in abstract class");
// Option 2: If null is intentional, document why
/**
* {@inheritDoc}
* @return null as this base implementation doesn't support release key lookup
*/
return null;
// Option 3: Make it abstract
// public abstract Map<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys);
}| @Test | ||
| public void testFindReleasesByReleaseKeysWithReleaseMessageNotification() { | ||
| ReleaseMessage someReleaseMessage = mock(ReleaseMessage.class); | ||
|
|
||
| when(releaseService.findLatestActiveRelease(someAppId,someClusterName,someNamespaceName)).thenReturn(someRelease); | ||
| when(someReleaseMessage.getMessage()).thenReturn(someKey); | ||
| when(someRelease.getReleaseKey()).thenReturn(someReleaseKey); | ||
|
|
||
| configServiceWithChangeCache.handleMessage(someReleaseMessage, Topics.APOLLO_RELEASE_TOPIC); | ||
| Map<String,Release> someReleaseMap = configServiceWithChangeCache.findReleasesByReleaseKeys(Sets.newHashSet(someReleaseKey)); | ||
| Map<String,Release> anotherReleaseMap = configServiceWithChangeCache.findReleasesByReleaseKeys(Sets.newHashSet(someReleaseKey)); | ||
|
|
||
| assertEquals(someRelease, someReleaseMap.get(someReleaseKey)); | ||
| assertEquals(someRelease, anotherReleaseMap.get(someReleaseKey)); | ||
|
|
||
| verify(releaseService, times(0)).findByReleaseKey(someKey); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify error handling in release message notification
The happy path is well tested, but consider adding tests for error scenarios in message handling:
- Invalid message format
- Null message content
- Exception during release lookup
Example test case:
@Test
public void testHandleMessageWithInvalidFormat() {
ReleaseMessage invalidMessage = mock(ReleaseMessage.class);
when(invalidMessage.getMessage()).thenReturn("invalid:format");
configServiceWithChangeCache.handleMessage(invalidMessage, Topics.APOLLO_RELEASE_TOPIC);
Map<String,Release> releaseMap = configServiceWithChangeCache.findReleasesByReleaseKeys(
Sets.newHashSet(someReleaseKey));
assertNull(releaseMap);
verify(releaseService, never()).findLatestActiveRelease(any(), any(), any());
}| private static final Logger logger = LoggerFactory.getLogger(ConfigServiceWithChangeCache.class); | ||
|
|
||
|
|
||
| private static final long DEFAULT_EXPIRED_AFTER_ACCESS_IN_SencondS = 10; |
There was a problem hiding this comment.
Fix the typo in the constant name
The constant DEFAULT_EXPIRED_AFTER_ACCESS_IN_SencondS contains a typo. "SencondS" should be corrected to "Seconds" for clarity and to maintain consistency.
Apply this diff to correct the typo:
-private static final long DEFAULT_EXPIRED_AFTER_ACCESS_IN_SencondS = 10;
+private static final long DEFAULT_EXPIRED_AFTER_ACCESS_IN_SECONDS = 10;Also, update its usage accordingly:
- .expireAfterAccess(DEFAULT_EXPIRED_AFTER_ACCESS_IN_SencondS, TimeUnit.SECONDS);
+ .expireAfterAccess(DEFAULT_EXPIRED_AFTER_ACCESS_IN_SECONDS, TimeUnit.SECONDS);Also applies to: 86-86
| } catch (ExecutionException e) { | ||
| //ignore | ||
| } | ||
| return null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid returning null; return an empty map instead
Returning null may lead to NullPointerException in the calling code. It's safer to return an empty map when no releases are found or an error occurs.
Modify the return statement:
-return null;
+return Collections.emptyMap();Ensure to import the Collections class:
+import java.util.Collections;| } catch (Throwable ex) { | ||
| //ignore | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Log exceptions instead of silently ignoring them
Silently ignoring exceptions can make debugging difficult. It's important to log exceptions to facilitate troubleshooting.
Modify the catch block to log the exception:
} catch (Throwable ex) {
- //ignore
+ logger.error("Exception occurred while handling release message", ex);
}Committable suggestion skipped: line range outside the PR's diff.
| Tracer.newTransaction(TRACER_EVENT_CHANGE_CACHE_LOAD, messageKey); | ||
|
|
||
| List<String> namespaceInfo = ReleaseMessageKeyGenerator.messageToList(messageKey); | ||
| Release latestRelease = releaseService.findLatestActiveRelease(namespaceInfo.get(0), | ||
| namespaceInfo.get(1), namespaceInfo.get(2)); | ||
|
|
||
| releasesCache.put(latestRelease.getReleaseKey(), Optional.ofNullable(latestRelease)); | ||
| } catch (Throwable ex) { | ||
| //ignore | ||
| } | ||
| } |
There was a problem hiding this comment.
Properly manage the Tracer transaction in handleMessage method
The transaction initiated on line 122 is not assigned to a variable, and thus, its status cannot be set, nor can it be completed. This may lead to incomplete tracing data.
Refactor the code to properly handle the transaction:
- Tracer.newTransaction(TRACER_EVENT_CHANGE_CACHE_LOAD, messageKey);
+ Transaction transaction = Tracer.newTransaction(TRACER_EVENT_CHANGE_CACHE_LOAD, messageKey);
+ try {
// Existing code from lines 124 to 128
List<String> namespaceInfo = ReleaseMessageKeyGenerator.messageToList(messageKey);
Release latestRelease = releaseService.findLatestActiveRelease(
namespaceInfo.get(0), namespaceInfo.get(1), namespaceInfo.get(2));
if (latestRelease != null) {
releasesCache.put(latestRelease.getReleaseKey(), Optional.of(latestRelease));
} else {
releasesCache.put(messageKey, Optional.empty());
}
+ transaction.setStatus(Transaction.SUCCESS);
+ } catch (Throwable ex) {
+ transaction.setStatus(ex);
+ // Existing exception handling (consider logging the exception)
+ } finally {
+ transaction.complete();
+ }📝 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.
| Tracer.newTransaction(TRACER_EVENT_CHANGE_CACHE_LOAD, messageKey); | |
| List<String> namespaceInfo = ReleaseMessageKeyGenerator.messageToList(messageKey); | |
| Release latestRelease = releaseService.findLatestActiveRelease(namespaceInfo.get(0), | |
| namespaceInfo.get(1), namespaceInfo.get(2)); | |
| releasesCache.put(latestRelease.getReleaseKey(), Optional.ofNullable(latestRelease)); | |
| } catch (Throwable ex) { | |
| //ignore | |
| } | |
| } | |
| Transaction transaction = Tracer.newTransaction(TRACER_EVENT_CHANGE_CACHE_LOAD, messageKey); | |
| try { | |
| List<String> namespaceInfo = ReleaseMessageKeyGenerator.messageToList(messageKey); | |
| Release latestRelease = releaseService.findLatestActiveRelease(namespaceInfo.get(0), | |
| namespaceInfo.get(1), namespaceInfo.get(2)); | |
| if (latestRelease != null) { | |
| releasesCache.put(latestRelease.getReleaseKey(), Optional.of(latestRelease)); | |
| } else { | |
| releasesCache.put(messageKey, Optional.empty()); | |
| } | |
| transaction.setStatus(Transaction.SUCCESS); | |
| } catch (Throwable ex) { | |
| transaction.setStatus(ex); | |
| //ignore | |
| } finally { | |
| transaction.complete(); | |
| } | |
| } |
| if(bizConfig.isConfigServiceChangeCacheEnabled()){ | ||
| LinkedHashSet<String> clientSideReleaseKeys = Sets.newLinkedHashSet( | ||
| Arrays.stream(clientSideReleaseKey.split("\\+")).collect(Collectors.toList())); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure robust handling of clientSideReleaseKey
Consider adding validation to handle cases where clientSideReleaseKey might be null or empty to prevent potential NullPointerException or unexpected behavior when the client does not provide a releaseKey.
Suggested change:
Add a null or empty check before processing clientSideReleaseKey:
if (Strings.isNullOrEmpty(clientSideReleaseKey)) {
// Handle the case appropriately, e.g., initialize to a default value or skip incremental sync
}| Map<String, Release> historyReleases = configService.findReleasesByReleaseKeys( | ||
| clientSideReleaseKeys); | ||
| //find history releases | ||
| if (historyReleases != null) { | ||
| //order by clientSideReleaseKeys | ||
| List<Release> historyReleasesWithOrder = new ArrayList<>(); | ||
| for (String item : clientSideReleaseKeys) { | ||
| Release release = historyReleases.get(item); | ||
| if(release!=null){ | ||
| historyReleasesWithOrder.add(release); | ||
| } | ||
| } | ||
|
|
||
| Map<String, String> historyConfigurations = mergeReleaseConfigurations | ||
| (historyReleasesWithOrder); | ||
|
|
||
| List<ConfigurationChange> configurationChanges = configService.calcConfigurationChanges | ||
| (latestConfigurations, historyConfigurations); | ||
|
|
||
| apolloConfig.setConfigurationChanges(configurationChanges); | ||
|
|
||
| apolloConfig.setConfigSyncType(ConfigSyncType.INCREMENTALSYNC.getValue()); | ||
| return apolloConfig; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Handle empty or null historyReleases gracefully
Currently, the code checks if historyReleases != null, but does not handle the case where historyReleases is empty. This could lead to scenarios where incremental synchronization is attempted without any historical releases, potentially causing incorrect behavior.
Suggested solution:
Modify the condition to check for empty historyReleases and handle accordingly:
if (historyReleases != null && !historyReleases.isEmpty()) {
// Existing logic
} else {
// Fallback to full synchronization or handle the situation appropriately
apolloConfig.setConfigurations(latestConfigurations);
}| @Test | ||
| public void testQueryConfigWithIncrementalSync() throws Exception { | ||
| when(bizConfig.isConfigServiceChangeCacheEnabled()) | ||
| .thenReturn(true); | ||
|
|
||
| String clientSideReleaseKey = "1"; | ||
| String someConfigurations = "{\"apollo.public.foo\": \"foo\"}"; | ||
| HttpServletResponse someResponse = mock(HttpServletResponse.class); | ||
| Map<String, Release> someReleaseMap = mock(Map.class); | ||
|
|
||
| String anotherConfigurations = "{\"apollo.public.foo\": \"foo\", \"apollo.public.bar\": \"bar\"}"; | ||
|
|
||
| when(configService.findReleasesByReleaseKeys(Sets.newHashSet(clientSideReleaseKey))).thenReturn(someReleaseMap); | ||
| when(someReleaseMap.get(clientSideReleaseKey)).thenReturn(someRelease); | ||
| when(someRelease.getConfigurations()).thenReturn(someConfigurations); | ||
| when(configService.loadConfig(someAppId, someClientIp, someClientLabel, someAppId, someClusterName, defaultNamespaceName, | ||
| someDataCenter, someNotificationMessages)).thenReturn(anotherRelease); | ||
| when(anotherRelease.getNamespaceName()).thenReturn(defaultNamespaceName); | ||
| when(anotherRelease.getConfigurations()).thenReturn(anotherConfigurations); | ||
|
|
||
| List<ConfigurationChange> configurationChanges=new ArrayList<>(); | ||
| configurationChanges.add(new ConfigurationChange("apollo.public.bar", "bar", ConfigurationChangeType.ADDED)); | ||
| when(configService.calcConfigurationChanges(gson.fromJson(anotherConfigurations, configurationTypeReference), | ||
| gson.fromJson(someConfigurations, configurationTypeReference))) | ||
| .thenReturn(configurationChanges); | ||
|
|
||
| ApolloConfig anotherResult = configController.queryConfig(someAppId, someClusterName, | ||
| defaultNamespaceName, someDataCenter, clientSideReleaseKey, | ||
| someClientIp, someClientLabel, someMessagesAsString, someRequest, someResponse); | ||
| assertEquals(ConfigSyncType.INCREMENTALSYNC.getValue(), anotherResult.getConfigSyncType()); | ||
| assertEquals(configurationChanges, anotherResult.getConfigurationChanges()); | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential Issues in testQueryConfigWithIncrementalSync
While the test method testQueryConfigWithIncrementalSync aims to verify incremental synchronization functionality, there are some issues:
- Mocking a
MapImplementation
In line 506:
Map<String, Release> someReleaseMap = mock(Map.class);Mocking concrete classes like Map can lead to unintended behavior. It's preferable to use a real instance.
Apply this change:
- Map<String, Release> someReleaseMap = mock(Map.class);
+ Map<String, Release> someReleaseMap = new HashMap<>();Ensure to import java.util.HashMap.
- Possible NullPointerException
In lines 520-522:
when(configService.calcConfigurationChanges(
gson.fromJson(anotherConfigurations, configurationTypeReference),
gson.fromJson(someConfigurations, configurationTypeReference)))
.thenReturn(configurationChanges);If configurationTypeReference is not properly initialized before usage, it could lead to a NullPointerException.
Ensure that configurationTypeReference is initialized before this usage. Since it's defined at line 625, but the test methods may not have access to it if it's not properly scoped.
Apply this change:
+ private static final Type configurationTypeReference = new TypeToken<Map<String, String>>() {}.getType();Move this declaration to a class-level variable if it's not already.
| @Test | ||
| public void testQueryConfigWithIncrementalSyncPublicNamespaceAndAppOverride() throws Exception { | ||
| when(bizConfig.isConfigServiceChangeCacheEnabled()) | ||
| .thenReturn(true); | ||
| String someAppClientSideReleaseKey = "1"; | ||
| String somePublicAppClientSideReleaseKey = "2"; | ||
| String someConfigurations = "{\"apollo.public.foo.client\": \"foo.override\"}"; | ||
| String somePublicConfigurations = "{\"apollo.public.foo.client\": \"foo\"}"; | ||
| Map<String, Release> someReleaseMap = mock(Map.class); | ||
| Release somePublicRelease = mock(Release.class); | ||
|
|
||
|
|
||
| when(configService.findReleasesByReleaseKeys(Sets.newHashSet(someAppClientSideReleaseKey, somePublicAppClientSideReleaseKey))).thenReturn(someReleaseMap); | ||
| when(someReleaseMap.get(someAppClientSideReleaseKey)).thenReturn(someRelease); | ||
| when(someReleaseMap.get(somePublicAppClientSideReleaseKey)).thenReturn(somePublicRelease); | ||
| when(someRelease.getConfigurations()).thenReturn(someConfigurations); | ||
| when(somePublicRelease.getConfigurations()).thenReturn(somePublicConfigurations); | ||
|
|
||
| String someAppServerSideReleaseKey = "3"; | ||
| String somePublicAppSideReleaseKey = "4"; | ||
|
|
||
| HttpServletResponse someResponse = mock(HttpServletResponse.class); | ||
| String somePublicAppId = "somePublicAppId"; | ||
| AppNamespace somePublicAppNamespace = | ||
| assemblePublicAppNamespace(somePublicAppId, somePublicNamespaceName); | ||
|
|
||
| when(anotherRelease.getConfigurations()).thenReturn("{\"apollo.public.foo\": \"foo-override\"}"); | ||
| when(anotherPublicRelease.getConfigurations()) | ||
| .thenReturn("{\"apollo.public.foo\": \"foo\", \"apollo.public.bar\": \"bar\"}"); | ||
|
|
||
| when(configService.loadConfig(someAppId, someClientIp, someClientLabel, someAppId, someClusterName, somePublicNamespaceName, | ||
| someDataCenter, someNotificationMessages)).thenReturn(anotherRelease); | ||
| when(anotherRelease.getReleaseKey()).thenReturn(someAppServerSideReleaseKey); | ||
| when(anotherRelease.getNamespaceName()).thenReturn(somePublicNamespaceName); | ||
| when(appNamespaceService.findPublicNamespaceByName(somePublicNamespaceName)) | ||
| .thenReturn(somePublicAppNamespace); | ||
| when(configService.loadConfig(someAppId, someClientIp, someClientLabel, somePublicAppId, someClusterName, somePublicNamespaceName, | ||
| someDataCenter, someNotificationMessages)).thenReturn(anotherPublicRelease); | ||
| when(anotherPublicRelease.getReleaseKey()).thenReturn(somePublicAppSideReleaseKey); | ||
| when(anotherPublicRelease.getAppId()).thenReturn(somePublicAppId); | ||
| when(anotherPublicRelease.getClusterName()).thenReturn(someDataCenter); | ||
| when(anotherPublicRelease.getNamespaceName()).thenReturn(somePublicNamespaceName); | ||
|
|
||
|
|
||
| String mergeServerSideConfigurations = "{\"apollo.public.bar\": \"bar\",\"apollo.public.foo\": \"foo-override\"}"; | ||
| String mergeClientSideConfigurations = "{\"apollo.public.foo.client\": \"foo.override\"}"; | ||
| List<ConfigurationChange> configurationChanges=new ArrayList<>(); | ||
| configurationChanges.add(new ConfigurationChange("apollo.public.bar", "bar", ConfigurationChangeType.ADDED)); | ||
| configurationChanges.add(new ConfigurationChange("apollo.public.foo", "foo-override", ConfigurationChangeType.ADDED)); | ||
| configurationChanges.add(new ConfigurationChange("apollo.public.foo.client", null, ConfigurationChangeType.DELETED)); | ||
| when(configService.calcConfigurationChanges(gson.fromJson(mergeServerSideConfigurations, configurationTypeReference), | ||
| gson.fromJson(mergeClientSideConfigurations, configurationTypeReference))) | ||
| .thenReturn(configurationChanges); | ||
|
|
||
| String mergeClientSideReleaseKey=Joiner.on(ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR) | ||
| .join(someAppClientSideReleaseKey, somePublicAppClientSideReleaseKey); | ||
| ApolloConfig result = configController.queryConfig(someAppId, someClusterName, somePublicNamespaceName, someDataCenter, | ||
| mergeClientSideReleaseKey, someClientIp, someClientLabel, someMessagesAsString, someRequest, someResponse); | ||
|
|
||
| assertEquals(Joiner.on(ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR) | ||
| .join(someAppServerSideReleaseKey, somePublicAppSideReleaseKey), | ||
| result.getReleaseKey()); | ||
| assertEquals(ConfigSyncType.INCREMENTALSYNC.getValue(), result.getConfigSyncType()); | ||
| assertEquals(configurationChanges, result.getConfigurationChanges()); | ||
| } |
There was a problem hiding this comment.
Enhancing testQueryConfigWithIncrementalSyncPublicNamespaceAndAppOverride
- Complexity in Test Setup
The test method has a complex setup with multiple mocked releases and configurations, which can hinder readability and maintainability.
Consider refactoring the test setup into helper methods or using test data builders to simplify the test initialization.
- Verification of Configuration Changes
While the test checks the configSyncType and configurationChanges, it could be enhanced by verifying the actual configuration values returned.
Add assertions to verify that the configurations in the ApolloConfig result match the expected merged configurations.
- Potential NullPointerException
Ensure that all mocked objects return non-null values where required to prevent NullPointerException during test execution.
Review the mocked methods and provide default returns where necessary.
jackie-coming
force-pushed
the
feautre/IncrementalSync
branch
from
6f230bf
to
b5c26d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
scripts/sql/profiles/mysql-default/apolloconfigdb.sql (1)
505-506: LGTM! Consider enhancing the configuration documentation.The new configuration entries are well-structured and align with the PR objectives for implementing incremental configuration synchronization. The default disabled state and clear memory consumption warnings are appropriate.
Consider adding the following details to the configuration comments to help operators make informed decisions:
- Memory consumption estimates or guidelines
- Recommended scenarios for enabling each feature
- Relationship between these two caching features
Example enhancement:
- ('config-service.cache.enabled', 'default', 'false', 'ConfigService是否开启缓存，开启后能提高性能，但是会增大内存消耗！'), + ('config-service.cache.enabled', 'default', 'false', 'ConfigService是否开启缓存，开启后能提高性能，但是会增大内存消耗！建议在内存充足的环境下开启，预计每1000个配置项消耗约10MB内存。'), - ('config-service.change.cache.enabled', 'default', 'false', 'ConfigService是否开启增量配置同步客户端，开启后能提高性能，但是会增大内存消耗！'); + ('config-service.change.cache.enabled', 'default', 'false', 'ConfigService是否开启增量配置同步客户端，开启后能提高性能，但是会增大内存消耗！建议在大型命名空间(1000+配置)场景下开启。需要先开启config-service.cache.enabled。');docs/zh/deployment/distributed-deployment-guide.md (1)
1586-1588: Fix blockquote formattingRemove the blank line between the two blockquotes to fix the markdown formatting.
> 开启缓存后必须确保应用中配置的`app.id`、`apollo.cluster` > 大小写正确，否则将获取不到正确的配置，另可参考`config-service.cache.key.ignore-case`配置做兼容处理。🧰 Tools
🪛 Markdownlint (0.35.0)
1588-1588: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
docs/en/deployment/distributed-deployment-guide.md (1)
1652-1654: Minor formatting improvement neededThe blockquote formatting could be improved by removing the blank line between the two notes to maintain consistency with other blockquotes in the document.
> Ensure that the `app.id`、`apollo.cluster` of the configuration in the application is in the correct case when caching is enabled, otherwise it will not fetch the correct configuration, You can also refer to the `config-service.cache.key.ignore-case` configuration for compatibility processing. - > `config-service.cache.enabled` configuration adjustment requires a restart of the config service to take effect🧰 Tools
🪛 Markdownlint (0.35.0)
1653-1653: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java(1 hunks)changes/changes-2.4.0.md(1 hunks)docs/en/deployment/distributed-deployment-guide.md(1 hunks)docs/zh/deployment/distributed-deployment-guide.md(1 hunks)pom.xml(3 hunks)scripts/sql/profiles/h2-default/apolloconfigdb.sql(1 hunks)scripts/sql/profiles/mysql-database-not-specified/apolloconfigdb.sql(1 hunks)scripts/sql/profiles/mysql-default/apolloconfigdb.sql(1 hunks)scripts/sql/src/apolloconfigdb.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java
- pom.xml
- scripts/sql/profiles/h2-default/apolloconfigdb.sql
- scripts/sql/profiles/mysql-database-not-specified/apolloconfigdb.sql
- scripts/sql/src/apolloconfigdb.sql
🧰 Additional context used
🪛 Markdownlint (0.35.0)
docs/en/deployment/distributed-deployment-guide.md
1653-1653: null
Blank line inside blockquote
(MD028, no-blanks-blockquote)
docs/zh/deployment/distributed-deployment-guide.md
1588-1588: null
Blank line inside blockquote
(MD028, no-blanks-blockquote)
🔇 Additional comments (2)
docs/zh/deployment/distributed-deployment-guide.md (1)
1577-1589: Documentation for incremental configuration synchronization looks good!
The new section clearly documents the important aspects of the feature:
- Version compatibility requirements
- Default value and memory considerations
- Case sensitivity requirements
- Restart requirements
This aligns well with the PR objectives to optimize configuration synchronization.
🧰 Tools
🪛 Markdownlint (0.35.0)
1588-1588: null
Blank line inside blockquote
(MD028, no-blanks-blockquote)
docs/en/deployment/distributed-deployment-guide.md (1)
1640-1655: LGTM! Clear and well-structured documentation for the new feature.
The documentation for the new incremental configuration synchronization feature is well-written, covering all essential aspects:
- Version requirements
- Feature description and benefits
- Default configuration and considerations
- Important notes about case sensitivity and restart requirements
🧰 Tools
🪛 Markdownlint (0.35.0)
1653-1653: null
Blank line inside blockquote
(MD028, no-blanks-blockquote)
| * [Feature support incremental configuration synchronization client](https://github.com/apolloconfig/apollo/pull/5288) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the changelog entry with crucial implementation details.
The changelog entry should provide more details about this significant feature to help users understand its benefits and requirements. Consider expanding it as follows:
-* [Feature support incremental configuration synchronization client](https://github.com/apolloconfig/apollo/pull/5288)
+* [Feature support incremental configuration synchronization client](https://github.com/apolloconfig/apollo/pull/5288)
+ * Optimizes configuration synchronization by updating only changed keys instead of entire namespace
+ * Reduces database bandwidth consumption significantly for large namespaces
+ * Requires server version 2.4.0+ and client version 2.3.0+
+ * Enable via `config-service.change.cache.enabled` configuration📝 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.
| * [Feature support incremental configuration synchronization client](https://github.com/apolloconfig/apollo/pull/5288) | |
| * [Feature support incremental configuration synchronization client](https://github.com/apolloconfig/apollo/pull/5288) | |
| * Optimizes configuration synchronization by updating only changed keys instead of entire namespace | |
| * Reduces database bandwidth consumption significantly for large namespaces | |
| * Requires server version 2.4.0+ and client version 2.3.0+ | |
| * Enable via `config-service.change.cache.enabled` configuration | |
I have read the CLA Document and I hereby sign the CLA |
jackie-coming
force-pushed
the
feautre/IncrementalSync
branch
from
8503249
to
07bb41c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCache.java (1)
98-101: Add error logging in the catch blockWhile the transaction status is set correctly, logging the exception would help with debugging cache loading issues.
} catch (Throwable ex) { transaction.setStatus(ex); + logger.error("Failed to load release for key: " + key, ex); throw ex; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCache.java(1 hunks)changes/changes-2.4.0.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- changes/changes-2.4.0.md
🔇 Additional comments (5)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCache.java (5)
57-57: Fix the typo in the constant name
The constant DEFAULT_EXPIRED_AFTER_ACCESS_IN_SENCONDS contains a typo. "SENCONDS" should be corrected to "SECONDS".
67-81: LGTM! Well-structured initialization
The constructor properly initializes all dependencies through the parent class, and the @PostConstruct method ensures correct initialization order.
122-126:
Add null check to prevent NPE
The code assumes latestRelease is non-null when accessing getReleaseKey(). This could lead to a NullPointerException.
Release latestRelease = releaseService.findLatestActiveRelease(namespaceInfo.get(0),
namespaceInfo.get(1), namespaceInfo.get(2));
-releasesCache.put(latestRelease.getReleaseKey(), Optional.ofNullable(latestRelease));
+if (latestRelease != null) {
+ releasesCache.put(latestRelease.getReleaseKey(), Optional.ofNullable(latestRelease));
+} else {
+ logger.warn("No active release found for namespace: {}", String.join("/", namespaceInfo));
+}Likely invalid or redundant comment.
129-132: 🛠️ Refactor suggestion
Improve exception handling
Silently ignoring exceptions makes debugging difficult. Consider logging the error for better observability.
} catch (Throwable ex) {
transaction.setStatus(ex);
- //ignore
+ logger.error("Failed to handle release message: " + messageKey, ex);
}Likely invalid or redundant comment.
150-154:
Return empty map instead of null and improve error handling
Returning null can lead to NPEs in calling code. Additionally, the ExecutionException is silently ignored.
} catch (ExecutionException e) {
- //ignore
+ logger.error("Failed to get releases by keys: " + releaseKeys, e);
}
-return null;
+return ImmutableMap.of();Likely invalid or redundant comment.
What's the purpose of this PR
XXXXX
Which issue(s) this PR fixes:
Fixes #5252
apollo-java : apolloconfig/apollo-java#90
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
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests