feat: added LRU cache for mockDB connections #36480
Conversation
WalkthroughThe changes introduced in this pull request include the addition of a new constant for the MongoDB plugin, the creation of a Changes
Assessment against linked issues
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
|
github-actions
bot
added
Enhancement
...ith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
NilanshBansal
requested review from
nidhi-nair and
sondermanish
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/DatasourcePluginContext.java (2)
9-12: Excellent use of Lombok annotations, students! But let's consider one more.Your use of @Getter, @Setter, and @tostring annotations is commendable. It keeps our code clean and readable. However, have you considered adding the @EqualsAndHashCode annotation as well? This would provide a proper implementation of the equals and hashCode methods, which could be useful if you need to compare DatasourcePluginContext objects or use them in collections.
Consider adding the @EqualsAndHashCode annotation:
@Getter @Setter @ToString +@EqualsAndHashCode public class DatasourcePluginContext<T> {
1-20: Overall, a commendable effort in supporting our caching strategy!Class, your
DatasourcePluginContextis a solid foundation for our LRU caching mechanism. It aligns well with our objective of managing mockDB connections more effectively. The inclusion of acreationTimefield is particularly useful for implementing cache eviction based on age.To further improve:
- Consider adding documentation comments to explain the purpose of this class and its fields.
- You might want to implement a
getAge()method that calculates the time elapsed since creation, which could be useful for the LRU algorithm.Remember, clear and well-structured code like this is key to solving our connection management issues (problem #36474). Keep up the good work!
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/PluginConstants.java (1)
27-27: Class, pay attention to this new addition!The addition of the
MONGO_PLUGINconstant is a welcome improvement to our constants collection. It's like adding a new word to our vocabulary! However, just as we arrange words alphabetically in a dictionary, it would be beneficial to maintain alphabetical order in our constants list.Consider reordering the constants alphabetically within the
PackageNameinterface. This will make it easier for your fellow students to find specific constants in the future. Remember, organization is key to maintaining a tidy codebase!app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java (2)
53-53: Please pay attention to spacing in your comments.In line 53, there's a missing space after the period. Proper spacing enhances readability. It should read: "connection. The cleanup process is performed after every 2 hours."
54-54: Clarify the term "movies mockDB" for better understanding.Using specific references like "movies mockDB" might be confusing if it's not commonly known. Consider using "the mockDB" or a more general term to make your documentation clear to all readers.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/PluginConstants.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/domains/DatasourcePluginContext.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java (9 hunks)
Additional comments not posted (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/DatasourcePluginContext.java (1)
1-8: Well done, class! The package declaration and imports are spot on.You've correctly declared the package and imported the necessary Lombok annotations and the
Instantclass. This shows good organization and understanding of the tools at your disposal.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java (4)
220-221: Excellent use of cache access for updating last accessed time.Accessing the cache with
getIfPresentis a good strategy to update the entry's last accessed time without affecting its value. This helps maintain the entry in the cache appropriately.
246-269: Good implementation of conditional caching for mock Mongo datasources.Your logic correctly ensures that only mock Mongo datasources are cached in the LRU cache. This is an effective way to manage resources specific to mock datasources.
297-297: Verify the parameters inupdateDatasourceStoragecall.It's important to confirm that passing
FALSEandfalseas parameters toupdateDatasourceStoragealigns with the method's expectations. Please ensure these parameters correctly represent the desired behavior.
503-507: Proper cache invalidation upon datasource context deletion.Invalidating the LRU cache when a datasource context is deleted helps prevent stale data and is a good practice.
...erver/appsmith-server/src/main/java/com/appsmith/server/domains/DatasourcePluginContext.java
Show resolved
Hide resolved
...erver/appsmith-server/src/main/java/com/appsmith/server/domains/DatasourcePluginContext.java
Outdated
Show resolved
Hide resolved
...ith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...ith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
…ssue-36474/implement-lru-caching-for-mockdb-connections
This comment was marked as outdated.
This comment was marked as outdated.
1 similar comment
This comment was marked as outdated.
This comment was marked as outdated.
...ith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...ith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...ith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java
Show resolved
Hide resolved
...ith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I overlooked this point during my review. This is an in-memory cache, not shared across different pods. Doesn’t that mean the cache will only be effective for subsequent requests to the same pod? Each pod will have its own cache, which could lead to excessive caching and undermine the original goal.
@subrata71 yes the cache is not shared across the different pods and as per the current and projected traffic on mockDB connections we are reaching the limit of 3k connections in 72 hours combined on all pods. |
|
@NilanshBansal |
|
As per discussion with @subrata71, I have added the maximum size limit of the cache per pod and also added the logs around the cache size after addition and eviction. |
|
Tag.All has already passed on this PR hence updated with Tag.Sanity as only log level changes are done post it. |
Failed server tests
|
NilanshBansal
deleted the
feat/issue-36474/implement-lru-caching-for-mockdb-connections
branch
Description
This PR implements an LRU (Least Recently Used) caching mechanism for the Mongo mockDB connections which get auto-cleaned up every 2 hours based on their access time.
This is done to stop the overpopulation of the open dangling connections to the mockDB resulting in the max connection limit.
The Caching Implementation used is Google Guave Cache.
Also refer - Working of Guava cache
Fixes #36474
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11066462645
Commit: 38fcf57
Cypress dashboard.
Tags:
@tag.SanitySpec:
Fri, 27 Sep 2024 08:01:28 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
DatasourcePluginContextclass to encapsulate datasource plugin context, including connection details and creation time.Bug Fixes