perf: speed up the first loading of namespace when startup meet 404 #61
Conversation
WalkthroughThe recent changes in the Changes
Assessment against linked issues
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 (
|
Anilople
changed the title
![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
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepository.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (1 hunks)
Files skipped from review due to trivial changes (2)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java (1 hunks)
Additional comments not posted (4)
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java (4)
Line range hint
58-104: Setup method is correctly implemented and provides a controlled environment for the tests.
Line range hint
106-109: Teardown method correctly cleans up after tests to prevent side effects.
Line range hint
273-323: ThetestLongPollingRefreshmethod effectively tests the long polling refresh functionality and correctly verifies that change listeners are notified.
Line range hint
325-354: ThetestAssembleQueryConfigUrlmethod correctly tests the URL assembly logic, ensuring that all parameters are included and properly encoded.
| assertEquals(ConfigSourceType.LOCAL, localFileConfigRepository.getSourceType()); | ||
| localFileConfigRepository.trySync(); |
There was a problem hiding this comment.
The call to trySync() in the test might not reflect the actual behavior anymore.
Given the changes in the main class where trySync() calls are commented out, this test might fail or not behave as expected because it assumes trySync() is operational. Consider adjusting the test to reflect the new logic, or if trySync() is conditionally called, ensure this condition is replicated in the test setup.
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (1)
apollo-client/src/test/java/com/ctrip/framework/apollo/BaseIntegrationTest.java (1)
Line range hint
220-220: Use encrypted sockets to enhance security.- ServerSocket socket = new ServerSocket(0); + SSLServerSocket socket = (SSLServerSocket) SSLServerSocketFactory.getDefault().createServerSocket(0);
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- apollo-client/pom.xml (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/BaseIntegrationTest.java (3 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/MockedConfigService.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/integration/ConfigIntegrationTest.java (23 hunks)
- apollo-client/src/test/resources/log4j2.xml (1 hunks)
- pom.xml (1 hunks)
Files skipped from review due to trivial changes (2)
- apollo-client/pom.xml
- pom.xml
Additional comments not posted (2)
apollo-client/src/test/resources/log4j2.xml (1)
32-32: Set logging level for "org.mockserver.log" toWARNto reduce verbosity in test logs.apollo-client/src/test/java/com/ctrip/framework/apollo/integration/ConfigIntegrationTest.java (1)
Line range hint
19-486: The integration tests inConfigIntegrationTestare comprehensive and effectively use mocks to isolate test scenarios.
apollo-client/src/test/java/com/ctrip/framework/apollo/MockedConfigService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- apollo-client/src/test/java/com/ctrip/framework/apollo/MockedConfigService.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- apollo-client/src/test/java/com/ctrip/framework/apollo/MockedConfigService.java
There was a problem hiding this comment.
Actionable comments posted: 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/AbstractConfigRepository.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigRepository.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfig.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepository.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
Additional comments not posted (3)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigRepository.java (1)
57-60: Addition ofinitialize()method in the interface standardizes initialization across different repository types.apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigRepository.java (1)
77-80: Implementation ofinitialize()method correctly invokessync()to ensure repository synchronization during initialization.apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfig.java (1)
74-74: Modification to callm_configRepository.initialize()aligns with the new initialization protocol and ensures robust error handling.
Anilople
changed the title
Anilople
changed the title
Anilople
changed the title
...client/src/test/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepositoryTest.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigRepository.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public void initialize() { | ||
| this.sync(); |
There was a problem hiding this comment.
Is it better to call trySync?
There was a problem hiding this comment.
Change to trySync here will slower startup. Let me try to illustrate it.
1. everything from the rate limiter in RemoteConfigRepository
If we do not wait for 2 consecutive calls to method sync, the rateLimter in RemoteConfigRepository will block current thread 500ms on the second call because code m_loadConfigRateLimiter.tryAcquire(5, TimeUnit.SECONDS), that is why so slower when startup.
sequenceDiagram
RemoteConfigRepository ->> RemoteConfigRepository: sync
RemoteConfigRepository ->> RemoteConfigRepository: loadApolloConfig
RemoteConfigRepository ->> m_loadConfigRateLimiter: tryAcquire(5, TimeUnit.SECONDS)
alt if Acquire success
m_loadConfigRateLimiter -->> RemoteConfigRepository: return immediately
else Acquire fail
m_loadConfigRateLimiter -->> RemoteConfigRepository: block current thread 500ms
end
2. how to solve it?
There are 2 solution to speed up.
solution 1: forbid 2 consecutive calls to method sync,
solution 2: use another method on m_loadConfigRateLimiter let it won't block current thread 500ms .
3. Back to talk about method initialize in DefaultConfig.
According to the code
m_configRepository.getConfig() will invoke method sync.
When we change to code
private void initialize() {
try {
m_configRepository.initialize();
updateConfig(m_configRepository.getConfig(), m_configRepository.getSourceType());
} catch (Throwable ex) {
Tracer.logError(ex);
logger.warn("Init Apollo Local Config failed - namespace: {}, reason: {}.",
m_namespace, ExceptionUtil.getDetailMessage(ex));
} finally {m_configRepository.initialize(); and m_configRepository.getConfig() alway invoke method sync,
i.e 2 consecutive calls to method sync, that will let startup slower.
But the different in m_configRepository.initialize();, if we use sync instead of trySync,
the exception will throw quickly, so updateConfig(m_configRepository.getConfig(), m_configRepository.getSourceType()); won't execute,
i.e choose solution 1: forbid 2 consecutive calls to method sync to speed up.
That's why use sync instead of trySync here.
Maybe there is better solution here?
There was a problem hiding this comment.
I agree with you. Let's keep it as it is.
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (3)
CHANGES.md (3)
Line range hint
1-1: Ensure headings are surrounded by blank lines for proper markdown formatting.
8-8: Lists should be surrounded by blank lines to adhere to markdown best practices.
14-14: Ensure the file ends with a single newline character.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- CHANGES.md (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigRepository.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepositoryTest.java (1 hunks)
Additional comments not posted (2)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigRepository.java (1)
56-60: The addition of a defaultinitialize()method is a good practice. It ensures backward compatibility with existing implementations of this interface.apollo-client/src/test/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepositoryTest.java (1)
189-190: The new assertions intestOnRepositoryChangecorrectly verify the source type before and after initialization, ensuring theinitialize()method functions as expected.
What's the purpose of this PR
Speed up the user's startup time,
by decrease the number of http requests.
Which issue(s) this PR fixes:
Fixes #60
Brief changelog
before change, it use 11494 ms to load 10 namespace when meet 404,
before-change.log
after delete 2 http requests when context init, it use 1490 ms.
after-change.log
i.e from 11494 ms to 1490 ms.
the code in
apollo-java/apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfig.java
Line 69 in bcc4053
will ensure pull at least one time from remote when startup.
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