feat: support incremental configuration synchronization client #5373
Conversation
…autre/IncrementalSync
…autre/IncrementalSync
# Conflicts: # changes/changes-2.4.0.md
dosubot
bot
added
the
size:XL
WalkthroughThis change introduces support for incremental configuration synchronization in the Apollo configuration service. It adds a new feature toggle, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ConfigController
participant ConfigService
participant IncrementalSyncService
Client->>ConfigController: queryConfig(appId, cluster, namespace, clientReleaseKey)
ConfigController->>ConfigService: load latest merged config
ConfigService-->>ConfigController: latestMergedReleaseKey, latestConfig
ConfigController->>BizConfig: isConfigServiceIncrementalChangeEnabled()
alt Incremental Sync Enabled
ConfigController->>ConfigService: findReleasesByReleaseKeys(clientReleaseKeys)
ConfigService-->>ConfigController: clientReleases
ConfigController->>IncrementalSyncService: getConfigurationChanges(latestMergedReleaseKey, latestConfig, clientReleaseKey, clientConfig)
IncrementalSyncService-->>ConfigController: configurationChanges
ConfigController-->>Client: ApolloConfig (changes only, syncType=INCREMENTAL_SYNC)
else Full Sync or Not Enabled
ConfigController-->>Client: ApolloConfig (full config)
end
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseService.java (1)
1-643: 💡 Verification agent🧩 Analysis chain
Address unchecked operation warning.
There's a GitHub Actions warning about unchecked operations in this file. While not directly related to your changes, it would be good to fix it.
🏁 Script executed:
#!/bin/bash # Search for potential unchecked cast operations in the file rg -n "operationContext\.get\(.*\)" apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseService.javaLength of output: 224
Fix unchecked cast for branch release keys in ReleaseService.java
The GitHub Actions warning originates from the unchecked cast on line 349 in thegetBranchReleaseKeysmethod. Please resolve this by adding a proper type check (e.g., usinginstanceof) or by leveraging a more specific Gson type reference to ensure that the cast toCollection<String>is safe.
- Location:
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseService.java(line 349)- Action: Instead of directly casting with
(Collection<String>), verify that the object returned byoperationContext.get(ReleaseOperationContext.BRANCH_RELEASE_KEYS)is an instance ofCollectioncontainingStringitems. Adjust the implementation accordingly to remove the unchecked cast warning.🧰 Tools
🪛 GitHub Actions: build
[warning] 1-1: uses unchecked or unsafe operations. Recompile with -Xlint:unchecked for details.
🧹 Nitpick comments (11)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigService.java (1)
26-26: Interface enhancement for incremental synchronization support.The
ConfigServiceinterface now extendsIncrementalSyncConfigServicein addition toReleaseMessageListener, enhancing its capabilities to support incremental configuration updates. This is an essential architectural change that allows the configuration service to calculate and provide only the changes between different configuration versions rather than always sending complete configurations.This extension aligns well with the configuration service's responsibility and promotes efficient data transfer by enabling differential updates. Make sure the implementations of this interface properly handle the increased memory requirements mentioned in the configuration comments.
docs/en/deployment/distributed-deployment-guide.md (1)
1644-1658: Well-documented configuration with minor confusion in the final note.The documentation clearly explains the purpose, benefits, and considerations for enabling incremental configuration synchronization. It properly mentions version requirements for both server and client.
However, in the last paragraph, there appears to be a copy-paste error:
- > `config-service.cache.enabled` configuration adjustment requires a restart of the config service to take effect + > `config-service.incremental.change.enabled` configuration adjustment requires a restart of the config service to take effectThe note should reference the incremental change configuration parameter instead of the cache configuration parameter since that's what this section is about.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1656-1656: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
docs/zh/deployment/distributed-deployment-guide.md (1)
1580-1591: Fix Markdown lint issue by removing extra blank line.There's a blank line inside the blockquote which may trigger a lint error. Removing the blank line ensures proper formatting:
> ... > > ... > ... > ...🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1590-1590: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCache.java (3)
57-57: Correct the constant name.
DEFAULT_EXPIRED_AFTER_ACCESS_IN_SENCONDShas a typographical error and should probably end with_SECONDS.- private static final long DEFAULT_EXPIRED_AFTER_ACCESS_IN_SENCONDS = 10; + private static final long DEFAULT_EXPIRED_AFTER_ACCESS_IN_SECONDS = 10;
64-64: Restrict the visibility of the cache reference.Consider making
releasesCacheprivate unless it must be accessed from externally. Public fields can introduce unintended external mutations.- public LoadingCache<String, Optional<Release>> releasesCache; + private LoadingCache<String, Optional<Release>> releasesCache;
150-152: Add logging for the caught ExecutionException.Swallowing the exception silently may hinder debugging. Logging the error or rethrowing a wrapped exception can help diagnose problems.
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/AbstractConfigService.java (2)
103-142: Consider using the enum for change types to avoid string literals.
The logic of detecting ADDED, DELETED, and MODIFIED configurations appears correct. However, you're currently using raw strings (e.g.,"ADDED") instead of theConfigurationChangeTypeenum. Relying on the enum would reduce the risk of typos and make the code more self-documenting.- changes.add(new ConfigurationChange(newKey, latestReleaseConfigurations.get(newKey), "ADDED")); + changes.add(new ConfigurationChange( + newKey, + latestReleaseConfigurations.get(newKey), + ConfigurationChangeType.ADDED.name() + ));
144-147: Avoid returning null in place of an unsupported or empty result.
findReleasesByReleaseKeysreturnsnull, which may cause NullPointerExceptions for callers if they don't anticipate that scenario. Consider throwing anUnsupportedOperationExceptionor returning an emptyMapto be more explicit and safer.-public Map<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys) { - return null; +public Map<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys) { + return Collections.emptyMap(); }apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCacheTest.java (1)
203-223: Behavior when a release is not found.
Returningnullfor a missing release map is consistent with your implementation inAbstractConfigService. However, keep in mind that using an empty map might be clearer for callers since it avoidsnullchecks.apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java (1)
163-194: Incremental change handling block.
Your incremental-change logic looks correct:
- Parsing client release keys into a LinkedHashSet.
- Looking up historical releases in order.
- Computing differences with
calcConfigurationChanges.- Setting the sync type to
INCREMENTAL_SYNC.As the controller method grows, consider extracting this block into a dedicated function in a service layer to enhance maintainability and keep the controller focused on request handling.
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigControllerTest.java (1)
538-572: Incremental sync test is implemented correctly.
Covers client- vs. server-side releases and assertsConfigSyncType.INCREMENTAL_SYNC.A complementary test for the case when
isConfigServiceIncrementalChangeEnabled()is false would further enhance coverage.
📜 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
🧬 Code Definitions (2)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/ConfigServiceAutoConfiguration.java (1)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCache.java (1)
ConfigServiceWithChangeCache(52-155)
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCacheTest.java (1)
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigControllerTest.java (1)
RunWith(58-675)
🪛 GitHub Actions: build
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseService.java
[warning] 1-1: uses unchecked or unsafe operations. Recompile with -Xlint:unchecked for details.
🪛 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 (65)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/repository/ReleaseRepository.java (2)
40-40: Appropriate addition of repository method.This method complements the existing
findByReleaseKeyInmethod by providing a way to fetch a single Release by its key, which will be useful for the incremental configuration synchronization feature.
48-48: Parameter name improvement.The parameter name change from
releaseKeytoreleaseKeysin this method signature improves clarity as it's dealing with a Set of keys.changes/changes-2.4.0.md (1)
12-14: Documentation updated correctly.The changelog entry appropriately documents the new incremental configuration synchronization client feature with a link to the related PR.
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (1)
243-245: Well-designed configuration flag.The method follows the established pattern of other feature flag methods in the class and defaults to
false, which is a safe choice for introducing new functionality. It will be used by the config service to determine whether to use incremental changes.apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseService.java (1)
122-124: Appropriate service method addition.This method provides a clean interface to the new repository functionality, following the same pattern as other similar methods in the class.
scripts/sql/profiles/mysql-default/apolloconfigdb.sql (1)
505-506: Added new configuration toggle for incremental sync feature.A new server configuration entry has been added to control the incremental configuration synchronization feature. The default value is set to
false, which is appropriate for a feature that increases memory consumption.The descriptive comment clearly explains the performance vs. memory trade-off, helping administrators make informed decisions when configuring the system. This follows the same pattern as the existing cache configuration.
scripts/sql/src/apolloconfigdb.sql (1)
493-494: Added configuration entry in source SQL template.This adds the incremental configuration synchronization toggle in the source SQL template, consistent with the changes in the derived SQL files.
The configuration is properly described with a comment that explains the performance benefits and memory implications. Maintaining consistency between this source template and the generated SQL files is important for the build process.
scripts/sql/profiles/mysql-database-not-specified/apolloconfigdb.sql (1)
500-501: Added incremental sync configuration toggle for non-specified database profile.The configuration entry for enabling incremental synchronization has been added consistently across all database profiles, including this one for non-specified databases.
The setting defaults to
false, which is a safe default that allows explicit opt-in after considering the memory implications. The comment explains the purpose and trade-offs clearly.apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/ConfigServiceAutoConfiguration.java (2)
33-33: Import for new feature.The import statement for
ConfigServiceWithChangeCacheis needed to support the new incremental configuration synchronization feature.
73-76: Well-implemented conditional logic for incremental change support.This code adds support for incremental configuration synchronization by checking if the feature is enabled before instantiating a
ConfigServiceWithChangeCache. The implementation follows the existing pattern of conditional service instantiation and maintains backward compatibility.The code properly reuses the existing constructor parameters, ensuring that all necessary dependencies are passed to the new implementation.
scripts/sql/profiles/h2-default/apolloconfigdb.sql (1)
486-487: Coherent configuration addition for incremental change feature.The new configuration entry
config-service.incremental.change.enabledproperly follows the existing pattern with a descriptive comment about the feature's impact on performance and memory consumption. Setting the default to 'false' is a safe choice as it requires explicit opt-in.apollo-configservice/src/main/resources/jpa/configdb.init.h2.sql (1)
22-23: Consistent H2 database configuration for incremental change feature.The addition of the same configuration parameter to the H2 initialization script ensures consistency between different database environments. The default value is properly set to 'false', requiring explicit opt-in.
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/IncrementalSyncConfigService.java (2)
35-36: Method name clarity is commendable.The method signature
calcConfigurationChangesis descriptively named and the parameters convey intent clearly.
38-42: Adequate interface definition for fetching releases by keys.The
findReleasesByReleaseKeysmethod is straightforward to understand, promoting code clarity.apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java (1)
65-67: Consider the design implications of exposing these fields as protected.
MakingreleaseServiceandbizConfigprotected allows subclasses to access them directly, which can be useful for extensions but can also reduce encapsulation and open up potential misuse if subclasses override or alter their behavior. Overall your change appears intentional to facilitate subclassing; just ensure that you trust subclasses to handle these references appropriately.apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/AbstractConfigService.java (1)
24-33: New imports look appropriate.
No immediate issues or concerns with these added imports; they align with your new method and data structures.apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCacheTest.java (7)
93-111: Test case for added configurations.
This test successfully verifies an added configuration scenario. The assertions are clear and the logic is correct.
113-127: Test case for null latest configurations.
The validation is accurate, confirming that previously existing keys are considered deleted. No issues found.
129-143: Test case for null history configurations.
This mirrors the above scenario and shows the correct handling of a null history by treating all new keys as added. Looks good.
145-162: Test case for updating configurations.
Properly validates the transition from one value to another as a MODIFIED change. Implementation is sound.
164-179: Test case for deleted configurations.
Correctly checks that removal of an existing key is flagged as DELETED. No problems noted.
181-201: Caching test for release lookups.
This verifies that repeated lookups hit the cache rather than calling the service each time. Well-structured test that ensures caching logic correctness.
225-245: Test notification-based cache invalidation.
This test checks that a valid release can be retrieved after handling a release message without extra calls tofindByReleaseKey. Implementation is correct.apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java (4)
19-52: New imports are appropriate.
The added imports (forBizConfig,ConfigurationChange,Sets, etc.) fit well with the incremental sync features and do not introduce unused references.
70-89: Constructor injection forBizConfig.
InjectingBizConfighere and storing it in a final member is consistent with other dependencies. Good use of constructor injection for clarity and testability.
147-157: 304 Not Modified early exit.
This logic checks if the client-side release key matches the latest merged release key and avoids sending a new payload if unchanged. This is a standard optimization to save bandwidth.
159-162: Creation of ApolloConfig with a merged release key.
Constructing theApolloConfigusing the newly combined release key is straightforward and clear.apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigControllerTest.java (38)
19-19: No issues with new import forBizConfig.
This import is needed for testing the new incremental sync feature.
29-30: New imports forConfigurationChangeandConfigSyncTypelook correct.
These classes are appropriately used to capture config changes and the sync type.
34-34: New import forSetsis valid.
Used for handling sets of release keys in incremental sync tests.
37-37: New import forTypeTokenis fine.
Allows parsing JSON configurations to maps for incremental sync checks.
44-44: ImportingTypeis correct.
Used in conjunction withTypeTokento parse configurations.
46-47: Imports ofArrayListandListare standard.
Used to store and verify configuration changes within tests.
66-67: MockingBizConfigis necessary for toggling incremental sync in tests.
This aligns with the new feature under test.
84-85: Mocking additionalReleaseobjects is valid.
Allows simulating multiple release scenarios in test methods.
119-124: Stubbing namespace utilities is consistent with the test flow.
Ensures correct filtering and normalization of namespaces.
127-128: Stubbing message transformation is correct.
Verifies that notification messages are handled properly in tests.
137-139: StubbingconfigService.loadConfig(...)with matching parameters is valid.
Reflects the intended test scenario for the default namespace.
147-149: VerifyingconfigService.loadConfig(...)invocation is standard.
Ensures the service call is triggered exactly once.
155-156: Instance config audit verification is handled properly.
Checks that audits occur with the correct parameters.
166-168: Reused stub for loading config is consistent.
Confirms that private “file” namespace logic is uniformly tested.
170-172: Stubbing namespace filtering and normalization is valid.
Ensures the test can confirm correct transformations for file-based namespaces.
197-198: StubbingconfigService.loadConfig(...)for private namespace is correct.
Validates the scenario for private configurations.
200-203: Namespace stubbing for private namespace is accurate.
Ensures filtering and normalization logic is tested thoroughly.
222-224: Returning null fromconfigService.loadConfig(...)tests the not-found case.
Good coverage for missing release scenarios.
240-242: Repeated stub for no-change scenario remains valid.
Tests the flow when client release matches server release key.
246-250: Query call with all arguments is properly formed.
Ensures the test checks for 304 Not Modified logic in the controller.
265-266: Stubbing config loading for an app-owned namespace is appropriate.
Ensures coverage for application-specific namespaces.
298-300: Stub for public namespace without app override stands correct.
Helps validate fallback logic in the test.
303-305: Loading config from the public app ID is handled properly.
Verifies fallback release is fetched as intended.
313-314: Method call line breaks are unchanged logically.
No issues with passing existing parameters.
322-323: Instance config audit utility verification is correct.
Ensures correct usage for auditing public namespaces.
336-338: Stubbing null return simulates missing app override.
Tests fallback to the public release.
341-343: Public config load stub is valid.
Checks that the public release is found and returned.
345-348: Namespace utility stubs ensure correct translation from the file extension.
No issues spotted.
349-350: StubbingappNamespaceService.findByAppIdAndNamespace(...)is consistent.
Expects a null return if no app-specific definition exists.
378-380: Valid stubbing for app-level release in combined override tests.
Helps confirm merges from multiple sources.
385-387: Public-level release stub is properly set up.
Facilitates testing of combined app & public overrides.
406-406: Instance config audit verification calls remain consistent.
Multiple audits are expected for app and public releases.Also applies to: 411-411
461-463: VerifyingconfigService.loadConfig(...)is never called is appropriate.
Checks the scenario forNO_APPID_PLACEHOLDERas expected.
482-483: Stub load config for public namespace with no actual app ID is correct.
Handles fallback to the public app in the test.
485-486: Namespace normalization stub is consistent.
Ensures placeholders are correctly mapped to real namespaces.
574-599: Handling missing old releases is well-tested.
The logic for incremental sync with a null or unknown release key is verified successfully.
601-671: Tests public namespace overrides within incremental sync thoroughly.
Effectively validates merging of configurations from app and public releases.
673-675: UsingTypeToken<Map<String, String>>to parse configurations is correct.
Essential for calculating configuration changes.
...va/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCache.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/com/ctrip/framework/apollo/configservice/ConfigServiceAutoConfiguration.java
Show resolved
Hide resolved
...vice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java
Show resolved
Hide resolved
...va/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithChangeCache.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java (1)
190-197: Business logic leakage into controller – consider moving to service layerThe controller assembles historical releases, merges configurations and
derives the diff. This mixes orchestration with business rules and
duplicates logic already living inIncrementalSyncService
(see previous review bynobodyiam).Prefer delegating the entire “get incremental response” operation to
IncrementalSyncService, letting the controller focus on HTTP concerns.
🧹 Nitpick comments (10)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/DefaultConfigService.java (1)
64-71: Comment is misleading compared to implementation.The comment "since there is no cache, so do nothing" doesn't match the actual implementation which does retrieve data from the releaseService.
Consider updating the comment to accurately reflect what the method does:
- // since there is no cache, so do nothing + // directly retrieve from releaseService without cacheAlso, consider returning an empty map instead of null when no releases are found, to avoid potential NullPointerExceptions:
if (releasesMap != null) { return ImmutableMap.copyOf(releasesMap.stream().collect(ImmutableMap.toImmutableMap(Release::getReleaseKey, release -> release))); } - return null; + return ImmutableMap.of();apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/IncrementalSyncService.java (1)
25-32: Add documentation for the interface and method.This new interface is critical for the incremental sync feature, but lacks documentation explaining its purpose and how it should be used.
Consider adding more detailed JavaDoc to explain:
- The purpose of this interface
- What ConfigurationChange represents
- The expected behavior when computing differences
- How to interpret the returned list
For example:
/** * Service to calculate incremental configuration changes between server and client configurations. * This service helps reduce network traffic by sending only the differences rather than full configurations. */ public interface IncrementalSyncService { /** * Computes the configuration changes between the latest server configuration and client-side configuration. * * @param latestMergedReleaseKey The release key of the latest merged configuration on server side * @param latestReleaseConfigurations The configuration map of the latest release on server side * @param clientSideReleaseKey The release key of the client's current configuration * @param clientSideConfigurations The configuration map of the client's current configuration * @return A list of configuration changes (additions, deletions, modifications) that the client * needs to apply to synchronize with the server */ List<ConfigurationChange> getConfigurationChanges(...); }docs/en/deployment/distributed-deployment-guide.md (1)
1666-1680: Documentation has minor inconsistencies.The documentation for the new incremental synchronization configuration is generally clear but has a few minor issues:
- The last paragraph contains an incorrect reference that needs to be updated:
-> `config-service.cache.enabled` configuration adjustment requires a restart of the config service to take effect +> `config-service.incremental.change.enabled` configuration adjustment requires a restart of the config service to take effect
- There's a blank line inside the blockquote at line 1678 that should be removed (as flagged by markdownlint).
Otherwise, the documentation provides good information about the new feature, its purpose, and considerations for enabling it.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1678-1678: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
docs/zh/deployment/distributed-deployment-guide.md (1)
1610-1611: Remove blank line inside blockquote to satisfy Markdown‑lint
markdownlint(rule MD028) warns about empty lines within blockquotes.
The two consecutive quoted lines should be merged into a single contiguous blockquote without a blank line in between.-> 开启缓存后必须确保应用中配置的`app.id`、`apollo.cluster` -> +> 开启缓存后必须确保应用中配置的`app.id`、`apollo.cluster`apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncServiceTest.java (2)
118-123: PreferassertNulloverassertEquals(null, …)Using the dedicated
assertNullassertion makes failures clearer and avoids
being flagged by static‑analysis tools.- assertEquals(null, result.get(0).getNewValue()); + assertNull(result.get(0).getNewValue());
102-106: Consider replacing hard‑coded strings with the enum to avoid fragile tests
"ADDED","MODIFIED", and"DELETED"are repeated string literals.
If the underlyingConfigurationChangeTypeenum ever changes, these
tests will silently compile but fail at runtime.
Import the enum and assert againstConfigurationChangeType.ADDED.name()
(or expose a getter returning the enum).apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCacheTest.java (1)
135-137: Duplicate verify statement
verify(releaseService, times(1)).findActiveOne(someId);appears twice
in succession, offering no additional value and slightly slowing the test.apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncService.java (2)
68-70: Remove unused local variable to avoid reader confusion
ReleaseKeyPair key = new ReleaseKeyPair(clientSideReleaseKey, latestMergedReleaseKey);
is created but never used incalcConfigurationChanges. This is a left‑over from an early refactor and can be safely deleted.- ReleaseKeyPair key = new ReleaseKeyPair(clientSideReleaseKey, latestMergedReleaseKey);
70-76: Prefer immutable empty maps instead of allocating newHashMapCreating new, mutable
HashMapinstances just to represent an empty map introduces unnecessary allocations and allows unintended mutation. Replace withCollections.emptyMap()which is cheaper and signals intent.- if (latestReleaseConfigurations == null) { - latestReleaseConfigurations = new HashMap<>(); - } + if (latestReleaseConfigurations == null) { + latestReleaseConfigurations = java.util.Collections.emptyMap(); + } - if (clientSideConfigurations == null) { - clientSideConfigurations = new HashMap<>(); - } + if (clientSideConfigurations == null) { + clientSideConfigurations = java.util.Collections.emptyMap(); + }apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigControllerTest.java (1)
544-546: State sharedbizConfigstub per test to avoid cross‑test leakage
when(bizConfig.isConfigServiceIncrementalChangeEnabled()).thenReturn(true);Because the same
bizConfigmock is reused across tests, set the default insetUp()and override in specific tests if needed. This prevents subtle order‑dependent failures when future tests expect the default (false).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java(1 hunks)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/ConfigServiceAutoConfiguration.java(1 hunks)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java(5 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(2 hunks)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java(6 hunks)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/DefaultConfigService.java(2 hunks)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncService.java(1 hunks)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/IncrementalSyncService.java(1 hunks)apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigControllerTest.java(22 hunks)apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCacheAndCacheKeyIgnoreCaseTest.java(1 hunks)apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCacheTest.java(7 hunks)apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/DefaultConfigServiceTest.java(1 hunks)apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncServiceTest.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(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/DefaultConfigServiceTest.java
- apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/AbstractConfigService.java
- apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCacheAndCacheKeyIgnoreCaseTest.java
- changes/changes-2.4.0.md
🚧 Files skipped from review as they are similar to previous changes (3)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java
- apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigService.java
- apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java (1)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/WebUtils.java (1)
WebUtils(27-41)
🪛 GitHub Actions: build
pom.xml
[error] 1-1: Failed to execute goal on project apollo-common: Could not resolve dependency com.ctrip.framework.apollo:apollo-core:jar:2.4.0-SNAPSHOT. Artifact not found in sonatype-nexus-snapshots repository.
🪛 markdownlint-cli2 (0.17.2)
docs/en/deployment/distributed-deployment-guide.md
1678-1678: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
docs/zh/deployment/distributed-deployment-guide.md
1616-1616: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
🔇 Additional comments (1)
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigControllerTest.java (1)
655-662: Use real release keys when asserting incremental diff
incrementalSyncService.getConfigurationChanges("", …, "", …)passes empty strings as release keys, while meaningful keys (someAppServerSideReleaseKey,mergeClientSideReleaseKey…) are available. Supplying real keys strengthens the contract and documents intent.-when(incrementalSyncService.getConfigurationChanges("", +when(incrementalSyncService.getConfigurationChanges( + Joiner.on(ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR) + .join(someAppServerSideReleaseKey, somePublicAppSideReleaseKey), gson.fromJson(mergeServerSideConfigurations, configurationTypeReference), - "", + mergeClientSideReleaseKey, gson.fromJson(mergeClientSideConfigurations, configurationTypeReference)))
| <revision>2.5.0-SNAPSHOT</revision> | ||
| <java.version>1.8</java.version> | ||
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
| <apollo-java.version>2.5.0-SNAPSHOT</apollo-java.version> | ||
| <apollo-java.version>2.4.0-SNAPSHOT</apollo-java.version> | ||
| <spring-boot.version>2.7.11</spring-boot.version> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Mismatch between project <revision> and <apollo-java.version>
The <apollo-java.version> has been downgraded to 2.4.0-SNAPSHOT, but the project’s <revision> remains at 2.5.0-SNAPSHOT. As a result, the build is looking for apollo-core:2.4.0-SNAPSHOT, which isn’t published in the snapshots repo, causing the pipeline failure.
Please align these versions by either:
- Bumping
<revision>down to2.4.0-SNAPSHOTso both properties match, or - Restoring
<apollo-java.version>to2.5.0-SNAPSHOTif you intend to stay on the 2.5.0 line.
Proposed fix (option 1: align to 2.4.0-SNAPSHOT):
- <revision>2.5.0-SNAPSHOT</revision>
+ <revision>2.4.0-SNAPSHOT</revision>
...
- <apollo-java.version>2.4.0-SNAPSHOT</apollo-java.version>
+ <apollo-java.version>2.4.0-SNAPSHOT</apollo-java.version>Or option 2 (restore to 2.5.0-SNAPSHOT):
- <apollo-java.version>2.4.0-SNAPSHOT</apollo-java.version>
+ <apollo-java.version>2.5.0-SNAPSHOT</apollo-java.version>To confirm how the apollo-java.version property is used and whether the apollo-core dependency is pulling in the mismatched version, let’s search for its definition and usage:
🏁 Script executed:
#!/bin/bash
# 1. Show where the apollo-java.version property is defined
echo "=== Definition of apollo-java.version property ==="
rg -n "<apollo-java.version>" -n pom.xml
# 2. Find all places that reference the apollo-java.version property
echo -e "\n=== Usage of the apollo-java.version property ==="
rg -n "\${apollo-java.version}" -n .
# 3. Locate any direct declaration of an apollo-core dependency
echo -e "\n=== Declaration of apollo-core dependency ==="
rg -n "<artifactId>apollo-core" -n .Length of output: 753
Let’s inspect how the apollo-core dependency is declared and verify which <version> it’s using:
🏁 Script executed:
#!/bin/bash
# 1. Show the surrounding lines in the root pom where apollo-core is declared
echo "=== Root pom.xml (lines around apollo-core) ==="
rg -n '<artifactId>apollo-core' -n pom.xml -A3 -B3
# 2. Show the surrounding lines in apollo-common/pom.xml where apollo-core is declared
echo -e "\n=== apollo-common/pom.xml (lines around apollo-core) ==="
rg -n '<artifactId>apollo-core' -n apollo-common/pom.xml -A3 -B3
# 3. Find any references to the apollo-java.version property
echo -e "\n=== References to apollo-java.version ==="
rg -n -F '${apollo-java.version}' -n .Length of output: 1113
Align <revision> with <apollo-java.version> to fix missing snapshot artifact
The project’s <revision> is still set to 2.5.0‑SNAPSHOT while <apollo-java.version> was lowered to 2.4.0‑SNAPSHOT. This causes Maven to request com.ctrip.framework.apollo:apollo-core:2.4.0‑SNAPSHOT (used in dependencies at lines 114 & 149), which isn’t published in our snapshots repo.
Please choose one of the following:
- Restore
<apollo-java.version>to 2.5.0‑SNAPSHOT so it matches<revision>, or - Downgrade
<revision>to 2.4.0‑SNAPSHOT if you really intend to stay on 2.4.x.
Affected lines in pom.xml:
- Line 63:
<revision> - Line 66:
<apollo-java.version>
Diff for option 1 (preferred):
- <apollo-java.version>2.4.0-SNAPSHOT</apollo-java.version>
+ <apollo-java.version>2.5.0-SNAPSHOT</apollo-java.version>📝 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.
| <revision>2.5.0-SNAPSHOT</revision> | |
| <java.version>1.8</java.version> | |
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | |
| <apollo-java.version>2.5.0-SNAPSHOT</apollo-java.version> | |
| <apollo-java.version>2.4.0-SNAPSHOT</apollo-java.version> | |
| <spring-boot.version>2.7.11</spring-boot.version> | |
| <revision>2.5.0-SNAPSHOT</revision> | |
| <java.version>1.8</java.version> | |
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | |
| - <apollo-java.version>2.4.0-SNAPSHOT</apollo-java.version> | |
| + <apollo-java.version>2.5.0-SNAPSHOT</apollo-java.version> | |
| <spring-boot.version>2.7.11</spring-boot.version> |
| //四种情况 | ||
| //开启增量更新 | ||
| if (bizConfig.isConfigServiceIncrementalChangeEnabled()) { | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implementation needed for incremental change condition.
The code adds a new conditional branch to check for incremental change support, but the branch is currently empty and has no implementation or return statement.
This seems to be a preparatory step for implementing the incremental change feature. Consider either:
- Implementing the actual logic inside this branch
- Adding a comment describing what will be implemented here in future changes
Also note the previous review comment suggesting that the config-service.cache.enabled and config-service.incremental.change.enabled flags could potentially be combined to simplify the configuration options.
| public void testFindReleasesByReleaseKeysNotFoundMultipleTimes() throws Exception { | ||
| String someReleaseKey = "someReleaseKey"; | ||
| long someId = 1; | ||
| Set<String> someReleaseKeys = Sets.newHashSet(someReleaseKey); | ||
|
|
||
| when(releaseService.findByReleaseKey(someReleaseKey)).thenReturn(null); | ||
| when(releaseService.findActiveOne(someId)).thenReturn(null); | ||
|
|
||
|
|
||
|
|
||
| ImmutableMap<String, Release> someReleaseMap = configServiceWithCache.findReleasesByReleaseKeys( | ||
| someReleaseKeys); | ||
| assertEquals(1, someReleaseMap.size()); | ||
| assertEquals(someRelease, someReleaseMap.get(someReleaseKey)); | ||
| verify(releaseService, times(1)).findByReleaseKey(someReleaseKey); | ||
| } |
There was a problem hiding this comment.
Failing expectation – asserting a non‑existent release
releaseService.findByReleaseKey(someReleaseKey) is stubbed to return
null, yet the test later asserts that the returned map contains
someRelease. This will always fail.
- when(releaseService.findByReleaseKey(someReleaseKey)).thenReturn(null);
+ // Expect not‑found; the map should contain a null value
+ when(releaseService.findByReleaseKey(someReleaseKey)).thenReturn(null);
...
- assertEquals(someRelease, someReleaseMap.get(someReleaseKey));
+ assertNull("When the release is not found the cache should contain null",
+ someReleaseMap.get(someReleaseKey));Double‑check the intended behaviour of findReleasesByReleaseKeys and
update either the production code or the assertion accordingly.
📝 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 testFindReleasesByReleaseKeysNotFoundMultipleTimes() throws Exception { | |
| String someReleaseKey = "someReleaseKey"; | |
| long someId = 1; | |
| Set<String> someReleaseKeys = Sets.newHashSet(someReleaseKey); | |
| when(releaseService.findByReleaseKey(someReleaseKey)).thenReturn(null); | |
| when(releaseService.findActiveOne(someId)).thenReturn(null); | |
| ImmutableMap<String, Release> someReleaseMap = configServiceWithCache.findReleasesByReleaseKeys( | |
| someReleaseKeys); | |
| assertEquals(1, someReleaseMap.size()); | |
| assertEquals(someRelease, someReleaseMap.get(someReleaseKey)); | |
| verify(releaseService, times(1)).findByReleaseKey(someReleaseKey); | |
| } | |
| public void testFindReleasesByReleaseKeysNotFoundMultipleTimes() throws Exception { | |
| String someReleaseKey = "someReleaseKey"; | |
| long someId = 1; | |
| Set<String> someReleaseKeys = Sets.newHashSet(someReleaseKey); | |
| // Expect not‑found; the map should contain a null value | |
| when(releaseService.findByReleaseKey(someReleaseKey)).thenReturn(null); | |
| when(releaseService.findActiveOne(someId)).thenReturn(null); | |
| ImmutableMap<String, Release> someReleaseMap = configServiceWithCache.findReleasesByReleaseKeys( | |
| someReleaseKeys); | |
| assertEquals(1, someReleaseMap.size()); | |
| assertNull("When the release is not found the cache should contain null", | |
| someReleaseMap.get(someReleaseKey)); | |
| verify(releaseService, times(1)).findByReleaseKey(someReleaseKey); | |
| } |
| if (bizConfig.isConfigServiceIncrementalChangeEnabled()) { | ||
| LinkedHashSet<String> clientSideReleaseKeys = Sets.newLinkedHashSet( | ||
| Arrays.stream( | ||
| clientSideReleaseKey.split(Pattern.quote(ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR))) | ||
| .collect(Collectors.toList())); | ||
|
|
||
| Map<String, Release> clientSideReleases = configService.findReleasesByReleaseKeys( | ||
| clientSideReleaseKeys); | ||
| //find history releases | ||
| if (clientSideReleases != null) { | ||
| //order by clientSideReleaseKeys | ||
| List<Release> historyReleasesWithOrder = new ArrayList<>(); | ||
| for (String item : clientSideReleaseKeys) { | ||
| Release release = clientSideReleases.get(item); | ||
| if (release != null) { | ||
| historyReleasesWithOrder.add(release); | ||
| } | ||
| } | ||
|
|
||
| Map<String, String> clientSideConfigurations = mergeReleaseConfigurations | ||
| (historyReleasesWithOrder); | ||
|
|
||
| List<ConfigurationChange> configurationChanges = incrementalSyncService.getConfigurationChanges( | ||
| latestMergedReleaseKey, | ||
| latestConfigurations, clientSideReleaseKey, clientSideConfigurations); | ||
|
|
||
| apolloConfig.setConfigurationChanges(configurationChanges); | ||
|
|
||
| apolloConfig.setConfigSyncType(ConfigSyncType.INCREMENTAL_SYNC.getValue()); | ||
| Tracer.logEvent("Apollo.Config.Found", assembleKey(appId, appClusterNameLoaded, | ||
| originalNamespace, dataCenter)); | ||
| return apolloConfig; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Skip incremental diff when client has no valid release key
If the client passes the default sentinel ("-1") or an empty value,
clientSideReleaseKeys will contain a single element "-1".
findReleasesByReleaseKeys will then perform an unnecessary cache/database
lookup and getConfigurationChanges will treat the entire namespace as
ADDED, sending a full payload disguised as “incremental”.
Add a guard clause before starting the incremental‑sync branch:
- if (bizConfig.isConfigServiceIncrementalChangeEnabled()) {
+ if (bizConfig.isConfigServiceIncrementalChangeEnabled()
+ && !"-1".equals(clientSideReleaseKey)
+ && !Strings.isNullOrEmpty(clientSideReleaseKey)) {This avoids wasted work and keeps wire‑format semantics intuitive.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (bizConfig.isConfigServiceIncrementalChangeEnabled()) { | |
| LinkedHashSet<String> clientSideReleaseKeys = Sets.newLinkedHashSet( | |
| Arrays.stream( | |
| clientSideReleaseKey.split(Pattern.quote(ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR))) | |
| .collect(Collectors.toList())); | |
| Map<String, Release> clientSideReleases = configService.findReleasesByReleaseKeys( | |
| clientSideReleaseKeys); | |
| //find history releases | |
| if (clientSideReleases != null) { | |
| //order by clientSideReleaseKeys | |
| List<Release> historyReleasesWithOrder = new ArrayList<>(); | |
| for (String item : clientSideReleaseKeys) { | |
| Release release = clientSideReleases.get(item); | |
| if (release != null) { | |
| historyReleasesWithOrder.add(release); | |
| } | |
| } | |
| Map<String, String> clientSideConfigurations = mergeReleaseConfigurations | |
| (historyReleasesWithOrder); | |
| List<ConfigurationChange> configurationChanges = incrementalSyncService.getConfigurationChanges( | |
| latestMergedReleaseKey, | |
| latestConfigurations, clientSideReleaseKey, clientSideConfigurations); | |
| apolloConfig.setConfigurationChanges(configurationChanges); | |
| apolloConfig.setConfigSyncType(ConfigSyncType.INCREMENTAL_SYNC.getValue()); | |
| Tracer.logEvent("Apollo.Config.Found", assembleKey(appId, appClusterNameLoaded, | |
| originalNamespace, dataCenter)); | |
| return apolloConfig; | |
| } | |
| } | |
| if (bizConfig.isConfigServiceIncrementalChangeEnabled() | |
| && !Strings.isNullOrEmpty(clientSideReleaseKey) | |
| && !"-1".equals(clientSideReleaseKey)) { | |
| LinkedHashSet<String> clientSideReleaseKeys = Sets.newLinkedHashSet( | |
| Arrays.stream( | |
| clientSideReleaseKey.split(Pattern.quote(ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR))) | |
| .collect(Collectors.toList())); | |
| Map<String, Release> clientSideReleases = configService.findReleasesByReleaseKeys( | |
| clientSideReleaseKeys); | |
| //find history releases | |
| if (clientSideReleases != null) { | |
| //order by clientSideReleaseKeys | |
| List<Release> historyReleasesWithOrder = new ArrayList<>(); | |
| for (String item : clientSideReleaseKeys) { | |
| Release release = clientSideReleases.get(item); | |
| if (release != null) { | |
| historyReleasesWithOrder.add(release); | |
| } | |
| } | |
| Map<String, String> clientSideConfigurations = mergeReleaseConfigurations | |
| (historyReleasesWithOrder); | |
| List<ConfigurationChange> configurationChanges = incrementalSyncService.getConfigurationChanges( | |
| latestMergedReleaseKey, | |
| latestConfigurations, clientSideReleaseKey, clientSideConfigurations); | |
| apolloConfig.setConfigurationChanges(configurationChanges); | |
| apolloConfig.setConfigSyncType(ConfigSyncType.INCREMENTAL_SYNC.getValue()); | |
| Tracer.logEvent("Apollo.Config.Found", assembleKey(appId, appClusterNameLoaded, | |
| originalNamespace, dataCenter)); | |
| return apolloConfig; | |
| } | |
| } |
| 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 = clientSideConfigurations.get(commonKey); | ||
| String currentValue = latestReleaseConfigurations.get(commonKey); | ||
| if (com.google.common.base.Objects.equal(previousValue, currentValue)) { | ||
| continue; | ||
| } | ||
| changes.add( | ||
| new ConfigurationChange(commonKey, currentValue, "MODIFIED")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hard‑coded change‑type literals – use an enum or constant
Passing raw strings ("ADDED", "DELETED", "MODIFIED") makes the call‑sites brittle and error‑prone. ConfigurationChange already carries semantics; consider:
public enum ChangeType { ADDED, DELETED, MODIFIED }and overload the constructor (or add a static factory) accepting ChangeType. This prevents typos and enables IDE refactors.
| @Override | ||
| public boolean equals(Object obj) { | ||
| if (this == obj) { | ||
| return true; | ||
| } | ||
| if (!(obj instanceof ReleaseKeyPair)) { | ||
| return false; | ||
| } | ||
| ReleaseKeyPair that = (ReleaseKeyPair) obj; | ||
| return clientSideReleaseKey.equals(that.clientSideReleaseKey) && | ||
| latestMergedReleaseKey.equals(that.latestMergedReleaseKey); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return clientSideReleaseKey.hashCode() * 31 + latestMergedReleaseKey.hashCode(); | ||
| } |
There was a problem hiding this comment.
ReleaseKeyPair is not null‑safe
equals and hashCode dereference the release‑key fields directly, leading to NullPointerException if either key is ever null (e.g., client sends empty release key on first fetch).
- return clientSideReleaseKey.equals(that.clientSideReleaseKey) &&
- latestMergedReleaseKey.equals(that.latestMergedReleaseKey);
+ return java.util.Objects.equals(clientSideReleaseKey, that.clientSideReleaseKey) &&
+ java.util.Objects.equals(latestMergedReleaseKey, that.latestMergedReleaseKey);
...
- return clientSideReleaseKey.hashCode() * 31 + latestMergedReleaseKey.hashCode();
+ return java.util.Objects.hash(clientSideReleaseKey, latestMergedReleaseKey);Alternatively, validate non‑nullity in the constructor and throw IllegalArgumentException.
📝 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 boolean equals(Object obj) { | |
| if (this == obj) { | |
| return true; | |
| } | |
| if (!(obj instanceof ReleaseKeyPair)) { | |
| return false; | |
| } | |
| ReleaseKeyPair that = (ReleaseKeyPair) obj; | |
| return clientSideReleaseKey.equals(that.clientSideReleaseKey) && | |
| latestMergedReleaseKey.equals(that.latestMergedReleaseKey); | |
| } | |
| @Override | |
| public int hashCode() { | |
| return clientSideReleaseKey.hashCode() * 31 + latestMergedReleaseKey.hashCode(); | |
| } | |
| @Override | |
| public boolean equals(Object obj) { | |
| if (this == obj) { | |
| return true; | |
| } | |
| if (!(obj instanceof ReleaseKeyPair)) { | |
| return false; | |
| } | |
| ReleaseKeyPair that = (ReleaseKeyPair) obj; | |
| return java.util.Objects.equals(clientSideReleaseKey, that.clientSideReleaseKey) && | |
| java.util.Objects.equals(latestMergedReleaseKey, that.latestMergedReleaseKey); | |
| } | |
| @Override | |
| public int hashCode() { | |
| return java.util.Objects.hash(clientSideReleaseKey, latestMergedReleaseKey); | |
| } |
| HttpServletResponse someResponse = mock(HttpServletResponse.class); | ||
| ImmutableMap<String, Release> someReleaseMap = mock(ImmutableMap.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); |
There was a problem hiding this comment.
Do not mock final/immutable value classes – use real instances
ImmutableMap is final; Mockito cannot mock it without the inline‑mock‑maker plugin, and doing so hides real behaviour.
Build an actual map instead:
-ImmutableMap<String, Release> someReleaseMap = mock(ImmutableMap.class);
+ImmutableMap<String, Release> someReleaseMap =
+ ImmutableMap.<String, Release>builder()
+ .put(clientSideReleaseKey, someRelease)
+ .build();Then remove the stubbing of get(...), which is now real.
📝 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.
| HttpServletResponse someResponse = mock(HttpServletResponse.class); | |
| ImmutableMap<String, Release> someReleaseMap = mock(ImmutableMap.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); | |
| HttpServletResponse someResponse = mock(HttpServletResponse.class); | |
| ImmutableMap<String, Release> someReleaseMap = | |
| ImmutableMap.<String, Release>builder() | |
| .put(clientSideReleaseKey, someRelease) | |
| .build(); | |
| 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); |
| if (bizConfig.isConfigServiceIncrementalChangeEnabled()) { | ||
|
|
||
| } | ||
| //开启本地缓存 |
There was a problem hiding this comment.
Prefer English comments in code.
| //开启本地缓存 | |
| // enable local cache |
| @@ -69,13 +69,18 @@ public GrayReleaseRulesHolder grayReleaseRulesHolder() { | |||
|
|
|||
| @Bean | |||
| public ConfigService configService() { | |||
| //四种情况 | |||
| //开启增量更新 | |||
| if (bizConfig.isConfigServiceIncrementalChangeEnabled()) { | |||
There was a problem hiding this comment.
Has this part not been submitted yet?
| @@ -23,7 +23,10 @@ | |||
|
|
|||
| import com.google.common.base.Strings; | |||
|
|
|||
| import com.google.common.collect.ImmutableMap; | |||
There was a problem hiding this comment.
These changes seem unnecessary.
| * @param releaseKeys | ||
| * @return the ReleaseMap | ||
| */ | ||
| ImmutableMap<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys); |
There was a problem hiding this comment.
It's better to use the interface type in the interface, while the implementation can return an ImmutableMap.
| ImmutableMap<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys); | |
| Map<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys); |
| private static final String TRACER_EVENT_CACHE_GET = "ConfigCache.Get"; | ||
| private static final String TRACER_EVENT_CACHE_GET_ID = "ConfigCache.GetById"; | ||
|
|
||
| private final ReleaseService releaseService; | ||
| protected final ReleaseService releaseService; |
There was a problem hiding this comment.
Why change private to protected?
| try { | ||
| ImmutableMap<String, Long> releaseKeyMap = releaseKeyCache.getAll(releaseKeys); | ||
| if (CollectionUtils.isEmpty(releaseKeyMap)) { | ||
| return null; |
There was a problem hiding this comment.
For collection return values, prefer returning an empty collection like Collections.emptyMap() instead of null.
| return ImmutableMap.copyOf(releases); | ||
| } | ||
|
|
||
| } catch (ExecutionException e) { |
There was a problem hiding this comment.
Would it be better to return the exception to the caller instead of leaving it out?
|
|
||
| ### 3.2.14 config-service.incremental.change.enabled - 是否开启增量配置同步客户端 | ||
|
|
||
| > 适用于服务端2.4.0及以上版本 && 客户端2.3.0及以上版本 |
There was a problem hiding this comment.
| > 适用于服务端2.4.0及以上版本 && 客户端2.3.0及以上版本 | |
| > 适用于服务端2.5.0及以上版本 && Java客户端2.4.0及以上版本 |
| @@ -63,7 +63,7 @@ | |||
| <revision>2.5.0-SNAPSHOT</revision> | |||
| <java.version>1.8</java.version> | |||
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | |||
| <apollo-java.version>2.5.0-SNAPSHOT</apollo-java.version> | |||
| <apollo-java.version>2.4.0-SNAPSHOT</apollo-java.version> | |||
There was a problem hiding this comment.
| <apollo-java.version>2.4.0-SNAPSHOT</apollo-java.version> | |
| <apollo-java.version>2.5.0-SNAPSHOT</apollo-java.version> |
What's the purpose of this PR
之前的讨论在这个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
New Features
config-service.incremental.change.enabled), configurable via server settings.Documentation
Bug Fixes
Tests
Chores