Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inject execution interceptors using multibinder #24404

Merged
merged 1 commit into from
Dec 11, 2024
Merged

Inject execution interceptors using multibinder #24404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions ...in/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/ForGlueHiveMetastore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* 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 io.trino.plugin.hive.metastore.glue;

import com.google.inject.BindingAnnotation;

import java.lang.annotation.Retention;
import java.lang.annotation.Target;

import static java.lang.annotation.ElementType.FIELD;
import static java.lang.annotation.ElementType.METHOD;
import static java.lang.annotation.ElementType.PARAMETER;
import static java.lang.annotation.RetentionPolicy.RUNTIME;

@Retention(RUNTIME)
@Target({FIELD, PARAMETER, METHOD})
@BindingAnnotation
public @interface ForGlueHiveMetastore {}
6 changes: 4 additions & 2 deletions ...-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveExecutionInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/
package io.trino.plugin.hive.metastore.glue;

import com.google.inject.Inject;
import software.amazon.awssdk.core.SdkRequest;
import software.amazon.awssdk.core.interceptor.Context;
import software.amazon.awssdk.core.interceptor.ExecutionAttributes;
Expand All @@ -24,9 +25,10 @@ public class GlueHiveExecutionInterceptor
{
private final boolean skipArchive;

GlueHiveExecutionInterceptor(boolean isSkipArchive)
@Inject
GlueHiveExecutionInterceptor(GlueHiveMetastoreConfig config)
{
this.skipArchive = isSkipArchive;
this.skipArchive = config.isSkipArchive();
}

@Override
Expand Down
41 changes: 35 additions & 6 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueMetastoreModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,15 @@
*/
package io.trino.plugin.hive.metastore.glue;

import com.google.common.collect.ImmutableList;
import com.google.inject.Binder;
import com.google.inject.Inject;
import com.google.inject.Key;
import com.google.inject.Provider;
import com.google.inject.Provides;
import com.google.inject.Scopes;
import com.google.inject.Singleton;
import com.google.inject.multibindings.Multibinder;
import com.google.inject.multibindings.ProvidesIntoOptional;
import io.airlift.configuration.AbstractConfigurationAwareModule;
import io.airlift.units.Duration;
Expand All @@ -32,6 +36,7 @@
import io.trino.spi.catalog.CatalogName;
import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider;
import software.amazon.awssdk.core.interceptor.ExecutionInterceptor;
import software.amazon.awssdk.http.apache.ApacheHttpClient;
import software.amazon.awssdk.http.apache.ProxyConfiguration;
import software.amazon.awssdk.regions.Region;
Expand All @@ -52,10 +57,12 @@

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
import static com.google.inject.multibindings.Multibinder.newSetBinder;
import static com.google.inject.multibindings.OptionalBinder.newOptionalBinder;
import static com.google.inject.multibindings.ProvidesIntoOptional.Type.DEFAULT;
import static io.airlift.configuration.ConfigBinder.configBinder;
import static io.trino.plugin.base.ClosingBinder.closingBinder;
import static java.util.Objects.requireNonNull;
import static org.weakref.jmx.guice.ExportBinder.newExporter;

public class GlueMetastoreModule
Expand All @@ -76,6 +83,10 @@ protected void setup(Binder binder)
.in(Scopes.SINGLETON);
binder.bind(Key.get(boolean.class, AllowHiveTableRename.class)).toInstance(false);

Multibinder<ExecutionInterceptor> executionInterceptorMultibinder = newSetBinder(binder, ExecutionInterceptor.class, ForGlueHiveMetastore.class);
executionInterceptorMultibinder.addBinding().toProvider(TelemetryExecutionInterceptorProvider.class).in(Scopes.SINGLETON);
executionInterceptorMultibinder.addBinding().to(GlueHiveExecutionInterceptor.class).in(Scopes.SINGLETON);

closingBinder(binder).registerCloseable(GlueClient.class);
}

