fix: connection pool config added for dynamoDB to avoid stale connections #38940
Conversation
WalkthroughThe pull request introduces improvements to the DynamoDB plugin's connection management by adding an Apache HTTP client dependency and configuring connection time-to-live settings. The changes involve updating the Changes
Sequence DiagramsequenceDiagram
participant Plugin as DynamoPlugin
participant ClientBuilder as DynamoDbClientBuilder
participant HttpClient as ApacheHttpClient
Plugin->>ClientBuilder: Configure with ApacheHttpClient
ClientBuilder->>HttpClient: Set connection time to live (1 day)
HttpClient-->>ClientBuilder: Return configured HTTP client
ClientBuilder-->>Plugin: Create DynamoDB Client
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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
|
sneha122
requested review from
NilanshBansal
and removed request for
sharat87
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/server/appsmith-plugins/dynamoPlugin/src/main/java/com/external/plugins/DynamoPlugin.java (1)
291-299: LGTM with a minor formatting suggestion.The connection TTL configuration is properly implemented. Consider reformatting the multi-line comment into a single line or proper Javadoc format.
- // Configuring connection time to live as 1 day so that we don't face issues with stale - // connections - // in the connection pool. - // 1 Day is added randomly as a time which is not too less or too high + // Configuring connection time to live as 1 day to prevent stale connections in the connection pool
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/server/appsmith-plugins/dynamoPlugin/pom.xml(1 hunks)app/server/appsmith-plugins/dynamoPlugin/src/main/java/com/external/plugins/DynamoPlugin.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (3)
app/server/appsmith-plugins/dynamoPlugin/pom.xml (1)
36-51: LGTM! Well-structured dependency addition.The apache-client dependency is properly configured with matching version and consistent exclusions.
app/server/appsmith-plugins/dynamoPlugin/src/main/java/com/external/plugins/DynamoPlugin.java (2)
32-32: LGTM! Good choice of TTL duration.The one-day connection TTL is a reasonable default that balances connection reuse with preventing stale connections.
Also applies to: 75-75
291-299: Consider adding integration tests.While this is an infrastructure change, it would be valuable to add integration tests verifying that connections are properly recycled after the TTL period.
✅ Verification successful
Additional integration tests not required
The existing test suite already provides comprehensive integration testing for connection management, including connection establishment and error scenarios. The new TTL configuration is a standard AWS SDK setting that's implicitly verified by the existing integration tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any existing integration tests for connection management rg -l "integration.*test.*connection" --type javaLength of output: 51
Script:
#!/bin/bash # Search for DynamoPlugin test files and any connection-related tests echo "=== Searching for DynamoPlugin test files ===" rg -l "DynamoPlugin.*Test" --type java echo -e "\n=== Searching for connection-related tests in plugin ===" rg -l "dynamoPlugin.*test" --type java echo -e "\n=== Looking for test files in plugin directory ===" fd "Test.java" -p "dynamoPlugin"Length of output: 657
Script:
#!/bin/bash # Examine DynamoPluginTest.java content for connection-related tests echo "=== DynamoPluginTest.java content ===" cat app/server/appsmith-plugins/dynamoPlugin/src/test/java/com/external/plugins/DynamoPluginTest.javaLength of output: 24467
app/server/appsmith-plugins/dynamoPlugin/src/main/java/com/external/plugins/DynamoPlugin.java
Outdated
Show resolved
Hide resolved
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13163867406. |
|
Deploy-Preview-URL: https://ce-38940.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13281035475. |
|
Deploy-Preview-URL: https://ce-38940.dp.appsmith.com |
app/server/appsmith-plugins/dynamoPlugin/src/main/java/com/external/plugins/DynamoPlugin.java
Show resolved
Hide resolved
Failed server tests
|
app/server/appsmith-plugins/dynamoPlugin/src/main/java/com/external/plugins/DynamoPlugin.java
Show resolved
Hide resolved
app/server/appsmith-plugins/dynamoPlugin/src/main/java/com/external/plugins/DynamoPlugin.java
Outdated
Show resolved
Hide resolved
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13451287966. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13452023463. |
|
Deploy-Preview-URL: https://ce-38940.dp.appsmith.com |
Failed server tests
|
Failed server tests
|
NilanshBansal
added
ok-to-test
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13498256216. |
|
Deploy-Preview-URL: https://ce-38940.dp.appsmith.com |
Failed server tests
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13500314594. |
|
Deploy-Preview-URL: https://ce-38940.dp.appsmith.com |
sneha122
added
ok-to-test
There was a problem hiding this comment.
Approved as this is only a temporary change and feature flag will be removed in the next 2 sprints as per discussion with @sneha122
Description
Problem statement:
In one of the instances where dynamoDB had close to 200 odd queries on top of it, queries were timing out after a month or so, this happened 3 times with one of the users, where after a month, queries would suddenly start timing out. One of the hunches was that since we create dynamoDBclient object only once during datasource creation, it could be getting stale thus resulting in query timeouts and with that hunch, we decided to add a fix where we close the old connections regularly after using for a day.
Solution:
This PR configures connection time to live property for DynamoDB plugin. This is configured to be 1 day which means after 1 day, active/idle connections will be closed. This is done to ensure that we don't face issues if the connection becomes stale. Default value of connection time to live is 0 which means the connection stays open for infinite amount of time.
To ensure that we do not cause any issues due to introduction of this property, we decided to add this behind a feature flag. All the feature flagging related code exists at the
appsmith-servermodule but dynamoDb code exists inappsmith-plugins -> dynamoDBmodule, so in order to provide feature flagging, ideally we should make feature flagging service available atappsmith-interfacemodule to ensure that it can get consumed across different modules but that's a known tech debt which we should resolve soon, but in the meantime to expedite the fix, I have passed in flag details to respective datasourceCreate functionality. datasourceCreate also gets called from testDatasource, so feature flagging has been implemented there as well.At some point in time, based on whether this potential fix works or not, we should plan to remove the feature flagging code, task for that added here
References:
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="@tag.Datasource"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13515219987
Commit: 953e570
Cypress dashboard.
Tags:
@tag.DatasourceSpec:
Tue, 25 Feb 2025 07:21:25 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Dependencies