perf: speed up when startup meet timeout #64
Conversation
WalkthroughThe updates enhance the Apollo Java client by optimizing the startup process, particularly when encountering timeouts. Key changes include improvements in HTTP request handling, task submission control, and configuration updates, significantly reducing the time taken to load namespaces under error conditions. Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant ConfigServiceLocator
participant ExecutorService
participant ConfigUtil
participant RemoteConfigService
Client->>ConfigServiceLocator: Initialize
ConfigServiceLocator->>ConfigUtil: Get timeout settings
ConfigServiceLocator->>ExecutorService: Submit update task
ExecutorService->>RemoteConfigService: Fetch config services
RemoteConfigService-->>ExecutorService: Response
ExecutorService-->>ConfigServiceLocator: Update config services
ConfigServiceLocator-->>Client: Return config services
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration 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: 0
Out of diff range and nitpick comments (1)
apollo-client/src/test/java/com/ctrip/framework/apollo/BaseIntegrationTest.java (1)
Line range hint
295-295: Use SSL for socket communication to enhance security.- ServerSocket socket = new ServerSocket(0); + SSLServerSocketFactory ssf = (SSLServerSocketFactory) SSLServerSocketFactory.getDefault(); + ServerSocket socket = ssf.createServerSocket(0);
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java (5 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/BaseIntegrationTest.java (6 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/MockedConfigService.java (4 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/integration/ConfigIntegrationTest.java (4 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/integration/ConfigServiceTimeoutIntegrationTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/ConfigServiceLocatorTest.java (3 hunks)
Additional comments not posted (13)
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/ConfigServiceLocatorTest.java (4)
19-21: Ensure the updated imports are utilized within the test cases.
25-25: EnsureApolloConfigExceptionis used appropriately in the test cases where exceptions are expected.
27-27: Transition to JUnit 5 annotations enhances the test's compatibility with newer testing paradigms.
78-87: Validates the system's behavior under no available config service scenario, aligning with the PR's objective to handle service downtime effectively.apollo-client/src/test/java/com/ctrip/framework/apollo/MockedConfigService.java (2)
42-42: Addition ofMETA_SERVER_PATHconstant improves maintainability by avoiding hard-coded strings across methods.
127-141: This method effectively simulates service delays, crucial for testing timeout scenarios as described in the PR objectives.apollo-client/src/test/java/com/ctrip/framework/apollo/BaseIntegrationTest.java (3)
19-33: Updated imports reflect the expanded scope of the base integration tests, including new utilities and services.
51-51: Changing the logger to a protected instance variable allows derived classes to utilize the same logger, facilitating better logging management across tests.
Line range hint
67-212: Addition of methods and fields for handling configurations and local cache files directly supports the PR's goal to enhance testing capabilities around configuration loading and error scenarios.apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java (2)
25-25: The addition ofCollectionsimport supports the initialization of empty lists, aligning with the fail-fast mechanism when no config services are available.
Line range hint
74-209: The modifications ininitializeandupdateConfigServicesmethods enhance the system's resilience by implementing a fail-fast mechanism and improving the refresh logic of config services.apollo-client/src/test/java/com/ctrip/framework/apollo/integration/ConfigServiceTimeoutIntegrationTest.java (1)
34-255: The new tests effectively simulate and handle timeout scenarios, ensuring the system behaves as expected under such conditions. This aligns well with the PR's goal to improve startup performance during service downtimes.apollo-client/src/test/java/com/ctrip/framework/apollo/integration/ConfigIntegrationTest.java (1)
49-51: The change to final fields forsomeReleaseKeyandsomeOtherNamespaceensures immutability, which is beneficial for the consistency of test values across different test methods.
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- CHANGES.md (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/integration/ConfigServiceTimeoutIntegrationTest.java (1 hunks)
Additional Context Used
LanguageTool (1)
CHANGES.md (1)
Near line 12: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...pull/61) * [perf: speed up when startup meet timeout](https://github.com/apolloconfi...
Additional comments not posted (4)
apollo-client/src/test/java/com/ctrip/framework/apollo/integration/ConfigServiceTimeoutIntegrationTest.java (4)
50-73: This test method correctly simulates and verifies the timeout scenario for fetching multiple namespaces.
75-99: This test method effectively uses a backup configuration for the default namespace in a timeout scenario, aligning with the PR's fast-fail objectives.
101-160: This test method comprehensively tests the backup configurations for multiple namespaces in a timeout scenario, ensuring correct behavior even when one namespace lacks a backup.
163-271: This test method effectively simulates a timeout scenario followed by a recovery, testing the system's ability to handle configuration updates post-recovery.
CHANGES.md
Outdated
| * [perf: speed up the first loading of namespace when startup meet 404](https://github.com/apolloconfig/apollo-java/pull/61) | ||
| * [perf: speed up when startup meet timeout](https://github.com/apolloconfig/apollo-java/pull/64) | ||
|
|
There was a problem hiding this comment.
Improve readability by adding blank lines around the list.
+
* [perf: speed up when startup meets timeout](https://github.com/apolloconfig/apollo-java/pull/64)
+ 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.
| * [perf: speed up the first loading of namespace when startup meet 404](https://github.com/apolloconfig/apollo-java/pull/61) | |
| * [perf: speed up when startup meet timeout](https://github.com/apolloconfig/apollo-java/pull/64) | |
| * [perf: speed up the first loading of namespace when startup meet 404](https://github.com/apolloconfig/apollo-java/pull/61) | |
| * [perf: speed up when startup meets timeout](https://github.com/apolloconfig/apollo-java/pull/64) | |
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java
Outdated
Show resolved
Hide resolved
…config service" This reverts commit a8e2070.
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java
Outdated
Show resolved
Hide resolved
| private RemoteConfigLongPollService remoteConfigLongPollService; | ||
|
|
||
| @BeforeEach | ||
| public void setUp() throws Exception { |
There was a problem hiding this comment.
move those code to parent class
| } | ||
|
|
||
| @AfterEach | ||
| public void tearDown() throws Exception { | ||
| // get the instance will trigger long poll task execute, so move it from setup to tearDown | ||
| RemoteConfigLongPollService remoteConfigLongPollService | ||
| = ApolloInjector.getInstance(RemoteConfigLongPollService.class); |
There was a problem hiding this comment.
ApolloInjector.getInstance(RemoteConfigLongPollService.class); will trigger context load, so move it to tearDown method
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
apollo-client/src/test/java/com/ctrip/framework/apollo/BaseIntegrationTest.java (1)
Line range hint
293-293: The methodfindFreePortusesServerSocketwithout encryption, which could expose the traffic to potential eavesdropping. While this might be acceptable in a testing environment, consider usingSSLSocketfor enhanced security, especially if this pattern is replicated in production code.- ServerSocket socket = new ServerSocket(0); + SSLServerSocketFactory ssf = (SSLServerSocketFactory) SSLServerSocketFactory.getDefault(); + ServerSocket socket = ssf.createServerSocket(0);
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java (2 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/BaseIntegrationTest.java (6 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/MockedConfigService.java (3 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/integration/ConfigIntegrationTest.java (7 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/ConfigServiceLocatorTest.java (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/MockedConfigService.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/integration/ConfigIntegrationTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/ConfigServiceLocatorTest.java
Additional comments not posted (6)
apollo-client/src/test/java/com/ctrip/framework/apollo/BaseIntegrationTest.java (6)
19-32: The addition of multiple imports reflects the expanded functionality of theBaseIntegrationTestclass, particularly in handling remote configurations and file operations. This aligns with the PR's objective to enhance startup performance and manage configurations more effectively.
50-50: Changing the logger fromprivate static finaltoprotected finalincreases the flexibility for subclasses to use the logger. This change is beneficial for extended classes that might want to log additional details in different contexts.
65-65: The introduction ofdefaultNamespaceas a protected final field is a good practice, ensuring that it is immutable and accessible within subclasses, which could be beneficial for tests that rely on the default namespace configuration.
124-128: The handling of theconfigDirdirectory is crucial for managing local cache files. The logic ensures that the directory is always fresh at the start of each test by deleting and recreating it. This is essential for tests that depend on the state of the filesystem and need a consistent starting environment.
134-138: Moving the instantiation and stopping ofRemoteConfigLongPollServicetotearDownhelps in ensuring that the long poll service does not interfere with other tests by running in the background. This is a good practice in test setup to avoid side effects between tests.
180-192: The methodcreateLocalCachePropertyFileis well-implemented for creating property files in the local cache. It handles file operations with proper exception management, which is crucial for reliability in file I/O operations. The use ofFileOutputStreamin a try-with-resources statement ensures that the file handle is properly closed in case of an exception.
| private void recursiveDelete(File file) { | ||
| if (!file.exists()) { | ||
| return; | ||
| } | ||
| if (file.isDirectory()) { | ||
| for (File f : file.listFiles()) { | ||
| recursiveDelete(f); | ||
| } | ||
| } | ||
| try { | ||
| Files.deleteIfExists(file.toPath()); | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); | ||
| } |
There was a problem hiding this comment.
The recursiveDelete method is a utility function to clean up directories. It's a necessary part of ensuring that tests do not leave residual data that could affect subsequent tests. However, the use of e.printStackTrace(); is not recommended as it clutters the test output. Consider using a logger to handle exceptions.
- e.printStackTrace();
+ log.error("Failed to delete file: " + file.getPath(), e);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.
| private void recursiveDelete(File file) { | |
| if (!file.exists()) { | |
| return; | |
| } | |
| if (file.isDirectory()) { | |
| for (File f : file.listFiles()) { | |
| recursiveDelete(f); | |
| } | |
| } | |
| try { | |
| Files.deleteIfExists(file.toPath()); | |
| } catch (IOException e) { | |
| e.printStackTrace(); | |
| } | |
| private void recursiveDelete(File file) { | |
| if (!file.exists()) { | |
| return; | |
| } | |
| if (file.isDirectory()) { | |
| for (File f : file.listFiles()) { | |
| recursiveDelete(f); | |
| } | |
| } | |
| try { | |
| Files.deleteIfExists(file.toPath()); | |
| } catch (IOException e) { | |
| log.error("Failed to delete file: " + file.getPath(), e); | |
| } |
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (1)
47-53: The addition of new configurations for discovery QPS, connect timeout, and read timeout, along with their customization options, is a positive change. It allows for more flexible configuration and can help in achieving the PR's goal of speeding up startup times under failure conditions.Consider adding documentation in the user guide or wiki to help users understand how to use these new settings effectively.
Also applies to: 237-270
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java (5 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (2 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/ConfigServiceLocatorTest.java (2 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java (6 hunks)
Files skipped from review as they are similar to previous changes (2)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/ConfigServiceLocatorTest.java
Additional comments not posted (1)
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java (1)
134-134: Removal ofstopLongPollingRefresh()aligns with the PR's goal to enhance performance by not unnecessarily stopping long polling.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
CHANGES.md (2)
Line range hint
1-1: Ensure headings are surrounded by blank lines for better readability.+ Changes by Version ================== +Tools
Markdownlint
15-15: null
Files should end with a single newline character
Line range hint
8-8: Improve readability by adding blank lines around the list.+ * [perf: speed up when startup meets timeout](https://github.com/apolloconfig/apollo-java/pull/64) +Tools
Markdownlint
15-15: null
Files should end with a single newline character
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- CHANGES.md (1 hunks)
Additional context used
Markdownlint
CHANGES.md
1-1: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
8-8: null
Lists should be surrounded by blank lines
15-15: null
Files should end with a single newline character
| @@ -9,6 +9,7 @@ Apollo Java 2.3.0 | |||
| * [Implement parsing time based on pattern for @ApolloJsonValue](https://github.com/apolloconfig/apollo-java/pull/53) | |||
| * [Enhance to load mocked properties from apollo.cache-dir](https://github.com/apolloconfig/apollo-java/pull/58) | |||
| * [perf: speed up the first loading of namespace when startup meet 404](https://github.com/apolloconfig/apollo-java/pull/61) | |||
| * [perf: speed up when startup meets timeout](https://github.com/apolloconfig/apollo-java/pull/64) | |||
|
|
|||
| ------------------ | |||
| All issues and pull requests are [here](https://github.com/apolloconfig/apollo-java/milestone/3?closed=1) | |||
There was a problem hiding this comment.
Ensure the file ends with a single newline character.
+ 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.
| All issues and pull requests are [here](https://github.com/apolloconfig/apollo-java/milestone/3?closed=1) | |
| All issues and pull requests are [here](https://github.com/apolloconfig/apollo-java/milestone/3?closed=1) | |
Tools
Markdownlint
15-15: null
Files should end with a single newline character
What's the purpose of this PR
Speed up when startup meet timeout, i.e when meet meta service and config service crash.
Which issue(s) this PR fixes:
Fixes #60
Brief changelog
There is a synchronized lock in
apollo-java/apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java
Line 193 in 3f0979d
It will block even use multiple thread.
When try to getConfigServices, if cache is empty, method
updateConfigServiceswill be invokeapollo-java/apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java
Lines 162 to 167 in 3f0979d
In this pr, the code change to fail quickly, so it can speed up when meta service and config service crash(that will cause timeout for client).
Maybe it better to let daemon thread to invoke method
updateConfigServices,another thread just use the result in memory cache,
If there is no available config service, throw exception quickly.
After change, load 10 namespaces timeusage
from 65929 ms to 5150ms when all of them meet timeout
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
Bug Fixes
Tests
Documentation