From c6d9073b5ed3abb62c1bd46391ebe7875ea4dff4 Mon Sep 17 00:00:00 2001 From: sneha122 Date: Tue, 25 Feb 2025 22:03:11 +0530 Subject: [PATCH] fix: connection pool config added for dynamoDB to avoid stale connections (#38940) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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-server` module but dynamoDb code exists in `appsmith-plugins -> dynamoDB` module, so in order to provide feature flagging, ideally we should make feature flagging service available at `appsmith-interface` module to ensure that it can get consumed across different modules but that's a [known tech debt](https://github.com/appsmithorg/appsmith/issues/39425) 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](https://github.com/appsmithorg/appsmith/issues/39426) **References:** - https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/http/apache/ApacheHttpClient.Builder.html#connectionTimeToLive(java.time.Duration) - https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/http/SdkHttpConfigurationOption.html#CONNECTION_TIME_TO_LIVE - https://github.com/aws/aws-sdk-java/issues/2788 Fixes #[`Issue Number`](https://github.com/appsmithorg/appsmith-ee/issues/6030) _or_ Fixes `Issue URL` > [!WARNING] > _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" ### :mag: Cypress test results > [!TIP] > ๐ŸŸข ๐ŸŸข ๐ŸŸข All cypress tests have passed! ๐ŸŽ‰ ๐ŸŽ‰ ๐ŸŽ‰ > Workflow run: > Commit: 953e570b2e153087fa7b27c8c24cb50f74333159 > Cypress dashboard. > Tags: `@tag.Datasource` > Spec: >
Tue, 25 Feb 2025 07:21:25 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit - **New Features** - Enhanced DynamoDB plugin connection management with configurable connection time-to-live - Added Apache HTTP client support for improved network connection handling - **Dependencies** - Updated Apache client dependency to version 2.15.3 --------- Co-authored-by: โ€œsneha122โ€ <โ€œsneha@appsmith.comโ€> --- .../external/enums/FeatureFlagEnum.java | 1 + .../external/plugins/PluginExecutor.java | 33 ++++++++ .../appsmith-plugins/dynamoPlugin/pom.xml | 17 ++++ .../com/external/plugins/DynamoPlugin.java | 79 ++++++++++++++----- .../base/DatasourceServiceCEImpl.java | 20 ++++- .../DatasourceContextServiceImpl.java | 6 +- .../ce/DatasourceContextServiceCEImpl.java | 30 ++++++- 7 files changed, 159 insertions(+), 27 deletions(-) diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java index bde30a1ef6..22f1423cb7 100644 --- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java @@ -20,6 +20,7 @@ public enum FeatureFlagEnum { // Deprecated CE flags over here release_git_autocommit_feature_enabled, release_git_autocommit_eligibility_enabled, + release_dynamodb_connection_time_to_live_enabled, // Add EE flags below this line, to avoid conflicts. } diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java index 5f9cdb89d1..3613857392 100644 --- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java @@ -295,6 +295,39 @@ public interface PluginExecutor extends ExtensionPoint, CrudTemplateService { Map featureFlagMap) { return this.trigger(connection, datasourceConfiguration, request); } + /* + * Feature flagging implementation is added here as a potential fix to dynamoDB query timeouts problem + * This is a temporary fix and will be removed once we get the confirmation from the user that issue is resolved + * Even if the issue is not resolved, we will know that fix does not work and hence will be removing the code in any case + * https://github.com/appsmithorg/appsmith/issues/39426 Created task here to remove this flag + * This implementation ensures that none of the existing plugins have any impact due to feature flagging, hence if else condition + * This applies to both datasourceCreate and testDatasource methods added below + * */ + default Mono datasourceCreate( + DatasourceConfiguration datasourceConfiguration, Boolean isDynamoDBConnectionTimeToLiveEnabled) { + return this.datasourceCreate(datasourceConfiguration); + } + + default Mono testDatasource( + DatasourceConfiguration datasourceConfiguration, Boolean isDynamoDBConnectionTimeToLiveEnabled) { + return this.datasourceCreate(datasourceConfiguration, isDynamoDBConnectionTimeToLiveEnabled) + .flatMap(connection -> { + return this.testDatasource(connection).doFinally(signal -> this.datasourceDestroy(connection)); + }) + .onErrorResume(error -> { + // We always expect to have an error object, but the error object may not be well-formed + final String errorMessage = error.getMessage() == null + ? AppsmithPluginError.PLUGIN_DATASOURCE_TEST_GENERIC_ERROR.getMessage() + : error.getMessage(); + if (error instanceof AppsmithPluginException + && StringUtils.hasLength(((AppsmithPluginException) error).getDownstreamErrorMessage())) { + return Mono.just(new DatasourceTestResult( + ((AppsmithPluginException) error).getDownstreamErrorMessage(), errorMessage)); + } + return Mono.just(new DatasourceTestResult(errorMessage)); + }) + .subscribeOn(Schedulers.boundedElastic()); + } /** * This function is responsible for preparing the action and datasource configurations to be ready for execution. diff --git a/app/server/appsmith-plugins/dynamoPlugin/pom.xml b/app/server/appsmith-plugins/dynamoPlugin/pom.xml index 54e544bc7a..d1febad2ed 100644 --- a/app/server/appsmith-plugins/dynamoPlugin/pom.xml +++ b/app/server/appsmith-plugins/dynamoPlugin/pom.xml @@ -32,6 +32,23 @@ + + + software.amazon.awssdk + apache-client + 2.15.3 + compile + + + org.slf4j + slf4j-api + + + com.fasterxml.jackson.core + jackson-databind + + + diff --git a/app/server/appsmith-plugins/dynamoPlugin/src/main/java/com/external/plugins/DynamoPlugin.java b/app/server/appsmith-plugins/dynamoPlugin/src/main/java/com/external/plugins/DynamoPlugin.java index 01949363b7..39b5f9e09a 100644 --- a/app/server/appsmith-plugins/dynamoPlugin/src/main/java/com/external/plugins/DynamoPlugin.java +++ b/app/server/appsmith-plugins/dynamoPlugin/src/main/java/com/external/plugins/DynamoPlugin.java @@ -29,6 +29,7 @@ import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; import software.amazon.awssdk.core.SdkBytes; import software.amazon.awssdk.core.SdkField; import software.amazon.awssdk.core.SdkPojo; +import software.amazon.awssdk.http.apache.ApacheHttpClient; import software.amazon.awssdk.regions.Region; import software.amazon.awssdk.services.dynamodb.DynamoDbClient; import software.amazon.awssdk.services.dynamodb.DynamoDbClientBuilder; @@ -42,6 +43,7 @@ import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; import java.lang.reflect.WildcardType; import java.net.URI; +import java.time.Duration; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -70,6 +72,7 @@ public class DynamoPlugin extends BasePlugin { private static final String DYNAMO_TYPE_BINARY_SET_LABEL = "BS"; private static final String DYNAMO_TYPE_MAP_LABEL = "M"; private static final String DYNAMO_TYPE_LIST_LABEL = "L"; + private static final Duration CONNECTION_TIME_TO_LIVE = Duration.ofDays(1); public DynamoPlugin(PluginWrapper wrapper) { super(wrapper); @@ -280,6 +283,40 @@ public class DynamoPlugin extends BasePlugin { .subscribeOn(scheduler); } + @Override + public Mono datasourceCreate( + DatasourceConfiguration datasourceConfiguration, Boolean isFlagEnabled) { + log.debug(Thread.currentThread().getName() + ": datasourceCreate() called for Dynamo plugin."); + return Mono.fromCallable(() -> { + log.debug(Thread.currentThread().getName() + ": creating dynamodbclient from DynamoDB plugin."); + + /** + * Current understanding is that the issue mentioned in #6030 is because of is because of connection object getting stale. + * Setting connection to live value to 1 day is a precaution move to make sure that we don't land into a situation + * where an older connection can malfunction. + * To understand what this config means please check here: + * {@link https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/http/apache/ApacheHttpClient.Builder.html#connectionTimeToLive(java.time.Duration)} + */ + DynamoDbClientBuilder builder; + if (isFlagEnabled) { + log.debug("DynamoDB client builder created with custom connection time to live"); + builder = DynamoDbClient.builder() + .httpClient(ApacheHttpClient.builder() + .connectionTimeToLive(CONNECTION_TIME_TO_LIVE) + .build()); + } else { + builder = DynamoDbClient.builder(); + } + + return getDynamoDBClientObject(datasourceConfiguration, builder); + }) + .subscribeOn(scheduler); + } + + // This method is not being used right now as we had created a separate method with the same name in the + // DynamoPlugin class. + // which creates dynamoDB client based on feature flagging + // Once feature flagging is removed, we can go back to using this method. @Override public Mono datasourceCreate(DatasourceConfiguration datasourceConfiguration) { log.debug(Thread.currentThread().getName() + ": datasourceCreate() called for Dynamo plugin."); @@ -287,26 +324,7 @@ public class DynamoPlugin extends BasePlugin { log.debug(Thread.currentThread().getName() + ": creating dynamodbclient from DynamoDB plugin."); final DynamoDbClientBuilder builder = DynamoDbClient.builder(); - if (!CollectionUtils.isEmpty(datasourceConfiguration.getEndpoints())) { - final Endpoint endpoint = - datasourceConfiguration.getEndpoints().get(0); - builder.endpointOverride( - URI.create("http://" + endpoint.getHost() + ":" + endpoint.getPort())); - } - - final DBAuth authentication = (DBAuth) datasourceConfiguration.getAuthentication(); - if (authentication == null || !StringUtils.hasLength(authentication.getDatabaseName())) { - throw new AppsmithPluginException( - AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR, - DynamoErrorMessages.MISSING_REGION_ERROR_MSG); - } - - builder.region(Region.of(authentication.getDatabaseName())); - - builder.credentialsProvider(StaticCredentialsProvider.create(AwsBasicCredentials.create( - authentication.getUsername(), authentication.getPassword()))); - - return builder.build(); + return getDynamoDBClientObject(datasourceConfiguration, builder); }) .subscribeOn(scheduler); } @@ -611,4 +629,25 @@ public class DynamoPlugin extends BasePlugin { } return true; } + + private static DynamoDbClient getDynamoDBClientObject( + DatasourceConfiguration dsConfig, DynamoDbClientBuilder builder) { + if (!CollectionUtils.isEmpty(dsConfig.getEndpoints())) { + final Endpoint endpoint = dsConfig.getEndpoints().get(0); + builder.endpointOverride(URI.create("http://" + endpoint.getHost() + ":" + endpoint.getPort())); + } + + final DBAuth authentication = (DBAuth) dsConfig.getAuthentication(); + if (authentication == null || !StringUtils.hasLength(authentication.getDatabaseName())) { + throw new AppsmithPluginException( + AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR, DynamoErrorMessages.MISSING_REGION_ERROR_MSG); + } + + builder.region(Region.of(authentication.getDatabaseName())); + + builder.credentialsProvider(StaticCredentialsProvider.create( + AwsBasicCredentials.create(authentication.getUsername(), authentication.getPassword()))); + + return builder.build(); + } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java index df7decf159..591ea75890 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java @@ -661,8 +661,24 @@ public class DatasourceServiceCEImpl implements DatasourceServiceCE { .switchIfEmpty(Mono.error(new AppsmithException( AppsmithError.NO_RESOURCE_FOUND, FieldName.PLUGIN, datasourceStorage.getPluginId()))); - return pluginExecutorMono.flatMap(pluginExecutor -> ((PluginExecutor) pluginExecutor) - .testDatasource(datasourceStorage.getDatasourceConfiguration())); + // Feature flagging implementation is added here as a potential fix to dynamoDB query timeouts problem + // This is a temporary fix and will be removed once we get the confirmation from the user that issue is resolved + // Even if the issue is not resolved, we will know that fix does not work and hence will be removing the code in + // any case + // https://github.com/appsmithorg/appsmith/issues/39426 Created task here to remove this flag + // This implementation ensures that none of the existing plugins have any impact due to feature flagging, hence + // if else condition + return featureFlagService + .check(FeatureFlagEnum.release_dynamodb_connection_time_to_live_enabled) + .flatMap(isDynamoDBConnectionTimeToLiveEnabled -> { + if (isDynamoDBConnectionTimeToLiveEnabled) { + return pluginExecutorMono.flatMap(pluginExecutor -> ((PluginExecutor) pluginExecutor) + .testDatasource(datasourceStorage.getDatasourceConfiguration(), true)); + } else { + return pluginExecutorMono.flatMap(pluginExecutor -> ((PluginExecutor) pluginExecutor) + .testDatasource(datasourceStorage.getDatasourceConfiguration())); + } + }); } /* diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/DatasourceContextServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/DatasourceContextServiceImpl.java index 1a686d1635..c2d77e79a2 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/DatasourceContextServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/DatasourceContextServiceImpl.java @@ -20,7 +20,8 @@ public class DatasourceContextServiceImpl extends DatasourceContextServiceCEImpl PluginService pluginService, PluginExecutorHelper pluginExecutorHelper, ConfigService configService, - DatasourcePermission datasourcePermission) { + DatasourcePermission datasourcePermission, + FeatureFlagService featureFlagService) { super( datasourceService, @@ -28,6 +29,7 @@ public class DatasourceContextServiceImpl extends DatasourceContextServiceCEImpl pluginService, pluginExecutorHelper, configService, - datasourcePermission); + datasourcePermission, + featureFlagService); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java index 69632105e0..d4a9682101 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java @@ -3,6 +3,7 @@ package com.appsmith.server.services.ce; import com.appsmith.external.constants.PluginConstants; import com.appsmith.external.dtos.ExecutePluginDTO; import com.appsmith.external.dtos.RemoteDatasourceDTO; +import com.appsmith.external.enums.FeatureFlagEnum; import com.appsmith.external.exceptions.pluginExceptions.AppsmithPluginException; import com.appsmith.external.exceptions.pluginExceptions.StaleConnectionException; import com.appsmith.external.models.DatasourceStorage; @@ -20,6 +21,7 @@ import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.helpers.PluginExecutorHelper; import com.appsmith.server.plugins.base.PluginService; import com.appsmith.server.services.ConfigService; +import com.appsmith.server.services.FeatureFlagService; import com.appsmith.server.solutions.DatasourcePermission; import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; @@ -68,6 +70,7 @@ public class DatasourceContextServiceCEImpl implements DatasourceContextServiceC private final PluginExecutorHelper pluginExecutorHelper; private final ConfigService configService; private final DatasourcePermission datasourcePermission; + private final FeatureFlagService featureFlagService; private final AppsmithException TOO_MANY_REQUESTS_EXCEPTION = new AppsmithException(AppsmithError.TOO_MANY_FAILED_DATASOURCE_CONNECTION_REQUESTS); @@ -79,7 +82,8 @@ public class DatasourceContextServiceCEImpl implements DatasourceContextServiceC PluginService pluginService, PluginExecutorHelper pluginExecutorHelper, ConfigService configService, - DatasourcePermission datasourcePermission) { + DatasourcePermission datasourcePermission, + FeatureFlagService featureFlagService) { this.datasourceService = datasourceService; this.datasourceStorageService = datasourceStorageService; this.pluginService = pluginService; @@ -89,6 +93,7 @@ public class DatasourceContextServiceCEImpl implements DatasourceContextServiceC this.datasourceContextSynchronizationMonitorMap = new ConcurrentHashMap<>(); this.configService = configService; this.datasourcePermission = datasourcePermission; + this.featureFlagService = featureFlagService; } private RemovalListener createRemovalListener() { @@ -209,8 +214,27 @@ public class DatasourceContextServiceCEImpl implements DatasourceContextServiceC /* Create a fresh datasource context */ DatasourceContext datasourceContext = new DatasourceContext<>(); - Mono connectionMonoCache = pluginExecutor - .datasourceCreate(datasourceStorage.getDatasourceConfiguration()) + + // Feature flagging implementation is added here as a potential fix to dynamoDB query timeouts + // problem + // This is a temporary fix and will be removed once we get the confirmation from the user that + // issue is resolved + // Even if the issue is not resolved, we will know that fix does not work and hence will be + // removing the code in any case + // https://github.com/appsmithorg/appsmith/issues/39426 Created task here to remove this flag + // This implementation ensures that none of the existing plugins have any impact due to feature + // flagging, hence if else condition + Mono connectionMonoCache = featureFlagService + .check(FeatureFlagEnum.release_dynamodb_connection_time_to_live_enabled) + .flatMap(isDynamoDBConnectionTimeToLiveEnabled -> { + if (isDynamoDBConnectionTimeToLiveEnabled) { + return pluginExecutor.datasourceCreate( + datasourceStorage.getDatasourceConfiguration(), true); + } else { + return pluginExecutor.datasourceCreate( + datasourceStorage.getDatasourceConfiguration()); + } + }) .cache(); Mono> datasourceContextMonoCache = connectionMonoCache