fix: Handle burst traffic for fetching feature flags during cron job #40808
Conversation
…g-running requests
WalkthroughA dedicated Changes
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ 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
Documentation and Community
|
Adjusted the scheduled task for fetching features to run every hour instead of every thirty minutes, and increased the lock TTL from 20 to 45 minutes to improve task execution reliability.
…ls for reusability
…vent multiple instances
… to maintain singleton pattern consistency
…S scheduler to 5 threads to prevent overwhelming CS - Increase connection pool from 50 to 100 for better concurrency - Add dedicated scheduler for scheduled CS tasks
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/server/appsmith-interfaces/src/main/java/com/appsmith/util/WebClientUtils.java (1)
117-125: Consider removing the alias method.While the alias method clarifies the singleton nature, it adds unnecessary API surface. The
createForCloudServices()method name already indicates its purpose clearly.- /** - * Gets the singleton WebClient instance for Cloud Services. - * This is an alias for createForCloudServices() but makes the singleton nature more explicit. - * - * @return Singleton WebClient configured for Cloud Services calls - */ - public static WebClient getCloudServicesWebClient() { - return createForCloudServices(); - }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/server/appsmith-interfaces/src/main/java/com/appsmith/util/WebClientUtils.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ScheduledTaskCEImpl.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ScheduledTaskCEImpl.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (6)
app/server/appsmith-interfaces/src/main/java/com/appsmith/util/WebClientUtils.java (6)
27-27: LGTM - Required import for timeout functionality.The Duration import is correctly added to support the new timeout configuration.
48-49: Well-defined timeout constant.The 60-second timeout aligns perfectly with the PR objective to prevent long-running Cloud Services API calls. The constant is appropriately public for reuse in other classes.
51-59: Robust connection provider configuration.The dedicated connection pool with 100 max connections and proper lifecycle management (idle time, max lifetime, eviction) is well-configured for Cloud Services workload. The increased connection limit appropriately handles concurrent CS calls while preventing connection exhaustion.
61-62: Correct volatile declaration for singleton pattern.The volatile keyword ensures proper visibility of the singleton instance across threads in the double-checked locking pattern.
94-115: Thread-safe singleton implementation.The double-checked locking pattern is correctly implemented with synchronized blocks and volatile field. This ensures thread safety while avoiding unnecessary synchronization overhead after initialization.
127-138: Appropriate test utility with proper access control.The reset method is correctly marked as deprecated and package-private, limiting its use to testing scenarios while discouraging production usage.
…max life time from 60 to 120 seconds and evict in background time from 120 to 150 seconds for improved resource management.
…on pool settings - Replace custom bounded scheduler with single scheduler for sequential processing - Increase connection lifetime and eviction settings for better stability - Remove unused scheduler code for cleaner implementation
…ate delay constant for Cloud Services calls (100ms vs 200ms) - Keep 200ms delay for Segment API to avoid rate limits - Reduce CS delay since single scheduler already provides sequential processing
…ant - Remove delay from Cloud Services calls (fetchFeatures) since we use sequential processing - Keep delay only for Segment API calls to avoid rate limits - Cleaner code with single delay constant
… delay between fetching features for organizations to improve processing flow and manage rate limits effectively.
| @@ -258,10 +257,10 @@ private Mono<Map<String, Long>> getUserTrackingDetails() { | |||
| UserTrackingType.MAU.name(), tuple.getT3())); | |||
| } | |||
|
|
|||
| @Scheduled(initialDelay = 10 * 1000 /* ten seconds */, fixedRate = 30 * 60 * 1000 /* thirty minutes */) | |||
| @Scheduled(initialDelay = 10 * 1000 /* ten seconds */, fixedRate = 1 * 60 * 60 * 1000 /* one hour */) | |||
There was a problem hiding this comment.
Increased this to 1hr to reduce the freq, as anyway we have a refresh rate of 115min downstream for org level flags.
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
/test Sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/15341007306
Commit: 27d22c8
Cypress dashboard.
Tags:
@tag.SanitySpec:
Fri, 30 May 2025 07:28:40 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit