From d5af8794fd38485682652d5e1b05a03906bd9dd4 Mon Sep 17 00:00:00 2001 From: Branimir Vujicic Date: Mon, 18 Sep 2023 10:34:17 +0200 Subject: [PATCH] Add Feature Toggles Feature Toggles should allow teams to modify system behavior without changing code. Feature Toggles are configured using google guice. Basic definition of toggles are crated using FeatureToggleBinder. FeatureToggleBinder creates FeatureToggle and additional configuration can be done using feature configuration. In current stage Feature Toggles supports: - if / else based feature toggles - Dependency Injection based Hot reloading implementation without restart require code refactoring to add an interface when injecting the new implementation/class - using various toggle strategies along with simple on / off toggles configuration: to allow feature toggle configuration four lines are needed in config.properties file ``` features.config-source-type=file features.config-source=/etc/feature-config.properties features.config-type=properties features.refresh-period=30s ``` `configuration-source-type` is source type for Feature Toggles configuration `features.config-source` is a source (file) of the configuration `features.config-type` format in which configuration is stored (json or properties) `features.refresh-period` configuration refresh period Defining Feature Toggles Feature toggle definition is done in google guice module using `FeatureToggleBinder` simple feature toggle definition ``` featureToggleBinder(binder) .featureId("featureXX") .bind() ``` This example creates bindings for @Inject ``` @Inject public Runner(@FeatureToggle("featureXX") Supplier isFeatureXXEnabled) { this.isFeatureXXEnabled = isFeatureXXEnabled; } ``` `isFeatureXXEnabled` can be used to test if feature is enabled or disabled: ``` boolean testFeatureXXEnabled() { return isFeatureXXEnabled.get(); } ``` hot reloadable feature toggle definition ``` featureToggleBinder(binder, Feature01.class) .featureId("feature01") .baseClass(Feature01.class) .defaultClass(Feature01Impl01.class) .allOf(Feature01Impl01.class, Feature01Impl02.class) .bind() ``` adding Feature Toggle switching strategy ``` featureToggleBinder(binder) .featureId("feature04") .toggleStrategy("AllowAll") .toggleStrategyConfig(ImmutableMap.of("key", "value", "key2", "value2")) ``` feature-config.properties file example ``` # feature query-logger feature.query-logger.enabled=true feature.query-logger.strategy=OsToggle feature.query-logger.strategy.os_name=.*Linux.* #feature.query-rate-limiter feature.query-rate-limiter.currentInstance=com.facebook.presto.server.protocol.QueryBlockingRateLimiter # feature.query-cancel feature.query-cancel.strategy=AllowList feature.query-cancel.strategy.allow-list-source=.*IDEA.* feature.query-cancel.strategy.allow-list-user=.*prestodb ``` in this example for first feature `query-logger` changing value of feature.query-logger.enabled to `false` will 'disable' this feature. Changes will be effective within refresh period. Pass column delimiter info to reader (#6338) Summary: Pull Request resolved: https://github.com/facebookincubator/velox/pull/6338 Reviewed By: Yuhta Differential Revision: D48457913 fbshipit-source-id: 57d76dfa229de3801bf3181f780a485b628427ad --- presto-main/etc/catalog/hive.properties | 6 + presto-main/etc/config.properties | 6 + .../etc/feature-toggle/file.properties | 3 + .../presto/server/CoordinatorModule.java | 37 ++++++ .../presto/server/ServerMainModule.java | 2 +- .../AnotherQueryBlockingRateLimiter.java | 118 ++++++++++++++++++ .../protocol/ExecutingStatementResource.java | 58 ++++++++- .../server/protocol/LocalQueryProvider.java | 4 +- .../presto/server/protocol/Query.java | 6 +- .../protocol/QueryBlockingRateLimiter.java | 4 + .../server/protocol/QueryRateLimiter.java | 34 +++++ .../protocol/QueuedStatementResource.java | 4 +- .../server/protocol/RetryCircuitBreaker.java | 4 + .../protocol/RetryCircuitBreakerInt.java | 26 ++++ .../presto/server/protocol/http-queries.http | 29 +++++ .../strategy/AllowListToggleStrategyTest.java | 1 - presto-memory-context/pom.xml | 11 ++ .../context/ft/MemoryContextModule.java | 31 +++++ .../ft/MemoryFeatureToggleInterface.java | 18 +++ .../ft/MemoryFeatureToggleInterfaceImpl.java | 19 +++ presto-memory/pom.xml | 11 ++ .../presto/plugin/memory/MemoryModule.java | 3 + 22 files changed, 421 insertions(+), 14 deletions(-) create mode 100644 presto-main/etc/feature-toggle/file.properties create mode 100644 presto-main/src/main/java/com/facebook/presto/server/protocol/AnotherQueryBlockingRateLimiter.java create mode 100644 presto-main/src/main/java/com/facebook/presto/server/protocol/QueryRateLimiter.java create mode 100644 presto-main/src/main/java/com/facebook/presto/server/protocol/RetryCircuitBreakerInt.java create mode 100644 presto-main/src/main/java/com/facebook/presto/server/protocol/http-queries.http create mode 100644 presto-memory-context/src/main/java/com/facebook/presto/memory/context/ft/MemoryContextModule.java create mode 100644 presto-memory-context/src/main/java/com/facebook/presto/memory/context/ft/MemoryFeatureToggleInterface.java create mode 100644 presto-memory-context/src/main/java/com/facebook/presto/memory/context/ft/MemoryFeatureToggleInterfaceImpl.java diff --git a/presto-main/etc/catalog/hive.properties b/presto-main/etc/catalog/hive.properties index 9060aff2a6a7..7b62d2853910 100644 --- a/presto-main/etc/catalog/hive.properties +++ b/presto-main/etc/catalog/hive.properties @@ -7,3 +7,9 @@ connector.name=hive-hadoop2 hive.metastore.uri=thrift://localhost:9083 + +hive.allow-drop-table=true +hive.allow-rename-table=true +hive.allow-add-column=true +hive.allow-drop-column=true +hive.allow-rename-column=true diff --git a/presto-main/etc/config.properties b/presto-main/etc/config.properties index ba7eeee32d0f..9123c16301e0 100644 --- a/presto-main/etc/config.properties +++ b/presto-main/etc/config.properties @@ -6,6 +6,7 @@ # # sample nodeId to provide consistency across test runs + node.id=ffffffff-ffff-ffff-ffff-ffffffffffff node.environment=test http-server.http.port=8080 @@ -54,3 +55,8 @@ plugin.bundles=\ presto.version=testversion node-scheduler.include-coordinator=true + +# feature toggles +features.config-source-type=file +features.refresh-period=30s +features.configuration-directory=etc/feature-toggle/ diff --git a/presto-main/etc/feature-toggle/file.properties b/presto-main/etc/feature-toggle/file.properties new file mode 100644 index 000000000000..6b468a38c5ee --- /dev/null +++ b/presto-main/etc/feature-toggle/file.properties @@ -0,0 +1,3 @@ +features.config-source-type=file +features.config-source=/home/bane/java/etc/feature-config.properties +features.config-type=properties diff --git a/presto-main/src/main/java/com/facebook/presto/server/CoordinatorModule.java b/presto-main/src/main/java/com/facebook/presto/server/CoordinatorModule.java index 153fd93dc034..db4b88998684 100644 --- a/presto-main/src/main/java/com/facebook/presto/server/CoordinatorModule.java +++ b/presto-main/src/main/java/com/facebook/presto/server/CoordinatorModule.java @@ -57,6 +57,7 @@ import com.facebook.presto.execution.scheduler.SectionExecutionFactory; import com.facebook.presto.execution.scheduler.SplitSchedulerStats; import com.facebook.presto.failureDetector.FailureDetectorModule; +import com.facebook.presto.features.strategy.AllowListToggleStrategy; import com.facebook.presto.memory.ClusterMemoryManager; import com.facebook.presto.memory.ForMemoryManager; import com.facebook.presto.memory.LowMemoryKiller; @@ -65,16 +66,20 @@ import com.facebook.presto.memory.NoneLowMemoryKiller; import com.facebook.presto.memory.TotalReservationLowMemoryKiller; import com.facebook.presto.memory.TotalReservationOnBlockedNodesLowMemoryKiller; +import com.facebook.presto.memory.context.ft.MemoryContextModule; import com.facebook.presto.metadata.CatalogManager; import com.facebook.presto.operator.ForScheduler; import com.facebook.presto.operator.OperatorInfo; import com.facebook.presto.resourcemanager.ForResourceManager; import com.facebook.presto.resourcemanager.ResourceManagerProxy; +import com.facebook.presto.server.protocol.AnotherQueryBlockingRateLimiter; import com.facebook.presto.server.protocol.ExecutingStatementResource; import com.facebook.presto.server.protocol.LocalQueryProvider; import com.facebook.presto.server.protocol.QueryBlockingRateLimiter; +import com.facebook.presto.server.protocol.QueryRateLimiter; import com.facebook.presto.server.protocol.QueuedStatementResource; import com.facebook.presto.server.protocol.RetryCircuitBreaker; +import com.facebook.presto.server.protocol.RetryCircuitBreakerInt; import com.facebook.presto.server.remotetask.HttpRemoteTaskFactory; import com.facebook.presto.server.remotetask.RemoteTaskStats; import com.facebook.presto.spi.memory.ClusterMemoryPoolManager; @@ -88,6 +93,7 @@ import com.facebook.presto.transaction.TransactionManagerConfig; import com.facebook.presto.util.PrestoDataDefBindingHelper; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.inject.Binder; import com.google.inject.Provides; import com.google.inject.Scopes; @@ -116,6 +122,7 @@ import static com.facebook.presto.execution.DDLDefinitionExecution.DDLDefinitionExecutionFactory; import static com.facebook.presto.execution.SessionDefinitionExecution.SessionDefinitionExecutionFactory; import static com.facebook.presto.execution.SqlQueryExecution.SqlQueryExecutionFactory; +import static com.facebook.presto.features.binder.FeatureToggleBinder.featureToggleBinder; import static com.google.inject.multibindings.MapBinder.newMapBinder; import static com.google.inject.multibindings.OptionalBinder.newOptionalBinder; import static java.util.concurrent.Executors.newCachedThreadPool; @@ -140,11 +147,37 @@ protected void setup(Binder binder) // presto coordinator announcement discoveryBinder(binder).bindHttpAnnouncement("presto-coordinator"); + featureToggleBinder(binder, QueryRateLimiter.class) + .featureId("query-rate-limiter") + .baseClass(QueryRateLimiter.class) + .defaultClass(QueryBlockingRateLimiter.class) + .allOf(QueryBlockingRateLimiter.class, AnotherQueryBlockingRateLimiter.class) + .bind(); + featureToggleBinder(binder) .featureId("query-logger") .enabled(true) .bind(); + featureToggleBinder(binder, RetryCircuitBreakerInt.class) + .featureId("circuit-breaker") + .defaultClass(RetryCircuitBreaker.class) + .enabled(true) + .bind(); + + featureToggleBinder(binder) + .featureId("query-cancel") + .enabled(true) + .toggleStrategy("AllowList") + .registerToggleStrategy("AllowList", AllowListToggleStrategy.class) + .toggleStrategyConfig(ImmutableMap.of("allow-list-source", ".*IDEA.*", "allow-list-user", ".*prestodb")) + .bind(); + + featureToggleBinder(binder) + .registerToggleStrategy("AllowList", AllowListToggleStrategy.class); + + binder.install(new MemoryContextModule()); + // statement resource jsonCodecBinder(binder).bindJsonCodec(QueryInfo.class); jsonCodecBinder(binder).bindJsonCodec(TaskInfo.class); @@ -298,6 +331,10 @@ protected void setup(Binder binder) // cleanup binder.bind(ExecutorCleanup.class).in(Scopes.SINGLETON); +// +// binder.bind(PrestoFeatureToggle.class).in(Singleton.class); +// +// jaxrsBinder(binder).bind(FeatureToggleInfo.class); } @Provides diff --git a/presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java b/presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java index c4fc762d74ec..a09a8776a641 100644 --- a/presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java +++ b/presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java @@ -758,7 +758,7 @@ public ListeningExecutorService createResourceManagerExecutor(ResourceManagerCon // Feature Toggle configBinder(binder).bindConfig(FeatureToggleConfig.class); - install(new FeatureToggleModule()); + binder.install(new FeatureToggleModule()); // Thrift RPC binder.install(new DriftNettyServerModule()); diff --git a/presto-main/src/main/java/com/facebook/presto/server/protocol/AnotherQueryBlockingRateLimiter.java b/presto-main/src/main/java/com/facebook/presto/server/protocol/AnotherQueryBlockingRateLimiter.java new file mode 100644 index 000000000000..2005ba007473 --- /dev/null +++ b/presto-main/src/main/java/com/facebook/presto/server/protocol/AnotherQueryBlockingRateLimiter.java @@ -0,0 +1,118 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.facebook.presto.server.protocol; + +import com.facebook.airlift.stats.CounterStat; +import com.facebook.airlift.stats.TimeStat; +import com.facebook.presto.execution.QueryManagerConfig; +import com.facebook.presto.spi.QueryId; +import com.google.common.cache.CacheBuilder; +import com.google.common.cache.CacheLoader; +import com.google.common.cache.LoadingCache; +import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.ListeningExecutorService; +import com.google.common.util.concurrent.RateLimiter; +import com.google.inject.Inject; +import io.airlift.units.Duration; +import org.weakref.jmx.Managed; +import org.weakref.jmx.Nested; + +import javax.annotation.PreDestroy; + +import java.util.concurrent.ExecutorService; +import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.ThreadPoolExecutor; +import java.util.concurrent.TimeUnit; + +import static com.facebook.airlift.concurrent.Threads.daemonThreadsNamed; +import static com.google.common.util.concurrent.Futures.immediateFailedFuture; +import static com.google.common.util.concurrent.Futures.immediateFuture; +import static com.google.common.util.concurrent.MoreExecutors.listeningDecorator; +import static java.util.Objects.requireNonNull; + +/* + * Rate Limiting per query with token bucket + * Rate = rateLimitBucketMaxSize/second + * When having sufficient tokens, Request will be responded immediately. + * When not having enough tokens available, it uses the delayed processing method. + */ +public class AnotherQueryBlockingRateLimiter + implements QueryRateLimiter +{ + private final long rateLimiterBucketMaxSize; + private final ListeningExecutorService rateLimiterExecutorService; + private final LoadingCache rateLimiterCache; + private final CounterStat rateLimiterTriggeredCounter = new CounterStat(); + private final TimeStat rateLimiterBlockTime = new TimeStat(); + + @Inject + public AnotherQueryBlockingRateLimiter(QueryManagerConfig queryManagerConfig) + { + requireNonNull(queryManagerConfig, "queryManagerConfig is null"); + this.rateLimiterBucketMaxSize = queryManagerConfig.getRateLimiterBucketMaxSize(); + // Using a custom thread pool with size 1-10 to reduce initial thread resources + ExecutorService executorService = new ThreadPoolExecutor(1, 10, 60, TimeUnit.SECONDS, new LinkedBlockingQueue<>(), daemonThreadsNamed("rate-limiter-listener")); + rateLimiterExecutorService = listeningDecorator(executorService); + rateLimiterCache = CacheBuilder.newBuilder().maximumSize(queryManagerConfig.getRateLimiterCacheLimit()).expireAfterAccess(queryManagerConfig.getRateLimiterCacheWindowMinutes(), TimeUnit.MINUTES).build(CacheLoader.from(key -> RateLimiter.create(rateLimiterBucketMaxSize))); + } + + /* + * For accidental bug-caused DoS, we will use delayed processing method to reduce the requests, even when user do not have back-off logic implemented + * Optimized to avoid blocking for normal usages with TryRequire first + * Fall back to delayed processing method to acquire a permit, in a separate thread pool + * Internal guava rate limiter returns time spent sleeping to enforce rate, in seconds; 0.0 if not rate-limited, we use a future to wrap around that. + */ + @Override + public ListenableFuture acquire(QueryId queryId) + { // if rateLimitBucketMaxSize < 0, we disable rate limiting by returning immediately + if (rateLimiterBucketMaxSize < 0) { + return immediateFuture(0.0); + } + if (queryId == null) { + return immediateFailedFuture(new IllegalArgumentException("queryId should not be null")); + } + RateLimiter rateLimiter = rateLimiterCache.getUnchecked(queryId); + if (rateLimiter.tryAcquire()) { + return immediateFuture(0.0); + } + ListenableFuture asyncTask = rateLimiterExecutorService.submit(() -> rateLimiter.acquire()); + rateLimiterTriggeredCounter.update(1); + return asyncTask; + } + + @Managed + @Nested + public CounterStat getRateLimiterTriggeredCounter() + { + return rateLimiterTriggeredCounter; + } + + @Override + public TimeStat getRateLimiterBlockTime() + { + return rateLimiterBlockTime; + } + + @Override + public void addRateLimiterBlockTime(Duration duration) + { + rateLimiterBlockTime.add(duration); + } + + @PreDestroy + public void destroy() + { + rateLimiterExecutorService.shutdownNow(); + } +} diff --git a/presto-main/src/main/java/com/facebook/presto/server/protocol/ExecutingStatementResource.java b/presto-main/src/main/java/com/facebook/presto/server/protocol/ExecutingStatementResource.java index 97cdb16608df..e511837de8be 100644 --- a/presto-main/src/main/java/com/facebook/presto/server/protocol/ExecutingStatementResource.java +++ b/presto-main/src/main/java/com/facebook/presto/server/protocol/ExecutingStatementResource.java @@ -14,13 +14,17 @@ package com.facebook.presto.server.protocol; import com.facebook.airlift.concurrent.BoundedExecutor; +import com.facebook.airlift.log.Logger; import com.facebook.airlift.stats.TimeStat; import com.facebook.presto.client.QueryResults; +import com.facebook.presto.features.annotations.FeatureToggle; +import com.facebook.presto.memory.context.ft.MemoryFeatureToggleInterface; import com.facebook.presto.server.ForStatementResource; import com.facebook.presto.server.ServerConfig; import com.facebook.presto.spi.QueryId; import com.google.common.collect.Ordering; import com.google.common.util.concurrent.ListenableFuture; +import com.google.inject.Provider; import io.airlift.units.DataSize; import io.airlift.units.Duration; import org.weakref.jmx.Managed; @@ -42,6 +46,9 @@ import javax.ws.rs.core.Response; import javax.ws.rs.core.UriInfo; +import java.util.function.Function; +import java.util.function.Supplier; + import static com.facebook.airlift.http.server.AsyncResponseHandler.bindAsyncResponse; import static com.facebook.presto.client.PrestoHeaders.PRESTO_PREFIX_URL; import static com.facebook.presto.server.protocol.QueryResourceUtil.abortIfPrefixUrlInvalid; @@ -60,6 +67,8 @@ @RolesAllowed(USER) public class ExecutingStatementResource { + private static final Logger log = Logger.get(ExecutingStatementResource.class); + private static final Duration MAX_WAIT_TIME = new Duration(1, SECONDS); private static final Ordering> WAIT_ORDERING = Ordering.natural().nullsLast(); private static final DataSize DEFAULT_TARGET_RESULT_SIZE = new DataSize(1, MEGABYTE); @@ -68,26 +77,39 @@ public class ExecutingStatementResource private final BoundedExecutor responseExecutor; private final LocalQueryProvider queryProvider; private final boolean compressionEnabled; - private final QueryBlockingRateLimiter queryRateLimiter; + private final Provider queryRateLimiter; + private final Supplier isQueryLoggerEnabled; + private final Function isQueryCancelEnabled; + private final Supplier isMemoryFeatureEnabled; + private final Provider memoryFeatureToggleInterface; @Inject public ExecutingStatementResource( @ForStatementResource BoundedExecutor responseExecutor, LocalQueryProvider queryProvider, ServerConfig serverConfig, - QueryBlockingRateLimiter queryRateLimiter) + @FeatureToggle("query-rate-limiter") Provider queryRateLimiter, + QueryRateLimiter xqueryRateLimiter, + @FeatureToggle("query-logger") Supplier isQueryLoggerEnabled, + @FeatureToggle("query-cancel") Function isQueryCancelEnabled, + @FeatureToggle("memory-feature") Supplier isMemoryFeatureEnabled, + @FeatureToggle("memory-feature") Provider memoryFeatureToggleInterface) { this.responseExecutor = requireNonNull(responseExecutor, "responseExecutor is null"); this.queryProvider = requireNonNull(queryProvider, "queryProvider is null"); this.compressionEnabled = requireNonNull(serverConfig, "serverConfig is null").isQueryResultsCompressionEnabled(); this.queryRateLimiter = requireNonNull(queryRateLimiter, "queryRateLimiter is null"); + this.isQueryLoggerEnabled = isQueryLoggerEnabled; + this.isQueryCancelEnabled = isQueryCancelEnabled; + this.isMemoryFeatureEnabled = isMemoryFeatureEnabled; + this.memoryFeatureToggleInterface = memoryFeatureToggleInterface; } @Managed @Nested public TimeStat getRateLimiterBlockTime() { - return queryRateLimiter.getRateLimiterBlockTime(); + return queryRateLimiter.get().getRateLimiterBlockTime(); } @GET @@ -118,6 +140,26 @@ public void getQueryResults( abortIfPrefixUrlInvalid(xPrestoPrefixUrl); Query query = queryProvider.getQuery(queryId, slug); + QueryRateLimiter queryRateLimiter = this.queryRateLimiter.get(); + + if (isQueryLoggerEnabled.get()) { + log.info("query-logger is ENABLED"); + log.info("DELETE localhost:8080/v1/statement/executing/%s/123456789?slug=%s", queryId.getId(), slug); + } + else { + log.info("query-logger is DISABLED"); + } + + if (isMemoryFeatureEnabled.get()) { + log.info("\"memory-feature\" is ENABLED"); + log.info("MemoryFeatureToggleInterface class " + memoryFeatureToggleInterface.get().getClass().getName()); + } + else { + log.info("\"memory-feature\" is DISABLED"); + } + + log.info("QueryRateLimiter class " + queryRateLimiter.getClass().getName()); + ListenableFuture acquirePermitAsync = queryRateLimiter.acquire(queryId); String effectiveFinalProto = proto; DataSize effectiveFinalTargetResultSize = targetResultSize; @@ -143,7 +185,13 @@ public Response cancelQuery( @PathParam("token") long token, @QueryParam("slug") String slug) { - queryProvider.cancel(queryId, slug); - return Response.noContent().build(); + boolean cancelEnabled = isQueryCancelEnabled.apply(queryId); + if (cancelEnabled) { + queryProvider.cancel(queryId, slug); + return Response.noContent().build(); + } + else { + throw new RuntimeException("Cancel is not allowed!"); + } } } diff --git a/presto-main/src/main/java/com/facebook/presto/server/protocol/LocalQueryProvider.java b/presto-main/src/main/java/com/facebook/presto/server/protocol/LocalQueryProvider.java index 1bbfbb6e7c12..45c1562750a2 100644 --- a/presto-main/src/main/java/com/facebook/presto/server/protocol/LocalQueryProvider.java +++ b/presto-main/src/main/java/com/facebook/presto/server/protocol/LocalQueryProvider.java @@ -55,7 +55,7 @@ public class LocalQueryProvider private final BlockEncodingSerde blockEncodingSerde; private final BoundedExecutor responseExecutor; private final ScheduledExecutorService timeoutExecutor; - private final RetryCircuitBreaker retryCircuitBreaker; + private final RetryCircuitBreakerInt retryCircuitBreaker; private final ConcurrentMap queries = new ConcurrentHashMap<>(); private final ScheduledExecutorService queryPurger = newSingleThreadScheduledExecutor(threadsNamed("execution-query-purger")); @@ -68,7 +68,7 @@ public LocalQueryProvider( BlockEncodingSerde blockEncodingSerde, @ForStatementResource BoundedExecutor responseExecutor, @ForStatementResource ScheduledExecutorService timeoutExecutor, - RetryCircuitBreaker retryCircuitBreaker) + RetryCircuitBreakerInt retryCircuitBreaker) { this.queryManager = requireNonNull(queryManager, "queryManager is null"); this.transactionManager = requireNonNull(transactionManager, "transactionManager is null"); diff --git a/presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java b/presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java index c9c5ef905140..a82f882a9c6e 100644 --- a/presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java +++ b/presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java @@ -108,7 +108,7 @@ class Query private final ScheduledExecutorService timeoutExecutor; private final PagesSerde serde; - private final RetryCircuitBreaker retryCircuitBreaker; + private final RetryCircuitBreakerInt retryCircuitBreaker; @GuardedBy("this") private OptionalLong nextToken = OptionalLong.of(0); @@ -173,7 +173,7 @@ public static Query create( Executor dataProcessorExecutor, ScheduledExecutorService timeoutExecutor, BlockEncodingSerde blockEncodingSerde, - RetryCircuitBreaker retryCircuitBreaker) + RetryCircuitBreakerInt retryCircuitBreaker) { Query result = new Query(session, slug, queryManager, transactionManager, exchangeClient, dataProcessorExecutor, timeoutExecutor, blockEncodingSerde, retryCircuitBreaker); @@ -198,7 +198,7 @@ private Query( Executor resultsProcessorExecutor, ScheduledExecutorService timeoutExecutor, BlockEncodingSerde blockEncodingSerde, - RetryCircuitBreaker retryCircuitBreaker) + RetryCircuitBreakerInt retryCircuitBreaker) { requireNonNull(session, "session is null"); requireNonNull(slug, "slug is null"); diff --git a/presto-main/src/main/java/com/facebook/presto/server/protocol/QueryBlockingRateLimiter.java b/presto-main/src/main/java/com/facebook/presto/server/protocol/QueryBlockingRateLimiter.java index 7cfc2e0947e6..c29b7e28f0c3 100644 --- a/presto-main/src/main/java/com/facebook/presto/server/protocol/QueryBlockingRateLimiter.java +++ b/presto-main/src/main/java/com/facebook/presto/server/protocol/QueryBlockingRateLimiter.java @@ -48,6 +48,7 @@ * When not having enough tokens available, it uses the delayed processing method. */ public class QueryBlockingRateLimiter + implements QueryRateLimiter { private final long rateLimiterBucketMaxSize; private final ListeningExecutorService rateLimiterExecutorService; @@ -72,6 +73,7 @@ public QueryBlockingRateLimiter(QueryManagerConfig queryManagerConfig) * Fall back to delayed processing method to acquire a permit, in a separate thread pool * Internal guava rate limiter returns time spent sleeping to enforce rate, in seconds; 0.0 if not rate-limited, we use a future to wrap around that. */ + @Override public ListenableFuture acquire(QueryId queryId) { // if rateLimitBucketMaxSize < 0, we disable rate limiting by returning immediately if (rateLimiterBucketMaxSize < 0) { @@ -96,11 +98,13 @@ public CounterStat getRateLimiterTriggeredCounter() return rateLimiterTriggeredCounter; } + @Override public TimeStat getRateLimiterBlockTime() { return rateLimiterBlockTime; } + @Override public void addRateLimiterBlockTime(Duration duration) { rateLimiterBlockTime.add(duration); diff --git a/presto-main/src/main/java/com/facebook/presto/server/protocol/QueryRateLimiter.java b/presto-main/src/main/java/com/facebook/presto/server/protocol/QueryRateLimiter.java new file mode 100644 index 000000000000..1755dfb50b36 --- /dev/null +++ b/presto-main/src/main/java/com/facebook/presto/server/protocol/QueryRateLimiter.java @@ -0,0 +1,34 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.facebook.presto.server.protocol; + +import com.facebook.airlift.stats.TimeStat; +import com.facebook.presto.spi.QueryId; +import com.google.common.util.concurrent.ListenableFuture; +import io.airlift.units.Duration; + +public interface QueryRateLimiter +{ + /* + * For accidental bug-caused DoS, we will use delayed processing method to reduce the requests, even when user do not have back-off logic implemented + * Optimized to avoid blocking for normal usages with TryRequire first + * Fall back to delayed processing method to acquire a permit, in a separate thread pool + * Internal guava rate limiter returns time spent sleeping to enforce rate, in seconds; 0.0 if not rate-limited, we use a future to wrap around that. + */ + ListenableFuture acquire(QueryId queryId); + + TimeStat getRateLimiterBlockTime(); + + void addRateLimiterBlockTime(Duration duration); +} diff --git a/presto-main/src/main/java/com/facebook/presto/server/protocol/QueuedStatementResource.java b/presto-main/src/main/java/com/facebook/presto/server/protocol/QueuedStatementResource.java index 67f4eefcef8d..638c8d0796d4 100644 --- a/presto-main/src/main/java/com/facebook/presto/server/protocol/QueuedStatementResource.java +++ b/presto-main/src/main/java/com/facebook/presto/server/protocol/QueuedStatementResource.java @@ -128,7 +128,7 @@ public class QueuedStatementResource private final TracerProviderManager tracerProviderManager; private final SessionPropertyManager sessionPropertyManager; // We may need some system default session property values at early query stage even before session is created. - private final QueryBlockingRateLimiter queryRateLimiter; + private final QueryRateLimiter queryRateLimiter; private final TimeStat queuedRateLimiterBlockTime = new TimeStat(); @Inject @@ -140,7 +140,7 @@ public QueuedStatementResource( ServerConfig serverConfig, TracerProviderManager tracerProviderManager, SessionPropertyManager sessionPropertyManager, - QueryBlockingRateLimiter queryRateLimiter) + QueryRateLimiter queryRateLimiter) { this.dispatchManager = requireNonNull(dispatchManager, "dispatchManager is null"); this.queryResultsProvider = queryResultsProvider; diff --git a/presto-main/src/main/java/com/facebook/presto/server/protocol/RetryCircuitBreaker.java b/presto-main/src/main/java/com/facebook/presto/server/protocol/RetryCircuitBreaker.java index 820151828483..f7471c700774 100644 --- a/presto-main/src/main/java/com/facebook/presto/server/protocol/RetryCircuitBreaker.java +++ b/presto-main/src/main/java/com/facebook/presto/server/protocol/RetryCircuitBreaker.java @@ -24,6 +24,7 @@ import static java.util.concurrent.TimeUnit.SECONDS; public class RetryCircuitBreaker + implements RetryCircuitBreakerInt { private final int retryLimit; private final DecayCounter counter; @@ -38,16 +39,19 @@ public RetryCircuitBreaker(QueryManagerConfig queryManagerConfig) this.counter = new DecayCounter(1.0 / globalRetryWindow.roundTo(SECONDS)); } + @Override public void incrementFailure() { counter.add(1); } + @Override public boolean isRetryAllowed() { return counter.getCount() < retryLimit; } + @Override @Managed public double getRetryCount() { diff --git a/presto-main/src/main/java/com/facebook/presto/server/protocol/RetryCircuitBreakerInt.java b/presto-main/src/main/java/com/facebook/presto/server/protocol/RetryCircuitBreakerInt.java new file mode 100644 index 000000000000..30ba27458ad5 --- /dev/null +++ b/presto-main/src/main/java/com/facebook/presto/server/protocol/RetryCircuitBreakerInt.java @@ -0,0 +1,26 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.facebook.presto.server.protocol; + +import org.weakref.jmx.Managed; + +public interface RetryCircuitBreakerInt +{ + void incrementFailure(); + + boolean isRetryAllowed(); + + @Managed + double getRetryCount(); +} diff --git a/presto-main/src/main/java/com/facebook/presto/server/protocol/http-queries.http b/presto-main/src/main/java/com/facebook/presto/server/protocol/http-queries.http new file mode 100644 index 000000000000..7fd82c135c7c --- /dev/null +++ b/presto-main/src/main/java/com/facebook/presto/server/protocol/http-queries.http @@ -0,0 +1,29 @@ +### cancel query +DELETE localhost:8080/v1/statement/executing/20221117_122518_00001_2fvf4/123456789?slug=xc40f4b0e3d644493891de1bd284355a1 + +### features +GET localhost:8080/v1/features + +### features enabled=true +GET localhost:8080/v1/features?enabled=true + +### features enabled=false +GET localhost:8080/v1/features?enabled=false + +### features featureId=query +GET localhost:8080/v1/features?featureId=query + +### features featureId=memory +GET localhost:8080/v1/features?featureId=memory + +### features featureId=query&enabled=true +GET localhost:8080/v1/features?featureId=query&enabled=true + +### features featureId=query&enabled=false +GET localhost:8080/v1/features?featureId=query&enabled=false + +### features /query-cancel +GET localhost:8080/v1/features/query-cancel + +### features /query-rate-limiter +GET localhost:8080/v1/features/query-rate-limiter diff --git a/presto-main/src/test/java/com/facebook/presto/features/strategy/AllowListToggleStrategyTest.java b/presto-main/src/test/java/com/facebook/presto/features/strategy/AllowListToggleStrategyTest.java index 0ba749777a0c..3a68a4cfa034 100644 --- a/presto-main/src/test/java/com/facebook/presto/features/strategy/AllowListToggleStrategyTest.java +++ b/presto-main/src/test/java/com/facebook/presto/features/strategy/AllowListToggleStrategyTest.java @@ -152,7 +152,6 @@ public void testAllowListStrategy() // configure Feature Toggle evaluation strategy .toggleStrategyConfig(ImmutableMap.of(AllowListToggleStrategy.ALLOW_LIST_SOURCE, ".*idea.*", AllowListToggleStrategy.ALLOW_LIST_USER, ".*prestodb")) .bind(), - // AllowListToggleStrategy uses QueryManager to evaluate condition binder -> binder.bind(QueryManager.class).to(StubQueryManager.class), // we will set query id value using provider diff --git a/presto-memory-context/pom.xml b/presto-memory-context/pom.xml index 509fc3a3e32d..1c8038cc7b7f 100644 --- a/presto-memory-context/pom.xml +++ b/presto-memory-context/pom.xml @@ -28,6 +28,17 @@ guava + + com.google.inject + guice + + + + com.facebook.presto + presto-feature-toggle + provided + + com.facebook.presto diff --git a/presto-memory-context/src/main/java/com/facebook/presto/memory/context/ft/MemoryContextModule.java b/presto-memory-context/src/main/java/com/facebook/presto/memory/context/ft/MemoryContextModule.java new file mode 100644 index 000000000000..fc55b13bb2f8 --- /dev/null +++ b/presto-memory-context/src/main/java/com/facebook/presto/memory/context/ft/MemoryContextModule.java @@ -0,0 +1,31 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.facebook.presto.memory.context.ft; + +import com.google.inject.Binder; +import com.google.inject.Module; + +import static com.facebook.presto.features.binder.FeatureToggleBinder.featureToggleBinder; + +public class MemoryContextModule + implements Module +{ + @Override + public void configure(Binder binder) + { + featureToggleBinder(binder, MemoryFeatureToggleInterface.class) + .defaultClass(MemoryFeatureToggleInterfaceImpl.class) + .featureId("memory-feature").bind(); + } +} diff --git a/presto-memory-context/src/main/java/com/facebook/presto/memory/context/ft/MemoryFeatureToggleInterface.java b/presto-memory-context/src/main/java/com/facebook/presto/memory/context/ft/MemoryFeatureToggleInterface.java new file mode 100644 index 000000000000..a64b224acf09 --- /dev/null +++ b/presto-memory-context/src/main/java/com/facebook/presto/memory/context/ft/MemoryFeatureToggleInterface.java @@ -0,0 +1,18 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.facebook.presto.memory.context.ft; + +public interface MemoryFeatureToggleInterface +{ +} diff --git a/presto-memory-context/src/main/java/com/facebook/presto/memory/context/ft/MemoryFeatureToggleInterfaceImpl.java b/presto-memory-context/src/main/java/com/facebook/presto/memory/context/ft/MemoryFeatureToggleInterfaceImpl.java new file mode 100644 index 000000000000..bae1f983f193 --- /dev/null +++ b/presto-memory-context/src/main/java/com/facebook/presto/memory/context/ft/MemoryFeatureToggleInterfaceImpl.java @@ -0,0 +1,19 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.facebook.presto.memory.context.ft; + +public class MemoryFeatureToggleInterfaceImpl + implements MemoryFeatureToggleInterface +{ +} diff --git a/presto-memory/pom.xml b/presto-memory/pom.xml index 7ea05b11315a..e159eae41f77 100644 --- a/presto-memory/pom.xml +++ b/presto-memory/pom.xml @@ -71,6 +71,11 @@ provided + + com.facebook.presto + presto-feature-toggle + + com.facebook.drift drift-api @@ -176,6 +181,12 @@ test + + org.openjdk.jol + jol-core + provided + + org.openjdk.jmh jmh-core diff --git a/presto-memory/src/main/java/com/facebook/presto/plugin/memory/MemoryModule.java b/presto-memory/src/main/java/com/facebook/presto/plugin/memory/MemoryModule.java index 12f6c7f386f6..9017a238f398 100644 --- a/presto-memory/src/main/java/com/facebook/presto/plugin/memory/MemoryModule.java +++ b/presto-memory/src/main/java/com/facebook/presto/plugin/memory/MemoryModule.java @@ -20,6 +20,7 @@ import com.google.inject.Scopes; import static com.facebook.airlift.configuration.ConfigBinder.configBinder; +import static com.facebook.presto.features.binder.FeatureToggleBinder.featureToggleBinder; import static java.util.Objects.requireNonNull; public class MemoryModule @@ -50,5 +51,7 @@ public void configure(Binder binder) binder.bind(MemoryPageSourceProvider.class).in(Scopes.SINGLETON); binder.bind(MemoryPageSinkProvider.class).in(Scopes.SINGLETON); configBinder(binder).bindConfig(MemoryConfig.class); + + featureToggleBinder(binder).featureId("memory-feature").bind(); } }