[WEB-2706] fix: Make SQL transactions smaller #6085
Conversation
- Fallback to server when mentions filter is used - Split load workspace into multiple transactions
WalkthroughThe changes involve modifications to several files related to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
web/core/local-db/utils/load-issues.ts (2)
40-40: Consider enhancing performance logging.While the current logging is helpful, consider adding more granular performance metrics:
- log("Inserted issues in ", `${insertEnd - insertStart}ms`, batchSize, issues.length); + const totalTime = insertEnd - insertStart; + const avgTimePerBatch = totalTime / Math.ceil(issues.length / batchSize); + log( + "Bulk insert completed", + `Total time: ${totalTime}ms`, + `Avg time per batch: ${avgTimePerBatch.toFixed(2)}ms`, + `Batch size: ${batchSize}`, + `Total issues: ${issues.length}` + );
Line range hint
18-41: Consider adding telemetry for monitoring bulk operation performance.To better understand the impact of these optimizations in production:
- Consider implementing metrics collection for bulk operation timings
- Track success/failure rates of batch operations
- Monitor transaction sizes and execution times
This data would help in:
- Validating the effectiveness of the batch size reduction
- Identifying potential bottlenecks
- Fine-tuning the batch size based on real-world usage
web/core/local-db/utils/load-workspace.ts (1)
123-144: Consider optimizing the data loading strategy.The current implementation loads all data into memory before processing and processes each data type sequentially. This could be optimized for better memory usage and performance.
Consider these improvements:
- Stream data in chunks instead of loading everything into memory:
async function* streamWorkspaceData(workspaceSlug: string) { yield { type: 'labels', data: await getLabels(workspaceSlug) }; yield { type: 'modules', data: await getModules(workspaceSlug) }; // ... etc }
- Process data types concurrently with controlled concurrency:
const concurrency = 2; // Process 2 data types at a time const chunks = streamWorkspaceData(workspaceSlug); for await (const chunk of chunks) { await withTransaction(() => batchInserts(chunk.data, chunk.type, schemas[chunk.type]) ); }
- Add progress tracking for better observability:
const total = await countWorkspaceData(workspaceSlug); let processed = 0; // ... during processing processed += chunk.data.length; log(`Progress: ${processed}/${total} (${(processed/total*100).toFixed(1)}%)`);Would you like me to help implement any of these improvements?
web/core/local-db/storage.sqlite.ts (3)
24-24: LGTM! Consider monitoring performance impact.The reduction in batch size from 500 to 50 aligns with the PR objective of making SQL transactions smaller. This should help reduce memory pressure and improve concurrency. However, since this is a significant reduction, it would be valuable to monitor:
- Transaction completion times
- Database connection pool usage
- Overall system performance
Line range hint
401-407: Consider enhancing error handling.The fallback mechanism is well implemented, but consider adding error handling for scenarios where:
- The local DB query fails
- The server call fails
getSubIssues = async (workspaceSlug: string, projectId: string, issueId: string) => { + try { const workspace_synced_at = await this.getOption("workspace_synced_at"); if (!workspace_synced_at) { const issueService = new IssueService(); return await issueService.subIssues(workspaceSlug, projectId, issueId); } return await getSubIssuesWithDistribution(issueId); + } catch (error) { + console.error("Failed to get sub-issues:", error); + // Fallback to server as last resort + const issueService = new IssueService(); + return await issueService.subIssues(workspaceSlug, projectId, issueId); + } };
Line range hint
24-407: Consider adding performance monitoring.To validate the impact of these changes, especially the reduced batch size, consider implementing monitoring for:
- Transaction timing metrics
- Memory usage patterns
- Server fallback frequency
- Error rates
This will help in fine-tuning these values in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
web/core/local-db/storage.sqlite.ts(2 hunks)web/core/local-db/utils/load-issues.ts(2 hunks)web/core/local-db/utils/load-workspace.ts(1 hunks)web/core/services/issue/issue.service.ts(2 hunks)web/helpers/issue.helper.ts(2 hunks)
🔇 Additional comments (7)
web/core/local-db/utils/load-issues.ts (1)
18-18: LGTM! Batch size reduction aligns with optimization goals.
The reduction in batch size from 100 to 50 should help reduce transaction lock times and improve concurrency.
Let's verify if this batch size is consistently applied across the codebase:
✅ Verification successful
Different batch sizes are appropriate for different operations
The batch size of 50 for addIssuesBulk and 500 for batchInserts are justified because:
addIssuesBulkhandles issues which likely contain more data and complex relationshipsbatchInsertsis used for simpler entities (labels, modules, states) that are less resource-intensive- Each function uses transactions appropriately for their use case
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other batch size configurations that might need alignment
rg -g '*.{ts,js}' 'batchSize.*=.*[0-9]+' --no-heading
Length of output: 300
Script:
#!/bin/bash
# Let's check the usage context of batchInserts in load-workspace.ts
rg -A 10 "batchInserts" web/core/local-db/utils/load-workspace.ts
Length of output: 1336
web/core/local-db/utils/load-workspace.ts (1)
123-144: Changes successfully achieve the PR objective.
The implementation effectively makes SQL transactions smaller by:
- Splitting bulk inserts into separate transactions per data type
- Isolating different data categories (labels, modules, etc.)
- Reducing the scope of each transaction
This aligns perfectly with the PR objective to "Make SQL transactions smaller" and should help reduce database lock contention.
web/helpers/issue.helper.ts (2)
2-2: LGTM!
The isEmpty import from lodash is appropriately placed and necessary for the new utility function.
313-325: Verify integration with transaction handling system.
The function helps determine when to use local DB vs server, which is crucial for the PR's objective of optimizing SQL transactions. However, we should verify its integration with the transaction handling system.
✅ Verification successful
Let's gather more specific information about how this function is integrated with transaction handling.
Let's check one more aspect to understand the complete flow of the local DB integration.
Function is properly integrated with transaction handling system
The implementation is correctly integrated with the transaction system:
- The function
getIssuesShouldFallbackToServeris used inIssueServiceto determine whether to use local DB or server - When local DB is used, proper transaction handling is implemented in
addIssuesBulkwith BEGIN/COMMIT blocks - The function helps optimize SQL transactions by:
- Preventing complex queries (with expand/mentions) from hitting local DB
- Directing them to server instead, which aligns with the PR's goal of "Make SQL transactions smaller"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how this function is used in transaction-related code
# Look for files that import and use this function
rg -l "getIssuesShouldFallbackToServer"
# Look for transaction-related code that might need to use this function
rg -l "transaction|batch" --type ts
Length of output: 1059
Script:
#!/bin/bash
# Check the actual usage of getIssuesShouldFallbackToServer in issue.service.ts
rg -A 10 "getIssuesShouldFallbackToServer" web/core/services/issue/issue.service.ts
# Check transaction handling in load-issues.ts since it's likely the main integration point
rg -A 10 "transaction" web/core/local-db/utils/load-issues.ts
# Look for the connection between these systems
rg -B 5 -A 10 "IssueService" web/core/local-db/utils/load-issues.ts
Length of output: 2248
Script:
#!/bin/bash
# Check the persistence implementation to understand the transaction handling
rg -B 5 -A 10 "getIssues" web/core/local-db/storage.sqlite.ts
# Look for the actual SQL transaction implementation
rg -B 5 -A 10 "BEGIN" web/core/local-db/worker/db.ts
# Check if there are any other relevant transaction handlers
rg -B 5 -A 10 "addIssuesBulk" web/core/local-db/utils/load-issues.ts
Length of output: 3798
web/core/services/issue/issue.service.ts (2)
13-13: LGTM!
The import statement follows the project's conventions and is properly organized with other helper imports.
Line range hint 70-74: Verify the fallback logic implementation
The changes look good and align with the PR objectives to optimize SQL transactions. The control flow is cleaner with the extracted helper function.
Let's verify the implementation of the fallback logic:
✅ Verification successful
Fallback logic implementation is correct and well-structured
The helper function getIssuesShouldFallbackToServer correctly implements the fallback logic with clear conditions:
- Falls back to server when there's an expand query without grouping
- Falls back to server when mentions are present
- Uses local persistence for all other cases
The implementation aligns with the service usage and maintains clean separation of concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of getIssuesShouldFallbackToServer helper
# Check the helper function implementation
echo "Checking helper function implementation..."
ast-grep --pattern 'export function getIssuesShouldFallbackToServer($_) {
$$$
}'
# Check for any direct usage of expand or group_by checks that might have been missed
echo "Checking for any remaining direct checks..."
rg -l 'expand.*group_by' --type ts
Length of output: 382
Script:
#!/bin/bash
# Let's check the actual implementation in the helper file
echo "Checking helper implementation in issue.helper.ts..."
rg -A 10 "getIssuesShouldFallbackToServer" web/helpers/issue.helper.ts
# Let's also check how it's used in the service
echo "Checking usage in issue.service.ts..."
rg -A 10 "getIssuesShouldFallbackToServer" web/core/services/issue/issue.service.ts
Length of output: 1593
web/core/local-db/storage.sqlite.ts (1)
385-388: LGTM! Improved robustness with better guards.
The additional checks enhance the method's reliability by:
- Ensuring the local DB is both enabled and ready before proceeding
- Using a more explicit array check
| /** | ||
| * This is to check if the issues list api should fall back to server or use local db | ||
| * @param queries | ||
| * @returns | ||
| */ | ||
| export const getIssuesShouldFallbackToServer = (queries: any) => { | ||
| // If there is expand query and is not grouped then fallback to server | ||
| if (!isEmpty(queries.expand as string) && !queries.group_by) return true; | ||
| // If query has mentions then fallback to server | ||
| if (!isEmpty(queries.mentions)) return true; | ||
|
|
||
| return false; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and documentation of the fallback function.
While the function implements the required fallback logic, there are several improvements that could make it more robust:
- Replace
anytype with a proper interface - Enhance JSDoc with parameter details and return value
- Add input validation
Consider this implementation:
+interface IssueListQueries {
+ expand?: string;
+ group_by?: string;
+ mentions?: string;
+}
/**
+ * Determines if the issues list API should fall back to server-side processing
+ * @param queries - The query parameters for the issues list
+ * @param queries.expand - Expansion options for the response
+ * @param queries.group_by - Field to group results by
+ * @param queries.mentions - Filter for mentioned users
+ * @returns boolean - True if should use server, false if can use local DB
+ */
-export const getIssuesShouldFallbackToServer = (queries: any) => {
+export const getIssuesShouldFallbackToServer = (queries: IssueListQueries): boolean => {
+ if (!queries) return false;
+
// If there is expand query and is not grouped then fallback to server
if (!isEmpty(queries.expand as string) && !queries.group_by) return true;
// If query has mentions then fallback to server
if (!isEmpty(queries.mentions)) return true;
return false;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * This is to check if the issues list api should fall back to server or use local db | |
| * @param queries | |
| * @returns | |
| */ | |
| export const getIssuesShouldFallbackToServer = (queries: any) => { | |
| // If there is expand query and is not grouped then fallback to server | |
| if (!isEmpty(queries.expand as string) && !queries.group_by) return true; | |
| // If query has mentions then fallback to server | |
| if (!isEmpty(queries.mentions)) return true; | |
| return false; | |
| }; | |
| interface IssueListQueries { | |
| expand?: string; | |
| group_by?: string; | |
| mentions?: string; | |
| } | |
| /** | |
| * Determines if the issues list API should fall back to server-side processing | |
| * @param queries - The query parameters for the issues list | |
| * @param queries.expand - Expansion options for the response | |
| * @param queries.group_by - Field to group results by | |
| * @param queries.mentions - Filter for mentioned users | |
| * @returns boolean - True if should use server, false if can use local DB | |
| */ | |
| export const getIssuesShouldFallbackToServer = (queries: IssueListQueries): boolean => { | |
| if (!queries) return false; | |
| // If there is expand query and is not grouped then fallback to server | |
| if (!isEmpty(queries.expand as string) && !queries.group_by) return true; | |
| // If query has mentions then fallback to server | |
| if (!isEmpty(queries.mentions)) return true; | |
| return false; | |
| }; |
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes