From 2ca379d2ed61922c9b51c79a0d2365f42994d2c2 Mon Sep 17 00:00:00 2001 From: James Faulkner Date: Tue, 7 Jul 2026 15:10:44 +0100 Subject: [PATCH 1/3] 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 60a144d6..f8312448 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; @@ -78,13 +77,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 3cb962b9b904a702b52abc622028fd7a56b7354f Mon Sep 17 00:00:00 2001 From: James Faulkner Date: Tue, 7 Jul 2026 16:15:34 +0100 Subject: [PATCH 2/3] 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 f8312448..7c3eac63 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 @@ -88,9 +88,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 124317f7cf6901b5d008d6d56baa1477df8e00f9 Mon Sep 17 00:00:00 2001 From: James Faulkner Date: Tue, 7 Jul 2026 16:54:54 +0100 Subject: [PATCH 3/3] 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();