fix: connection pool config added for dynamoDB to avoid stale connections (#38940)
## 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" ### 🔍 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/13515219987> > Commit: 953e570b2e153087fa7b27c8c24cb50f74333159 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13515219987&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Datasource` > Spec: > <hr>Tue, 25 Feb 2025 07:21:25 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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 <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: “sneha122” <“sneha@appsmith.com”>
This commit is contained in:
sneha122
GitHub
parent
63b391d723
commit
c6d9073b5e
|
|
@ -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.
|
||||
}
|
||||
|
|
|
|||
|
|
@ -295,6 +295,39 @@ public interface PluginExecutor<C> extends ExtensionPoint, CrudTemplateService {
|
|||
Map<String, Boolean> 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<C> datasourceCreate(
|
||||
DatasourceConfiguration datasourceConfiguration, Boolean isDynamoDBConnectionTimeToLiveEnabled) {
|
||||
return this.datasourceCreate(datasourceConfiguration);
|
||||
}
|
||||
|
||||
default Mono<DatasourceTestResult> 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.
|
||||
|
|
|
|||
|
|
@ -32,6 +32,23 @@
|
|||
</exclusion>
|
||||
</exclusions>
|
||||
</dependency>
|
||||
|
||||
<dependency>
|
||||
<groupId>software.amazon.awssdk</groupId>
|
||||
<artifactId>apache-client</artifactId>
|
||||
<version>2.15.3</version>
|
||||
<scope>compile</scope>
|
||||
<exclusions>
|
||||
<exclusion>
|
||||
<groupId>org.slf4j</groupId>
|
||||
<artifactId>slf4j-api</artifactId>
|
||||
</exclusion>
|
||||
<exclusion>
|
||||
<groupId>com.fasterxml.jackson.core</groupId>
|
||||
<artifactId>jackson-databind</artifactId>
|
||||
</exclusion>
|
||||
</exclusions>
|
||||
</dependency>
|
||||
</dependencies>
|
||||
|
||||
<build>
|
||||
|
|
|
|||
|
|
@ -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<DynamoDbClient> 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<DynamoDbClient> 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();
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<Object>) 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<Object>) pluginExecutor)
|
||||
.testDatasource(datasourceStorage.getDatasourceConfiguration(), true));
|
||||
} else {
|
||||
return pluginExecutorMono.flatMap(pluginExecutor -> ((PluginExecutor<Object>) pluginExecutor)
|
||||
.testDatasource(datasourceStorage.getDatasourceConfiguration()));
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
/*
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<DatasourceContextIdentifier, DatasourcePluginContext> createRemovalListener() {
|
||||
|
|
@ -209,8 +214,27 @@ public class DatasourceContextServiceCEImpl implements DatasourceContextServiceC
|
|||
|
||||
/* Create a fresh datasource context */
|
||||
DatasourceContext<Object> datasourceContext = new DatasourceContext<>();
|
||||
Mono<Object> 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<Object> 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<DatasourceContext<Object>> datasourceContextMonoCache = connectionMonoCache
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user