chore: Use Lettuce client to run clear keys across Redis nodes #36862
Conversation
07/10 - Daily promotion
08/10 Daily Promotion
nidhi-nair
requested review from
sharat87,
abhvsn and
AnaghHegde
as code owners
WalkthroughThe changes made in this pull request focus on enhancing plugin and database management within a MongoDB context. Key modifications include changing method visibility, adding new migration steps via Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11326054119. |
![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 and nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog1.java (3)
Line range hint
803-808: Add Null Check forfirestorePluginto Prevent PossibleNullPointerExceptionIt's important to handle cases where the Firestore plugin might not be found in the database. Before calling
setUiComponentandsave, please check iffirestorePluginis notnull. This will help prevent a potentialNullPointerException.Here's how you can modify the code:
Plugin firestorePlugin = mongoTemplate.findOne(query(where("packageName").is("firestore-plugin")), Plugin.class); + if (firestorePlugin != null) { firestorePlugin.setUiComponent("UQIDbEditorForm"); // Update plugin data. mongoTemplate.save(firestorePlugin); + } else { + log.warn("Firestore plugin not found in the database."); + }
Line range hint
817-822: Ensure Null Safety and Clarify Method NamingOnce again, please add a null check for
firestorePluginto prevent a possibleNullPointerException. Additionally, consider renaming the methodmigrateFirestorePluginToUqi3to something more descriptive to enhance code readability and maintainability.Apply the following changes:
Plugin firestorePlugin = mongoTemplate.findOne(query(where("packageName").is("firestore-plugin")), Plugin.class); + if (firestorePlugin != null) { firestorePlugin.setUiComponent("UQIDbEditorForm"); // Update plugin data. mongoTemplate.save(firestorePlugin); + } else { + log.warn("Firestore plugin not found in the database."); + }Consider renaming the method for clarity, for example:
-public void migrateFirestorePluginToUqi3(MongoTemplate mongoTemplate) { +public void migrateFirestorePluginToUqiVersion3(MongoTemplate mongoTemplate) {
Line range hint
843-848: Replaceassertwith Explicit Null Check for RobustnessUsing
assertto check ifpluginis notnullcan be insufficient because assertions might be disabled at runtime. It's better to use an explicit null check to ensure that the code behaves correctly regardless of runtime settings. This approach enhances the robustness of your code.Here's the improved code:
Plugin plugin = mongoTemplate.findOne(query(where("packageName").is("mssql-plugin")), Plugin.class); - assert plugin != null; + if (plugin != null) { plugin.setGenerateCRUDPageComponent(null); mongoTemplate.save(plugin); + } else { + log.warn("MSSQL plugin not found in the database."); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog1.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration063CacheBustSpringBoot3_3.java (2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration063CacheBustSpringBoot3_3.java (2)
24-26: Updating Method Signature to Use ReactiveRedisOperationsWell done on updating the method signature to use
ReactiveRedisOperations<String, Object>. This change aligns the code with the latest best practices in Spring Data Redis, ensuring better compatibility and maintainability.
25-25:⚠️ Potential issueConsider Avoiding Blocking Calls in Reactive Streams
In reactive programming, we aim to keep the data flow non-blocking to fully leverage the benefits of asynchronous processing. The use of
.block()can introduce blocking behavior, which might lead to performance issues.As an alternative, you could modify the
executemethod to return aMono<Void>and handle the reactive stream without blocking:- public void execute( + public Mono<Void> execute( @Qualifier("reactiveRedisOperations") ReactiveRedisOperations<String, Object> reactiveRedisOperations) { - scanForKeysAcrossCluster(reactiveRedisOperations, "*").block(); + return scanForKeysAcrossCluster(reactiveRedisOperations, "*"); }However, please verify whether the migration framework supports reactive return types. If it doesn't, and blocking is necessary in this context, consider documenting the reason to inform future developers.
Please check the migration framework's capabilities regarding reactive return types and adjust accordingly.
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog1.java (2)
Line range hint
791-797: Consider the Visibility Change ofdoClearRedisKeysMethodDear student, I notice that you've changed the
doClearRedisKeysmethod fromprotectedtopublic. Exposing this method publicly means it can now be accessed from outside its class and package. Please ensure that this change is necessary and aligns with the design principles of encapsulation and security. Public methods should be carefully managed to prevent unintended usage or security vulnerabilities.
Line range hint
850-862: Verify Index Names Against Length LimitationsWhen creating indexes with names like
"unpublishedActionPageId_deleted_compound_index", it's essential to ensure that they comply with MongoDB's index name length limitations. Long index names can cause errors or be truncated unexpectedly, leading to potential issues during database operations. Please verify that all index names meet the necessary constraints.
| private Mono<Void> scanForKeysAcrossCluster( | ||
| ReactiveRedisOperations<String, Object> reactiveRedisOperations, String pattern) { | ||
| return reactiveRedisOperations | ||
| .execute(connection -> { | ||
| Flux<ByteBuffer> scanFlux = connection | ||
| .keyCommands() | ||
| .scan(ScanOptions.scanOptions() | ||
| .match(pattern) | ||
| .count(1000) | ||
| .build()); | ||
| return scanFlux.flatMap(scannedKey -> { | ||
| return connection.keyCommands().del(scannedKey); | ||
| }) | ||
| .then(); | ||
| }) | ||
| .then(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimizing Key Scanning and Deletion in Redis
You've effectively encapsulated the Redis key scanning and deletion logic within the scanForKeysAcrossCluster method. Here are some suggestions to enhance its performance and reliability:
-
Refine the Scan Pattern: Scanning with a wildcard
"*"can be resource-intensive, especially in a production environment with a large number of keys. If possible, narrow down the pattern to target only the relevant keys that need deletion. -
Batch Deletion for Efficiency: Deleting keys individually may lead to increased overhead. Batching the keys and performing a bulk delete can improve performance significantly.
-
Handle Potential Exceptions: It's important to anticipate and handle any exceptions that might occur during the scanning and deletion process to prevent unexpected failures.
Here's how you could modify the method to incorporate these suggestions:
private Mono<Void> scanForKeysAcrossCluster(
ReactiveRedisOperations<String, Object> reactiveRedisOperations, String pattern) {
return reactiveRedisOperations
.execute(connection -> {
Flux<ByteBuffer> scanFlux = connection
.keyCommands()
.scan(ScanOptions.scanOptions()
.match(pattern)
.count(1000)
.build());
- return scanFlux.flatMap(scannedKey -> {
- return connection.keyCommands().del(scannedKey);
- })
+ return scanFlux.buffer(1000)
+ .flatMap(keys -> connection.keyCommands().del(keys.toArray(new ByteBuffer[0])))
+ .onErrorContinue((throwable, obj) -> {
+ log.error("Error deleting keys: {}", throwable.getMessage());
+ });
})
.then();
}- Batching with
buffer(1000): This groups the keys into batches of 1000 before deletion. - Bulk Deletion: Deleting the keys in bulk reduces the number of network calls and improves efficiency.
- Error Handling with
onErrorContinue: This ensures that if an error occurs during deletion, it logs the error but continues processing the remaining keys.
If you'd like further assistance in implementing these changes or have questions about Redis key management, please let me know!
📝 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.
| private Mono<Void> scanForKeysAcrossCluster( | |
| ReactiveRedisOperations<String, Object> reactiveRedisOperations, String pattern) { | |
| return reactiveRedisOperations | |
| .execute(connection -> { | |
| Flux<ByteBuffer> scanFlux = connection | |
| .keyCommands() | |
| .scan(ScanOptions.scanOptions() | |
| .match(pattern) | |
| .count(1000) | |
| .build()); | |
| return scanFlux.flatMap(scannedKey -> { | |
| return connection.keyCommands().del(scannedKey); | |
| }) | |
| .then(); | |
| }) | |
| .then(); | |
| private Mono<Void> scanForKeysAcrossCluster( | |
| ReactiveRedisOperations<String, Object> reactiveRedisOperations, String pattern) { | |
| return reactiveRedisOperations | |
| .execute(connection -> { | |
| Flux<ByteBuffer> scanFlux = connection | |
| .keyCommands() | |
| .scan(ScanOptions.scanOptions() | |
| .match(pattern) | |
| .count(1000) | |
| .build()); | |
| return scanFlux.buffer(1000) | |
| .flatMap(keys -> connection.keyCommands().del(keys.toArray(new ByteBuffer[0]))) | |
| .onErrorContinue((throwable, obj) -> { | |
| log.error("Error deleting keys: {}", throwable.getMessage()); | |
| }); | |
| }) | |
| .then(); | |
| } |
|
Deploy-Preview-URL: https://ce-36862.dp.appsmith.com |
nidhi-nair
changed the title
github-actions
bot
added
the
skip-changelog
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog1.java
Outdated
Show resolved
Hide resolved
07/10 - Daily promotion## 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.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD 2e47baf yet
Mon, 14 Oct 2024 11:26:21 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Improvements