From e5883d0dad053febe53d7a00829727eca8284074 Mon Sep 17 00:00:00 2001 From: James Faulkner Date: Tue, 7 Jul 2026 15:10:44 +0100 Subject: [PATCH 1/5] feat: add optional Glue retry with exponential backoff and HTTP-level metrics Glue calls occasionally fail with ConcurrentModificationException or timeouts. Retries are off by default (safe for HMS threads) and enabled via GLUE_RETRY_ENABLED=true for Dronefly. A RequestHandler2-based GlueMetricRequestHandler records per-operation call duration and error counts via Micrometer for full observability. Co-Authored-By: Claude Sonnet 4.6 --- .../extensions/gluesync/cli/GlueSyncCli.java | 10 +- .../gluesync/listener/ApiaryGlueSync.java | 5 +- .../gluesync/listener/GlueClientFactory.java | 112 ++++++++++++++++++ .../listener/GlueMetricRequestHandler.java | 87 ++++++++++++++ .../listener/metrics/MetricConstants.java | 12 +- .../listener/metrics/MetricService.java | 44 +++++++ .../listener/GlueClientFactoryTest.java | 102 ++++++++++++++++ .../GlueMetricRequestHandlerTest.java | 107 +++++++++++++++++ .../listener/metrics/MetricServiceTest.java | 38 ++++++ 9 files changed, 504 insertions(+), 13 deletions(-) create mode 100644 hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java create mode 100644 hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandler.java create mode 100644 hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactoryTest.java create mode 100644 hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandlerTest.java diff --git a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/cli/GlueSyncCli.java b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/cli/GlueSyncCli.java index 9423fcce..80835b59 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/cli/GlueSyncCli.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/cli/GlueSyncCli.java @@ -1,5 +1,5 @@ /** - * Copyright (C) 2018-2025 Expedia, Inc. + * Copyright (C) 2018-2026 Expedia, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -13,7 +13,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - package com.expediagroup.apiary.extensions.gluesync.cli; import java.util.ArrayList; @@ -35,11 +34,11 @@ import com.amazonaws.ClientConfiguration; import com.amazonaws.services.glue.AWSGlue; -import com.amazonaws.services.glue.AWSGlueClientBuilder; import com.expediagroup.apiary.extensions.events.metastore.consumer.common.thrift.ThriftHiveClient; import com.expediagroup.apiary.extensions.events.metastore.consumer.common.thrift.ThriftHiveClientFactory; import com.expediagroup.apiary.extensions.gluesync.listener.ApiaryGlueSync; +import com.expediagroup.apiary.extensions.gluesync.listener.GlueClientFactory; import com.expediagroup.apiary.extensions.gluesync.listener.service.GlueDatabaseService; import com.expediagroup.apiary.extensions.gluesync.listener.service.GluePartitionService; import com.expediagroup.apiary.extensions.gluesync.listener.service.GlueTableService; @@ -66,10 +65,7 @@ public class GlueSyncCli { public GlueSyncCli() { ClientConfiguration clientConfig = new ClientConfiguration(); clientConfig.setRequestTimeout(600000); - AWSGlue glueClient = AWSGlueClientBuilder.standard() - .withRegion(System.getenv("AWS_REGION")) - .withClientConfiguration(clientConfig) - .build(); + AWSGlue glueClient = GlueClientFactory.buildClient(System.getenv("AWS_REGION"), clientConfig, null); this.thriftHiveClientFactory = new ThriftHiveClientFactory(); thriftHiveClient = thriftHiveClientFactory.newInstance(THRIFT_CONNECTION_URI, THRIFT_CONNECTION_TIMEOUT); metastoreClient = thriftHiveClient.getMetaStoreClient(); diff --git a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/ApiaryGlueSync.java b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/ApiaryGlueSync.java index f500854f..6be20d82 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/ApiaryGlueSync.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/ApiaryGlueSync.java @@ -39,7 +39,6 @@ import org.slf4j.LoggerFactory; import com.amazonaws.services.glue.AWSGlue; -import com.amazonaws.services.glue.AWSGlueClientBuilder; import com.amazonaws.services.glue.model.AlreadyExistsException; import com.amazonaws.services.glue.model.EntityNotFoundException; @@ -83,13 +82,13 @@ public ApiaryGlueSync(Configuration config) { public ApiaryGlueSync(Configuration config, boolean throwExceptions) { super(config); - this.glueClient = AWSGlueClientBuilder.standard().withRegion(System.getenv("AWS_REGION")).build(); + this.metricService = new MetricService(); + this.glueClient = GlueClientFactory.buildClient(System.getenv("AWS_REGION"), metricService); String gluePrefix = System.getenv("GLUE_PREFIX"); this.glueDatabaseService = new GlueDatabaseService(glueClient, gluePrefix); this.gluePartitionService = new GluePartitionService(glueClient, gluePrefix); this.glueTableService = new GlueTableService(glueClient, gluePartitionService, gluePrefix); this.isIcebergPredicate = new IsIcebergTablePredicate(); - this.metricService = new MetricService(); this.throwExceptions = throwExceptions; log.debug("ApiaryGlueSync created"); } diff --git a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java new file mode 100644 index 00000000..6494657c --- /dev/null +++ b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java @@ -0,0 +1,112 @@ +/** + * Copyright (C) 2018-2026 Expedia, Inc. + * + * 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.expediagroup.apiary.extensions.gluesync.listener; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import com.amazonaws.AmazonServiceException; +import com.amazonaws.ClientConfiguration; +import com.amazonaws.retry.PredefinedRetryPolicies; +import com.amazonaws.retry.RetryPolicy; +import com.amazonaws.services.glue.AWSGlue; +import com.amazonaws.services.glue.AWSGlueClientBuilder; + +import com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricService; + +/** + * Builds the AWSGlue client with optional exponential-backoff retries and Micrometer metrics. + * + * Retries are disabled by default to avoid blocking HMS threads. Enable for Dronefly (CLI) via: + * GLUE_RETRY_ENABLED=true + * GLUE_RETRY_MAX_ATTEMPTS=3 (optional, default 3) + */ +public class GlueClientFactory { + + private static final Logger log = LoggerFactory.getLogger(GlueClientFactory.class); + + static final String ENV_RETRY_ENABLED = "GLUE_RETRY_ENABLED"; + static final String ENV_RETRY_MAX_ATTEMPTS = "GLUE_RETRY_MAX_ATTEMPTS"; + static final int DEFAULT_MAX_ATTEMPTS = 3; + + private GlueClientFactory() {} + + public static AWSGlue buildClient(String region, MetricService metricService) { + return buildClient(region, new ClientConfiguration(), metricService); + } + + public static AWSGlue buildClient(String region, ClientConfiguration config, MetricService metricService) { + if (retryEnabled()) { + int maxAttempts = maxAttempts(); + log.info("Glue client retries enabled: max {} attempts per call", maxAttempts); + config.setRetryPolicy(buildRetryPolicy(maxAttempts, metricService)); + } else { + log.info("Glue client retries disabled (set {}=true to enable)", ENV_RETRY_ENABLED); + } + + AWSGlueClientBuilder builder = AWSGlueClientBuilder.standard() + .withRegion(region) + .withClientConfiguration(config); + + if (metricService != null) { + builder.withRequestHandlers(new GlueMetricRequestHandler(metricService)); + } + return builder.build(); + } + + static boolean retryEnabled() { + return "true".equalsIgnoreCase(System.getenv(ENV_RETRY_ENABLED)); + } + + static int maxAttempts() { + String val = System.getenv(ENV_RETRY_MAX_ATTEMPTS); + if (val != null) { + try { + int n = Integer.parseInt(val.trim()); + if (n > 0) { + return n; + } + } catch (NumberFormatException ignored) { + log.warn("Invalid value for {}: '{}', using default {}", ENV_RETRY_MAX_ATTEMPTS, val, DEFAULT_MAX_ATTEMPTS); + } + } + return DEFAULT_MAX_ATTEMPTS; + } + + static RetryPolicy buildRetryPolicy(int maxAttempts, MetricService metricService) { + RetryPolicy.RetryCondition condition = (request, exception, retriesAttempted) -> { + if (PredefinedRetryPolicies.DEFAULT_RETRY_CONDITION.shouldRetry(request, exception, retriesAttempted)) { + recordRetry(metricService, exception.getClass().getSimpleName()); + return true; + } + if (exception instanceof AmazonServiceException) { + String errorCode = ((AmazonServiceException) exception).getErrorCode(); + if ("ConcurrentModificationException".equals(errorCode)) { + recordRetry(metricService, errorCode); + return true; + } + } + return false; + }; + return new RetryPolicy(condition, PredefinedRetryPolicies.DEFAULT_BACKOFF_STRATEGY, maxAttempts, true); + } + + private static void recordRetry(MetricService metricService, String exceptionType) { + if (metricService != null) { + metricService.recordGlueRetryAttempt(exceptionType); + } + } +} diff --git a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandler.java b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandler.java new file mode 100644 index 00000000..fa5ffcaa --- /dev/null +++ b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandler.java @@ -0,0 +1,87 @@ +/** + * Copyright (C) 2018-2026 Expedia, Inc. + * + * 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.expediagroup.apiary.extensions.gluesync.listener; + +import com.amazonaws.AmazonServiceException; +import com.amazonaws.AmazonWebServiceRequest; +import com.amazonaws.Request; +import com.amazonaws.Response; +import com.amazonaws.handlers.HandlerContextKey; +import com.amazonaws.handlers.RequestHandler2; + +import com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants; +import com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricService; + +/** + * AWS SDK v1 RequestHandler2 that records Glue client metrics via Micrometer. + * + * Fires per HTTP attempt (including retries), giving accurate latency and retry amplification visibility: + * glue_client_call_duration{operation, result} + * glue_client_error_total{operation, error_code} + */ +public class GlueMetricRequestHandler extends RequestHandler2 { + + static final HandlerContextKey START_TIME = new HandlerContextKey<>("GlueCallStartTime"); + + private final MetricService metricService; + + public GlueMetricRequestHandler(MetricService metricService) { + this.metricService = metricService; + } + + @Override + public void beforeRequest(Request request) { + request.addHandlerContext(START_TIME, System.currentTimeMillis()); + } + + @Override + public void afterResponse(Request request, Response response) { + metricService.recordGlueCallDuration(operationName(request), MetricConstants.RESULT_SUCCESS, elapsed(request)); + } + + @Override + public void afterError(Request request, Response response, Exception e) { + String operation = operationName(request); + metricService.recordGlueCallDuration(operation, MetricConstants.RESULT_FAILURE, elapsed(request)); + metricService.recordGlueClientError(operation, errorCode(e)); + } + + private long elapsed(Request request) { + Long start = request.getHandlerContext(START_TIME); + return start != null ? System.currentTimeMillis() - start : 0L; + } + + private String operationName(Request request) { + AmazonWebServiceRequest original = request.getOriginalRequest(); + if (original == null) { + return "unknown"; + } + String name = original.getClass().getSimpleName(); + if (name.endsWith("Request")) { + name = name.substring(0, name.length() - 7); + } + // CamelCase -> snake_case: "GetTable" -> "get_table" + return name.replaceAll("([A-Z])", "_$1").toLowerCase().replaceFirst("^_", ""); + } + + private String errorCode(Exception e) { + if (e instanceof AmazonServiceException) { + String code = ((AmazonServiceException) e).getErrorCode(); + return code != null ? code : "unknown"; + } + return e.getClass().getSimpleName(); + } +} diff --git a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricConstants.java b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricConstants.java index f56cccb8..a3e9597f 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricConstants.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricConstants.java @@ -30,9 +30,15 @@ public class MetricConstants { public static final String LISTENER_EVENT = "glue_listener_event"; public static final String LISTENER_TABLE_RENAME_DURATION = "glue_listener_table_rename_duration"; - public static final String TAG_OPERATION = "operation"; - public static final String TAG_RESULT = "result"; - public static final String TAG_OUTCOME = "outcome"; + public static final String GLUE_CLIENT_CALL_DURATION = "glue_client_call_duration"; + public static final String GLUE_CLIENT_ERROR_TOTAL = "glue_client_error_total"; + public static final String GLUE_RETRY_ATTEMPT = "glue_listener_retry_attempt"; + + public static final String TAG_OPERATION = "operation"; + public static final String TAG_RESULT = "result"; + public static final String TAG_OUTCOME = "outcome"; + public static final String TAG_ERROR_CODE = "error_code"; + public static final String TAG_EXCEPTION = "exception_type"; public static final String RESULT_SUCCESS = "success"; public static final String RESULT_FAILURE = "failure"; diff --git a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricService.java b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricService.java index 4caa0080..4afb76f6 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricService.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricService.java @@ -15,7 +15,12 @@ */ package com.expediagroup.apiary.extensions.gluesync.listener.metrics; +import static com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants.GLUE_CLIENT_CALL_DURATION; +import static com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants.GLUE_CLIENT_ERROR_TOTAL; +import static com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants.GLUE_RETRY_ATTEMPT; import static com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants.LISTENER_EVENT; +import static com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants.TAG_ERROR_CODE; +import static com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants.TAG_EXCEPTION; import static com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants.TAG_OPERATION; import static com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants.TAG_OUTCOME; import static com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants.TAG_RESULT; @@ -48,6 +53,9 @@ public class MetricService { private final MeterRegistry registry; private final Map metrics; private final Map events = new ConcurrentHashMap<>(); + private final Map callDurations = new ConcurrentHashMap<>(); + private final Map clientErrors = new ConcurrentHashMap<>(); + private final Map retryAttempts = new ConcurrentHashMap<>(); public MetricService(MeterRegistry registry) { this.registry = registry; @@ -137,4 +145,40 @@ public void recordEvent(String operation, String result, String outcome) { log.warn("Unable to record event {} {} {}", operation, result, outcome, e); } } + + public void recordGlueCallDuration(String operation, String result, long durationMs) { + try { + callDurations.computeIfAbsent(operation + "|" + result, k -> + Timer.builder(GLUE_CLIENT_CALL_DURATION) + .tags(TAG_OPERATION, operation, TAG_RESULT, result) + .register(registry)) + .record(durationMs, TimeUnit.MILLISECONDS); + } catch (Exception e) { + log.warn("Unable to record Glue call duration {} {} {}ms", operation, result, durationMs, e); + } + } + + public void recordGlueClientError(String operation, String errorCode) { + try { + clientErrors.computeIfAbsent(operation + "|" + errorCode, k -> + Counter.builder(GLUE_CLIENT_ERROR_TOTAL) + .tags(TAG_OPERATION, operation, TAG_ERROR_CODE, errorCode) + .register(registry)) + .increment(); + } catch (Exception e) { + log.warn("Unable to record Glue client error {} {}", operation, errorCode, e); + } + } + + public void recordGlueRetryAttempt(String exceptionType) { + try { + retryAttempts.computeIfAbsent(exceptionType, k -> + Counter.builder(GLUE_RETRY_ATTEMPT) + .tags(TAG_EXCEPTION, exceptionType) + .register(registry)) + .increment(); + } catch (Exception e) { + log.warn("Unable to record Glue retry attempt {}", exceptionType, e); + } + } } diff --git a/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactoryTest.java b/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactoryTest.java new file mode 100644 index 00000000..da91a490 --- /dev/null +++ b/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactoryTest.java @@ -0,0 +1,102 @@ +/** + * Copyright (C) 2018-2026 Expedia, Inc. + * + * 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.expediagroup.apiary.extensions.gluesync.listener; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; + +import org.junit.Before; +import org.junit.Test; + +import io.micrometer.core.instrument.simple.SimpleMeterRegistry; + +import com.amazonaws.AmazonServiceException; +import com.amazonaws.retry.RetryPolicy; + +import com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants; +import com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricService; + +public class GlueClientFactoryTest { + + private SimpleMeterRegistry registry; + private MetricService metricService; + + @Before + public void setUp() { + registry = new SimpleMeterRegistry(); + metricService = new MetricService(registry); + } + + @Test + public void defaultMaxAttemptsIsThree() { + assertThat(GlueClientFactory.maxAttempts(), is(GlueClientFactory.DEFAULT_MAX_ATTEMPTS)); + } + + @Test + public void retryPolicyRetryCountMatchesMaxAttempts() { + RetryPolicy policy = GlueClientFactory.buildRetryPolicy(5, null); + assertThat(policy.getMaxErrorRetry(), is(5)); + } + + @Test + public void concurrentModificationExceptionIsRetried() { + RetryPolicy policy = GlueClientFactory.buildRetryPolicy(3, null); + AmazonServiceException ex = new AmazonServiceException("conflict"); + ex.setErrorCode("ConcurrentModificationException"); + + boolean shouldRetry = policy.getRetryCondition().shouldRetry(null, ex, 0); + assertThat(shouldRetry, is(true)); + } + + @Test + public void unknownServiceExceptionIsNotRetried() { + RetryPolicy policy = GlueClientFactory.buildRetryPolicy(3, null); + AmazonServiceException ex = new AmazonServiceException("bad input"); + ex.setErrorCode("InvalidInputException"); + ex.setStatusCode(400); + + boolean shouldRetry = policy.getRetryCondition().shouldRetry(null, ex, 0); + assertThat(shouldRetry, is(false)); + } + + @Test + public void retryRecordsMetricWhenMetricServiceIsPresent() { + RetryPolicy policy = GlueClientFactory.buildRetryPolicy(3, metricService); + AmazonServiceException ex = new AmazonServiceException("conflict"); + ex.setErrorCode("ConcurrentModificationException"); + + policy.getRetryCondition().shouldRetry(null, ex, 0); + + assertThat(registry.get(MetricConstants.GLUE_RETRY_ATTEMPT) + .tags(MetricConstants.TAG_EXCEPTION, "ConcurrentModificationException") + .counter().count(), is(1.0)); + } + + @Test + public void retryDoesNotThrowWhenMetricServiceIsNull() { + RetryPolicy policy = GlueClientFactory.buildRetryPolicy(3, null); + AmazonServiceException ex = new AmazonServiceException("conflict"); + ex.setErrorCode("ConcurrentModificationException"); + + // should not throw + policy.getRetryCondition().shouldRetry(null, ex, 0); + } + + @Test + public void retryIsDisabledByDefault() { + assertThat(GlueClientFactory.retryEnabled(), is(false)); + } +} diff --git a/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandlerTest.java b/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandlerTest.java new file mode 100644 index 00000000..a29035c1 --- /dev/null +++ b/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandlerTest.java @@ -0,0 +1,107 @@ +/** + * Copyright (C) 2018-2026 Expedia, Inc. + * + * 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.expediagroup.apiary.extensions.gluesync.listener; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; + +import java.util.concurrent.TimeUnit; + +import org.junit.Before; +import org.junit.Test; + +import io.micrometer.core.instrument.simple.SimpleMeterRegistry; + +import com.amazonaws.AmazonServiceException; +import com.amazonaws.DefaultRequest; +import com.amazonaws.Request; +import com.amazonaws.services.glue.model.GetTableRequest; +import com.amazonaws.services.glue.model.UpdateTableRequest; + +import com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants; +import com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricService; + +public class GlueMetricRequestHandlerTest { + + private SimpleMeterRegistry registry; + private MetricService metricService; + private GlueMetricRequestHandler handler; + + @Before + public void setUp() { + registry = new SimpleMeterRegistry(); + metricService = new MetricService(registry); + handler = new GlueMetricRequestHandler(metricService); + } + + @Test + public void successRecordsDurationWithSuccessTag() { + Request request = buildRequest(new GetTableRequest()); + handler.beforeRequest(request); + handler.afterResponse(request, null); + + assertThat(registry.get(MetricConstants.GLUE_CLIENT_CALL_DURATION) + .tags(MetricConstants.TAG_OPERATION, "get_table", MetricConstants.TAG_RESULT, MetricConstants.RESULT_SUCCESS) + .timer().count(), is(1L)); + } + + @Test + public void errorRecordsDurationWithFailureTagAndErrorCounter() { + Request request = buildRequest(new UpdateTableRequest()); + AmazonServiceException exception = new AmazonServiceException("conflict"); + exception.setErrorCode("ConcurrentModificationException"); + + handler.beforeRequest(request); + handler.afterError(request, null, exception); + + assertThat(registry.get(MetricConstants.GLUE_CLIENT_CALL_DURATION) + .tags(MetricConstants.TAG_OPERATION, "update_table", MetricConstants.TAG_RESULT, MetricConstants.RESULT_FAILURE) + .timer().count(), is(1L)); + + assertThat(registry.get(MetricConstants.GLUE_CLIENT_ERROR_TOTAL) + .tags(MetricConstants.TAG_OPERATION, "update_table", + MetricConstants.TAG_ERROR_CODE, "ConcurrentModificationException") + .counter().count(), is(1.0)); + } + + @Test + public void nonServiceExceptionUsesSimpleClassName() { + Request request = buildRequest(new GetTableRequest()); + handler.beforeRequest(request); + handler.afterError(request, null, new RuntimeException("oops")); + + assertThat(registry.get(MetricConstants.GLUE_CLIENT_ERROR_TOTAL) + .tags(MetricConstants.TAG_OPERATION, "get_table", + MetricConstants.TAG_ERROR_CODE, "RuntimeException") + .counter().count(), is(1.0)); + } + + @Test + public void missingStartTimeRecordsDurationOfZero() { + Request request = buildRequest(new GetTableRequest()); + // do NOT call beforeRequest — START_TIME not set + handler.afterResponse(request, null); + + assertThat(registry.get(MetricConstants.GLUE_CLIENT_CALL_DURATION) + .tags(MetricConstants.TAG_OPERATION, "get_table", MetricConstants.TAG_RESULT, MetricConstants.RESULT_SUCCESS) + .timer().totalTime(TimeUnit.MILLISECONDS), is(0.0)); + } + + private Request buildRequest(T originalRequest) { + DefaultRequest request = new DefaultRequest<>(originalRequest, "AWSGlue"); + return request; + } +} diff --git a/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricServiceTest.java b/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricServiceTest.java index 2464887c..4c7bb0ff 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricServiceTest.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricServiceTest.java @@ -91,6 +91,44 @@ public void jmxRegistryExposesCountersAsMBeans() throws Exception { jmxRegistry.close(); } + @Test + public void recordGlueCallDurationRegistersTaggedTimer() { + MeterRegistry registry = new SimpleMeterRegistry(); + MetricService metricService = new MetricService(registry); + + metricService.recordGlueCallDuration("get_table", "success", 42L); + + assertThat(registry.get(MetricConstants.GLUE_CLIENT_CALL_DURATION) + .tags(MetricConstants.TAG_OPERATION, "get_table", MetricConstants.TAG_RESULT, "success") + .timer().count(), is(1L)); + } + + @Test + public void recordGlueClientErrorRegistersTaggedCounter() { + MeterRegistry registry = new SimpleMeterRegistry(); + MetricService metricService = new MetricService(registry); + + metricService.recordGlueClientError("update_table", "ConcurrentModificationException"); + metricService.recordGlueClientError("update_table", "ConcurrentModificationException"); + + assertThat(registry.get(MetricConstants.GLUE_CLIENT_ERROR_TOTAL) + .tags(MetricConstants.TAG_OPERATION, "update_table", + MetricConstants.TAG_ERROR_CODE, "ConcurrentModificationException") + .counter().count(), is(2.0)); + } + + @Test + public void recordGlueRetryAttemptRegistersTaggedCounter() { + MeterRegistry registry = new SimpleMeterRegistry(); + MetricService metricService = new MetricService(registry); + + metricService.recordGlueRetryAttempt("ConcurrentModificationException"); + + assertThat(registry.get(MetricConstants.GLUE_RETRY_ATTEMPT) + .tags(MetricConstants.TAG_EXCEPTION, "ConcurrentModificationException") + .counter().count(), is(1.0)); + } + @Test public void taggedEventCounterExposesTagsAsJmxKeyProperties() throws Exception { MetricRegistry dropwizardRegistry = new MetricRegistry(); From 218ff15329b0fb0cd54b2b6077860db7c792dbf8 Mon Sep 17 00:00:00 2001 From: James Faulkner Date: Tue, 7 Jul 2026 16:15:34 +0100 Subject: [PATCH 2/5] fix: address code review findings in Glue retry and metrics implementation - Switch GlueMetricRequestHandler to beforeAttempt/afterAttempt hooks so metrics fire per HTTP attempt (including retries) rather than once per logical call - Clarify 'retries disabled' log: SDK default retries still apply when custom policy is off; the custom policy adds CME retry on top - Fix GlueSyncCli to share a single AWSGlue client across ApiaryGlueSync, GluePartitionService, and GlueDatabaseService so the 600s request timeout applies to all table operations - Rename maxAttempts -> maxRetries throughout to match SDK semantics (maxErrorRetry is a retry count, not total attempts) - Unify retry metric tag namespace: use getErrorCode() for AmazonServiceException in both retry condition branches Co-Authored-By: Claude Sonnet 4.6 --- .../extensions/gluesync/cli/GlueSyncCli.java | 8 ++-- .../gluesync/listener/ApiaryGlueSync.java | 3 -- .../gluesync/listener/GlueClientFactory.java | 40 ++++++++++++------- .../listener/GlueMetricRequestHandler.java | 28 +++++++------ .../listener/GlueClientFactoryTest.java | 4 +- .../GlueMetricRequestHandlerTest.java | 29 +++++++++----- 6 files changed, 67 insertions(+), 45 deletions(-) diff --git a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/cli/GlueSyncCli.java b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/cli/GlueSyncCli.java index 80835b59..6eb6fd1e 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/cli/GlueSyncCli.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/cli/GlueSyncCli.java @@ -39,6 +39,7 @@ import com.expediagroup.apiary.extensions.events.metastore.consumer.common.thrift.ThriftHiveClientFactory; import com.expediagroup.apiary.extensions.gluesync.listener.ApiaryGlueSync; import com.expediagroup.apiary.extensions.gluesync.listener.GlueClientFactory; +import com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricService; import com.expediagroup.apiary.extensions.gluesync.listener.service.GlueDatabaseService; import com.expediagroup.apiary.extensions.gluesync.listener.service.GluePartitionService; import com.expediagroup.apiary.extensions.gluesync.listener.service.GlueTableService; @@ -63,16 +64,17 @@ public class GlueSyncCli { private IsIcebergTablePredicate isIcebergTablePredicate; public GlueSyncCli() { + MetricService metricService = new MetricService(); ClientConfiguration clientConfig = new ClientConfiguration(); clientConfig.setRequestTimeout(600000); - AWSGlue glueClient = GlueClientFactory.buildClient(System.getenv("AWS_REGION"), clientConfig, null); + String gluePrefix = System.getenv("GLUE_PREFIX"); + AWSGlue glueClient = GlueClientFactory.buildClient(System.getenv("AWS_REGION"), clientConfig, metricService); this.thriftHiveClientFactory = new ThriftHiveClientFactory(); thriftHiveClient = thriftHiveClientFactory.newInstance(THRIFT_CONNECTION_URI, THRIFT_CONNECTION_TIMEOUT); metastoreClient = thriftHiveClient.getMetaStoreClient(); Configuration config = new Configuration(); - this.apiaryGlueSync = new ApiaryGlueSync(config, true); + this.apiaryGlueSync = new ApiaryGlueSync(config, glueClient, gluePrefix, metricService, true); this.isIcebergTablePredicate = new IsIcebergTablePredicate(); - String gluePrefix = System.getenv("GLUE_PREFIX"); this.gluePartitionService = new GluePartitionService(glueClient, gluePrefix); this.glueDatabaseService = new GlueDatabaseService(glueClient, gluePrefix); } diff --git a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/ApiaryGlueSync.java b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/ApiaryGlueSync.java index 6be20d82..fd905f65 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/ApiaryGlueSync.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/ApiaryGlueSync.java @@ -93,9 +93,6 @@ public ApiaryGlueSync(Configuration config, boolean throwExceptions) { log.debug("ApiaryGlueSync created"); } - /** - * Just for testing. - */ public ApiaryGlueSync(Configuration config, AWSGlue glueClient, String gluePrefix, MetricService metricService, boolean throwExceptions) { this(config, glueClient, gluePrefix, metricService, throwExceptions, null); diff --git a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java index 6494657c..e841cdfb 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java @@ -15,6 +15,8 @@ */ package com.expediagroup.apiary.extensions.gluesync.listener; +import static com.amazonaws.retry.PredefinedRetryPolicies.DEFAULT_RETRY_CONDITION; + import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -28,11 +30,11 @@ import com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricService; /** - * Builds the AWSGlue client with optional exponential-backoff retries and Micrometer metrics. + * Builds the AWSGlue client with optional CME retry policy and Micrometer metrics. * - * Retries are disabled by default to avoid blocking HMS threads. Enable for Dronefly (CLI) via: + * CME retries are disabled by default to avoid blocking HMS threads. Enable for Dronefly (CLI) via: * GLUE_RETRY_ENABLED=true - * GLUE_RETRY_MAX_ATTEMPTS=3 (optional, default 3) + * GLUE_RETRY_MAX_RETRIES=3 (optional, default 3 retries = 4 total calls) */ public class GlueClientFactory { @@ -40,7 +42,7 @@ public class GlueClientFactory { static final String ENV_RETRY_ENABLED = "GLUE_RETRY_ENABLED"; static final String ENV_RETRY_MAX_ATTEMPTS = "GLUE_RETRY_MAX_ATTEMPTS"; - static final int DEFAULT_MAX_ATTEMPTS = 3; + static final int DEFAULT_MAX_RETRIES = 3; private GlueClientFactory() {} @@ -50,11 +52,11 @@ public static AWSGlue buildClient(String region, MetricService metricService) { public static AWSGlue buildClient(String region, ClientConfiguration config, MetricService metricService) { if (retryEnabled()) { - int maxAttempts = maxAttempts(); - log.info("Glue client retries enabled: max {} attempts per call", maxAttempts); - config.setRetryPolicy(buildRetryPolicy(maxAttempts, metricService)); + int maxRetries = maxRetries(); + log.info("Glue client custom retry policy active: max {} retries per call (covers throttles, 5xx, and CME)", maxRetries); + config.setRetryPolicy(buildRetryPolicy(maxRetries, metricService)); } else { - log.info("Glue client retries disabled (set {}=true to enable)", ENV_RETRY_ENABLED); + log.info("Glue client custom retry policy not active; SDK default retries (throttles/5xx) still apply. Set {}=true to also retry CME.", ENV_RETRY_ENABLED); } AWSGlueClientBuilder builder = AWSGlueClientBuilder.standard() @@ -71,7 +73,7 @@ static boolean retryEnabled() { return "true".equalsIgnoreCase(System.getenv(ENV_RETRY_ENABLED)); } - static int maxAttempts() { + static int maxRetries() { String val = System.getenv(ENV_RETRY_MAX_ATTEMPTS); if (val != null) { try { @@ -80,16 +82,16 @@ static int maxAttempts() { return n; } } catch (NumberFormatException ignored) { - log.warn("Invalid value for {}: '{}', using default {}", ENV_RETRY_MAX_ATTEMPTS, val, DEFAULT_MAX_ATTEMPTS); + log.warn("Invalid value for {}: '{}', using default {}", ENV_RETRY_MAX_ATTEMPTS, val, DEFAULT_MAX_RETRIES); } } - return DEFAULT_MAX_ATTEMPTS; + return DEFAULT_MAX_RETRIES; } - static RetryPolicy buildRetryPolicy(int maxAttempts, MetricService metricService) { + static RetryPolicy buildRetryPolicy(int maxRetries, MetricService metricService) { RetryPolicy.RetryCondition condition = (request, exception, retriesAttempted) -> { - if (PredefinedRetryPolicies.DEFAULT_RETRY_CONDITION.shouldRetry(request, exception, retriesAttempted)) { - recordRetry(metricService, exception.getClass().getSimpleName()); + if (DEFAULT_RETRY_CONDITION.shouldRetry(request, exception, retriesAttempted)) { + recordRetry(metricService, exceptionTag(exception)); return true; } if (exception instanceof AmazonServiceException) { @@ -101,7 +103,15 @@ static RetryPolicy buildRetryPolicy(int maxAttempts, MetricService metricService } return false; }; - return new RetryPolicy(condition, PredefinedRetryPolicies.DEFAULT_BACKOFF_STRATEGY, maxAttempts, true); + return new RetryPolicy(condition, PredefinedRetryPolicies.DEFAULT_BACKOFF_STRATEGY, maxRetries, true); + } + + private static String exceptionTag(Exception e) { + if (e instanceof AmazonServiceException) { + String code = ((AmazonServiceException) e).getErrorCode(); + return code != null ? code : e.getClass().getSimpleName(); + } + return e.getClass().getSimpleName(); } private static void recordRetry(MetricService metricService, String exceptionType) { diff --git a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandler.java b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandler.java index fa5ffcaa..712f3b1e 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandler.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandler.java @@ -18,7 +18,8 @@ import com.amazonaws.AmazonServiceException; import com.amazonaws.AmazonWebServiceRequest; import com.amazonaws.Request; -import com.amazonaws.Response; +import com.amazonaws.handlers.HandlerAfterAttemptContext; +import com.amazonaws.handlers.HandlerBeforeAttemptContext; import com.amazonaws.handlers.HandlerContextKey; import com.amazonaws.handlers.RequestHandler2; @@ -28,7 +29,8 @@ /** * AWS SDK v1 RequestHandler2 that records Glue client metrics via Micrometer. * - * Fires per HTTP attempt (including retries), giving accurate latency and retry amplification visibility: + * Uses beforeAttempt/afterAttempt hooks, which fire once per HTTP attempt including retries, + * giving accurate per-call latency and retry amplification visibility: * glue_client_call_duration{operation, result} * glue_client_error_total{operation, error_code} */ @@ -43,20 +45,22 @@ public GlueMetricRequestHandler(MetricService metricService) { } @Override - public void beforeRequest(Request request) { - request.addHandlerContext(START_TIME, System.currentTimeMillis()); + public void beforeAttempt(HandlerBeforeAttemptContext context) { + context.getRequest().addHandlerContext(START_TIME, System.currentTimeMillis()); } @Override - public void afterResponse(Request request, Response response) { - metricService.recordGlueCallDuration(operationName(request), MetricConstants.RESULT_SUCCESS, elapsed(request)); - } - - @Override - public void afterError(Request request, Response response, Exception e) { + public void afterAttempt(HandlerAfterAttemptContext context) { + Request request = context.getRequest(); String operation = operationName(request); - metricService.recordGlueCallDuration(operation, MetricConstants.RESULT_FAILURE, elapsed(request)); - metricService.recordGlueClientError(operation, errorCode(e)); + long durationMs = elapsed(request); + Exception exception = context.getException(); + if (exception == null) { + metricService.recordGlueCallDuration(operation, MetricConstants.RESULT_SUCCESS, durationMs); + } else { + metricService.recordGlueCallDuration(operation, MetricConstants.RESULT_FAILURE, durationMs); + metricService.recordGlueClientError(operation, errorCode(exception)); + } } private long elapsed(Request request) { diff --git a/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactoryTest.java b/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactoryTest.java index da91a490..2fb4dd46 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactoryTest.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactoryTest.java @@ -41,8 +41,8 @@ public void setUp() { } @Test - public void defaultMaxAttemptsIsThree() { - assertThat(GlueClientFactory.maxAttempts(), is(GlueClientFactory.DEFAULT_MAX_ATTEMPTS)); + public void defaultMaxRetriesIsThree() { + assertThat(GlueClientFactory.maxRetries(), is(GlueClientFactory.DEFAULT_MAX_RETRIES)); } @Test diff --git a/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandlerTest.java b/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandlerTest.java index a29035c1..6626712e 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandlerTest.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandlerTest.java @@ -28,6 +28,8 @@ import com.amazonaws.AmazonServiceException; import com.amazonaws.DefaultRequest; import com.amazonaws.Request; +import com.amazonaws.handlers.HandlerAfterAttemptContext; +import com.amazonaws.handlers.HandlerBeforeAttemptContext; import com.amazonaws.services.glue.model.GetTableRequest; import com.amazonaws.services.glue.model.UpdateTableRequest; @@ -50,8 +52,8 @@ public void setUp() { @Test public void successRecordsDurationWithSuccessTag() { Request request = buildRequest(new GetTableRequest()); - handler.beforeRequest(request); - handler.afterResponse(request, null); + handler.beforeAttempt(before(request)); + handler.afterAttempt(after(request, null)); assertThat(registry.get(MetricConstants.GLUE_CLIENT_CALL_DURATION) .tags(MetricConstants.TAG_OPERATION, "get_table", MetricConstants.TAG_RESULT, MetricConstants.RESULT_SUCCESS) @@ -64,8 +66,8 @@ public void errorRecordsDurationWithFailureTagAndErrorCounter() { AmazonServiceException exception = new AmazonServiceException("conflict"); exception.setErrorCode("ConcurrentModificationException"); - handler.beforeRequest(request); - handler.afterError(request, null, exception); + handler.beforeAttempt(before(request)); + handler.afterAttempt(after(request, exception)); assertThat(registry.get(MetricConstants.GLUE_CLIENT_CALL_DURATION) .tags(MetricConstants.TAG_OPERATION, "update_table", MetricConstants.TAG_RESULT, MetricConstants.RESULT_FAILURE) @@ -80,8 +82,8 @@ public void errorRecordsDurationWithFailureTagAndErrorCounter() { @Test public void nonServiceExceptionUsesSimpleClassName() { Request request = buildRequest(new GetTableRequest()); - handler.beforeRequest(request); - handler.afterError(request, null, new RuntimeException("oops")); + handler.beforeAttempt(before(request)); + handler.afterAttempt(after(request, new RuntimeException("oops"))); assertThat(registry.get(MetricConstants.GLUE_CLIENT_ERROR_TOTAL) .tags(MetricConstants.TAG_OPERATION, "get_table", @@ -92,8 +94,8 @@ public void nonServiceExceptionUsesSimpleClassName() { @Test public void missingStartTimeRecordsDurationOfZero() { Request request = buildRequest(new GetTableRequest()); - // do NOT call beforeRequest — START_TIME not set - handler.afterResponse(request, null); + // do NOT call beforeAttempt — START_TIME not set + handler.afterAttempt(after(request, null)); assertThat(registry.get(MetricConstants.GLUE_CLIENT_CALL_DURATION) .tags(MetricConstants.TAG_OPERATION, "get_table", MetricConstants.TAG_RESULT, MetricConstants.RESULT_SUCCESS) @@ -101,7 +103,14 @@ public void missingStartTimeRecordsDurationOfZero() { } private Request buildRequest(T originalRequest) { - DefaultRequest request = new DefaultRequest<>(originalRequest, "AWSGlue"); - return request; + return new DefaultRequest<>(originalRequest, "AWSGlue"); + } + + private HandlerBeforeAttemptContext before(Request request) { + return HandlerBeforeAttemptContext.builder().withRequest(request).build(); + } + + private HandlerAfterAttemptContext after(Request request, Exception exception) { + return HandlerAfterAttemptContext.builder().withRequest(request).withException(exception).build(); } } From 81abea590bbb8d630ce8f6bf8e6f4732b8c8f398 Mon Sep 17 00:00:00 2001 From: James Faulkner Date: Tue, 7 Jul 2026 16:54:54 +0100 Subject: [PATCH 3/5] refactor: remove GlueMetricRequestHandler, simplify metrics to retry counter only Per-call duration and error total metrics via RequestHandler2 were removed in favour of the existing ApiaryGlueSync success/failure counters, which already provide sufficient operational coverage. The glue_listener_retry_attempt counter from GlueClientFactory is retained to surface CME and throttle retry activity. Co-Authored-By: Claude Sonnet 4.6 --- .../gluesync/listener/GlueClientFactory.java | 3 - .../listener/GlueMetricRequestHandler.java | 91 -------------- .../listener/metrics/MetricConstants.java | 13 +- .../listener/metrics/MetricService.java | 29 ----- .../GlueMetricRequestHandlerTest.java | 116 ------------------ .../listener/metrics/MetricServiceTest.java | 26 ---- 6 files changed, 5 insertions(+), 273 deletions(-) delete mode 100644 hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandler.java delete mode 100644 hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandlerTest.java diff --git a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java index e841cdfb..c7433ba0 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java @@ -63,9 +63,6 @@ public static AWSGlue buildClient(String region, ClientConfiguration config, Met .withRegion(region) .withClientConfiguration(config); - if (metricService != null) { - builder.withRequestHandlers(new GlueMetricRequestHandler(metricService)); - } return builder.build(); } diff --git a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandler.java b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandler.java deleted file mode 100644 index 712f3b1e..00000000 --- a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandler.java +++ /dev/null @@ -1,91 +0,0 @@ -/** - * Copyright (C) 2018-2026 Expedia, Inc. - * - * 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.expediagroup.apiary.extensions.gluesync.listener; - -import com.amazonaws.AmazonServiceException; -import com.amazonaws.AmazonWebServiceRequest; -import com.amazonaws.Request; -import com.amazonaws.handlers.HandlerAfterAttemptContext; -import com.amazonaws.handlers.HandlerBeforeAttemptContext; -import com.amazonaws.handlers.HandlerContextKey; -import com.amazonaws.handlers.RequestHandler2; - -import com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants; -import com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricService; - -/** - * AWS SDK v1 RequestHandler2 that records Glue client metrics via Micrometer. - * - * Uses beforeAttempt/afterAttempt hooks, which fire once per HTTP attempt including retries, - * giving accurate per-call latency and retry amplification visibility: - * glue_client_call_duration{operation, result} - * glue_client_error_total{operation, error_code} - */ -public class GlueMetricRequestHandler extends RequestHandler2 { - - static final HandlerContextKey START_TIME = new HandlerContextKey<>("GlueCallStartTime"); - - private final MetricService metricService; - - public GlueMetricRequestHandler(MetricService metricService) { - this.metricService = metricService; - } - - @Override - public void beforeAttempt(HandlerBeforeAttemptContext context) { - context.getRequest().addHandlerContext(START_TIME, System.currentTimeMillis()); - } - - @Override - public void afterAttempt(HandlerAfterAttemptContext context) { - Request request = context.getRequest(); - String operation = operationName(request); - long durationMs = elapsed(request); - Exception exception = context.getException(); - if (exception == null) { - metricService.recordGlueCallDuration(operation, MetricConstants.RESULT_SUCCESS, durationMs); - } else { - metricService.recordGlueCallDuration(operation, MetricConstants.RESULT_FAILURE, durationMs); - metricService.recordGlueClientError(operation, errorCode(exception)); - } - } - - private long elapsed(Request request) { - Long start = request.getHandlerContext(START_TIME); - return start != null ? System.currentTimeMillis() - start : 0L; - } - - private String operationName(Request request) { - AmazonWebServiceRequest original = request.getOriginalRequest(); - if (original == null) { - return "unknown"; - } - String name = original.getClass().getSimpleName(); - if (name.endsWith("Request")) { - name = name.substring(0, name.length() - 7); - } - // CamelCase -> snake_case: "GetTable" -> "get_table" - return name.replaceAll("([A-Z])", "_$1").toLowerCase().replaceFirst("^_", ""); - } - - private String errorCode(Exception e) { - if (e instanceof AmazonServiceException) { - String code = ((AmazonServiceException) e).getErrorCode(); - return code != null ? code : "unknown"; - } - return e.getClass().getSimpleName(); - } -} diff --git a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricConstants.java b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricConstants.java index a3e9597f..0ebdd00d 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricConstants.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricConstants.java @@ -30,15 +30,12 @@ public class MetricConstants { public static final String LISTENER_EVENT = "glue_listener_event"; public static final String LISTENER_TABLE_RENAME_DURATION = "glue_listener_table_rename_duration"; - public static final String GLUE_CLIENT_CALL_DURATION = "glue_client_call_duration"; - public static final String GLUE_CLIENT_ERROR_TOTAL = "glue_client_error_total"; - public static final String GLUE_RETRY_ATTEMPT = "glue_listener_retry_attempt"; + public static final String GLUE_RETRY_ATTEMPT = "glue_listener_retry_attempt"; - public static final String TAG_OPERATION = "operation"; - public static final String TAG_RESULT = "result"; - public static final String TAG_OUTCOME = "outcome"; - public static final String TAG_ERROR_CODE = "error_code"; - public static final String TAG_EXCEPTION = "exception_type"; + public static final String TAG_OPERATION = "operation"; + public static final String TAG_RESULT = "result"; + public static final String TAG_OUTCOME = "outcome"; + public static final String TAG_EXCEPTION = "exception_type"; public static final String RESULT_SUCCESS = "success"; public static final String RESULT_FAILURE = "failure"; diff --git a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricService.java b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricService.java index 4afb76f6..89aef498 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricService.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricService.java @@ -15,11 +15,8 @@ */ package com.expediagroup.apiary.extensions.gluesync.listener.metrics; -import static com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants.GLUE_CLIENT_CALL_DURATION; -import static com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants.GLUE_CLIENT_ERROR_TOTAL; import static com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants.GLUE_RETRY_ATTEMPT; import static com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants.LISTENER_EVENT; -import static com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants.TAG_ERROR_CODE; import static com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants.TAG_EXCEPTION; import static com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants.TAG_OPERATION; import static com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants.TAG_OUTCOME; @@ -53,8 +50,6 @@ public class MetricService { private final MeterRegistry registry; private final Map metrics; private final Map events = new ConcurrentHashMap<>(); - private final Map callDurations = new ConcurrentHashMap<>(); - private final Map clientErrors = new ConcurrentHashMap<>(); private final Map retryAttempts = new ConcurrentHashMap<>(); public MetricService(MeterRegistry registry) { @@ -146,30 +141,6 @@ public void recordEvent(String operation, String result, String outcome) { } } - public void recordGlueCallDuration(String operation, String result, long durationMs) { - try { - callDurations.computeIfAbsent(operation + "|" + result, k -> - Timer.builder(GLUE_CLIENT_CALL_DURATION) - .tags(TAG_OPERATION, operation, TAG_RESULT, result) - .register(registry)) - .record(durationMs, TimeUnit.MILLISECONDS); - } catch (Exception e) { - log.warn("Unable to record Glue call duration {} {} {}ms", operation, result, durationMs, e); - } - } - - public void recordGlueClientError(String operation, String errorCode) { - try { - clientErrors.computeIfAbsent(operation + "|" + errorCode, k -> - Counter.builder(GLUE_CLIENT_ERROR_TOTAL) - .tags(TAG_OPERATION, operation, TAG_ERROR_CODE, errorCode) - .register(registry)) - .increment(); - } catch (Exception e) { - log.warn("Unable to record Glue client error {} {}", operation, errorCode, e); - } - } - public void recordGlueRetryAttempt(String exceptionType) { try { retryAttempts.computeIfAbsent(exceptionType, k -> diff --git a/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandlerTest.java b/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandlerTest.java deleted file mode 100644 index 6626712e..00000000 --- a/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueMetricRequestHandlerTest.java +++ /dev/null @@ -1,116 +0,0 @@ -/** - * Copyright (C) 2018-2026 Expedia, Inc. - * - * 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.expediagroup.apiary.extensions.gluesync.listener; - -import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.MatcherAssert.assertThat; - -import java.util.concurrent.TimeUnit; - -import org.junit.Before; -import org.junit.Test; - -import io.micrometer.core.instrument.simple.SimpleMeterRegistry; - -import com.amazonaws.AmazonServiceException; -import com.amazonaws.DefaultRequest; -import com.amazonaws.Request; -import com.amazonaws.handlers.HandlerAfterAttemptContext; -import com.amazonaws.handlers.HandlerBeforeAttemptContext; -import com.amazonaws.services.glue.model.GetTableRequest; -import com.amazonaws.services.glue.model.UpdateTableRequest; - -import com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricConstants; -import com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricService; - -public class GlueMetricRequestHandlerTest { - - private SimpleMeterRegistry registry; - private MetricService metricService; - private GlueMetricRequestHandler handler; - - @Before - public void setUp() { - registry = new SimpleMeterRegistry(); - metricService = new MetricService(registry); - handler = new GlueMetricRequestHandler(metricService); - } - - @Test - public void successRecordsDurationWithSuccessTag() { - Request request = buildRequest(new GetTableRequest()); - handler.beforeAttempt(before(request)); - handler.afterAttempt(after(request, null)); - - assertThat(registry.get(MetricConstants.GLUE_CLIENT_CALL_DURATION) - .tags(MetricConstants.TAG_OPERATION, "get_table", MetricConstants.TAG_RESULT, MetricConstants.RESULT_SUCCESS) - .timer().count(), is(1L)); - } - - @Test - public void errorRecordsDurationWithFailureTagAndErrorCounter() { - Request request = buildRequest(new UpdateTableRequest()); - AmazonServiceException exception = new AmazonServiceException("conflict"); - exception.setErrorCode("ConcurrentModificationException"); - - handler.beforeAttempt(before(request)); - handler.afterAttempt(after(request, exception)); - - assertThat(registry.get(MetricConstants.GLUE_CLIENT_CALL_DURATION) - .tags(MetricConstants.TAG_OPERATION, "update_table", MetricConstants.TAG_RESULT, MetricConstants.RESULT_FAILURE) - .timer().count(), is(1L)); - - assertThat(registry.get(MetricConstants.GLUE_CLIENT_ERROR_TOTAL) - .tags(MetricConstants.TAG_OPERATION, "update_table", - MetricConstants.TAG_ERROR_CODE, "ConcurrentModificationException") - .counter().count(), is(1.0)); - } - - @Test - public void nonServiceExceptionUsesSimpleClassName() { - Request request = buildRequest(new GetTableRequest()); - handler.beforeAttempt(before(request)); - handler.afterAttempt(after(request, new RuntimeException("oops"))); - - assertThat(registry.get(MetricConstants.GLUE_CLIENT_ERROR_TOTAL) - .tags(MetricConstants.TAG_OPERATION, "get_table", - MetricConstants.TAG_ERROR_CODE, "RuntimeException") - .counter().count(), is(1.0)); - } - - @Test - public void missingStartTimeRecordsDurationOfZero() { - Request request = buildRequest(new GetTableRequest()); - // do NOT call beforeAttempt — START_TIME not set - handler.afterAttempt(after(request, null)); - - assertThat(registry.get(MetricConstants.GLUE_CLIENT_CALL_DURATION) - .tags(MetricConstants.TAG_OPERATION, "get_table", MetricConstants.TAG_RESULT, MetricConstants.RESULT_SUCCESS) - .timer().totalTime(TimeUnit.MILLISECONDS), is(0.0)); - } - - private Request buildRequest(T originalRequest) { - return new DefaultRequest<>(originalRequest, "AWSGlue"); - } - - private HandlerBeforeAttemptContext before(Request request) { - return HandlerBeforeAttemptContext.builder().withRequest(request).build(); - } - - private HandlerAfterAttemptContext after(Request request, Exception exception) { - return HandlerAfterAttemptContext.builder().withRequest(request).withException(exception).build(); - } -} diff --git a/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricServiceTest.java b/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricServiceTest.java index 4c7bb0ff..0ab3cbaf 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricServiceTest.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/metrics/MetricServiceTest.java @@ -91,32 +91,6 @@ public void jmxRegistryExposesCountersAsMBeans() throws Exception { jmxRegistry.close(); } - @Test - public void recordGlueCallDurationRegistersTaggedTimer() { - MeterRegistry registry = new SimpleMeterRegistry(); - MetricService metricService = new MetricService(registry); - - metricService.recordGlueCallDuration("get_table", "success", 42L); - - assertThat(registry.get(MetricConstants.GLUE_CLIENT_CALL_DURATION) - .tags(MetricConstants.TAG_OPERATION, "get_table", MetricConstants.TAG_RESULT, "success") - .timer().count(), is(1L)); - } - - @Test - public void recordGlueClientErrorRegistersTaggedCounter() { - MeterRegistry registry = new SimpleMeterRegistry(); - MetricService metricService = new MetricService(registry); - - metricService.recordGlueClientError("update_table", "ConcurrentModificationException"); - metricService.recordGlueClientError("update_table", "ConcurrentModificationException"); - - assertThat(registry.get(MetricConstants.GLUE_CLIENT_ERROR_TOTAL) - .tags(MetricConstants.TAG_OPERATION, "update_table", - MetricConstants.TAG_ERROR_CODE, "ConcurrentModificationException") - .counter().count(), is(2.0)); - } - @Test public void recordGlueRetryAttemptRegistersTaggedCounter() { MeterRegistry registry = new SimpleMeterRegistry(); From 6e5ee6be6113f6a19e2ab63371d2eb64b6a25146 Mon Sep 17 00:00:00 2001 From: James Faulkner Date: Wed, 5 Aug 2026 17:06:02 +0100 Subject: [PATCH 4/5] fix: address PR review findings - changelog, readme, and doc fixes Add missing CHANGELOG entry for the Glue retry feature, document the new GLUE_RETRY_ENABLED/GLUE_RETRY_MAX_ATTEMPTS env vars in the README, fix a javadoc typo (GLUE_RETRY_MAX_RETRIES -> GLUE_RETRY_MAX_ATTEMPTS), and log a warning when GLUE_RETRY_MAX_ATTEMPTS is zero/negative instead of silently falling back to the default. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 4 ++++ hive-event-listeners/apiary-gluesync-listener/README.md | 2 ++ .../extensions/gluesync/listener/GlueClientFactory.java | 3 ++- 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39487abe..b19ed3d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,10 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). +## 8.2.5 - 2026-08-05 +### Added +- `apiary-gluesync-listener`: optional Glue client retry with exponential backoff, via `GlueClientFactory`. Off by default (safe for HMS threads); enable for Dronefly with `GLUE_RETRY_ENABLED=true` (`GLUE_RETRY_MAX_ATTEMPTS`, default `3`). Retries `ConcurrentModificationException` in addition to the AWS SDK's default transient error retries, recording each attempt via a new `glue_listener_retry_attempt` Micrometer counter tagged by `exception_type`. + ## 8.2.4 - 2026-07-17 ### Added - `apiary-gluesync-listener`: per-event observability via a new `glue_listener_event` Micrometer counter, tagged with `operation` (e.g. `create_table`), `result` (`success`, `failure`, `ignored`), and `outcome` (e.g. `created`, `updated`, `deleted`, `not_found`, `renamed`, exception class name). Covers all 8 HMS event handlers. A `glue_listener_table_rename_duration` timer is also recorded on every table rename. diff --git a/hive-event-listeners/apiary-gluesync-listener/README.md b/hive-event-listeners/apiary-gluesync-listener/README.md index eb100ac3..eb3e86dd 100644 --- a/hive-event-listeners/apiary-gluesync-listener/README.md +++ b/hive-event-listeners/apiary-gluesync-listener/README.md @@ -17,6 +17,8 @@ The GlueSync listener can be configured by setting the following System Environm GLUE_PREFIX|No|Prefix added to Glue databases to handle database name collisions when synchronizing multiple metastores to the Glue catalog. ENABLE_HIVE_TO_GLUE_RENAME_OPERATION|No|Set to true in case you would like to enable Hive table renames when syncing into Glue. Default value is false. GLUE_SKIP_ARCHIVE|No|Default value applied to `SkipArchive` on AWS Glue `UpdateTable` requests when a table does not set the `apiary.gluesync.skipArchive` property. Accepts `true` or `false` (case-insensitive). When unset, the built-in default (`true`) is used. Any other value causes the listener to fail on startup. +GLUE_RETRY_ENABLED|No|Set to `true` to also retry `ConcurrentModificationException` (in addition to the AWS SDK's default throttle/5xx retries) with exponential backoff. Off by default so it doesn't block HMS threads; intended for the Dronefly/CLI deployment. Default value is `false`. +GLUE_RETRY_MAX_ATTEMPTS|No|Maximum number of retries per Glue API call when `GLUE_RETRY_ENABLED=true`. Default value is `3`. ## Table update SkipArchive [AWS default](https://docs.aws.amazon.com/glue/latest/webapi/API_UpdateTable.html#Glue-UpdateTable-request-SkipArchive) is to archive the table on every update. With Iceberg tables this can lead to a lot of table versions. In Glue you can only have a certain limit of the number of versions and you'll get exceptions when trying to update a table once you hit that limit. Manual version removal through AWS api is then needed. To counter this the listener defaults to `skipArchive=true`, so it does *not* make an archive of the table when updating. diff --git a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java index c7433ba0..20c565f2 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java @@ -34,7 +34,7 @@ * * CME retries are disabled by default to avoid blocking HMS threads. Enable for Dronefly (CLI) via: * GLUE_RETRY_ENABLED=true - * GLUE_RETRY_MAX_RETRIES=3 (optional, default 3 retries = 4 total calls) + * GLUE_RETRY_MAX_ATTEMPTS=3 (optional, default 3 retries = 4 total calls) */ public class GlueClientFactory { @@ -78,6 +78,7 @@ static int maxRetries() { if (n > 0) { return n; } + log.warn("Invalid value for {}: '{}', using default {}", ENV_RETRY_MAX_ATTEMPTS, val, DEFAULT_MAX_RETRIES); } catch (NumberFormatException ignored) { log.warn("Invalid value for {}: '{}', using default {}", ENV_RETRY_MAX_ATTEMPTS, val, DEFAULT_MAX_RETRIES); } From 3e4a7d06e9e19a3c38a3e0a7044482728649bc71 Mon Sep 17 00:00:00 2001 From: James Faulkner Date: Thu, 6 Aug 2026 11:27:05 +0100 Subject: [PATCH 5/5] feat: make retryable exception(s) configurable via GLUE_RETRY_EXCEPTIONS Address PR review comment asking for the retried exception(s) to be configurable instead of hardcoded, while defaulting to ConcurrentModificationException when the env var is unset. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 2 +- .../apiary-gluesync-listener/README.md | 3 +- .../gluesync/listener/GlueClientFactory.java | 42 +++++++++++++++---- .../listener/GlueClientFactoryTest.java | 32 ++++++++++++++ 4 files changed, 70 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b19ed3d6..3928a888 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) a ## 8.2.5 - 2026-08-05 ### Added -- `apiary-gluesync-listener`: optional Glue client retry with exponential backoff, via `GlueClientFactory`. Off by default (safe for HMS threads); enable for Dronefly with `GLUE_RETRY_ENABLED=true` (`GLUE_RETRY_MAX_ATTEMPTS`, default `3`). Retries `ConcurrentModificationException` in addition to the AWS SDK's default transient error retries, recording each attempt via a new `glue_listener_retry_attempt` Micrometer counter tagged by `exception_type`. +- `apiary-gluesync-listener`: optional Glue client retry with exponential backoff, via `GlueClientFactory`. Off by default (safe for HMS threads); enable for Dronefly with `GLUE_RETRY_ENABLED=true` (`GLUE_RETRY_MAX_ATTEMPTS`, default `3`). Retries `ConcurrentModificationException` by default in addition to the AWS SDK's default transient error retries; configure which exception(s) to retry via `GLUE_RETRY_EXCEPTIONS` (comma-separated). Each retry is recorded via a new `glue_listener_retry_attempt` Micrometer counter tagged by `exception_type`. ## 8.2.4 - 2026-07-17 ### Added diff --git a/hive-event-listeners/apiary-gluesync-listener/README.md b/hive-event-listeners/apiary-gluesync-listener/README.md index eb3e86dd..bb041ed6 100644 --- a/hive-event-listeners/apiary-gluesync-listener/README.md +++ b/hive-event-listeners/apiary-gluesync-listener/README.md @@ -17,8 +17,9 @@ The GlueSync listener can be configured by setting the following System Environm GLUE_PREFIX|No|Prefix added to Glue databases to handle database name collisions when synchronizing multiple metastores to the Glue catalog. ENABLE_HIVE_TO_GLUE_RENAME_OPERATION|No|Set to true in case you would like to enable Hive table renames when syncing into Glue. Default value is false. GLUE_SKIP_ARCHIVE|No|Default value applied to `SkipArchive` on AWS Glue `UpdateTable` requests when a table does not set the `apiary.gluesync.skipArchive` property. Accepts `true` or `false` (case-insensitive). When unset, the built-in default (`true`) is used. Any other value causes the listener to fail on startup. -GLUE_RETRY_ENABLED|No|Set to `true` to also retry `ConcurrentModificationException` (in addition to the AWS SDK's default throttle/5xx retries) with exponential backoff. Off by default so it doesn't block HMS threads; intended for the Dronefly/CLI deployment. Default value is `false`. +GLUE_RETRY_ENABLED|No|Set to `true` to also retry the exception(s) named in `GLUE_RETRY_EXCEPTIONS` (in addition to the AWS SDK's default throttle/5xx retries) with exponential backoff. Off by default so it doesn't block HMS threads; intended for the Dronefly/CLI deployment. Default value is `false`. GLUE_RETRY_MAX_ATTEMPTS|No|Maximum number of retries per Glue API call when `GLUE_RETRY_ENABLED=true`. Default value is `3`. +GLUE_RETRY_EXCEPTIONS|No|Comma-separated list of AWS Glue exception names to retry when `GLUE_RETRY_ENABLED=true`, e.g. `ConcurrentModificationException,ThrottlingException`. Default value is `ConcurrentModificationException`. ## Table update SkipArchive [AWS default](https://docs.aws.amazon.com/glue/latest/webapi/API_UpdateTable.html#Glue-UpdateTable-request-SkipArchive) is to archive the table on every update. With Iceberg tables this can lead to a lot of table versions. In Glue you can only have a certain limit of the number of versions and you'll get exceptions when trying to update a table once you hit that limit. Manual version removal through AWS api is then needed. To counter this the listener defaults to `skipArchive=true`, so it does *not* make an archive of the table when updating. diff --git a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java index 20c565f2..e9775b32 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/main/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactory.java @@ -17,6 +17,11 @@ import static com.amazonaws.retry.PredefinedRetryPolicies.DEFAULT_RETRY_CONDITION; +import java.util.Arrays; +import java.util.Collections; +import java.util.Set; +import java.util.stream.Collectors; + import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -30,11 +35,13 @@ import com.expediagroup.apiary.extensions.gluesync.listener.metrics.MetricService; /** - * Builds the AWSGlue client with optional CME retry policy and Micrometer metrics. + * Builds the AWSGlue client with optional extra-exception retry policy and Micrometer metrics. * - * CME retries are disabled by default to avoid blocking HMS threads. Enable for Dronefly (CLI) via: + * Extra retries are disabled by default to avoid blocking HMS threads. Enable for Dronefly (CLI) via: * GLUE_RETRY_ENABLED=true - * GLUE_RETRY_MAX_ATTEMPTS=3 (optional, default 3 retries = 4 total calls) + * GLUE_RETRY_MAX_ATTEMPTS=3 (optional, default 3 retries = 4 total calls) + * GLUE_RETRY_EXCEPTIONS=ConcurrentModificationException,SomeOtherException (optional, comma-separated, + * defaults to ConcurrentModificationException) */ public class GlueClientFactory { @@ -42,7 +49,9 @@ public class GlueClientFactory { static final String ENV_RETRY_ENABLED = "GLUE_RETRY_ENABLED"; static final String ENV_RETRY_MAX_ATTEMPTS = "GLUE_RETRY_MAX_ATTEMPTS"; + static final String ENV_RETRY_EXCEPTIONS = "GLUE_RETRY_EXCEPTIONS"; static final int DEFAULT_MAX_RETRIES = 3; + static final String DEFAULT_RETRY_EXCEPTION = "ConcurrentModificationException"; private GlueClientFactory() {} @@ -53,10 +62,13 @@ public static AWSGlue buildClient(String region, MetricService metricService) { public static AWSGlue buildClient(String region, ClientConfiguration config, MetricService metricService) { if (retryEnabled()) { int maxRetries = maxRetries(); - log.info("Glue client custom retry policy active: max {} retries per call (covers throttles, 5xx, and CME)", maxRetries); - config.setRetryPolicy(buildRetryPolicy(maxRetries, metricService)); + Set retryExceptions = retryExceptions(); + log.info("Glue client custom retry policy active: max {} retries per call (covers throttles, 5xx, and {})", + maxRetries, retryExceptions); + config.setRetryPolicy(buildRetryPolicy(maxRetries, retryExceptions, metricService)); } else { - log.info("Glue client custom retry policy not active; SDK default retries (throttles/5xx) still apply. Set {}=true to also retry CME.", ENV_RETRY_ENABLED); + log.info("Glue client custom retry policy not active; SDK default retries (throttles/5xx) still apply. Set {}=true to also retry {}.", + ENV_RETRY_ENABLED, DEFAULT_RETRY_EXCEPTION); } AWSGlueClientBuilder builder = AWSGlueClientBuilder.standard() @@ -86,7 +98,23 @@ static int maxRetries() { return DEFAULT_MAX_RETRIES; } + static Set retryExceptions() { + String val = System.getenv(ENV_RETRY_EXCEPTIONS); + if (val == null || val.trim().isEmpty()) { + return Collections.singleton(DEFAULT_RETRY_EXCEPTION); + } + Set exceptions = Arrays.stream(val.split(",")) + .map(String::trim) + .filter(exceptionType -> !exceptionType.isEmpty()) + .collect(Collectors.toSet()); + return exceptions.isEmpty() ? Collections.singleton(DEFAULT_RETRY_EXCEPTION) : exceptions; + } + static RetryPolicy buildRetryPolicy(int maxRetries, MetricService metricService) { + return buildRetryPolicy(maxRetries, Collections.singleton(DEFAULT_RETRY_EXCEPTION), metricService); + } + + static RetryPolicy buildRetryPolicy(int maxRetries, Set retryExceptions, MetricService metricService) { RetryPolicy.RetryCondition condition = (request, exception, retriesAttempted) -> { if (DEFAULT_RETRY_CONDITION.shouldRetry(request, exception, retriesAttempted)) { recordRetry(metricService, exceptionTag(exception)); @@ -94,7 +122,7 @@ static RetryPolicy buildRetryPolicy(int maxRetries, MetricService metricService) } if (exception instanceof AmazonServiceException) { String errorCode = ((AmazonServiceException) exception).getErrorCode(); - if ("ConcurrentModificationException".equals(errorCode)) { + if (errorCode != null && retryExceptions.contains(errorCode)) { recordRetry(metricService, errorCode); return true; } diff --git a/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactoryTest.java b/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactoryTest.java index 2fb4dd46..dc2a923f 100644 --- a/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactoryTest.java +++ b/hive-event-listeners/apiary-gluesync-listener/src/test/java/com/expediagroup/apiary/extensions/gluesync/listener/GlueClientFactoryTest.java @@ -18,6 +18,9 @@ import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; +import java.util.Collections; +import java.util.Set; + import org.junit.Before; import org.junit.Test; @@ -99,4 +102,33 @@ public void retryDoesNotThrowWhenMetricServiceIsNull() { public void retryIsDisabledByDefault() { assertThat(GlueClientFactory.retryEnabled(), is(false)); } + + @Test + public void defaultRetryExceptionsIsConcurrentModificationException() { + assertThat(GlueClientFactory.retryExceptions(), + is(Collections.singleton(GlueClientFactory.DEFAULT_RETRY_EXCEPTION))); + } + + @Test + public void configuredExceptionIsRetried() { + Set retryExceptions = Collections.singleton("ThrottlingException"); + RetryPolicy policy = GlueClientFactory.buildRetryPolicy(3, retryExceptions, null); + AmazonServiceException ex = new AmazonServiceException("throttled"); + ex.setErrorCode("ThrottlingException"); + ex.setStatusCode(400); + + boolean shouldRetry = policy.getRetryCondition().shouldRetry(null, ex, 0); + assertThat(shouldRetry, is(true)); + } + + @Test + public void exceptionNotInConfiguredSetIsNotRetried() { + Set retryExceptions = Collections.singleton("ThrottlingException"); + RetryPolicy policy = GlueClientFactory.buildRetryPolicy(3, retryExceptions, null); + AmazonServiceException ex = new AmazonServiceException("conflict"); + ex.setErrorCode("ConcurrentModificationException"); + + boolean shouldRetry = policy.getRetryCondition().shouldRetry(null, ex, 0); + assertThat(shouldRetry, is(false)); + } }