chore: Add indexes for policyMap to improve query response time #35676
Conversation
WalkthroughThe recent changes enhance the structure and functionality of the application by introducing new constants for better field management, optimizing database interactions with index migrations, and refining policy handling for tenants. These updates improve code readability, efficiency, and support for multi-tenant functionality, ultimately contributing to a more robust application architecture. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Application
participant Database
User->>Application: Request policy information
Application->>Database: Query policies
Database-->>Application: Return policy data
Application-->>User: Send policy information
Note over Application: Migration processes add indexes and update tenant policies in the background.
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
github-actions
bot
added
the
skip-changelog
| }) | ||
| .subscribeOn(Schedulers.boundedElastic()); | ||
|
|
||
| Mono.zip(readWorkspaceMono, manageWorkspaceMono).block(); |
There was a problem hiding this comment.
This is being referenced from performance advisor.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration060AddIndexesForPolicyMap.java (4)
4-6: Ensure Correct Use of Annotations.The
@ChangeUnit,@Execution, and@RollbackExecutionannotations are used correctly for defining a migration unit. However, ensure that theauthorfield is filled out appropriately as per project conventions.
34-34: Fill in theauthorfield in@ChangeUnit.The
authorfield in the@ChangeUnitannotation is currently empty. It's important to fill this field with the appropriate author's name or identifier to maintain proper documentation and traceability.
38-39: Consider Implementing a Rollback Strategy.Currently, the
rollbackExecutionmethod is empty. While the learnings indicate that not all migrations require a rollback, consider whether a rollback strategy is necessary for this migration to ensure data integrity in case of issues.
78-90: Handle Exceptions Appropriately.The
createAndApplyIndexmethod handles exceptions by logging errors. This is a good practice, but consider whether additional actions should be taken in case of failure, such as alerting or retry mechanisms, depending on the criticality of the index creation.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/constants/DeprecatedFieldName.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/constants/FieldName.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration060AddIndexesForPolicyMap.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration061TenantPolicySetToPolicyMap.java (1 hunks)
Additional context used
Learnings (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration060AddIndexesForPolicyMap.java (2)
Learnt from: mohanarpit PR: appsmithorg/appsmith#29676 File: app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration039AddCompoundIndexForDatasourceStorage.java:18-18 Timestamp: 2023-12-18T07:46:35.484Z Learning: The team does not practice including the author's name in the `@ChangeUnit` annotation.Learnt from: trishaanand PR: appsmithorg/appsmith#30087 File: app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration043AddIndexActionCollectionPageID.java:27-28 Timestamp: 2024-01-08T07:11:52.229Z Learning: The user has clarified that the migration `Migration043AddIndexActionCollectionPageID` is not critical and does not require a rollback strategy.
Additional comments not posted (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/constants/DeprecatedFieldName.java (2)
4-5: Good practice: Constants are now immutable.Changing
POLICIEStopublic static finalenforces immutability, which is a good practice for constants. The addition ofDELETEDas a constant also enhances clarity in the codebase.
5-5: Verify usage of the newDELETEDconstant.Ensure that the
DELETEDconstant is used consistently across the codebase where applicable. This helps maintain clarity and consistency in handling deprecated fields.Verification successful
The
DELETEDconstant is used appropriately in the migration script.The
DELETEDconstant is being used in theMigration060AddIndexesForPolicyMap.javafile to create a compound index with thedeletedAtfield. This is a valid and consistent use of the constant, ensuring efficient query operations involving these fields.
- File:
Migration060AddIndexesForPolicyMap.java- Usage: Creating a compound index with
deletedAtto optimize database queries.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new `DELETED` constant across the codebase. # Test: Search for the usage of `DELETED`. Expect: Consistent usage across relevant files. rg --type java 'DeprecatedFieldName.DELETED'Length of output: 246
Script:
#!/bin/bash # Extract lines around the usage of `DELETED` in the Migration060AddIndexesForPolicyMap.java file for context. rg --type java -A 10 -B 10 'DeprecatedFieldName.DELETED' app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration060AddIndexesForPolicyMap.javaLength of output: 1488
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/constants/FieldName.java (2)
5-7: New constants improve clarity and support new functionalities.The addition of
DELETED_AT,TENANT_ID, andPERMISSION_GROUPSas constants enhances the codebase by providing clear, consistent references for these fields.
5-7: Verify usage of new constants.Ensure that the new constants are used consistently across the codebase where applicable. This helps maintain clarity and consistency in handling these fields.
Verification successful
The new constants are used consistently across the codebase.
The constants
DELETED_AT,TENANT_ID, andPERMISSION_GROUPSare utilized in various parts of the codebase, including repository implementations and migration scripts. This consistent usage helps maintain clarity and consistency in handling these fields.
- Files where constants are used:
BaseAppsmithRepositoryCEImpl.javaBaseRepositoryImpl.java- Various migration files such as
Migration017UnsetEncryptionVersion2Fields.java,Migration027AddIndexDatasourceIdAndDeletedInAction.java, and more.This indicates that the new constants are integrated well into the existing code structure. If there are specific areas of concern, a targeted manual review might be necessary.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new constants across the codebase. # Test: Search for the usage of `DELETED_AT`, `TENANT_ID`, and `PERMISSION_GROUPS`. Expect: Consistent usage across relevant files. rg --type java 'FieldName.DELETED_AT|FieldName.TENANT_ID|FieldName.PERMISSION_GROUPS'Length of output: 4039
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration061TenantPolicySetToPolicyMap.java (1)
21-57: Migration logic is clear and well-structured.The migration effectively transitions policy sets to policy maps and includes cache eviction to maintain consistency. Ensure thorough testing of this migration in a safe environment before deployment.
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration060AddIndexesForPolicyMap.java (1)
41-76: Parallel Execution of Index Creation.The use of
Mono.zipto execute the index creation in parallel is a good approach for non-blocking operations. Ensure that this does not lead to any race conditions or unexpected behavior in your specific use case.
...c/main/java/com/appsmith/server/migrations/db/ce/Migration061TenantPolicySetToPolicyMap.java
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration060AddIndexesForPolicyMap.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration060AddIndexesForPolicyMap.java
## Description Ref thread: https://theappsmith.slack.com/archives/C0341RERY4R/p1723548842255209?thread_ts=1723445955.346459&cid=C0341RERY4R /test Workspace ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/10380730889> > Commit: 915e932 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10380730889&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Workspace` > Spec: > <hr>Wed, 14 Aug 2024 03:42:23 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced new constants to improve handling of deprecated field names and enhance multi-tenant functionalities, including `DELETED_AT`, `TENANT_ID`, and `PERMISSION_GROUPS`. - Implemented migration processes for updating tenant policies and optimizing query performance with new database indexes. - **Bug Fixes** - Improved system stability by refining the declaration of constants, ensuring immutability for `POLICIES` and enhancing error logging for database migrations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit 8ad9b4a)
Description
Ref thread: https://theappsmith.slack.com/archives/C0341RERY4R/p1723548842255209?thread_ts=1723445955.346459&cid=C0341RERY4R
/test Workspace
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10380730889
Commit: 915e932
Cypress dashboard.
Tags:
@tag.WorkspaceSpec:
Wed, 14 Aug 2024 03:42:23 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
DELETED_AT,TENANT_ID, andPERMISSION_GROUPS.Bug Fixes
POLICIESand enhancing error logging for database migrations.