feat: Moved Tenant Information to Redis for quick access #33309
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
Enhancement
Failed server tests
|
Failed server tests
|
NilanshBansal
changed the title
NilanshBansal
changed the title
Failed server tests
|
mohanarpit
reviewed
May 10, 2024
...th-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java
Outdated
Show resolved
Hide resolved
mohanarpit
reviewed
May 10, 2024
...er/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/TenantRepositoryCE.java
Show resolved
Hide resolved
mohanarpit
reviewed
May 10, 2024
...erver/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java
Outdated
Show resolved
Hide resolved
…ervices/ce/TenantServiceCEImpl.java Co-authored-by: Arpit Mohan <mohanarpit@users.noreply.github.com>
Failed server tests
|
…g-to-redis' into feature/issue-33083/tenant-config-to-redis
Failed server tests
|
|
Test 
|
abhvsn
previously requested changes
May 13, 2024
There was a problem hiding this comment.
@NilanshBansal added couple of minor comments. Also we are updating the tenant in restartTenant which is making a call to BaseService and not covered as a part of invalidation. I'm little concerned on invalidation part, let's make sure to override the base service and repository methods which are saving and updating the DB object.
...erver/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java
Outdated
Show resolved
Hide resolved
...erver/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java
Outdated
Show resolved
Hide resolved
…e/issue-33083/tenant-config-to-redis
Failed server tests
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
...erver/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java
Show resolved
Hide resolved
…e/issue-33083/tenant-config-to-redis
NilanshBansal
added
ok-to-test
abhvsn
approved these changes
May 15, 2024
This was referenced May 15, 2024
hetunandu
pushed a commit
that referenced
this pull request
May 15, 2024
## Description After merging the PR #33309, we started to store the Tenant Information in the Redis cache. Redis needs to serialize the objects upon storing and deserialize post fetching. To do that we need to implement serialization for all the nested user defined Domains. This was missed for the LicenseCE and Payment object. Corresponding EE PR: appsmithorg/appsmith-ee#4191 Fixes #33486 ## Automation /ok-to-test tags="@tag.Settings, @tag.LicenseAndBilling" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!CAUTION] > 🔴 🔴 🔴 Some tests have failed. > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9096922670> > Commit: 559bcd6 > Cypress dashboard: <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9096922670&attempt=3&selectiontype=test&testsstatus=failed&specsstatus=fail" target="_blank"> Click here!</a> > The following are new failures, please fix them before merging the PR: <ol> > <li>cypress/e2e/Regression/ClientSide/ProductRamps/PrivateEmbedRamp_spec.ts </ol> > To know the list of identified flaky tests - <a href="https://internal.appsmith.com/app/cypress-dashboard/identified-flaky-tests-65890b3c81d7400d08fa9ee3?branch=master" target="_blank">Refer here</a> <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No
NilanshBansal
added a commit
that referenced
this pull request
May 16, 2024
)" This reverts commit 306e8c1.
This was referenced May 16, 2024
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The tenant is fetched multiple times across the appsmith codebase but is rarely updated (from the admin settings). Every time a fetch call to the database is costly both in terms of resources and time taken.
The consolidated api also makes a call to fetch the tenant and return to the client. To improve the performance of fetching the tenant information, we are moving the tenant information to redis cache for quicker fetch.
This will improve the performance of the consolidated api and also reduce the time taken by all the different functionalities within the backend codebase which depend on tenant to process further.
TL;DR
Adds tenant information
tenantService.getDefaultTenant()to redis.Fixes #33083
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9079438120
Commit: 306e77f
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?