Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Task]: Invalid Tenant Cache should not bring the instance down - [Stability] #33504

Closed
NilanshBansal opened this issue May 16, 2024 · 4 comments
Closed

[Task]: Invalid Tenant Cache should not bring the instance down - [Stability] #33504

NilanshBansal opened this issue May 16, 2024 · 4 comments
Assignees
@NilanshBansal
Labels
Integrations Pod General Issues related to the Integrations Pod that don't fit into other tags. Integrations Pod Issues related to a specific integration Performance Pod All things related to Appsmith performance Performance Issues related to performance Task A simple Todo

Comments

@NilanshBansal
Copy link
Contributor

SubTasks

The instance is heavily dependent on the tenants response in the consolidated API to work.
Recently we saw an occurrence when the release was down because of a serialization error in the tenant cache. https://theappsmith.slack.com/archives/CGBPVEJ5C/p1715747695304469

The system should be fault-tolerant and be able to auto-heal in such scenarios. If the redis cache is invalid or if serializing from redis has an issue, the code should default to querying the database and return response.
@NilanshBansal NilanshBansal added Task A simple Todo Integrations Pod General Issues related to the Integrations Pod that don't fit into other tags. Performance Issues related to performance labels May 16, 2024
@NilanshBansal NilanshBansal self-assigned this May 16, 2024
@github-actions github-actions bot added Integrations Pod Issues related to a specific integration Performance Pod All things related to Appsmith performance labels May 16, 2024
@NilanshBansal
Copy link
Contributor Author

Not required now as the change is reverted.

@NilanshBansal
Copy link
Contributor Author

Re-opening as we may implement this again by handling GAC permissions.
Ref: https://theappsmith.slack.com/archives/C02K2MZERSL/p1715847322614589?thread_ts=1715829340.668969&cid=C02K2MZERSL

@NilanshBansal NilanshBansal reopened this May 17, 2024
@AdeshAtole
Copy link
Contributor

Hi @NilanshBansal , I would like to see what went wrong during deserialization / serialization and would also like to fix the root cause. I am part of the community discord server, if you want to connect there. I can look at this if no one else is.

@akshayvijayjain akshayvijayjain mentioned this issue May 20, 2024
Closed
NilanshBansal added a commit that referenced this issue May 22, 2024
@NilanshBansal
fix: handled cache deserialization errors (#33504)
d11d404
@NilanshBansal NilanshBansal mentioned this issue May 22, 2024
Merged
2 tasks
@NilanshBansal
Copy link
Contributor Author

NilanshBansal commented May 22, 2024 •
edited

This task is implemented and completed in this commit d11d404.
The PR is in draft state as there should be test cases and a counterpart EE PR to complete the functionality of implementing tenant caching. This task cannot be shipped independently hence moving to blocked state.

NilanshBansal added a commit that referenced this issue May 27, 2024
@NilanshBansal
fix: handled cache deserialization errors (#33504)
e247b85
NilanshBansal added a commit that referenced this issue May 28, 2024
@NilanshBansal @mohanarpit
feat: Adding Tenant Information to Redis Cache for quick fetch (#33641)
b6b00d0 
## 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
<!-- This is an auto-generated comment: 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>

<!-- end of auto-generated comment: Cypress test results  -->












## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [ ] No

---------

Co-authored-by: Arpit Mohan <mohanarpit@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@NilanshBansal NilanshBansal

Labels
Integrations Pod General Issues related to the Integrations Pod that don't fit into other tags. Integrations Pod Issues related to a specific integration Performance Pod All things related to Appsmith performance Performance Issues related to performance Task A simple Todo
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@AdeshAtole @NilanshBansal