From e19f5d7d5fdc889b4feabe4a7ad3184bbd809a98 Mon Sep 17 00:00:00 2001 From: Artem Bilan Date: Thu, 11 Jan 2024 14:52:53 -0500 Subject: [PATCH] GH-422: Log label in `RetryTemplate` Fixes: #422 * Push `MethodInvocationRetryCallback.getLabel()` to the `default` method in the `RetryCallback` * Use `ClassUtils.getQualifiedMethodName(invocation.getMethod())` for default label in the `MethodInvocationRetryCallback` if not provided from the outside * Refactor some logic in the `RetryOperationsInterceptor` to cover `context.removeAttribute("__proxy__")` for use-case when `recoverer` is not provided. Remove extra label resolution since it is already done in the `MethodInvocationRetryCallback` * Fix Javadoc typos in the affected classes --- .../springframework/retry/RetryCallback.java | 11 +++ .../MethodInvocationRetryCallback.java | 17 +++-- .../RetryOperationsInterceptor.java | 67 +++++-------------- .../retry/support/RetryTemplate.java | 20 +++--- 4 files changed, 52 insertions(+), 63 deletions(-) diff --git a/src/main/java/org/springframework/retry/RetryCallback.java b/src/main/java/org/springframework/retry/RetryCallback.java index 7ba13e4e..f0f9fb37 100644 --- a/src/main/java/org/springframework/retry/RetryCallback.java +++ b/src/main/java/org/springframework/retry/RetryCallback.java @@ -24,6 +24,7 @@ * @param the type of exception it declares may be thrown * @author Rob Harrop * @author Dave Syer + * @author Artem Bilan */ public interface RetryCallback { @@ -37,4 +38,14 @@ public interface RetryCallback { */ T doWithRetry(RetryContext context) throws E; + /** + * A logical identifier for this callback to distinguish retries around business + * operations. + * @return the identifier for this callback. + * @since 2.0.6 + */ + default String getLabel() { + return null; + } + } diff --git a/src/main/java/org/springframework/retry/interceptor/MethodInvocationRetryCallback.java b/src/main/java/org/springframework/retry/interceptor/MethodInvocationRetryCallback.java index 8d64c1a8..01b82ab3 100644 --- a/src/main/java/org/springframework/retry/interceptor/MethodInvocationRetryCallback.java +++ b/src/main/java/org/springframework/retry/interceptor/MethodInvocationRetryCallback.java @@ -1,5 +1,5 @@ /* - * Copyright 2006-2019 the original author or authors. + * Copyright 2006-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,14 +17,17 @@ package org.springframework.retry.interceptor; import org.aopalliance.intercept.MethodInvocation; + +import org.springframework.lang.Nullable; import org.springframework.retry.RetryCallback; import org.springframework.retry.RetryOperations; +import org.springframework.util.ClassUtils; import org.springframework.util.StringUtils; /** * Callback class for a Spring AOP reflective `MethodInvocation` that can be retried using * a {@link RetryOperations}. - * + *

