-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix:RefreshAdminServerAddressTask supports dynamic configuration of time interval #5248
base: master
Are you sure you want to change the base?
Conversation
Warning Rate limit exceeded@youngzil has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/PortalConfig.java (2)
199-202
: Consider increasing the minimum allowed intervalThe method successfully implements configurable normal refresh intervals, which aligns with the PR objectives. However, the minimum allowed value of 5 milliseconds seems extremely low for a production environment.
Consider increasing the minimum value to a more reasonable duration, such as 1000 milliseconds (1 second) or higher, to prevent potential system stress from overly frequent refreshes.
- return checkInt(interval, 5, Integer.MAX_VALUE, DEFAULT_REFRESH_ADMIN_SERVER_ADDRESS_TASK_NORMAL_INTERVAL_IN_MILLI); + return checkInt(interval, 1000, Integer.MAX_VALUE, DEFAULT_REFRESH_ADMIN_SERVER_ADDRESS_TASK_NORMAL_INTERVAL_IN_MILLI);
204-207
: Consider increasing the minimum allowed interval for consistencyThe method successfully implements configurable offline refresh intervals, aligning with the PR objectives. However, similar to the normal refresh interval, the minimum allowed value of 5 milliseconds is extremely low.
For consistency with the previous suggestion and to prevent potential system stress, consider increasing the minimum value to 1000 milliseconds (1 second) or higher.
- return checkInt(interval, 5, Integer.MAX_VALUE, DEFAULT_REFRESH_ADMIN_SERVER_ADDRESS_TASK_OFFLINE_INTERVAL_IN_MILLI); + return checkInt(interval, 1000, Integer.MAX_VALUE, DEFAULT_REFRESH_ADMIN_SERVER_ADDRESS_TASK_OFFLINE_INTERVAL_IN_MILLI);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AdminServiceAddressLocator.java (3 hunks)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/PortalConfig.java (3 hunks)
🧰 Additional context used
🔇 Additional comments (9)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/PortalConfig.java (3)
46-47
: LGTM: Default refresh interval constants addedThe addition of these constants provides clear default values for the normal and offline refresh intervals. The chosen values (5 minutes for normal and 10 seconds for offline) seem reasonable for their respective scenarios.
321-326
: LGTM: Helpful integer validation method addedThe
checkInt()
method is a well-implemented utility for validating integer configuration values. It promotes code reuse and ensures consistent behavior across different configuration settings.
Line range hint
46-326
: Summary: Successful implementation of configurable refresh intervalsThe changes in this file successfully implement configurable refresh intervals for the
RefreshAdminServerAddressTask
, addressing the objectives outlined in the PR and the linked issue #5245. The implementation allows for dynamic configuration of both normal and offline refresh intervals, which should improve the handling of admin service addresses in multi-AZ disaster recovery scenarios.Key points:
- Default constants are well-defined.
- New methods for retrieving configurable intervals are implemented correctly.
- A helper method for integer validation promotes code reuse and consistency.
Minor suggestions have been made to increase the minimum allowed interval values to prevent potential system stress from overly frequent refreshes. Overall, the implementation is solid and achieves the desired functionality.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AdminServiceAddressLocator.java (6)
19-19
: ImportPortalConfig
for dynamic configuration.The import statement correctly adds
PortalConfig
to enable dynamic refresh intervals.
58-58
: AddportalConfig
as a final member variable.Introducing
portalConfig
as afinal
member variable ensures it is immutable after initialization.
64-65
: Update constructor to includePortalConfig
.The constructor now accepts
PortalConfig portalConfig
, allowing the class to access dynamic configurations.
70-70
: AssignportalConfig
within the constructor.Assigning
this.portalConfig = portalConfig;
correctly initializes the member variable.
110-113
: Handle scheduling failures appropriately.If both
refreshSuccess
andcurrentEnvRefreshResult
arefalse
, the task schedules using the offline interval. Ensure that this logic aligns with the desired retry strategy when refreshes fail.Please confirm that the retry mechanism after a failed refresh operates as intended.
110-113
: Use dynamic refresh intervals fromPortalConfig
.Replacing hardcoded constants with dynamic values from
portalConfig
enhances configurability. Ensure thatrefreshAdminServerAddressTaskNormalIntervalInMilli()
andrefreshAdminServerAddressTaskOfflineIntervalInMilli()
provide valid intervals.To confirm that these methods return appropriate values, consider running the following script:
✅ Verification successful
Verification Successful: Dynamic Refresh Intervals Confirmed
The methods
refreshAdminServerAddressTaskNormalIntervalInMilli()
andrefreshAdminServerAddressTaskOfflineIntervalInMilli()
are correctly implemented to return positive integers, ensuring dynamic configurability as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the refresh intervals are correctly configured and greater than zero. # Expect: The methods should return positive integers. # Find method implementations and check default values. rg --type java 'public.*refreshAdminServerAddressTaskNormalIntervalInMilli\(\)' -A 5 rg --type java 'public.*refreshAdminServerAddressTaskOfflineIntervalInMilli\(\)' -A 5Length of output: 2171
…_interval_support_config
@@ -193,6 +196,16 @@ public String portalAddress() { | |||
return getValue("apollo.portal.address"); | |||
} | |||
|
|||
public int refreshAdminServerAddressTaskNormalIntervalInMilli() { | |||
int interval = getIntProperty("refresh.admin.server.address.task.normal.interval.ms", DEFAULT_REFRESH_ADMIN_SERVER_ADDRESS_TASK_NORMAL_INTERVAL_IN_MILLI); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to let the user configure the interval in seconds instead of milliseconds.
…ime interval
What's the purpose of this PR
XXXXX
Which issue(s) this PR fixes:
Fixes #5245
Brief changelog
XXXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean test
to make sure this pull request doesn't break anything.CHANGES
log.Summary by CodeRabbit
New Features
Bug Fixes
Refactor