Expand Down Expand Up @@ -121,16 +132,12 @@ public static GlueCache createGlueCache(CachingHiveMetastoreConfig config, Catal

@Provides
@Singleton
public static GlueClient createGlueClient(GlueHiveMetastoreConfig config, OpenTelemetry openTelemetry)
public static GlueClient createGlueClient(GlueHiveMetastoreConfig config, @ForGlueHiveMetastore Set<ExecutionInterceptor> executionInterceptors)
{
GlueClientBuilder glue = GlueClient.builder();

glue.overrideConfiguration(builder -> builder
.addExecutionInterceptor(AwsSdkTelemetry.builder(openTelemetry)
.setCaptureExperimentalSpanAttributes(true)
.setRecordIndividualHttpError(true)
.build().newExecutionInterceptor())
.addExecutionInterceptor(new GlueHiveExecutionInterceptor(config.isSkipArchive()))
.executionInterceptors(ImmutableList.copyOf(executionInterceptors))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are loosing ordering here. Does it matter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The set's iteration order is consistent with the binding order. This is convenient when multiple elements are contributed by the same module because that module can order its bindings appropriately. Avoid relying on the iteration order of elements contributed by different modules, since there is no equivalent mechanism to order modules.

So far order is maintained. If we keep adding new elements following the method above is safe. If we add an element in different module, then it could be added in random place (before or after this module).

I have checked the existing interceptors and they are independent from each other, and I believe it is important thing for them. If order would matter then I guess it would have to work as a stack because you have methods there like beforeX and afterX.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I believe order does not matter here.

.retryStrategy(retryBuilder -> retryBuilder
.retryOnException(throwable -> throwable instanceof ConcurrentModificationException)
.backoffStrategy(BackoffStrategy.exponentialDelay(
Expand Down Expand Up @@ -209,4 +216,26 @@ else if (config.getPinGlueClientToCurrentRegion()) {

return sts.build();
}

private static class TelemetryExecutionInterceptorProvider
implements Provider<ExecutionInterceptor>
{
private final OpenTelemetry openTelemetry;

@Inject
public TelemetryExecutionInterceptorProvider(OpenTelemetry openTelemetry)
{
this.openTelemetry = requireNonNull(openTelemetry, "openTelemetry is null");
}

@Override
public ExecutionInterceptor get()
{
return AwsSdkTelemetry.builder(openTelemetry)
.setCaptureExperimentalSpanAttributes(true)
.setRecordIndividualHttpError(true)
.build()
.newExecutionInterceptor();
}
}
}
4 changes: 2 additions & 2 deletions plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveGlueMetadataListing.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import io.airlift.log.Logger;
import io.opentelemetry.api.OpenTelemetry;
import io.trino.Session;
import io.trino.plugin.hive.metastore.glue.GlueHiveMetastore;
import io.trino.plugin.hive.metastore.glue.GlueHiveMetastoreConfig;
Expand All @@ -27,6 +26,7 @@
import io.trino.tpch.TpchTable;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.Test;
import org.testcontainers.shaded.com.google.common.collect.ImmutableSet;
import software.amazon.awssdk.services.glue.GlueClient;
import software.amazon.awssdk.services.glue.model.CreateTableRequest;
import software.amazon.awssdk.services.glue.model.TableInput;
Expand Down Expand Up @@ -124,7 +124,7 @@ private void createBrokenTable(Path dataDirectory)
{
GlueHiveMetastoreConfig glueConfig = new GlueHiveMetastoreConfig()
.setDefaultWarehouseDir(dataDirectory.toString());
try (GlueClient glueClient = createGlueClient(glueConfig, OpenTelemetry.noop())) {
try (GlueClient glueClient = createGlueClient(glueConfig, ImmutableSet.of())) {
TableInput tableInput = TableInput.builder()
.name(FAILING_TABLE_WITH_NULL_STORAGE_DESCRIPTOR_NAME)
.tableType("HIVE")
Expand Down
4 changes: 2 additions & 2 deletions ...java/io/trino/plugin/hive/metastore/glue/TestHiveConcurrentModificationGlueMetastore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@
*/
package io.trino.plugin.hive.metastore.glue;

import io.opentelemetry.api.OpenTelemetry;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
import org.testcontainers.shaded.com.google.common.collect.ImmutableSet;
import software.amazon.awssdk.awscore.exception.AwsErrorDetails;
import software.amazon.awssdk.core.client.config.ClientOverrideConfiguration;
import software.amazon.awssdk.retries.api.RefreshRetryTokenRequest;
Expand All @@ -36,7 +36,7 @@ public class TestHiveConcurrentModificationGlueMetastore
@Test
public void testGlueClientShouldRetryConcurrentModificationException()
{
try (GlueClient glueClient = createGlueClient(new GlueHiveMetastoreConfig(), OpenTelemetry.noop())) {
try (GlueClient glueClient = createGlueClient(new GlueHiveMetastoreConfig(), ImmutableSet.of())) {
ClientOverrideConfiguration clientOverrideConfiguration = glueClient.serviceClientConfiguration().overrideConfiguration();
RetryStrategy retryStrategy = clientOverrideConfiguration.retryStrategy().orElseThrow();

Expand Down
4 changes: 2 additions & 2 deletions ...rino-hive/src/test/java/io/trino/plugin/hive/metastore/glue/TestingGlueHiveMetastore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
*/
package io.trino.plugin.hive.metastore.glue;

import io.opentelemetry.api.OpenTelemetry;
import com.google.common.collect.ImmutableSet;
import io.trino.plugin.hive.metastore.glue.GlueHiveMetastore.TableKind;
import software.amazon.awssdk.services.glue.GlueClient;

Expand Down Expand Up @@ -52,7 +52,7 @@ public static GlueHiveMetastore createTestingGlueHiveMetastore(URI warehouseUri,
{
GlueHiveMetastoreConfig glueConfig = new GlueHiveMetastoreConfig()
.setDefaultWarehouseDir(warehouseUri.toString());
GlueClient glueClient = createGlueClient(glueConfig, OpenTelemetry.noop());
GlueClient glueClient = createGlueClient(glueConfig, ImmutableSet.of());
registerResource.accept(glueClient);
return new GlueHiveMetastore(
glueClient,
Expand Down
Loading