* In a concrete {@link org.springframework.retry.RetryListener} implementation, the * `MethodInvocation` can be analysed for providing insights on the method called as well * as its parameter values which could then be used for monitoring purposes. @@ -35,6 +38,7 @@ * @see RetryOperationsInterceptor * @see org.springframework.retry.listener.MethodInvocationRetryListenerSupport * @author Marius Grama + * @author Artem Bilan * @since 1.3 */ public abstract class MethodInvocationRetryCallback implements RetryCallback { @@ -48,22 +52,23 @@ public abstract class MethodInvocationRetryCallback impl * @param invocation the method invocation * @param label a unique label for statistics reporting. */ - public MethodInvocationRetryCallback(MethodInvocation invocation, String label) { + public MethodInvocationRetryCallback(MethodInvocation invocation, @Nullable String label) { this.invocation = invocation; if (StringUtils.hasText(label)) { this.label = label; } else { - this.label = invocation.getMethod().toGenericString(); + this.label = ClassUtils.getQualifiedMethodName(invocation.getMethod()); } } public MethodInvocation getInvocation() { - return invocation; + return this.invocation; } + @Override public String getLabel() { - return label; + return this.label; } } diff --git a/src/main/java/org/springframework/retry/interceptor/RetryOperationsInterceptor.java b/src/main/java/org/springframework/retry/interceptor/RetryOperationsInterceptor.java index 449db329..2fa20977 100644 --- a/src/main/java/org/springframework/retry/interceptor/RetryOperationsInterceptor.java +++ b/src/main/java/org/springframework/retry/interceptor/RetryOperationsInterceptor.java @@ -1,5 +1,5 @@ /* - * Copyright 2006-2022 the original author or authors. + * Copyright 2006-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,12 +16,11 @@ package org.springframework.retry.interceptor; -import java.util.Arrays; - import org.aopalliance.intercept.MethodInterceptor; import org.aopalliance.intercept.MethodInvocation; import org.springframework.aop.ProxyMethodInvocation; +import org.springframework.lang.Nullable; import org.springframework.retry.RecoveryCallback; import org.springframework.retry.RetryCallback; import org.springframework.retry.RetryContext; @@ -35,9 +34,9 @@ /** * A {@link MethodInterceptor} that can be used to automatically retry calls to a method * on a service if it fails. The injected {@link RetryOperations} is used to control the - * number of retries. By default it will retry a fixed number of times, according to the + * number of retries. By default, it will retry a fixed number of times, according to the * defaults in {@link RetryTemplate}. - * + *

* Hint about transaction boundaries. If you want to retry a failed transaction you need * to make sure that the transaction boundary is inside the retry, otherwise the * successful attempt will roll back with the whole transaction. If the method being @@ -47,11 +46,13 @@ * * @author Rob Harrop * @author Dave Syer + * @author Artem Bilan */ public class RetryOperationsInterceptor implements MethodInterceptor { private RetryOperations retryOperations = new RetryTemplate(); + @Nullable private MethodInvocationRecoverer recoverer; private String label; @@ -71,18 +72,7 @@ public void setRecoverer(MethodInvocationRecoverer recoverer) { @Override public Object invoke(final MethodInvocation invocation) throws Throwable { - - String name; - if (StringUtils.hasText(this.label)) { - name = this.label; - } - else { - name = invocation.getMethod().toGenericString(); - } - final String label = name; - - RetryCallback retryCallback = new MethodInvocationRetryCallback( - invocation, label) { + RetryCallback retryCallback = new MethodInvocationRetryCallback<>(invocation, this.label) { @Override public Object doWithRetry(RetryContext context) throws Exception { @@ -117,42 +107,21 @@ public Object doWithRetry(RetryContext context) throws Exception { }; - if (this.recoverer != null) { - ItemRecovererCallback recoveryCallback = new ItemRecovererCallback(invocation.getArguments(), - this.recoverer); - try { - Object recovered = this.retryOperations.execute(retryCallback, recoveryCallback); - return recovered; - } - finally { - RetryContext context = RetrySynchronizationManager.getContext(); - if (context != null) { - context.removeAttribute("__proxy__"); - } + RecoveryCallback recoveryCallback = (this.recoverer != null) + ? new ItemRecovererCallback(invocation.getArguments(), this.recoverer) : null; + try { + return this.retryOperations.execute(retryCallback, recoveryCallback); + } + finally { + RetryContext context = RetrySynchronizationManager.getContext(); + if (context != null) { + context.removeAttribute("__proxy__"); } } - - return this.retryOperations.execute(retryCallback); - } - /** - * @author Dave Syer - * - */ - private static final class ItemRecovererCallback implements RecoveryCallback { - - private final Object[] args; - - private final MethodInvocationRecoverer recoverer; - - /** - * @param args the item that failed. - */ - private ItemRecovererCallback(Object[] args, MethodInvocationRecoverer recoverer) { - this.args = Arrays.asList(args).toArray(); - this.recoverer = recoverer; - } + private record ItemRecovererCallback(Object[] args, + MethodInvocationRecoverer recoverer) implements RecoveryCallback { @Override public Object recover(RetryContext context) { diff --git a/src/main/java/org/springframework/retry/support/RetryTemplate.java b/src/main/java/org/springframework/retry/support/RetryTemplate.java index c6a52a5f..512dcd41 100644 --- a/src/main/java/org/springframework/retry/support/RetryTemplate.java +++ b/src/main/java/org/springframework/retry/support/RetryTemplate.java @@ -1,5 +1,5 @@ /* - * Copyright 2006-2022 the original author or authors. + * Copyright 2006-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -317,6 +317,9 @@ protected T doExecute(RetryCallback retryCallback } } + Object label = retryCallback.getLabel(); + String labelMessage = (label != null) ? "; for: '" + label + "'" : ""; + /* * We allow the whole loop to be skipped if the policy or context already * forbid the first try. This is used in the case of external retry to allow a @@ -327,7 +330,7 @@ protected T doExecute(RetryCallback retryCallback try { if (this.logger.isDebugEnabled()) { - this.logger.debug("Retry: count=" + context.getRetryCount()); + this.logger.debug("Retry: count=" + context.getRetryCount() + labelMessage); } // Reset the last exception, so if we are successful // the close interceptors will not think we failed... @@ -355,22 +358,23 @@ protected T doExecute(RetryCallback retryCallback backOffPolicy.backOff(backOffContext); } catch (BackOffInterruptedException ex) { - lastException = e; // back off was prevented by another thread - fail the retry if (this.logger.isDebugEnabled()) { - this.logger.debug("Abort retry because interrupted: count=" + context.getRetryCount()); + this.logger.debug("Abort retry because interrupted: count=" + context.getRetryCount() + + labelMessage); } throw ex; } } if (this.logger.isDebugEnabled()) { - this.logger.debug("Checking for rethrow: count=" + context.getRetryCount()); + this.logger.debug("Checking for rethrow: count=" + context.getRetryCount() + labelMessage); } if (shouldRethrow(retryPolicy, context, state)) { if (this.logger.isDebugEnabled()) { - this.logger.debug("Rethrow in retry for policy: count=" + context.getRetryCount()); + this.logger + .debug("Rethrow in retry for policy: count=" + context.getRetryCount() + labelMessage); } throw RetryTemplate.wrapIfNecessary(e); } @@ -388,7 +392,7 @@ protected T doExecute(RetryCallback retryCallback } if (state == null && this.logger.isDebugEnabled()) { - this.logger.debug("Retry failed last attempt: count=" + context.getRetryCount()); + this.logger.debug("Retry failed last attempt: count=" + context.getRetryCount() + labelMessage); } exhausted = true; @@ -525,7 +529,7 @@ private RetryContext doOpenInternal(RetryPolicy retryPolicy) { /** * Actions to take after final attempt has failed. If there is state clean up the * cache. If there is a recovery callback, execute that and return its result. - * Otherwise throw an exception. + * Otherwise, throw an exception. * @param recoveryCallback the callback for recovery (might be null) * @param context the current retry context * @param state the {@link RetryState}