feat: Moved Tenant Information to Redis for quick access by NilanshBansal · Pull Request #33309 · appsmithorg/appsmith

NilanshBansal · 2024-05-09T06:19:07Z

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?

Yes
No

github-actions · 2024-05-09T06:43:43Z

Failed server tests

com.appsmith.server.services.ce.TenantServiceCETest#setEmailVerificationEnabled_WithValidSMTPHost_Success
com.appsmith.server.services.ce.TenantServiceCETest#setMapsKeyAndGetItBack
com.appsmith.server.services.ce.TenantServiceCETest#updateTenantConfiguration_updateStrongPasswordPolicy_success

github-actions · 2024-05-09T07:48:39Z

Failed server tests

com.external.plugins.RestApiPluginTest#testHttpGetRequestRawBody

github-actions · 2024-05-09T10:33:38Z

Failed server tests

com.appsmith.server.services.ce.TenantServiceCETest#setEmailVerificationEnabled_WithValidSMTPHost_Success
com.appsmith.server.services.ce.TenantServiceCETest#updateTenantConfiguration_updateStrongPasswordPolicy_success
com.appsmith.server.solutions.ApplicationForkingServiceTests#cloneApplicationForkWithConfigurationFalseWithActionsThrice

...th-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java

...er/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/TenantRepositoryCE.java

...erver/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java

…ervices/ce/TenantServiceCEImpl.java Co-authored-by: Arpit Mohan <mohanarpit@users.noreply.github.com>

github-actions · 2024-05-10T08:08:57Z

Failed server tests

com.appsmith.server.services.ce.ApplicationServiceCETest#testUploadNavigationLogo_invalidImageSize
com.appsmith.server.services.ce.TenantServiceCETest#setEmailVerificationEnabled_WithValidSMTPHost_Success
com.appsmith.server.services.ce.TenantServiceCETest#updateTenantConfiguration_updateStrongPasswordPolicy_success

…g-to-redis' into feature/issue-33083/tenant-config-to-redis

github-actions · 2024-05-10T09:14:51Z

Failed server tests

com.appsmith.server.services.ce.TenantServiceCETest#setEmailVerificationEnabled_WithValidSMTPHost_Success
com.appsmith.server.services.ce.TenantServiceCETest#updateTenantConfiguration_updateStrongPasswordPolicy_success

NilanshBansal · 2024-05-10T09:17:24Z

Test updateTenantConfiguration_updateStrongPasswordPolicy_success is failing weirdly with 2 different tenantIds getting populated in redis for a single test case. This is creating inconsistency and leading to failure of this test 😞

abhvsn

@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

…e/issue-33083/tenant-config-to-redis

This reverts commit da5af58.

github-actions · 2024-05-14T11:42:50Z

Failed server tests

com.appsmith.server.services.ce.ApplicationServiceCETest#testUploadNavigationLogo_invalidImageSize
com.appsmith.server.solutions.ApplicationForkingServiceTests#cloneApplicationForkWithConfigurationFalseWithActionsThrice

coderabbitai

Actionable comments posted: 1

...erver/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java

…e/issue-33083/tenant-config-to-redis

addressed

## 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  > [!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>  ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No

)" This reverts commit 306e8c1.

## 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. > The old PR implementation #33309 had to be reverted due to the tenant GAC permissions not in sync between database and redis. RCA [ref](https://www.notion.so/appsmith/Reversion-for-Tenant-Caching-implementation-after-merging-to-release-cd720b9959e4413f98decb884c375ad2). This PR uses the same branch as the old PR and builds the pending functionalities to complete the implementation. **Counterpart EE PR**: appsmithorg/appsmith-ee#4275 **TL;DR** Adds tenant information `tenantService.getDefaultTenant()` to redis. Fixes #33083, #33504, #33578 ## Automation /ok-to-test tags="@tag.Settings" ### 🔍 Cypress test results  > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9253953003> > Commit: 7b4bf8d > Cypress dashboard url: <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9253953003&attempt=1" target="_blank">Click here!</a>  ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No --------- Co-authored-by: Arpit Mohan <mohanarpit@users.noreply.github.com>

NilanshBansal added 3 commits May 6, 2024 13:51

Merge branch 'release' of github.com:appsmithorg/appsmith into release

63a005d

Merge branch 'release' of github.com:appsmithorg/appsmith into release

60dd0f9

initial setup

9b144b6

NilanshBansal self-assigned this May 9, 2024

github-actions bot added Enhancement New feature or request Integrations Product Issues related to a specific integration Integrations Pod General Issues related to the Integrations Pod that don't fit into other tags. Task A simple Todo labels May 9, 2024

