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 incremental configuration synchronization to the Apollo Config Service. It adds new configuration flags, service interfaces, and implementations to enable clients to receive only configuration changes instead of full configurations. The update includes code, database schema, and documentation changes, along with comprehensive tests for the new incremental sync logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ConfigController
participant ConfigService
participant IncrementalSyncService
participant BizConfig
Client->>ConfigController: queryConfig(clientReleaseKeys)
ConfigController->>BizConfig: isConfigServiceIncrementalChangeEnabled()
alt Incremental Sync Enabled
ConfigController->>ConfigService: findReleasesByReleaseKeys(clientReleaseKeys)
ConfigService-->>ConfigController: Map<releaseKey, Release>
ConfigController->>IncrementalSyncService: getConfigurationChanges(latest, client)
IncrementalSyncService-->>ConfigController: List<ConfigurationChange>
ConfigController-->>Client: ApolloConfig (INCREMENTAL_SYNC, changes)
else
ConfigController-->>Client: ApolloConfig (FULL, latest 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Outdated
Show resolved
Hide resolved
...vice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java
Outdated
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)))
| //四种情况 | ||
| //开启增量更新 | ||
| 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.
...a/com/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncService.java
Outdated
Show resolved
Hide resolved
| 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); |
dosubot
bot
added
size:XXL
…autre/IncrementalSync
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java (2)
169-204: 🛠️ Refactor suggestionAdd a guard clause to optimize incremental sync path
The current implementation processes incremental sync logic even when the client sends default or empty release keys. This leads to unnecessary database/cache lookups and potentially misleading incremental payloads.
Implement the suggested guard clause before starting the incremental sync branch:
- if (bizConfig.isConfigServiceIncrementalChangeEnabled()) { + if (bizConfig.isConfigServiceIncrementalChangeEnabled() + && !"-1".equals(clientSideReleaseKey) + && !Strings.isNullOrEmpty(clientSideReleaseKey)) {This will prevent unnecessary processing and maintain intuitive wire format semantics.
192-195: 🛠️ Refactor suggestionMove business logic to the service layer
The calculation of configuration changes should be encapsulated within the service layer rather than coordinated from the controller.
Consider refactoring the incremental sync logic into the
IncrementalSyncService. The service should handle fetching data from the appropriate source (cache or database) based on configuration and cache the comparison results for reuse.This would simplify the controller code and improve separation of concerns.
🧹 Nitpick comments (2)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java (2)
180-187: Simplify release collection logic with stream APIThe current implementation using a for-loop to collect releases can be simplified using Java's stream API.
Consider using Java streams to simplify the code:
- List<Release> historyReleasesWithOrder = new ArrayList<>(); - for (String item : clientSideReleaseKeys) { - Release release = clientSideReleases.get(item); - if (release != null) { - historyReleasesWithOrder.add(release); - } - } + List<Release> historyReleasesWithOrder = clientSideReleaseKeys.stream() + .map(clientSideReleases::get) + .filter(Objects::nonNull) + .collect(Collectors.toList());
169-204: Consider caching incremental diff resultsFor frequently accessed configurations, calculating the diff each time may be inefficient, especially for larger configuration sets.
Consider implementing a caching mechanism for the calculated diffs between popular release key combinations. This could significantly improve performance for common configurations that multiple clients access.
You might want to implement an LRU cache with time-based expiration to store recent diff results based on (latestMergedReleaseKey, clientSideReleaseKey) pairs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (21)
logs/apollo-adminservice.logis excluded by!**/*.loglogs/apollo-adminservice.log.2023-12-05.0.gzis excluded by!**/*.gzlogs/apollo-adminservice.log.2023-12-06.0.gzis excluded by!**/*.gzlogs/apollo-adminservice.log.2023-12-07.0.gzis excluded by!**/*.gzlogs/apollo-adminservice.log.2023-12-08.0.gzis excluded by!**/*.gzlogs/apollo-adminservice.log.2024-11-21.0.gzis excluded by!**/*.gzlogs/apollo-assembly.log.2024-10-15.0.gzis excluded by!**/*.gzlogs/apollo-configService.log.2023-12-02.0.gzis excluded by!**/*.gzlogs/apollo-configService.log.2023-12-05.0.gzis excluded by!**/*.gzlogs/apollo-configService.log.2023-12-06.0.gzis excluded by!**/*.gzlogs/apollo-configService.log.2023-12-07.0.gzis excluded by!**/*.gzlogs/apollo-configService.log.2023-12-08.0.gzis excluded by!**/*.gzlogs/apollo-configService.log.2024-11-11.0.gzis excluded by!**/*.gzlogs/apollo-configservice.logis excluded by!**/*.loglogs/apollo-configservice.log.2024-11-21.0.gzis excluded by!**/*.gzlogs/apollo-portal.logis excluded by!**/*.loglogs/apollo-portal.log.2023-12-02.0.gzis excluded by!**/*.gzlogs/apollo-portal.log.2023-12-05.0.gzis excluded by!**/*.gzlogs/apollo-portal.log.2023-12-06.0.gzis excluded by!**/*.gzlogs/apollo-portal.log.2023-12-07.0.gzis excluded by!**/*.gzlogs/apollo-portal.log.2023-12-08.0.gzis excluded by!**/*.gz
📒 Files selected for processing (16)
.vscode/launch.json(1 hunks).vscode/settings.json(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(5 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/service/config/ConfigServiceWithCacheTest.java(7 hunks)logs/apollo-adminservice.log.2024-10-23.0381294484598291.tmp(1 hunks)logs/apollo-adminservice.log.2024-10-24.0410999228093125.tmp(1 hunks)logs/apollo-adminservice.log.2024-10-25.0433743820168333.tmp(1 hunks)logs/apollo-adminservice.log.2024-10-26.0434938286277791.tmp(1 hunks)logs/apollo-configService.log.2024-10-25.0433730397369416.tmp(1 hunks)logs/apollo-configService.log.2024-10-26.0434962061952791.tmp(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- .vscode/settings.json
- .vscode/launch.json
- logs/apollo-adminservice.log.2024-10-25.0433743820168333.tmp
- logs/apollo-configService.log.2024-10-25.0433730397369416.tmp
- logs/apollo-adminservice.log.2024-10-26.0434938286277791.tmp
- logs/apollo-adminservice.log.2024-10-24.0410999228093125.tmp
- logs/apollo-configService.log.2024-10-26.0434962061952791.tmp
- logs/apollo-adminservice.log.2024-10-23.0381294484598291.tmp
🚧 Files skipped from review as they are similar to previous changes (7)
- apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/ConfigServiceAutoConfiguration.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/IncrementalSyncService.java
- apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/DefaultConfigService.java
- apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java
- apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncService.java
- apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCacheTest.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)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: license
🔇 Additional comments (1)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java (1)
172-174:❓ Verification inconclusive
Potential parsing issue with release keys containing separator characters
The current implementation uses
splitwithConfigConsts.CLUSTER_NAMESPACE_SEPARATORto parse client-side release keys. If a release key itself contains this separator, it could lead to incorrect parsing.
🏁 Script executed:
#!/bin/bash # Check if any existing release keys contain the separator character SEPARATOR=$(grep -r "CLUSTER_NAMESPACE_SEPARATOR" --include="*.java" . | grep -o '"[^"]*"' | head -1 | tr -d '"') echo "Searching for release keys containing the separator: $SEPARATOR" rg "getReleaseKey\(\)" -A 3 --no-ignore | grep -v "//\|/\*\|\*/" | grep "return" | grep "$SEPARATOR"Length of output: 391
Let’s first confirm the actual separator value in the codebase:
🏁 Script executed:
#!/bin/bash # Locate the definition of CLUSTER_NAMESPACE_SEPARATOR and its literal value rg -n 'CLUSTER_NAMESPACE_SEPARATOR' --include='*.java'Length of output: 496
Let's correct the
rgflags and search for the constant definition:
🏁 Script executed:
#!/bin/bash # Find where CLUSTER_NAMESPACE_SEPARATOR is defined and its literal value rg -n 'CLUSTER_NAMESPACE_SEPARATOR' -g '*.java'Length of output: 6404
🏁 Script executed:
#!/bin/bash # Locate and inspect where CLUSTER_NAMESPACE_SEPARATOR is defined CONFIG_CONSTS_FILE=$(find . -type f -name 'ConfigConsts.java' | head -n1) echo "ConfigConsts file: $CONFIG_CONSTS_FILE" grep -n 'CLUSTER_NAMESPACE_SEPARATOR' "$CONFIG_CONSTS_FILE"Length of output: 199
Manual verification required: confirm separator value and release-key format
I couldn’t locate the
ConfigConsts.CLUSTER_NAMESPACE_SEPARATORdefinition in this module’s source—please:
- Verify the literal value of
CLUSTER_NAMESPACE_SEPARATOR(often in a sharedConfigConsts.javaunder the common module).- Ensure your release-key generator (e.g., MD5 hex digest) cannot produce the separator character.
- If release keys could include that character, consider:
- Choosing a delimiter that keys can never contain, or
- Escaping keys before joining/splitting.
...vice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigControllerTest.java (1)
547-549: Do not mock final/immutable value classes – use real instances
ImmutableMapisfinal; 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(...)on line 554, which is now implemented by the real object.
🧹 Nitpick comments (5)
.vscode/settings.json (1)
1-4: Question the inclusion of editor-specific settings. This commit adds a VS Code workspace configuration that sets Java null-analysis and auto-build options. Since these are IDE-specific preferences, evaluate whether they should be versioned for all contributors. If they’re intended for team-wide use, consider moving them to a documented “Editor Setup” guide or a shared.vscode/extensions.json/settings.jsoninside a recommended workspace. Otherwise, add.vscode/to.gitignoreto avoid polluting the repository with personal editor settings.apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigControllerTest.java (4)
564-568: Use consistent release key values in test expectationsThe method call to
incrementalSyncService.getConfigurationChangesuses hardcoded strings "someServerSideNewReleaseKey" that don't match variables defined in the test setup. This makes the test brittle and harder to understand.-when(incrementalSyncService.getConfigurationChanges("someServerSideNewReleaseKey", - gson.fromJson(anotherConfigurations, configurationTypeReference), - "someServerSideNewReleaseKey", - gson.fromJson(someConfigurations, configurationTypeReference))) +// Assign the release key to a variable first +String serverReleaseKey = "serverReleaseKey"; +when(incrementalSyncService.getConfigurationChanges(serverReleaseKey, + gson.fromJson(anotherConfigurations, configurationTypeReference), + clientSideReleaseKey, + gson.fromJson(someConfigurations, configurationTypeReference)))
613-614: Avoid redundant mock creationYou're creating a new mock for
somePublicReleasewhich is already declared as a class-level mock at line 84. This could lead to confusion in the test.-Release somePublicRelease = mock(Release.class);
651-652: Extract configuration JSON strings as constantsThese JSON strings are used in several places and would be more maintainable as constants at the class level. Additionally, the variable names could better describe their purpose.
Consider extracting these to class constants:
private static final String MERGED_SERVER_CONFIG_JSON = "{\"apollo.public.bar\": \"bar\",\"apollo.public.foo\": \"foo-override\"}"; private static final String MERGED_CLIENT_CONFIG_JSON = "{\"apollo.public.foo.client\": \"foo.override\"}";
657-661: Improve clarity with variables for empty stringsUsing empty strings for release keys in the mock setup doesn't clearly indicate the intent. Consider using meaningful variable names instead.
-when(incrementalSyncService.getConfigurationChanges("", - gson.fromJson(mergeServerSideConfigurations, configurationTypeReference), - "", - gson.fromJson(mergeClientSideConfigurations, configurationTypeReference))) +String mergedServerReleaseKey = ""; // Empty string for merged keys case +String mergedClientReleaseKey = ""; // Empty string for merged keys case +when(incrementalSyncService.getConfigurationChanges(mergedServerReleaseKey, + gson.fromJson(mergeServerSideConfigurations, configurationTypeReference), + mergedClientReleaseKey, + gson.fromJson(mergeClientSideConfigurations, configurationTypeReference)))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
.vscode/settings.json(1 hunks)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncService.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/ConfigServiceWithCacheTest.java(7 hunks)apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncServiceTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCacheTest.java
- apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncService.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: license
🔇 Additional comments (3)
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncServiceTest.java (2)
96-96: Check if this value assignment is intentional.In line 96,
newReleaseKeyis set to the same value assomeReleaseKey(assigned in line 91). Consider using a different value to make the test more robust, or add a comment explaining why they are intentionally the same.
1-278: Test coverage looks comprehensive and well-structured!The test class thoroughly covers the core functionality of
DefaultIncrementalSyncService, including:
- Cache hit/miss scenarios
- Handling of null values
- Configuration changes (add, modify, delete)
- Edge cases with empty configurations
The test methods follow a clear pattern and provide good coverage of the incremental sync feature.
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/ConfigControllerTest.java (1)
541-675: Comprehensive test coverage for incremental sync featureThe new tests thoroughly verify the behavior of the incremental synchronization feature in the
ConfigController, including:
- Basic incremental sync with cache hits
- Handling of missing client release keys
- Complex scenarios with public namespaces and app overrides
The tests cover the core functionality and edge cases effectively.
...vice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java
Outdated
Show resolved
Hide resolved
...vice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java
Outdated
Show resolved
Hide resolved
| * @param releaseKeys | ||
| * @return the ReleaseMap | ||
| */ | ||
| Map<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys) throws ExecutionException; |
There was a problem hiding this comment.
Why is ExecutionException declared in the interface if it's never thrown?
There was a problem hiding this comment.
ExecutionException will be thrown here
There was a problem hiding this comment.
Since only some implementations throw ExecutionException, it shouldn't be declared in the interface. I suggest removing it and handling exceptions within the relevant implementations.
| Map<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys) throws ExecutionException; | |
| Map<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys); |
|
|
||
| private final ReleaseService releaseService; | ||
| private final ReleaseMessageService releaseMessageService; | ||
| private final BizConfig bizConfig; | ||
| protected final BizConfig bizConfig; |
There was a problem hiding this comment.
Why changing private to protected?
...ain/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
The unit test failed with the error:
Error: ApolloSqlConverterAutoGeneratedTest.checkAutoGenerated:56->checkSqlList:63->checkMysqlDefaultList:104->doCheck:182 invalid sql file content, please run 'mvn compile -pl apollo-build-sql-converter -Psql-converter' to regenerated
...vice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java
Outdated
Show resolved
Hide resolved
| * @param releaseKeys | ||
| * @return the ReleaseMap | ||
| */ | ||
| Map<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys) throws ExecutionException; |
There was a problem hiding this comment.
Since only some implementations throw ExecutionException, it shouldn't be declared in the interface. I suggest removing it and handling exceptions within the relevant implementations.
| Map<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys) throws ExecutionException; | |
| Map<String, Release> findReleasesByReleaseKeys(Set<String> releaseKeys); |
...vice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java
Show resolved
Hide resolved
…configservice/controller/ConfigController.java Co-authored-by: Jason Song <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java (3)
189-189: Consider fallback condition for empty client-side releases.If
clientSideReleasesis empty (e.g., when client sends-1on first load), the current logic will create an emptyhistoryReleasesWithOrderlist and then an emptyclientSideConfigurationsmap, eventually falling through to full sync. This is correct behavior but could be optimized.Consider adding an early fallback check:
Map<String, Release> clientSideReleases = configService.findReleasesByReleaseKeys( clientSideReleaseKeys); + if (CollectionUtils.isEmpty(clientSideReleases)) { + // fallback to full sync when no historical releases found + } else { //find history releases - if (!CollectionUtils.isEmpty(clientSideReleases)) { //order by clientSideReleaseKeys List<Release> historyReleasesWithOrder = new ArrayList<>(); // ... rest of logic + }
169-175: Add guard clause to skip incremental sync for default release key.The current implementation will perform unnecessary work when the client sends the default sentinel value
"-1". This will result in a database/cache lookup and treat the entire namespace asADDED, 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.
169-211: Reduce try block scope to only cover operations that can throw ExecutionException.The try block is too large and catches
Exceptioninstead of the specificExecutionExceptionmentioned in past comments. Based on previous feedback, only theconfigService.findReleasesByReleaseKeyscall should be wrapped if we're specifically handlingExecutionException.Either narrow the try block scope to just the risky operation:
+ if (bizConfig.isConfigServiceIncrementalChangeEnabled()) { + LinkedHashSet<String> clientSideReleaseKeys = Sets.newLinkedHashSet( + Arrays.stream( + clientSideReleaseKey.split(Pattern.quote(ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR))) + .collect(Collectors.toList())); + + try { + Map<String, Release> clientSideReleases = configService.findReleasesByReleaseKeys( + clientSideReleaseKeys); + } catch (ExecutionException e) { + Tracer.logError("Failed to do incremental sync, fallback to full sync", e); + // fallback to full sync continues below + } + // rest of incremental sync logic... + }Or if you want to catch all errors in incremental sync, change to
catch (Throwable e)to be explicit about the broad exception handling.
🧹 Nitpick comments (1)
docs/en/deployment/distributed-deployment-guide.md (1)
1665-1677: Fix markdown formatting issue in blockquote.The blank line at 1675 inside the blockquote violates markdown formatting rules.
[static_analysis_hints]
Remove the blank line inside the blockquote:> `config-service.cache.enabled` configuration adjustment requires a restart of the config service to take effect -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
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/ConfigService.java(2 hunks)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java(7 hunks)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/DefaultConfigService.java(2 hunks)apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCacheTest.java(4 hunks)apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncServiceTest.java(1 hunks)docs/en/deployment/distributed-deployment-guide.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- 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
- apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/DefaultConfigService.java
- apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCacheTest.java
- apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncServiceTest.java
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/en/deployment/distributed-deployment-guide.md
1675-1675: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
🔇 Additional comments (4)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/ConfigController.java (2)
19-19: New dependencies and imports look appropriate.The addition of
IncrementalSyncService,BizConfig, and related imports are correctly structured for the incremental sync feature implementation.Also applies to: 25-25, 31-32, 37-37, 40-41, 52-54, 69-69, 74-74, 82-82, 87-87, 89-89, 94-94
153-154: Variable renaming improves code clarity.The rename from
mergedReleaseKeytolatestMergedReleaseKeybetter reflects the variable's purpose in the context of incremental sync logic.docs/en/deployment/distributed-deployment-guide.md (2)
1666-1677: Documentation content is comprehensive and accurate.The documentation correctly describes the incremental sync feature toggle, version requirements, performance considerations, and important configuration notes. The content aligns well with the implementation in the ConfigController.
1636-1636: Verify section numbering consistency.There appear to be multiple sections numbered 3.2.14 in the document (lines 1636 and 1665). This creates ambiguous references and should be corrected.
Run the following script to check for duplicate section numbers:
#!/bin/bash # Description: Check for duplicate section numbering in the documentation # Expected: No duplicate section numbers should exist echo "Checking for duplicate section numbers..." grep -n "^### 3\.2\.[0-9]\+" docs/en/deployment/distributed-deployment-guide.md | sort -k2 -t: | uniq -f1 -D echo -e "\nAll section 3.2.x headers:" grep -n "^### 3\.2\.[0-9]\+" docs/en/deployment/distributed-deployment-guide.mdAlso applies to: 1665-1665
...m/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncServiceTest.java
Outdated
Show resolved
Hide resolved
...m/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncServiceTest.java
Outdated
Show resolved
Hide resolved
changes/changes-2.4.0.md
Outdated
| @@ -29,5 +29,8 @@ Apollo 2.4.0 | |||
| * [Feature: support to assign users management authority by the cluster (modify, publish)](https://github.com/apolloconfig/apollo/pull/5302) | |||
| * [Feature: notification by email when releasing by OpenApi also](https://github.com/apolloconfig/apollo/pull/5324) | |||
|
|
|||
| * [Feature support incremental configuration synchronization client](https://github.com/apolloconfig/apollo/pull/5288) | |||
There was a problem hiding this comment.
Please add this change to CHANGES.md, not here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncServiceTest.java (2)
19-21: Remove unused imports.Several imports are not used in the test class and should be removed for cleaner code.
-import com.ctrip.framework.apollo.biz.entity.Release; -import com.ctrip.framework.apollo.biz.service.ReleaseMessageService; -import com.ctrip.framework.apollo.biz.service.ReleaseService;-import org.mockito.Mock;Also applies to: 33-33
87-88: Remove unnecessary empty lines.There are several unnecessary empty lines that should be removed for cleaner formatting.
- -Also applies to: 207-208, 252-253
🧹 Nitpick comments (2)
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncServiceTest.java (2)
47-47: Consider using standard JUnit runner.Since no mocks are used in this test class,
MockitoJUnitRunneris unnecessary. Consider using the standard JUnit runner or removing the@RunWithannotation entirely.-@RunWith(MockitoJUnitRunner.class) public class DefaultIncrementalSyncServiceTest {
39-42: Standardize JUnit assertion imports.The test mixes JUnit 4 and JUnit 5 assertion imports, which creates inconsistency. Consider standardizing on one version.
-import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertSame;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncServiceTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncServiceTest.java (2)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/utils/ReleaseMessageKeyGenerator.java (1)
ReleaseMessageKeyGenerator(29-50)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncService.java (2)
DefaultIncrementalSyncService(34-138)ReleaseKeyPair(108-135)
🔇 Additional comments (4)
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/DefaultIncrementalSyncServiceTest.java (4)
63-86: Good test setup with proper cache access.The setup method correctly initializes the service and uses reflection appropriately to access the private cache field for testing purposes. The comprehensive test data setup supports various test scenarios.
89-99: Well-implemented cache access helper.The reflection-based approach to access the private cache field is appropriate for testing purposes, with proper exception handling and justified suppression of unchecked cast warnings.
101-158: Comprehensive cache behavior testing.The cache-related tests effectively verify:
- Cache hits return the same object instance
- Cache misses with different keys work correctly
- Null configuration handling is properly cached
The use of
assertSameto verify object identity for cache hits is particularly good.
160-251: Thorough coverage of configuration change scenarios.The test methods comprehensively cover all configuration change types (ADD, MODIFY, DELETE) and edge cases with null configurations. The test logic is correct and assertions properly verify the expected behavior.
|
@jackie-coming Is this PR ready for another review? The update to CHANGES.md still appears to be missing. |
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) to toggle incremental sync, defaulting to disabled.Documentation
Bug Fixes
Tests
Chores