feat: support incremental configuration synchronization client #5325
Conversation
…autre/IncrementalSync
…autre/IncrementalSync
# Conflicts: # changes/changes-2.4.0.md
dosubot
bot
added
the
size:XL
WalkthroughThis pull request introduces a new feature for incremental configuration synchronization in Apollo's config service. The changes span multiple files and components, adding support for caching and retrieving configuration changes between releases. A new configuration option Changes
Sequence DiagramsequenceDiagram
participant Client
participant ConfigService
participant ReleaseRepository
participant BizConfig
Client->>ConfigService: Request configuration
ConfigService->>BizConfig: Check incremental change enabled
BizConfig-->>ConfigService: Return configuration setting
alt Incremental Change Enabled
ConfigService->>ReleaseRepository: Fetch historical releases
ReleaseRepository-->>ConfigService: Return release data
ConfigService->>ConfigService: Calculate configuration changes
ConfigService->>Client: Return incremental configuration
else Incremental Change Disabled
ConfigService->>Client: Return full configuration
end
Possibly related PRs
Suggested Labels
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 (
|
|
@nobodyiam ：The logic for calculating configurationChanges should be placed in the IncrementalSyncConfigService. If the config cache is not enabled, data should be directly fetched from the database for comparison. If the config cache is enabled, data should be fetched from the cache for comparison, and the comparison results themselves can also be stored in the cache for reuse. My question is, when incremental synchronization is enabled, is it better to use caching by default? Because not using caching will double the database I/O pressure compared to full synchronization. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseService.java (1)
Line range hint
391-391: Add type safety to collection casting.The unchecked cast
(Collection<String>)could lead to runtime errors if the JSON structure doesn't match the expected type.Apply this diff to add type safety:
- return (Collection<String>) operationContext.get(ReleaseOperationContext.BRANCH_RELEASE_KEYS); + Object branchReleaseKeys = operationContext.get(ReleaseOperationContext.BRANCH_RELEASE_KEYS); + if (branchReleaseKeys instanceof Collection<?>) { + Collection<?> collection = (Collection<?>) branchReleaseKeys; + if (collection.isEmpty() || collection.iterator().next() instanceof String) { + @SuppressWarnings("unchecked") + Collection<String> result = (Collection<String>) collection; + return result; + } + } + return null;🧰 Tools
🪛 GitHub Actions: build
[warning] Uses unchecked or unsafe operations
🧹 Nitpick comments (13)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java (1)
67-67: Consider using protected getter methods instead of protected fields.While making
bizConfigprotected enables subclass access, it might be better to provide protected getter methods to maintain better encapsulation. This would give you more flexibility to change the internal implementation in the future.Consider this alternative approach:
- protected final BizConfig bizConfig; + private final BizConfig bizConfig; + + protected BizConfig getBizConfig() { + return bizConfig; + }apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCache.java (3)
57-57: Correct the spelling of the constant.There's a small typo in
DEFAULT_EXPIRED_AFTER_ACCESS_IN_SENCONDS. Renaming it toDEFAULT_EXPIRED_AFTER_ACCESS_IN_SECONDSwould make it clearer and more consistent.- private static final long DEFAULT_EXPIRED_AFTER_ACCESS_IN_SENCONDS = 10; + private static final long DEFAULT_EXPIRED_AFTER_ACCESS_IN_SECONDS = 10;
88-88: Consider parameterizing the CacheBuilder.The local variable
CacheBuilder releasesCacheBuilder = CacheBuilder.newBuilder();is raw-typed. Providing explicit generic parameters for clarity (e.g.,<String, Optional<Release>>) would enforce type consistency and improve readability.- CacheBuilder releasesCacheBuilder = CacheBuilder.newBuilder() + CacheBuilder<Object, Object> releasesCacheBuilder = CacheBuilder.newBuilder() .expireAfterAccess(DEFAULT_EXPIRED_AFTER_ACCESS_IN_SECONDS, TimeUnit.SECONDS);
110-132: Handle exceptions with appropriate logging or rethrow.When loading or updating the cache in
handleMessage, the catch block logs the transaction status but silently ignores the exception. Consider additional logging to help with debugging or rethrow a custom exception if ignoring is not intended.apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java (1)
163-194: Consider partial incremental sync.The incremental sync block is helpful when
historyReleases != null. If only some of the client-side keys are found, partially computing changes (instead of returning full config) might be beneficial for performance. If partial incremental sync isn't desired, ensure that it's clearly documented as an all-or-nothing approach.apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigControllerTest.java (1)
538-572: Incremental sync test coverage looks good.The test verifies both newly calculated configuration changes and the incremental sync type. For extra rigor, consider adding negative tests for mismatched or partially missing releases.
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/IncrementalSyncConfigService.java (2)
30-36: Enhance method documentation for better clarity.The Javadoc for
calcConfigurationChangesshould include:
- Parameter descriptions explaining the expected format and content
- Return value details including possible change types
- Validation requirements or constraints
/** + * Calculates the differences between two configuration maps. * @param latestReleaseConfigurations Map of the latest configuration keys to their values * @param historyConfigurations Map of historical configuration keys to their values - * @return the ConfigurationChanges + * @return List of ConfigurationChange objects representing added, modified, or deleted entries + * @throws IllegalArgumentException if either map contains invalid entries */
38-42: Improve documentation for findReleasesByReleaseKeys method.The Javadoc lacks essential information about the method's behavior.
/** + * Retrieves Release objects for the specified release keys. * @param releaseKeys Set of release keys to look up - * @return the ReleaseMap + * @return Map of release keys to their corresponding Release objects, or empty map if none found + * @throws IllegalArgumentException if any release key is invalid */apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/ConfigServiceAutoConfiguration.java (1)
73-76: Add logging for configuration service initialization.Add debug logging to help track which configuration service implementation is being used.
if (bizConfig.isConfigServiceIncrementalChangeEnabled()) { + logger.debug("Initializing ConfigServiceWithChangeCache for incremental changes"); return new ConfigServiceWithChangeCache(releaseService, releaseMessageService, grayReleaseRulesHolder(), bizConfig, meterRegistry); }apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCacheTest.java (1)
1-246: LGTM! Comprehensive test coverage for configuration changes.The test class thoroughly covers the core functionality including:
- Configuration changes (add/update/delete)
- Edge cases with null configurations
- Release key caching behavior
Consider adding these test scenarios:
- Concurrent access patterns
- Memory consumption under heavy load
- Cache eviction scenarios
changes/changes-2.4.0.md (1)
12-13: Enhance the feature documentation.Consider adding more details about:
- Performance benefits and memory trade-offs
- Configuration requirements
- Migration guidelines for existing clients
apollo-configservice/src/main/resources/jpa/configdb.init.h2.sql (1)
22-23: Consider adding English translations for comments.The configuration comments are currently only in Chinese. Consider adding English translations to improve international developer experience.
- ('config-service.incremental.change.enabled', 'default', 'false', 'ConfigService是否开启客户端同步增量配置，开启后能提高性能，但是会增大内存消耗！', 'default', '1970-01-01 00:00:00'); + ('config-service.incremental.change.enabled', 'default', 'false', 'Whether to enable incremental configuration sync for ConfigService. Enabling improves performance but increases memory consumption! (ConfigService是否开启客户端同步增量配置，开启后能提高性能，但是会增大内存消耗！)', 'default', '1970-01-01 00:00:00');scripts/sql/src/apolloconfigdb.sql (1)
493-494: LGTM! Consider documenting the memory consumption impact.The new configuration entry follows the existing pattern and has a safe default value of 'false'. The comment clearly explains the performance benefit and memory trade-off.
Consider adding documentation that:
- Quantifies the expected memory increase
- Provides guidelines for when to enable this feature
- Describes monitoring recommendations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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(6 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(23 hunks)apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCacheTest.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)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
🪛 GitHub Actions: build
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseService.java
[warning] Uses unchecked or unsafe operations
scripts/sql/profiles/mysql-default/apolloconfigdb.sql
[error] 507-507: Invalid SQL file content detected. The line contains incorrect spacing - expected '-- ' but found ''. Run 'mvn compile -pl apollo-build-sql-converter -Psql-converter' to regenerate the SQL file.
🪛 markdownlint-cli2 (0.17.2)
docs/en/deployment/distributed-deployment-guide.md
1656-1656: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
docs/zh/deployment/distributed-deployment-guide.md
1590-1590: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
🔇 Additional comments (19)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/repository/ReleaseRepository.java (2)
40-41: LGTM! Method signature follows Spring Data JPA conventions.The new method provides a clean way to fetch a single Release by its key.
48-48: LGTM! Parameter name change improves clarity.The plural form 'releaseKeys' better represents a Set of keys.
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseService.java (1)
122-124: LGTM! Method provides a clean service-layer API.The implementation correctly delegates to the repository layer.
🧰 Tools
🪛 GitHub Actions: build
[warning] Uses unchecked or unsafe operations
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java (1)
65-65: Verify the necessity of protected access for releaseService.The change from private to protected access suggests new functionality through inheritance. Please ensure this modification is necessary for the incremental sync implementation.
Run this script to analyze the usage:
✅ Verification successful
Protected access for releaseService is properly utilized
The protected modifier is justified as ConfigServiceWithChangeCache directly uses releaseService for specialized release lookup methods (findByReleaseKey) to implement incremental sync functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how releaseService is used in the subclass # Expected: Find evidence of direct field access in ConfigServiceWithChangeCache # Search for ConfigServiceWithChangeCache class definition and its usage of releaseService ast-grep --pattern 'class ConfigServiceWithChangeCache extends ConfigServiceWithCache { $$$ }' # Search for direct releaseService field access rg "releaseService\." --type javaLength of output: 27329
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java (5)
Line range hint
19-36: No issues with the new imports.The added imports for
BizConfig,ConfigurationChange,ConfigSyncType, andSetsappear aligned with incremental configuration logic.
70-71: Good injection approach.Storing
BizConfigas a final field aligns with dependency injection best practices. This is a straightforward, maintainable way to make configuration features accessible within the controller.
147-148: Minor naming change is fine.Renaming the release key variable to
latestMergedReleaseKeyclarifies intention and does not introduce issues.
159-160: LGTM: ApolloConfig creation.Constructing the new
ApolloConfigobject with a more explicit release key name is consistent and clear.
81-82: Verify constructor usage across the codebase.Adding
BizConfigto the constructor changes its signature. Ensure that no third-party code or older references call this constructor without the new parameter.✅ Verification successful
Constructor change is safe ✅
The only direct instantiation of
ConfigControlleris in the test code which already uses the new constructor signature withBizConfig. All other usages are handled by Spring's dependency injection system, which will automatically manage the new parameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for references to the ConfigController constructor with fewer params ast-grep --pattern $'new ConfigController($_, $_, $_, $_, $_)'Length of output: 63
Script:
#!/bin/bash # Search for any ConfigController instantiation ast-grep --pattern 'new ConfigController($$$)' # Search for ConfigController references in files rg "ConfigController" -l # Search specifically in test files fd "Test.*\.java" --exec rg "ConfigController" {} \; # Look for Spring configuration files fd ".*\.xml" --exec rg "ConfigController" {} \;Length of output: 8319
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigControllerTest.java (2)
574-599: Validation of missing release keys scenario is complete.This test confirms that if
historyReleasesis null, the controller falls back to returning the full configuration. The validation logic here is correct.
602-670: Well-structured test for public namespace overrides.This test thoroughly checks overlapping releases from different namespaces and confirms correct final values. It's a strong example of multi-source incremental sync coverage.
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigService.java (1)
26-26: LGTM! Good interface extension.The ConfigService interface correctly extends IncrementalSyncConfigService, maintaining backward compatibility while adding incremental sync capability.
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/AbstractConfigService.java (1)
103-142: 🛠️ Refactor suggestionAdd null checks for configuration values.
The method handles null maps but not null values within the maps, which could lead to NPEs.
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 map values + if (latestReleaseConfigurations.containsValue(null) || historyConfigurations.containsValue(null)) { + throw new IllegalArgumentException("Configuration maps cannot contain null values"); + } Set<String> previousKeys = historyConfigurations.keySet();apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (1)
243-245: LGTM! Well-implemented feature flag.The implementation follows the established pattern for feature flags and safely defaults to disabled.
scripts/sql/profiles/h2-default/apolloconfigdb.sql (1)
486-487: LGTM! Generated file is consistent with the source template.The configuration entries match the source template as expected.
scripts/sql/profiles/mysql-database-not-specified/apolloconfigdb.sql (1)
500-501: LGTM! Generated file is consistent with the source template.The configuration entries match the source template as expected.
scripts/sql/profiles/mysql-default/apolloconfigdb.sql (1)
505-506: LGTM! Generated file is consistent with the source template.The configuration entries match the source template as expected.
docs/zh/deployment/distributed-deployment-guide.md (1)
1579-1591: LGTM! Documentation for the new configuration is clear and complete.The documentation for
config-service.incremental.change.enabledis well structured and includes all necessary information:
- Version requirements
- Purpose and behavior
- Default value and considerations
- Important notes about case sensitivity and restart requirements
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1590-1590: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
docs/en/deployment/distributed-deployment-guide.md (1)
1643-1659: LGTM! English documentation matches Chinese version.The English documentation for
config-service.incremental.change.enabledis consistent with the Chinese version and provides equivalent information:
- Version requirements
- Purpose and behavior
- Default value and considerations
- Important notes about case sensitivity and restart requirements
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1656-1656: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
| } catch (ExecutionException e) { | ||
| //ignore | ||
| } | ||
| return null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return an empty map instead of null.
Returning null in findReleasesByReleaseKeys can lead to NullPointerExceptions. Returning an empty map (or partial results) is often more robust.
- return null;
+ return Collections.emptyMap();📝 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 null; | |
| return Collections.emptyMap(); |
| @Override | ||
| public Map<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Return empty map instead of null.
Returning null violates the interface contract and forces unnecessary null checks in clients. Return an empty map instead.
@Override
public Map<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys) {
- return null;
+ return new HashMap<>();
}📝 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.
| @Override | |
| public Map<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys) { | |
| return null; | |
| } | |
| @Override | |
| public Map<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys) { | |
| return new HashMap<>(); | |
| } |
| @@ -502,7 +502,8 @@ VALUES | |||
| ('namespace.lock.switch', 'default', 'false', '一次发布只能有一个人修改开关'), | |||
| ('item.key.length.limit', 'default', '128', 'item key 最大长度限制'), | |||
| ('item.value.length.limit', 'default', '20000', 'item value最大长度限制'), | |||
| ('config-service.cache.enabled', 'default', 'false', 'ConfigService是否开启缓存，开启后能提高性能，但是会增大内存消耗！'); | |||
| ('config-service.cache.enabled', 'default', 'false', 'ConfigService是否开启缓存，开启后能提高性能，但是会增大内存消耗！'), | |||
| ('config-service.incremental.change.enabled', 'default', 'false', 'ConfigService是否开启增量配置同步客户端，开启后能提高性能，但是会增大内存消耗！'); | |||
|
|
|||
There was a problem hiding this comment.
Fix the SQL formatting issue.
The pipeline failure indicates incorrect spacing after the comment marker on line 507. The line should start with '-- ' (two dashes followed by a space).
Apply this fix:
-'--'
+'-- 'Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: build
[error] 507-507: Invalid SQL file content detected. The line contains incorrect spacing - expected '-- ' but found ''. Run 'mvn compile -pl apollo-build-sql-converter -Psql-converter' to regenerate the SQL file.
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseService.java (1)
Add type parameters and @SuppressWarnings annotations to address unchecked operations
The following locations in ReleaseService.java need attention:
- Add type parameter to
BRANCH_RELEASE_OPERATIONS:private static final Set<Integer> BRANCH_RELEASE_OPERATIONS- Add
@SuppressWarnings("unchecked")annotation to thegetBranchReleaseKeysmethod to suppress the verified safe type casting warning:@SuppressWarnings("unchecked") private Collection<String> getBranchReleaseKeys(long releaseId) { ... return (Collection<String>) operationContext.get(ReleaseOperationContext.BRANCH_RELEASE_KEYS); }🔗 Analysis chain
Line range hint
1-674: Address unchecked operation warnings.The build pipeline reports warnings about unchecked operations. Consider adding appropriate type parameters or suppression annotations where type safety is guaranteed.
Run this script to locate the unchecked operations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find potential unchecked operations in the codebase rg "(\@SuppressWarnings\(\"unchecked\"\)|[^<]List [^<]|Map [^<]|Set [^<])" --type javaLength of output: 221
Script:
#!/bin/bash # Search for raw type usages and unchecked operations with simpler patterns echo "=== Raw type usages ===" rg "(?<!<)(List|Map|Set|Collection)(\s+\w+\s*[=;)])" --type java echo -e "\n=== Type casting operations ===" rg "\([^)]+\)\s*\w+\." --type java echo -e "\n=== Existing SuppressWarnings ===" rg "@SuppressWarnings" --type javaLength of output: 6870
🧰 Tools
🪛 GitHub Actions: build
[warning] Uses unchecked or unsafe operations
🧹 Nitpick comments (11)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java (1)
65-65: Consider alternative design patterns before weakening encapsulation.While making
releaseServiceandbizConfigprotected enables inheritance, consider these alternatives:
- Protected accessor methods instead of exposing fields directly
- Template method pattern for customizing behavior
- Strategy pattern for varying the configuration retrieval logic
This would maintain better encapsulation while still allowing extensibility.
Example of using protected methods instead:
- protected final ReleaseService releaseService; - protected final BizConfig bizConfig; + private final ReleaseService releaseService; + private final BizConfig bizConfig; + + protected ReleaseService getReleaseService() { + return releaseService; + } + + protected BizConfig getBizConfig() { + return bizConfig; + }Also applies to: 67-67
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCache.java (2)
57-57: Correct the spelling.There's a minor typo in the constant name:
DEFAULT_EXPIRED_AFTER_ACCESS_IN_SENCONDS. Consider renaming it toDEFAULT_EXPIRED_AFTER_ACCESS_IN_SECONDSfor consistency.- private static final long DEFAULT_EXPIRED_AFTER_ACCESS_IN_SENCONDS = 10; + private static final long DEFAULT_EXPIRED_AFTER_ACCESS_IN_SECONDS = 10;
137-154: Return partial results or empty map instead of null.When at least one release key is missing, the method currently returns
null. This design can force downstream code to handlenullchecks rather than degrade gracefully. Consider returning a partial or empty map to simplify usage.- return null; + return Collections.emptyMap();apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java (1)
70-71: Clarify usage for BizConfig.The
BizConfiginjected here is crucial for controlling incremental sync toggles. Make sure to document how the BizConfig property is configured to avoid confusion for other maintainers.apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/IncrementalSyncConfigService.java (1)
28-44: Enhance interface documentation.
- Clarify concurrency or thread-safety expectations for these methods, as they're often called by highly concurrent services.
- Specify behavior for partial or missing
releaseKeys—should the method return a partial map or exclude them entirely?apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigControllerTest.java (3)
601-670: LGTM! Thorough test of complex scenario.The test covers the intricate case of public namespace with app override, including:
- Multiple release keys
- Configuration merging
- Change detection
Consider adding tests for:
- Concurrent modification scenarios
- Large configuration sets
- Error cases in change calculation
538-571: Consider reorganizing incremental sync tests.While the test coverage is good, consider:
- Using @nested classes to group related tests
- Extracting common setup to @beforeeach methods
- Using more descriptive test names
Example structure:
@Nested class IncrementalSyncTests { @BeforeEach void setUp() { when(bizConfig.isConfigServiceIncrementalChangeEnabled()).thenReturn(true); } @Test void shouldReturnIncrementalChangesWhenEnabled() { // existing test code } }
601-674: Improve test readability.The test is complex and could benefit from:
- Breaking down into smaller test methods
- Using helper methods for setup
- Adding comments explaining the test scenarios
- Using constants for test data
Example:
private static final String APP_CONFIG = "{\"apollo.public.foo\": \"foo-override\"}"; private static final String PUBLIC_CONFIG = "{\"apollo.public.foo\": \"foo\", \"apollo.public.bar\": \"bar\"}"; private void setupReleases(String appConfig, String publicConfig) { when(anotherRelease.getConfigurations()).thenReturn(appConfig); when(anotherPublicRelease.getConfigurations()).thenReturn(publicConfig); }docs/en/deployment/distributed-deployment-guide.md (1)
1643-1659: Documentation looks good with minor formatting suggestions.The documentation for the new configuration option is clear, complete and well-structured. It covers all the important aspects including version requirements, behavior, considerations and warnings.
Consider these minor formatting improvements for consistency:
-### 3.2.14 config-service.incremental.change.enabled - whether to enable incremental configuration synchronization client +### 3.2.14 config-service.incremental.change.enabled - Whether to enable incremental configuration synchronization client -> for server versions 2.4.0 and above && client versions 2.3.0 and above +> For server versions 2.4.0 and above && client versions 2.3.0 and above -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 +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.apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCacheTest.java (2)
93-179: Consider adding edge case tests.While the existing tests cover the basic scenarios well, consider adding tests for:
- Empty string values
- Special characters in keys/values
- Very large configurations
- Concurrent modifications
181-223: Consider adding performance tests for caching.The caching behavior tests are good, but consider adding:
- Cache performance under high load
- Cache eviction tests
- Memory consumption tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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(6 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(23 hunks)apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCacheTest.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)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
🪛 GitHub Actions: build
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseService.java
[warning] Uses unchecked or unsafe operations
scripts/sql/profiles/mysql-default/apolloconfigdb.sql
[error] 507-507: Invalid SQL file content detected. The line contains incorrect spacing - expected '-- ' but found ''. Run 'mvn compile -pl apollo-build-sql-converter -Psql-converter' to regenerate the SQL file.
🔇 Additional comments (24)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java (1)
65-67: Verify the inheritance hierarchy for incremental sync implementation.The change from private to protected suggests this class will be extended by
IncrementalSyncConfigService. Let's verify this assumption and ensure there are no unintended exposures.apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java (3)
Line range hint
19-36: Imports look good.All newly added imports are relevant to the incremental sync logic. No issues noted.
81-88: No objections to constructor injection.Injecting
BizConfigthrough the constructor is consistent with best practices for dependency injection.
147-150: Validate merged releaseKey equality check.The equality check between
latestMergedReleaseKeyandclientSideReleaseKeyeffectively returns a 304 to the client. Ensure that client-side releaseKey strings are well-formed to avoid false positives (e.g., trailing separators).apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigService.java (1)
26-26: Verify implementations of IncrementalSyncConfigService.The interface now extends
IncrementalSyncConfigService. Ensure all implementing classes properly implement the new methods from this interface.✅ Verification successful
Implementation of IncrementalSyncConfigService is complete and correct ✅
The
calcConfigurationChangesmethod is properly implemented inAbstractConfigServiceand inherited by all concrete implementations. The implementation handles all required cases including additions, deletions, and modifications of configurations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all classes implementing ConfigService echo "Searching for ConfigService implementations..." ast-grep --pattern 'class $_ implements ConfigService' ast-grep --pattern 'class $_ extends $_ implements ConfigService'Length of output: 227
Script:
#!/bin/bash # Get a broader view of ConfigService usage and implementation echo "Searching for ConfigService references..." rg "ConfigService" -l echo -e "\nSearching for IncrementalSyncConfigService definition and usage..." rg "IncrementalSyncConfigService" -A 5Length of output: 8299
Script:
#!/bin/bash # Check AbstractConfigService and its implementations echo "Checking AbstractConfigService implementation..." rg -A 20 "class AbstractConfigService" apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/AbstractConfigService.java echo -e "\nChecking concrete implementations..." rg -A 10 "class (DefaultConfigService|ConfigServiceWithCache|ConfigServiceWithChangeCache)" apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/Length of output: 7080
Script:
#!/bin/bash # Check for IncrementalSyncConfigService implementation echo "Checking IncrementalSyncConfigService interface..." rg -A 10 "interface IncrementalSyncConfigService" apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/IncrementalSyncConfigService.java echo -e "\nChecking AbstractConfigService for implementation..." rg -B 5 -A 20 "calculateConfigurationChanges" apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/AbstractConfigService.javaLength of output: 905
Script:
#!/bin/bash # Broader search for implementation echo "Searching for configuration changes implementation..." rg -l "calcConfigurationChanges|calculateConfigurationChanges" apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ echo -e "\nChecking full content of AbstractConfigService..." cat apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/AbstractConfigService.javaLength of output: 6578
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/repository/ReleaseRepository.java (2)
40-40: LGTM! Added single release key lookup method.The new method provides a more efficient way to fetch a single release by its key.
48-48: LGTM! Improved parameter naming.Parameter name change from
releaseKeytoreleaseKeysbetter reflects that it's a collection parameter.apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/ConfigServiceAutoConfiguration.java (1)
73-76: Document configuration hierarchy and verify ConfigServiceWithChangeCache.The configuration selection logic looks correct, but would benefit from documentation explaining the precedence of different configurations.
Add Javadoc explaining the configuration hierarchy:
@Bean public ConfigService configService() { + /** + * Configuration hierarchy: + * 1. If incremental change is enabled: use ConfigServiceWithChangeCache + * 2. If config service cache is enabled: use ConfigServiceWithCache + * 3. Otherwise: use DefaultConfigService + */ if (bizConfig.isConfigServiceIncrementalChangeEnabled()) {✅ Verification successful
Implementation verified, documentation suggestion remains valid
The
ConfigServiceWithChangeCacheimplementation follows the correct inheritance chain throughConfigServiceWithCacheandAbstractConfigService, implementing all required interfaces. The original suggestion to add documentation explaining the configuration hierarchy would still improve code maintainability.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify ConfigServiceWithChangeCache implements required interfaces echo "Checking ConfigServiceWithChangeCache implementation..." ast-grep --pattern 'class ConfigServiceWithChangeCache extends AbstractConfigService'Length of output: 208
Script:
#!/bin/bash # Find ConfigServiceWithChangeCache class file echo "Searching for ConfigServiceWithChangeCache class..." fd ConfigServiceWithChangeCache.java # If found, examine its content echo -e "\nExamining class implementation..." rg -A 5 "class ConfigServiceWithChangeCache" --type javaLength of output: 2366
Script:
#!/bin/bash # Check ConfigServiceWithCache implementation echo "Examining ConfigServiceWithCache implementation..." rg -A 5 "class ConfigServiceWithCache" --type java # Also check AbstractConfigService to verify the complete hierarchy echo -e "\nExamining AbstractConfigService..." rg -A 5 "class AbstractConfigService" --type javaLength of output: 4444
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (2)
243-245: LGTM! Safe default for incremental sync feature flag.The implementation follows the established pattern and defaults to false, ensuring backward compatibility.
243-245: LGTM! Well-implemented feature flag.The implementation follows the existing pattern and has a safe default value of false. This allows for gradual rollout of the incremental change feature.
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseService.java (2)
122-124: LGTM! Simple and focused method addition.The method provides a clean way to find a release by key.
🧰 Tools
🪛 GitHub Actions: build
[warning] Uses unchecked or unsafe operations
Line range hint
1-674: Fix unchecked operations warning.The pipeline reports warnings about unchecked operations. Consider adding appropriate type parameters or suppressing warnings with justification where appropriate.
Run this script to locate the unchecked operations:
🧰 Tools
🪛 GitHub Actions: build
[warning] Uses unchecked or unsafe operations
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCacheTest.java (2)
1-246: LGTM! Comprehensive test coverage.The test class provides thorough coverage of the caching functionality, including:
- Basic operations
- Edge cases
- Cache behavior verification
1-91: LGTM! Well-structured test class setup.The test class is well organized with proper mocking of dependencies and initialization of test data.
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigControllerTest.java (2)
538-572: LGTM! Well-structured test for basic incremental sync.The test thoroughly verifies:
- Feature flag check
- Configuration change detection
- Correct sync type setting
574-599: LGTM! Good coverage of error case.The test properly handles the case when a release is not found, ensuring graceful fallback.
changes/changes-2.4.0.md (1)
12-13: LGTM! Changelog entry is well-formatted.The entry follows the established format and properly references the original PR.
apollo-configservice/src/main/resources/jpa/configdb.init.h2.sql (1)
22-23: LGTM! Configuration entry is well-defined.The new configuration entry:
- Follows consistent naming convention
- Has a safe default value (false)
- Includes clear documentation about performance implications
scripts/sql/src/apolloconfigdb.sql (1)
493-494: LGTM! Configuration entry matches H2 script.The configuration entry is consistent with the H2 database script, ensuring uniform behavior across different database environments.
scripts/sql/profiles/h2-default/apolloconfigdb.sql (1)
486-487: LGTM! Configuration entry is consistent across all SQL scripts.The configuration entry maintains consistency with other database initialization scripts, ensuring uniform behavior across different deployment profiles.
scripts/sql/profiles/mysql-database-not-specified/apolloconfigdb.sql (1)
500-501: LGTM! Configuration entry is consistent.The configuration entry maintains consistency with other database initialization scripts.
scripts/sql/profiles/mysql-default/apolloconfigdb.sql (2)
505-506: LGTM! Well-structured configuration entries.The new configuration entries are properly added with clear descriptions:
config-service.cache.enabled- Controls config service cachingconfig-service.incremental.change.enabled- Controls incremental configuration sync for clients
507-507:⚠️ Potential issueFix the SQL file formatting error.
The pipeline is failing due to incorrect spacing. The line should start with
--(note the space after --).Apply this fix:
- +--Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: build
[error] 507-507: Invalid SQL file content detected. The line contains incorrect spacing - expected '-- ' but found ''. Run 'mvn compile -pl apollo-build-sql-converter -Psql-converter' to regenerate the SQL file.
docs/zh/deployment/distributed-deployment-guide.md (1)
1579-1591: LGTM! Comprehensive documentation for the new feature.The documentation for
config-service.incremental.change.enabledis well-structured and includes:
- Clear version requirements (Server 2.4.0+ && Client 2.3.0+)
- Purpose and benefits of the feature
- Important considerations for enabling it
- Restart requirement note
| public void handleMessage(ReleaseMessage message, String channel) { | ||
| logger.info("message received - channel: {}, message: {}", channel, message); | ||
| if (!Topics.APOLLO_RELEASE_TOPIC.equals(channel) || Strings.isNullOrEmpty( | ||
| message.getMessage())) { | ||
| return; | ||
| } | ||
| String messageKey = message.getMessage(); | ||
| if (bizConfig.isConfigServiceCacheKeyIgnoreCase()) { | ||
| messageKey = messageKey.toLowerCase(); | ||
| } | ||
| 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)); | ||
|
|
||
| releasesCache.put(latestRelease.getReleaseKey(), Optional.ofNullable(latestRelease)); | ||
|
|
There was a problem hiding this comment.
Guard against null releases.
When releaseService.findLatestActiveRelease(...) returns null, invoking latestRelease.getReleaseKey() will cause a NullPointerException. Add a null check before updating the cache to make this more robust.
Release latestRelease = releaseService.findLatestActiveRelease(namespaceInfo.get(0),
namespaceInfo.get(1), namespaceInfo.get(2));
+ if (latestRelease == null) {
+ logger.warn("No release found for message key: {}", messageKey);
+ return;
+ }
releasesCache.put(latestRelease.getReleaseKey(), Optional.ofNullable(latestRelease));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void handleMessage(ReleaseMessage message, String channel) { | |
| logger.info("message received - channel: {}, message: {}", channel, message); | |
| if (!Topics.APOLLO_RELEASE_TOPIC.equals(channel) || Strings.isNullOrEmpty( | |
| message.getMessage())) { | |
| return; | |
| } | |
| String messageKey = message.getMessage(); | |
| if (bizConfig.isConfigServiceCacheKeyIgnoreCase()) { | |
| messageKey = messageKey.toLowerCase(); | |
| } | |
| 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)); | |
| releasesCache.put(latestRelease.getReleaseKey(), Optional.ofNullable(latestRelease)); | |
| public void handleMessage(ReleaseMessage message, String channel) { | |
| logger.info("message received - channel: {}, message: {}", channel, message); | |
| if (!Topics.APOLLO_RELEASE_TOPIC.equals(channel) || Strings.isNullOrEmpty( | |
| message.getMessage())) { | |
| return; | |
| } | |
| String messageKey = message.getMessage(); | |
| if (bizConfig.isConfigServiceCacheKeyIgnoreCase()) { | |
| messageKey = messageKey.toLowerCase(); | |
| } | |
| 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) { | |
| logger.warn("No release found for message key: {}", messageKey); | |
| return; | |
| } | |
| releasesCache.put(latestRelease.getReleaseKey(), Optional.ofNullable(latestRelease)); |
| if (bizConfig.isConfigServiceIncrementalChangeEnabled()) { | ||
| LinkedHashSet<String> clientSideReleaseKeys = Sets.newLinkedHashSet( | ||
| Arrays.stream( | ||
| clientSideReleaseKey.split(Pattern.quote(ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR))) | ||
| .collect(Collectors.toList())); | ||
|
|
||
| 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.INCREMENTAL_SYNC.getValue()); | ||
| return apolloConfig; | ||
| } | ||
|
|
||
| } | ||
|
|
||
| apolloConfig.setConfigurations(latestConfigurations); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle invalid or missing releaseKeys more gracefully.
- When
clientSideReleaseKeyis-1or empty, parsing may yield invalid sets of keys. - If some historical releases are missing, the logic proceeds with a full sync by returning null for
historyReleaseschecks. - Consider logging a message or providing partial change sets to avoid possibly confusing behavior for clients.
| @Override | ||
| public Map<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Implementation required: findReleasesByReleaseKeys returns null.
The method should not return null as it's part of the interface contract. Implement proper release lookup logic.
| public List<ConfigurationChange> calcConfigurationChanges( | ||
| Map<String, String> latestReleaseConfigurations, Map<String, String> historyConfigurations) { | ||
| if (latestReleaseConfigurations == null) { | ||
| latestReleaseConfigurations = new HashMap<>(); | ||
| } | ||
|
|
||
| if (historyConfigurations == null) { | ||
| historyConfigurations = new HashMap<>(); | ||
| } | ||
|
|
||
| Set<String> previousKeys = historyConfigurations.keySet(); | ||
| Set<String> currentKeys = latestReleaseConfigurations.keySet(); | ||
|
|
||
| Set<String> commonKeys = Sets.intersection(previousKeys, currentKeys); | ||
| Set<String> newKeys = Sets.difference(currentKeys, commonKeys); | ||
| Set<String> removedKeys = Sets.difference(previousKeys, commonKeys); | ||
|
|
||
| List<ConfigurationChange> changes = Lists.newArrayList(); | ||
|
|
||
| for (String newKey : newKeys) { | ||
| changes.add(new ConfigurationChange(newKey, latestReleaseConfigurations.get(newKey), | ||
| "ADDED")); | ||
| } | ||
|
|
||
| for (String removedKey : removedKeys) { | ||
| changes.add(new ConfigurationChange(removedKey, null, "DELETED")); | ||
| } | ||
|
|
||
| for (String commonKey : commonKeys) { | ||
| String previousValue = historyConfigurations.get(commonKey); | ||
| String currentValue = latestReleaseConfigurations.get(commonKey); | ||
| if (com.google.common.base.Objects.equal(previousValue, currentValue)) { | ||
| continue; | ||
| } | ||
| changes.add( | ||
| new ConfigurationChange(commonKey, currentValue, "MODIFIED")); | ||
| } | ||
|
|
||
| return changes; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance robustness and maintainability of configuration change calculation.
Several improvements can be made to the implementation:
- Add null checks for map values
- Extract change types as constants
- Replace deprecated
com.google.common.base.Objects.equalwithjava.util.Objects.equals
Here's the suggested implementation:
+ private static final String CHANGE_TYPE_ADDED = "ADDED";
+ private static final String CHANGE_TYPE_DELETED = "DELETED";
+ private static final String CHANGE_TYPE_MODIFIED = "MODIFIED";
public List<ConfigurationChange> calcConfigurationChanges(
Map<String, String> latestReleaseConfigurations, Map<String, String> historyConfigurations) {
if (latestReleaseConfigurations == null) {
latestReleaseConfigurations = new HashMap<>();
}
if (historyConfigurations == null) {
historyConfigurations = new HashMap<>();
}
Set<String> previousKeys = historyConfigurations.keySet();
Set<String> currentKeys = latestReleaseConfigurations.keySet();
Set<String> commonKeys = Sets.intersection(previousKeys, currentKeys);
Set<String> newKeys = Sets.difference(currentKeys, commonKeys);
Set<String> removedKeys = Sets.difference(previousKeys, commonKeys);
List<ConfigurationChange> changes = Lists.newArrayList();
for (String newKey : newKeys) {
+ String newValue = latestReleaseConfigurations.get(newKey);
+ if (newValue != null) {
- changes.add(new ConfigurationChange(newKey, latestReleaseConfigurations.get(newKey),
- "ADDED"));
+ changes.add(new ConfigurationChange(newKey, newValue, CHANGE_TYPE_ADDED));
+ }
}
for (String removedKey : removedKeys) {
- changes.add(new ConfigurationChange(removedKey, null, "DELETED"));
+ changes.add(new ConfigurationChange(removedKey, null, CHANGE_TYPE_DELETED));
}
for (String commonKey : commonKeys) {
String previousValue = historyConfigurations.get(commonKey);
String currentValue = latestReleaseConfigurations.get(commonKey);
- if (com.google.common.base.Objects.equal(previousValue, currentValue)) {
+ if (java.util.Objects.equals(previousValue, currentValue)) {
continue;
}
changes.add(
- new ConfigurationChange(commonKey, currentValue, "MODIFIED"));
+ new ConfigurationChange(commonKey, currentValue, CHANGE_TYPE_MODIFIED));
}
return changes;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public List<ConfigurationChange> calcConfigurationChanges( | |
| Map<String, String> latestReleaseConfigurations, Map<String, String> historyConfigurations) { | |
| if (latestReleaseConfigurations == null) { | |
| latestReleaseConfigurations = new HashMap<>(); | |
| } | |
| if (historyConfigurations == null) { | |
| historyConfigurations = new HashMap<>(); | |
| } | |
| Set<String> previousKeys = historyConfigurations.keySet(); | |
| Set<String> currentKeys = latestReleaseConfigurations.keySet(); | |
| Set<String> commonKeys = Sets.intersection(previousKeys, currentKeys); | |
| Set<String> newKeys = Sets.difference(currentKeys, commonKeys); | |
| Set<String> removedKeys = Sets.difference(previousKeys, commonKeys); | |
| List<ConfigurationChange> changes = Lists.newArrayList(); | |
| for (String newKey : newKeys) { | |
| changes.add(new ConfigurationChange(newKey, latestReleaseConfigurations.get(newKey), | |
| "ADDED")); | |
| } | |
| for (String removedKey : removedKeys) { | |
| changes.add(new ConfigurationChange(removedKey, null, "DELETED")); | |
| } | |
| for (String commonKey : commonKeys) { | |
| String previousValue = historyConfigurations.get(commonKey); | |
| String currentValue = latestReleaseConfigurations.get(commonKey); | |
| if (com.google.common.base.Objects.equal(previousValue, currentValue)) { | |
| continue; | |
| } | |
| changes.add( | |
| new ConfigurationChange(commonKey, currentValue, "MODIFIED")); | |
| } | |
| return changes; | |
| } | |
| private static final String CHANGE_TYPE_ADDED = "ADDED"; | |
| private static final String CHANGE_TYPE_DELETED = "DELETED"; | |
| private static final String CHANGE_TYPE_MODIFIED = "MODIFIED"; | |
| public List<ConfigurationChange> calcConfigurationChanges( | |
| Map<String, String> latestReleaseConfigurations, Map<String, String> historyConfigurations) { | |
| if (latestReleaseConfigurations == null) { | |
| latestReleaseConfigurations = new HashMap<>(); | |
| } | |
| if (historyConfigurations == null) { | |
| historyConfigurations = new HashMap<>(); | |
| } | |
| Set<String> previousKeys = historyConfigurations.keySet(); | |
| Set<String> currentKeys = latestReleaseConfigurations.keySet(); | |
| Set<String> commonKeys = Sets.intersection(previousKeys, currentKeys); | |
| Set<String> newKeys = Sets.difference(currentKeys, commonKeys); | |
| Set<String> removedKeys = Sets.difference(previousKeys, commonKeys); | |
| List<ConfigurationChange> changes = Lists.newArrayList(); | |
| for (String newKey : newKeys) { | |
| String newValue = latestReleaseConfigurations.get(newKey); | |
| if (newValue != null) { | |
| changes.add(new ConfigurationChange(newKey, newValue, CHANGE_TYPE_ADDED)); | |
| } | |
| } | |
| for (String removedKey : removedKeys) { | |
| changes.add(new ConfigurationChange(removedKey, null, CHANGE_TYPE_DELETED)); | |
| } | |
| for (String commonKey : commonKeys) { | |
| String previousValue = historyConfigurations.get(commonKey); | |
| String currentValue = latestReleaseConfigurations.get(commonKey); | |
| if (java.util.Objects.equals(previousValue, currentValue)) { | |
| continue; | |
| } | |
| changes.add( | |
| new ConfigurationChange(commonKey, currentValue, CHANGE_TYPE_MODIFIED)); | |
| } | |
| return changes; | |
| } |
| ('config-service.cache.enabled', 'default', 'false', 'ConfigService是否开启缓存，开启后能提高性能，但是会增大内存消耗！'), | ||
| ('config-service.incremental.change.enabled', 'default', 'false', 'ConfigService是否开启客户端同步增量配置，开启后能提高性能，但是会增大内存消耗！'); |
There was a problem hiding this comment.
💡 Codebase verification
Minor inconsistency found in configuration description text
The configuration entry exists in all SQL scripts with consistent key, namespace, and value, but there's a minor text order difference in the description:
- Most files use: "ConfigService是否开启增量配置同步客户端"
mysql-database-not-specifieduses: "ConfigService是否开启客户端同步增量配置"
Consider standardizing the description text across all SQL scripts for better maintainability.
🔗 Analysis chain
Verify consistency across all SQL scripts.
Let's ensure the new configuration entry is consistent across all SQL scripts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new configuration entry is consistent across all SQL scripts
# Expected: All SQL scripts should have the same configuration entry
echo "Checking configuration entry in SQL scripts..."
rg -A 1 "config-service.incremental.change.enabled" --type sql
Length of output: 1519
| public Release findByReleaseKey(String releaseKey) { | ||
| return releaseRepository.findByReleaseKey(releaseKey); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check and documentation.
Consider these improvements:
- Add null check for the releaseKey parameter
- Add Javadoc explaining the method's purpose and return value
- Consider returning Optional instead of nullable Release
+ /**
+ * Find a release by its unique release key.
+ *
+ * @param releaseKey the unique identifier of the release
+ * @return the Release if found, null otherwise
+ * @throws IllegalArgumentException if releaseKey is null or empty
+ */
public Release findByReleaseKey(String releaseKey) {
+ if (releaseKey == null || releaseKey.trim().isEmpty()) {
+ throw new IllegalArgumentException("Release key cannot be null or empty");
+ }
return releaseRepository.findByReleaseKey(releaseKey);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Release findByReleaseKey(String releaseKey) { | |
| return releaseRepository.findByReleaseKey(releaseKey); | |
| } | |
| /** | |
| * Find a release by its unique release key. | |
| * | |
| * @param releaseKey the unique identifier of the release | |
| * @return the Release if found, null otherwise | |
| * @throws IllegalArgumentException if releaseKey is null or empty | |
| */ | |
| public Release findByReleaseKey(String releaseKey) { | |
| if (releaseKey == null || releaseKey.trim().isEmpty()) { | |
| throw new IllegalArgumentException("Release key cannot be null or empty"); | |
| } | |
| return releaseRepository.findByReleaseKey(releaseKey); | |
| } |
🧰 Tools
🪛 GitHub Actions: build
[warning] Uses unchecked or unsafe operations
| @Test | ||
| public void testQueryConfigWithIncrementalSyncNotFound() throws Exception { | ||
| when(bizConfig.isConfigServiceIncrementalChangeEnabled()) | ||
| .thenReturn(true); | ||
|
|
||
| String someClientSideReleaseKey = "1"; | ||
| String someServerSideNewReleaseKey = "2"; | ||
| HttpServletResponse someResponse = mock(HttpServletResponse.class); | ||
|
|
||
| when(configService.loadConfig(someAppId, someClientIp, someClientLabel, someAppId, | ||
| someClusterName, defaultNamespaceName, | ||
| someDataCenter, someNotificationMessages)).thenReturn(someRelease); | ||
| when(configService.findReleasesByReleaseKeys( | ||
| Sets.newHashSet(someClientSideReleaseKey))).thenReturn(null); | ||
|
|
||
| when(someRelease.getReleaseKey()).thenReturn(someServerSideNewReleaseKey); | ||
| when(someRelease.getNamespaceName()).thenReturn(defaultNamespaceName); | ||
| String configurations = "{\"apollo.public.foo\": \"foo\"}"; | ||
| when(someRelease.getConfigurations()).thenReturn(configurations); | ||
|
|
||
| ApolloConfig result = configController.queryConfig(someAppId, someClusterName, | ||
| defaultNamespaceName, someDataCenter, someClientSideReleaseKey, | ||
| someClientIp, someClientLabel, someMessagesAsString, someRequest, someResponse); | ||
| assertEquals(1, result.getConfigurations().size()); | ||
| assertEquals("foo", result.getConfigurations().get("apollo.public.foo")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions for error scenarios.
The test should verify:
- Error logging
- Response status codes
- Error messages in the response
@Test
public void testQueryConfigWithIncrementalSyncNotFound() throws Exception {
// ... existing code ...
+ verify(someResponse, times(1)).setStatus(HttpServletResponse.SC_OK);
+ verify(logger).warn(contains("Release not found for key: {}"), eq(someClientSideReleaseKey));
}Committable suggestion skipped: line range outside the PR's diff.
|
@jackie-coming It seems you accidentally closed the previous pull request...
|
In this context, I believe there are two variables: the size of the full configuration being pushed and the scale of the client base receiving the push. |
What's the purpose of this PR
#5288
之前的讨论在这个pr，为什么会被关闭？ 我重新提交了pr，继续修复代码
Which issue(s) this PR fixes:
Fixes #
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
Release Notes for Apollo Configuration Service v2.4.0
New Features
Configuration Options
config-service.incremental.change.enabledintroducedfalsePerformance
Compatibility