fix: Clear all session for Spring upgrade #36695
Conversation
WalkthroughThe changes in this pull request involve significant modifications 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
Documentation and Community
|
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11208592711. |
|
Deploy-Preview-URL: https://ce-36695.dp.appsmith.com |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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/Migration063CacheBustSpringBoot3_3.java (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration063CacheBustSpringBoot3_3.java (2)
7-8: Great job updating the imports appropriatelyBy importing
ReactiveRedisTemplateand@Qualifier, you've set up the class to correctly inject the necessary Redis template. This ensures that the correct bean is used for Redis operations, which is essential for maintaining the application's stability.
18-25: Verify that flushing the Redis database is safe in all environmentsFlushing the entire Redis database will remove all data stored in Redis. It's crucial to ensure that this action won't inadvertently affect other parts of the system, especially if Redis is used for more than just session management.
Would you like assistance in verifying that Redis is exclusively used for session data across the codebase? This can prevent unexpected data loss in other components.
| public void execute( | ||
| @Qualifier("reactiveRedisTemplate") final ReactiveRedisTemplate<String, Object> reactiveRedisTemplate) { | ||
| reactiveRedisTemplate | ||
| .getConnectionFactory() | ||
| .getReactiveConnection() | ||
| .serverCommands() | ||
| .flushDb() | ||
| .block(); |
There was a problem hiding this comment.
Ensure proper error handling for the Redis flush operation
It's important to handle potential exceptions when performing critical operations like flushing the Redis database. Without proper error handling, any failures during this operation could lead to unhandled exceptions and impact the application's stability.
Consider enhancing your code to gracefully handle any errors:
reactiveRedisTemplate
.getConnectionFactory()
.getReactiveConnection()
.serverCommands()
.flushDb()
.doOnError(error -> log.error("Failed to flush Redis database during migration", error))
.block();This way, you log any exceptions, allowing for easier debugging and ensuring the migration process can handle failures appropriately.
🛠️ Refactor suggestion
Avoid blocking calls in reactive streams
Remember, using block() in reactive programming can negate the benefits of non-blocking, asynchronous execution. It can lead to thread blocking and reduce application performance.
Consider refactoring the code to use non-blocking alternatives. For example:
reactiveRedisTemplate
.getConnectionFactory()
.getReactiveConnection()
.serverCommands()
.flushDb()
.subscribe(
unused -> log.info("Redis database flushed successfully during migration"),
error -> log.error("Failed to flush Redis database during migration", error)
);This approach allows the operation to proceed asynchronously, handling success and error cases without blocking the thread.
There was a problem hiding this comment.
@nidhi-nair Sorry for being late here but why can't current custom JSON (de)serialiser handle this issue?
Also this may affect pg as well, and Flyway migrations run before any beans are initialised so we may have to look into that as well.
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 42bc1fc yet
Fri, 04 Oct 2024 07:56:44 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit