-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
chore: Update feature flag refresh interval to 29 minutes #38248
base: release
Are you sure you want to change the base?
Conversation
WalkthroughThe pull request modifies the Changes
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
Documentation and Community
|
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
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCEImpl.java (1)
37-37
: Consider environment-specific configurationWhile the 29-minute refresh interval benefits testing, it could increase load in production. Consider making this value configurable per environment.
- private static final long TENANT_FEATURES_CACHE_TIME_MIN = 29; + @Value("${feature.flag.cache.refresh.interval:29}") + private long tenantFeaturesCacheTimeMin;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCEImpl.java
(1 hunks)
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCEImpl.java (1)
37-37
: Verify cron execution interval to prevent race conditions
The comment mentions avoiding race conditions by keeping refresh rate lower than cron execution interval. Please verify that the new 29-minute cache time is still safely below the cron schedule defined in ScheduledTaskCEImpl
.
✅ Verification successful
Cache time of 29 minutes is safely below scheduled task interval
Based on the findings, the scheduled task runs with a fixed rate of 30 minutes (fixedRate = 30 * 60 * 1000
). The new cache time of 29 minutes ensures the cache refresh happens before the next scheduled execution, preventing any race conditions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for cron schedule configuration in ScheduledTaskCEImpl
rg -A 5 "cron" app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ScheduledTaskCEImpl.java
Length of output: 113
Script:
#!/bin/bash
# Let's try searching for cron or scheduled task related configurations more broadly
rg -i "scheduled|cron|fixedrate|fixeddelay" app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ScheduledTaskCEImpl.java
# Also search for any Spring scheduling annotations
rg "@Scheduled" app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ScheduledTaskCEImpl.java
# Let's also check if there's a different implementation class
fd -e java ScheduledTask
Length of output: 2212
Script:
#!/bin/bash
# Let's check the feature flag migration method to understand how it's scheduled
rg -A 10 "getUpdatedFlagsWithPendingMigration" app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ScheduledTaskCEImpl.java
# Also check if there are any other scheduled tasks that might interact with feature flags
rg -A 5 "@Scheduled.*feature" app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/
Length of output: 249
/build-deploy-preview skip-tests=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12407465720. |
Deploy-Preview-URL: https://ce-38248.dp.appsmith.com |
Description
This PR increases the frequency of feature flag cache update so that we can test features faster
Fixes #
Issue Number
or
Fixes
Issue URL
Warning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12409924411
Commit: 4669afc
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
Thu, 19 Dec 2024 11:01:22 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit