MM-67505 Add AnalyticsQueryTimeout setting and use when refreshing materialized views#35906
MM-67505 Add AnalyticsQueryTimeout setting and use when refreshing materialized views#35906
Conversation
…terialized views
Documentation Impact Analysis — updates neededDocumentation Impact AnalysisOverall Assessment: Documentation Updates Required Changes SummaryThis PR introduces a new Documentation Impact Details
Recommended Actions
ConfidenceHigh — This is a straightforward new configuration setting with well-defined behavior and clear documentation requirements. The existing QueryTimeout setting in the same file provides an exact template for documenting this new setting. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a configurable SQL analytics query timeout, introduces a SqlStore.analyticsContext helper used to run materialized-view refreshes with cancellable contexts, validates and defaults the new setting, and exposes it in admin UI, TypeScript types, translations, tests, and default configs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webapp/channels/src/components/admin_console/database_settings.tsx`:
- Line 125: Change the assignment to ensure AnalyticsQueryTimeout cannot be
zero: replace the use of parseIntNonNegative with parseIntNonZero when setting
config.SqlSettings.AnalyticsQueryTimeout in the component (so it matches backend
validation in SqlSettings.isValid()); update the statement that currently calls
parseIntNonNegative(this.state.analyticsQueryTimeout) to call
parseIntNonZero(this.state.analyticsQueryTimeout) instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 790ba636-8805-4bed-9772-8d9bee9a3e9d
⛔ Files ignored due to path filters (1)
webapp/channels/src/components/admin_console/__snapshots__/database_settings.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
server/channels/store/sqlstore/file_info_store.goserver/channels/store/sqlstore/post_store.goserver/channels/store/sqlstore/store.goserver/channels/store/sqlstore/user_store.goserver/i18n/en.jsonserver/public/model/config.gowebapp/channels/src/components/admin_console/database_settings.test.tsxwebapp/channels/src/components/admin_console/database_settings.tsxwebapp/channels/src/i18n/en.jsonwebapp/platform/types/src/config.ts
|
Docs PR: mattermost/docs#8846 |
|
The failing E2E test is unrelated to these changes |
agarciamontoro
left a comment
There was a problem hiding this comment.
Looking good, thank you!
Summary
The queries to refresh the materialized views which are now used for calculating some of the contents of the statistics pages in the system console take longer than other queries, but that's fine because they should only be ran in the background periodically. This PR adds a new timeout for them which defaults to 5 minutes.
Note that this isn't used for other analytics queries like I originally thought based on the ticket's name and trying to understand the accompanying thread without context. After originally doing that, I realized this was meant more for those specific queries which didn't need as many changes, so I reverted that part because I didn't think it was a good idea to increase the timeout on the other queries which are called directly via the API. Let me know if I should add this timeout to more queries though.
Ticket Link
https://mattermost.atlassian.net/browse/MM-67505
Release Note
Change Impact: 🟡 Medium
Regression Risk: Modest — changes are limited to adding a new SqlSettings field and applying context-based timeouts only to background materialized-view refreshes; this may cause long-running refreshes to fail under high DB load but does not alter API call paths or core logic.
QA Recommendation: Manual verification recommended: exercise materialized-view refreshes on environments with varying DB performance (local, staging, cloud) and validate the admin UI, config persistence, and validation for the new AnalyticsQueryTimeout.
Release Notes
Added DatabaseSettings.AnalyticsQueryTimeout setting for use when running long analytics queries in the background.
Generated by CodeRabbitAI