-
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
fix: Added indices for cross referencing changes #35401
Conversation
Important Review skippedAuto reviews are limited to specific labels. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe introduction of Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
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
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/Migration058AddIndexesToBranchAwareDomains.java (1 hunks)
Additional context used
Learnings (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration058AddIndexesToBranchAwareDomains.java (2)
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.
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.
Additional comments not posted (7)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration058AddIndexesToBranchAwareDomains.java (7)
1-13
: Good job on the package and import statements!All necessary imports are included and no unnecessary imports are present.
20-28
: Well done on the class declaration and constructor!The class is correctly annotated and the constructor initializes the required fields.
30-31
: TherollbackExecution
method is correctly implemented as empty.This follows the team's practice for non-critical migrations.
33-61
: Great work on theexecuteMigration
method!The method correctly creates indices for the specified collections using reactive programming and handles errors appropriately.
63-82
: Excellent implementation of thecreateNewPageIndex
method!The method correctly creates the index for the
NewPage
collection and handles errors appropriately.
84-103
: Great job on thecreateNewActionIndex
method!The method correctly creates the index for the
NewAction
collection and handles errors appropriately.
105-125
: Well done on thecreateActionCollectionIndex
method!The method correctly creates the index for the
ActionCollection
collection and handles errors appropriately.
} | ||
|
||
private void createNewPageIndex() { | ||
String indexName = "baseId_branchName_compound_index"; |
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.
Nit: Since it's common for all three domains, let's take this out?
same for the index?
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.
Approved with a nit
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
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/Migration058AddIndexesToBranchAwareDomains.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/Migration058AddIndexesToBranchAwareDomains.java
Description
Creates indices for new cross referencing structure for CE domains.
Skipping EE since those domains do not have any indices yet.
Fixes #35396
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Chores