feat: added tenant configuration to redis cache (#33083)

9f37234

code optimised to replace usage of tenantRepository to cached function

e889d83

NilanshBansal changed the title ~~feat: Adding & Fetching Tenant Configuration to Redis~~ feat: Adding & Fetching Tenant Info to Redis May 9, 2024

NilanshBansal changed the title ~~feat: Adding & Fetching Tenant Info to Redis~~ feat: Moved Tenant Information to Redis for quick access May 9, 2024

fixed cyclic dependency

b2c301e

NilanshBansal requested a review from abhvsn May 10, 2024 03:39

mohanarpit reviewed May 10, 2024

View reviewed changes

...th-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java Outdated Show resolved Hide resolved

mohanarpit reviewed May 10, 2024

View reviewed changes

...er/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/TenantRepositoryCE.java Show resolved Hide resolved

mohanarpit reviewed May 10, 2024

View reviewed changes

...erver/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java Outdated Show resolved Hide resolved

Update app/server/appsmith-server/src/main/java/com/appsmith/server/s…

883b05a

…ervices/ce/TenantServiceCEImpl.java Co-authored-by: Arpit Mohan <mohanarpit@users.noreply.github.com>

NilanshBansal added 2 commits May 10, 2024 14:19

cache evicted after db update to avoid inconsistency

064d0b1

Merge remote-tracking branch 'origin/feature/issue-33083/tenant-confi…

0702551

…g-to-redis' into feature/issue-33083/tenant-config-to-redis

NilanshBansal added 2 commits May 13, 2024 10:13

fixed tests

9543f94

added implements serializable for tenant configuration

958f5e2

NilanshBansal requested a review from nidhi-nair May 13, 2024 08:29

appsmithorg deleted a comment from github-actions bot May 13, 2024

NilanshBansal added the ok-to-test Required label for CI label May 13, 2024

abhvsn previously requested changes May 13, 2024

View reviewed changes

...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

NilanshBansal added 3 commits May 14, 2024 09:45

updated cacheName and added notDeleted criteria

2d1115d

Merge branch 'release' of github.com:appsmithorg/appsmith into featur…

bdae9ad

…e/issue-33083/tenant-config-to-redis

cache evicted from update func

4d1deed

NilanshBansal requested a review from abhvsn May 14, 2024 04:55

NilanshBansal added 2 commits May 14, 2024 12:42

moved cache invalidation to repository layer

da5af58

Revert "moved cache invalidation to repository layer"

1910c61

This reverts commit da5af58.

optimisations

371f2d1

coderabbitai bot reviewed May 14, 2024

View reviewed changes

...erver/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java Show resolved Hide resolved

NilanshBansal added 2 commits May 14, 2024 17:43

code rabbit suggestion

bb21eae

Merge branch 'release' of github.com:appsmithorg/appsmith into featur…

306e77f

…e/issue-33083/tenant-config-to-redis

NilanshBansal added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels May 14, 2024

NilanshBansal mentioned this pull request May 14, 2024

[Task]: Replace usage of TenantRepository.save to TenantService.save in EE #33438

Closed

abhvsn approved these changes May 15, 2024

View reviewed changes

NilanshBansal merged commit 306e8c1 into release May 15, 2024

NilanshBansal deleted the feature/issue-33083/tenant-config-to-redis branch May 15, 2024 01:45

NilanshBansal added a commit that referenced this pull request May 16, 2024

Revert "feat: Moved Tenant Information to Redis for quick access (#33309

adcb201

)" This reverts commit 306e8c1.

This was referenced May 16, 2024

chore: Revert "feat: Moved Tenant Information to Redis for quick access" #33512

Merged

[Task]: Implement Redis caching for tenant configuration #33083

Closed

NilanshBansal restored the feature/issue-33083/tenant-config-to-redis branch May 21, 2024 12:25

NilanshBansal mentioned this pull request May 22, 2024

feat: Adding Tenant Information to Redis Cache for quick fetch #33641

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Moved Tenant Information to Redis for quick access#33309

feat: Moved Tenant Information to Redis for quick access#33309
NilanshBansal merged 20 commits intoreleasefrom
feature/issue-33083/tenant-config-to-redis

NilanshBansal commented May 9, 2024 •

edited by github-actions bot

Loading

Uh oh!

github-actions bot commented May 9, 2024

Uh oh!

github-actions bot commented May 9, 2024

Uh oh!

github-actions bot commented May 9, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented May 10, 2024

Uh oh!

github-actions bot commented May 10, 2024

Uh oh!

NilanshBansal commented May 10, 2024 •

edited

Loading

Uh oh!

abhvsn left a comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented May 14, 2024

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

NilanshBansal commented May 9, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Automation

🔍 Cypress test results

Communication

Uh oh!

github-actions bot commented May 9, 2024

Failed server tests

Uh oh!

github-actions bot commented May 9, 2024

Failed server tests

Uh oh!

github-actions bot commented May 9, 2024

Failed server tests

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented May 10, 2024

Failed server tests

Uh oh!

github-actions bot commented May 10, 2024

Failed server tests

Uh oh!

NilanshBansal commented May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abhvsn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented May 14, 2024

Failed server tests

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

NilanshBansal commented May 9, 2024 •

edited by github-actions bot

Loading

NilanshBansal commented May 10, 2024 •

edited

Loading