GH-2496: Reuse retry topic for maxInterval delay #2497

jgslima · 2022-12-05T02:47:51Z

Resolves #2496

Created SameIntervalTopicReuseStrategy, used to keep retrying in the same retry topic when the delays of the last retries are the same.

Resolves spring-projects#2496 Created SameIntervalTopicReuseStrategy, used to keep retrying in the same retry topic when the delays of the last retries are the same.

jgslima · 2022-12-05T02:53:28Z

...kafka/src/main/java/org/springframework/kafka/retrytopic/SameIntervalTopicReuseStrategy.java

+ * @since 3.1
+ *
+ */
+public enum SameIntervalTopicReuseStrategy {


Initially I had modeled something like ExponentialBackoffMaxDelayTopicReuseStrategy, but it would be too specific for exponential backoff. So I went with a more generic concept, with the logic in DefaultDestinationTopicResolver also being more generic.

It turns out that, with this SameIntervalTopicReuseStrategy idea, the FixedDelayStrategy could be removed, because the logic implemented for SameIntervalTopicReuseStrategy itself would be enough to conclude that a single retry topic should be used also for fixed backoffs. In fact, for DefaultDestinationTopicResolver there is no difference between these 2 cases, it is a single implementation.

Unfortunately, the feature already went GA. Maybe we may mark FixedDelayStrategy as deprecated, with it being removed and replaced by SameIntervalTopicReuseStrategy in some future major release.

I think this makes total sense - fixed delays are only a specific case of the broader topic resuse concept. So IMHO having both concepts at the same time tend to be a bit confusing. I'm thinking we could deprecate the FixedDelayStrategy at once in this PR and use the new option. WDYT?

But for that to work, I think we'd need to adapt the FixedDelayStrategy part to respond to both configurations.

The part that called my attention is this one:

spring-kafka/spring-kafka/src/main/java/org/springframework/kafka/retrytopic/DestinationTopicPropertiesFactory.java

Lines 114 to 120 in 3f645b1

private boolean isSingleTopicFixedDelay() {

return isFixedDelay() && isSingleTopicStrategy();

}

private boolean isSingleTopicStrategy() {

return FixedDelayStrategy.SINGLE_TOPIC.equals(this.fixedDelayStrategy);

}

Maybe it should also look for the new configuration, and if it's SINGLE_TOPIC respect that regardless of the FIXED strategy?

This way we can deprecate the old enum and methods and users can rely only on the new one. Of course, the old configurations need to work the same way as before.

I'm thinking we could deprecate the FixedDelayStrategy at once in this PR and use the new option. WDYT?

@tomazfernandes, you are the bosses here. Feel free to direct me.

Now that I read again, I think I mostly said what you'd already said 😄

Only adding that in order to deprecate the FixedDelayStrategy, which I believe we should do, we need to make the SameIntervalTopicReuseStrategy also work for fixed delays.

I will wait for feedbacks from @garyrussell before actually changing things.

I plan to look at this tomorrow; but since @tomazfernandes is the original author of this feature, I trust his words.

I agree with deprecating the FDS, with the aim to remove it in 3.1.

We should probably also add a disclaimer in the javadocs / documentation that SameIntervalTopicReuseStrategy doesn't work (at least yet) when the same interval is in the middle of the backoff values. Though the OOTB BackOff policies doesn't allow that, we've seen users create custom policies that would allow such cases.

jgslima · 2022-12-05T03:31:44Z

...afka/src/main/java/org/springframework/kafka/retrytopic/DefaultDestinationTopicResolver.java

+				Assert.isTrue((i == (destinationsToAdd.size() - 1) ||
+						((i == (destinationsToAdd.size() - 2)) && (destinationsToAdd.get(i + 1).isDltTopic()))),
+						String.format("In the destination topic chain, the type %s can only be "
+								+ "specified as the last retry topic.", Type.REUSABLE_RETRY_TOPIC));


This invariant was implemented because right now, the idea is to have topic reuse only for exponential backoff, therefore for the maxDelay retries, which stay in the end of the chain.
In order to allow reuse of attempts with the same interval in the middle of the chain, more sophistication would be needed, maybe to have DestinationTopic.Properties to carry the maxAttempts of the specific topic.

But anyway, no existing backoff policy should have attempts sequences with a same interval in the middle of the chain.

garyrussell · 2022-12-07T19:54:57Z

@jgslima Thanks for the PR; it might take a bit of time to get it reviewed; with the upcoming holidays (and after recently getting out major releases) we are beginning to wind down a little. After today, I will not be working until the 19th and then, only for a few days before breaking again until the New Year.

I just wanted to let you know, in case you thought we were ignoring you; your work is definitely appreciated.

Thanks.

jgslima · 2022-12-07T20:01:24Z

@jgslima Thanks for the PR; it might take a bit of time to get it reviewed; with the upcoming holidays (and after recently getting out major releases) we are beginning to wind down a little. After today, I will not be working until the 19th and then, only for a few days before breaking again until the New Year.

I just wanted to let you know, in case you thought we were ignoring you; your work is definitely appreciated.

Thanks.

Ok, good to know, have a nice vacation.

garyrussell · 2023-01-26T19:21:06Z

@jgslima Sorry for the delay - I hope to get to this next week.

tomazfernandes

Cool feature @jgslima, thanks!!

Overall this looks great. I've left a few comments, and let's see what @garyrussell has to say.

tomazfernandes · 2023-01-28T23:22:32Z

...kafka/src/main/java/org/springframework/kafka/retrytopic/SameIntervalTopicReuseStrategy.java

+ * @since 3.1
+ *
+ */
+public enum SameIntervalTopicReuseStrategy {


I think this makes total sense - fixed delays are only a specific case of the broader topic resuse concept. So IMHO having both concepts at the same time tend to be a bit confusing. I'm thinking we could deprecate the FixedDelayStrategy at once in this PR and use the new option. WDYT?

But for that to work, I think we'd need to adapt the FixedDelayStrategy part to respond to both configurations.

The part that called my attention is this one:

spring-kafka/spring-kafka/src/main/java/org/springframework/kafka/retrytopic/DestinationTopicPropertiesFactory.java

Lines 114 to 120 in 3f645b1

private boolean isSingleTopicFixedDelay() {

return isFixedDelay() && isSingleTopicStrategy();

}

private boolean isSingleTopicStrategy() {

return FixedDelayStrategy.SINGLE_TOPIC.equals(this.fixedDelayStrategy);

}

Maybe it should also look for the new configuration, and if it's SINGLE_TOPIC respect that regardless of the FIXED strategy?

This way we can deprecate the old enum and methods and users can rely only on the new one. Of course, the old configurations need to work the same way as before.

tomazfernandes · 2023-01-28T23:26:03Z

spring-kafka-docs/src/main/asciidoc/retrytopic.adoc

+
+[[single-topic-maxinterval-delay]]
+===== Single Topic for maxInterval Exponential Delay 
+


I think you know this, but there's a one sentence per line rule for docs, this line has 2.

I think you know this, but there's a one sentence per line rule for docs, this line has 2.

I did not know that (first time with asciidoc). Maybe due to the file having been changed since then, the highlighted section does not show a multi-sentence line, but I will search here.

I did not know that (first time with asciidoc).

I thought you did because everything else was ok :)

Maybe due to the file having been changed since then, the highlighted section does not show a multi-sentence line, but I will search here.

I probably highlighted the wrong line, sorry. It's the line right next to that one ("If you're using exponential backoff policy")

We use one sentence per line to isolate future changes to a single sentence.

tomazfernandes · 2023-01-28T23:32:33Z

spring-kafka/src/main/java/org/springframework/kafka/retrytopic/DestinationTopic.java

@@ -68,8 +68,8 @@ public boolean isNoOpsTopic() {
 		return Type.NO_OPS.equals(this.properties.type);
 	}

-	public boolean isSingleTopicRetry() {
-		return Type.SINGLE_TOPIC_RETRY.equals(this.properties.type);
+	public boolean isReusableRetryTopic() {


I think this is a breaking change - we can keep both methods and deprecate the old one.

garyrussell

Good stuff; thanks!!

garyrussell · 2023-02-01T15:44:24Z

spring-kafka-docs/src/main/asciidoc/retrytopic.adoc

@@ -683,7 +650,7 @@ IMPORTANT: Note that the blocking retries behavior is allowlist - you add the ex

 IMPORTANT: The non-blocking exception classification behavior also depends on the specific topic's configuration.

-==== Topic Naming
+==== Topic Amount and Naming


I don't think we need to change the section title.

garyrussell · 2023-02-01T15:45:16Z

spring-kafka-docs/src/main/asciidoc/retrytopic.adoc

@@ -693,6 +660,11 @@ Examples:

 "my-other-topic" -> "my-topic-myRetrySuffix-1000", "my-topic-myRetrySuffix-2000", ..., "my-topic-myDltSuffix".

+NOTE: The default behavior is creating separate retry topics for each attempt, appended with their index value: retry-0, retry-1, ..., retry-n. Therefore, by default the amount of retry topics is the configured `maxAttempts` minus 1.


Suggested change

NOTE: The default behavior is creating separate retry topics for each attempt, appended with their index value: retry-0, retry-1, ..., retry-n. Therefore, by default the amount of retry topics is the configured `maxAttempts` minus 1.

NOTE: The default behavior is to create separate retry topics for each attempt, appended with an index value: retry-0, retry-1, ..., retry-n.

Therefore, by default the number of retry topics is the configured `maxAttempts` minus 1.

garyrussell · 2023-02-01T15:46:45Z

spring-kafka-docs/src/main/asciidoc/retrytopic.adoc

+
+[[single-topic-maxinterval-delay]]
+===== Single Topic for maxInterval Exponential Delay 
+


We use one sentence per line to isolate future changes to a single sentence.

garyrussell · 2023-02-01T15:50:34Z

...ka/src/main/java/org/springframework/kafka/retrytopic/DestinationTopicPropertiesFactory.java

-				.mapToObj(this::createRetryOrDltTopicSuffixes)
+				? retryTopicsAmount
+				: retryTopicsAmount + 1)
+				.mapToObj(this::createTopicProperties)
 				.collect(Collectors.toList());


Please restore the indentation.

garyrussell · 2023-02-01T15:51:50Z

...kafka/src/main/java/org/springframework/kafka/retrytopic/SameIntervalTopicReuseStrategy.java

+ * @since 3.1
+ *
+ */
+public enum SameIntervalTopicReuseStrategy {


I agree with deprecating the FDS, with the aim to remove it in 3.1.

garyrussell · 2023-02-01T15:54:47Z

...c/test/java/org/springframework/kafka/retrytopic/DestinationTopicPropertiesFactoryTests.java

@@ -88,7 +94,7 @@ void shouldCreateMainAndDltProperties() {
 		List<DestinationTopic.Properties> propertiesList =
 				new DestinationTopicPropertiesFactory(retryTopicSuffix, dltSuffix, backOffValues,
 						classifier, numPartitions, kafkaOperations, fixedDelayStrategy,
-						dltStrategy, defaultTopicSuffixingStrategy, RetryTopicConstants.NOT_SET)
+						dltStrategy, suffixWithDelayValueSuffixingStrategy, multipleTopicsSameIntervalReuseStrategy, RetryTopicConstants.NOT_SET)


Please limit code to 120 chars max (javadocs 90 max).

jgslima · 2023-02-02T17:52:59Z

Thank you for the reviews, I will be able to work on the changes on the weekend.

jgslima · 2023-02-05T17:56:25Z

In order to deprecate FixedDelayStrategy, new code that calls only the new methods in RetryTopicConfigurationBuilder should work fine. So the new methods like sameIntervalTopicReuseStrategy() should work also when using a single fixed delay topic.

However, by keeping the existing methods (like useSingleTopicForFixedDelays()) the builder would have 2 pairs of methods that would act on the same configuration, and therefore, the configuration would end up having the value of the method called last (which would override the previous call), which might have misleading results.

Since today nobody uses the new methods dedicated to SameIntervalTopicReuseStrategy (like the new one useSingleTopicForSameIntervals()), we could make the builder to throw an error if both kinds of methods were called, requiring the user to call just one of them.

But the problem is with RetryableTopicAnnotationProcessor. The new SameIntervalTopicReuseStrategy attribute of @RetryableTopic must have a default value (not just to simplify the annotation use, but also, to avoid breaking all existing declarations). Since both annotation attributes have default values, and are never null, for RetryableTopicAnnotationProcessor it is hard to know how to interact with the builder.

For the 2 cases below, I think we could give precedence for the sameIntervalTopicReuseStrategy and assume SINGLE, because, we can tell that the user actually declared something for sameIntervalTopicReuseStrategy (since it is different from its default value of MULTIPLE):

@RetryableTopic(
     sameIntervalTopicReuseStrategy = SameIntervalTopicReuseStrategy.SINGLE_TOPIC
)

@RetryableTopic(
     sameIntervalTopicReuseStrategy = SameIntervalTopicReuseStrategy.SINGLE_TOPIC,
     fixedDelayTopicStrategy = FixedDelayStrategy.MULTIPLE_TOPICS
)

But the problem occurs in these 2 cases:

// below, SameIntervalTopicReuseStrategy.MULTIPLE_TOPICS is assumed since it is the default value.
@RetryableTopic(
     fixedDelayTopicStrategy = FixedDelayStrategy.SINGLE_TOPIC
)

@RetryableTopic(
     fixedDelayTopicStrategy = FixedDelayStrategy.SINGLE_TOPIC
     sameIntervalTopicReuseStrategy = SameIntervalTopicReuseStrategy.MULTIPLE_TOPICS
)

For them, we cannot actually give some form of precedence for the configuration of SameIntervalTopicReuseStrategy, because we cannot tell whether the user actually configured it (I do not know means in the Java reflection API to tell whether an annotation attribute was actually declared).
If we could tell whether the user actually declared the attribute, we could assume SINGLE in the first case, and assume MULTIPLE in the second case (or even throw an error, requiring the user to configure just one of them).

We cannot assume MULTIPLE, because this would change the behaviour of existing codes like the first case.

We cannot also give precedence for the configuration of fixedDelayTopicStrategy = FixedDelayStrategy.SINGLE_TOPIC because in the second case, it would not make sense to give precedence to a deprecated property.

So I am not being capable of thinking about a way to make RetryableTopicAnnotationProcessor to work fine with the new behaviour of RetryTopicConfigurationBuilder. :(

jgslima · 2023-02-05T18:00:15Z

I thought about having RetryableTopicAnnotationProcessor to check if both annotation attributes values are "compatible", throwing an error if not, so that only the method builder.sameIntervalTopicReuseStrategy() would be called. But this would break existing code that just declares fixedDelayTopicStrategy = FixedDelayStrategy.SINGLE_TOPIC, since this would be incompatible with SameIntervalTopicReuseStrategy.MULTIPLE_TOPICS, which must be the default value.

tomazfernandes · 2023-02-05T19:00:59Z

Hi @jgslima, thanks for looking into this.

I might be missing something, but what's on my mind seems a bit simpler:

Both enums default to MULTIPLE_TOPICS
For the new logic, only SameIntervalTopicReuseStrategy is looked up, so no problems there.
For existing logic, we can look at both enum values - if any of them is SINGLE_TOPIC, single topic it is.

My reasoning for the last one is:

Existing logic with SINGLE_TOPIC for fixedDelayTopicStrategy would work as expected;
Existing logic with default values / MULTIPLE_TOPICS would work as expected;
New user logic using SameIntervalTopicReuseStrategy#SINGLE_TOPIC for fixed delays would work as expected
New user logic using SameIntervalTopicReuseStrategy#MULTIPLE_TOPICS for fixed delays would work as expected.

What am I missing?

Thanks.

jgslima · 2023-02-06T16:19:29Z

Tonight I may be able to take a deep dive into your thoughts. At first, one case I can think of is existing code like where the developer declared FixedDelayStrategy.SINGLE_TOPIC, but is in fact using an exponential backoff:

@RetryableTopic(
  backoff = xxx,  // here, a backoff that actually results in exponential.
  fixedDelayTopicStrategy = FixedDelayStrategy.SINGLE_TOPIC
)

For instance, @Backoff may be configured with a delayExpression that sometimes may result in exponential, other times in fixed. Or it is always exponential, and fixedDelayTopicStrategy is there without need.

These cases, currently, result in multiple topics for the maxDelay. By making RetryableTopicAnnotationProcessor to assume single, would change the behaviour.

Later I will try to think about all combinations to see if we can make RetryableTopicAnnotationProcessor to assume the strategy based also on the actual backoff type (it can inspect the actual instance created for the backoff policy).

tomazfernandes · 2023-02-06T16:29:49Z

These cases, currently, result in multiple topics for the maxDelay. By making RetryableTopicAnnotationProcessor to assume single, would change the behaviour.

I don't think RetryableTopicAnnotationProcessor should really assume anything - it's role is to simply create the RetryTopicConfiguration with what's on the annotation. So, if it's FixedDelayStrategy.SINGLE_TOPIC, we just forward it. Same with the new configuration. Both configurations will need to cohexist in the RetryTopicConfiguration class, one being deprecated.

When we get to the point where we check for the strategy, we just check if all backoff values are the same, so if it's an exponential backoff we should simply ignore that configuration.

The fixedDelayTopicStrategy shouldn't have any interference in how the exponential backoff behaves, with old or new logic.

Hope this helps :)

Thanks.

jgslima · 2023-02-06T22:18:58Z

Ok then.

I thought that, if a method is deprecated, ideally the framework should provide a recommended, non-deprecated alternative.

That's why I thought that the new methods in the builder should work to configure for fixed delay as well. Which in turn means a single configuration inside.

Anyway, I will proceed with the simple approach for the deprecation. Existing code that refers to FDS will have to be changed in 3.1.

RetryTopic docs and code fixes and small refactorings.

jgslima · 2023-02-07T01:41:54Z

I believe I have made all the requested changes. Please review. Thanks.

jgslima · 2023-02-22T19:18:09Z

Hello guys. Any news on this? Thank you.

garyrussell · 2023-02-22T20:20:52Z

Sorry for the delay; it's on my TODO list, will try to get to it soon.

tomazfernandes · 2023-02-25T19:16:10Z

spring-kafka/src/main/java/org/springframework/kafka/retrytopic/FixedDelayStrategy.java

 *
 */
+@Deprecated(forRemoval = true) // in 3.1


Hi João! I'm not sure I understand - as far as I am aware we won't remove anything in 3.1, as we need to keep backwards compatibility until the next major or at least a few minors. Care to elaborate on the comment? Thanks!

I followed this:
#2497 (comment)

Anyway, I can remove the removal mentions, just want to hear from @garyrussell again.

Oh, I didn't remember this comment. Perhaps @garyrussell can enlighten us on this?

My current plan is to look at this tomorrow, unless something more urgent comes in.

I am not opposed to removing stuff in the next minor release, it really depends on how critical or invasive it is.

We need @SuppressWarnings("deprecation") where still used (including tests). Also, remove imports and fully qualify the class name where used.

tomazfernandes · 2023-02-25T19:16:39Z

spring-kafka/src/main/java/org/springframework/kafka/retrytopic/FixedDelayStrategy.java

@@ -22,8 +22,10 @@
 *
 * @author Tomaz Fernandes
 * @since 2.7
+ * @deprecated in a future release, will be replaced by {@link SameIntervalTopicReuseStrategy}.


We can simply say: deprecated in favor of {@link SameIntervalTopicReuseStrategy}

Ok I will proceed with the changes

We can simply say: deprecated in favor of {@link SameIntervalTopicReuseStrategy}

Ok.

I do not think we can say that.
Previously, I had said that I was having hard time trying to condense (internally at RetryTopicConfigurationBuilder and DestinationTopicPropertiesFactory) both configurations, in order for these components to work only with the SameIntervalTopicReuseStrategy configuration. So that, when a method like RetryTopicConfigurationBuilder.useSingleTopicForFixedDelays() was called, internally this would actually configure the SameIntervalTopicReuseStrategy.

But then, we aggreed on keeping both configurations. Just marking the old configurations as deprecated, aiming to remove them in the future (when DestinationTopicPropertiesFactory should be changed).

So we cannot say to make use of SameIntervalTopicReuseStrategy, because right now, for Fixed delay topics, FixedDelayStrategy still must be used.

And to recap... when trying to condense the configs, thus allowing new code to configure single fixed delay topic using the new methods like useSingleTopicForSameIntervals(), the real problem is in RetryableTopicAnnotationProcessor and how it would interact correctly with the builder.

Like I explained here.

Sorry, I've read your comment before, I read it again now, and I'm having a hard time understanding why this is so complicated.

In my head, we can just leave both methods / parameters in all classes you mentioned, deprecating the old ones, and solve any ambiguity in the DestinationTopicPropertiesFactory class, by the rules I mentioned in the other comment I made earlier:

Both enums default to MULTIPLE_TOPICS
For the new logic, only SameIntervalTopicReuseStrategy is looked up, so no problems there.
For existing logic, we can look at both enum values - if any of them is SINGLE_TOPIC, single topic it is.

Can you please help me understand why wouldn't this work? Thanks.

Or being more specific:

For fixed delays, we can look at both the old and new enums - whichever has SINGLE_TOPIC has precedence.

For exponential (non-fixed) delays we only look at the new enum.

In your example:

At first, one case I can think of is existing code like where the developer declared FixedDelayStrategy.SINGLE_TOPIC, but is in fact using an exponential backoff

For this case, we'd assume SINGLE_TOPIC for fixed delays, just as it is now. For exponential delays the new enum would be MULTIPLE_TOPICS so wouldn't change functionality. If the user sets the new enum for SINGLE_TOPIC, the new enum should take precedence and impact both the fixed delay and exponential delay variants.

Also, we don't really need to take care of all combinations. E.g. if the user explicitly sets the old enum to MULTIPLE and the new one to SINGLE, that's a misconfiguration (using both a deprecated and the new configuration at the same time) and we can just assume SINGLE. At most we'd need to document this in the javadocs and / or documentation.

Does this make sense? Thanks.

I agree with @tomazfernandes here.

…irar_removal

garyrussell · 2023-02-27T15:35:16Z

spring-kafka/src/main/java/org/springframework/kafka/retrytopic/FixedDelayStrategy.java

 *
 */
+@Deprecated(forRemoval = true) // in 3.1


We need @SuppressWarnings("deprecation") where still used (including tests). Also, remove imports and fully qualify the class name where used.

garyrussell · 2023-02-27T15:36:13Z

spring-kafka/src/main/java/org/springframework/kafka/retrytopic/DestinationTopic.java

+		return Type.REUSABLE_RETRY_TOPIC.equals(this.properties.type);
+	}
+
+	@Deprecated(forRemoval = true) // in 3.1


Need @SuppressWarnings("deprecation") in DestinationTopicPropertiesFactoryTests.

garyrussell · 2023-02-27T15:38:28Z

spring-kafka/src/main/java/org/springframework/kafka/retrytopic/DestinationTopic.java

@@ -152,15 +160,17 @@ public static class Properties {
 		 * @param kafkaOperations the {@link KafkaOperations}.
 		 * @param shouldRetryOn the exception classifications.
 		 * @param timeout the timeout.
+		 * @param firstAttemptIndex the first attempt this topic is used with.
 		 */
 		public Properties(long delayMs, String suffix, Type type,


Since this is a public API, we need to keep (and deprecate) the old CTOR.

garyrussell · 2023-02-27T15:41:56Z

spring-kafka/src/main/java/org/springframework/kafka/retrytopic/DestinationTopic.java

 		 */
 		public Properties(long delayMs, String suffix, Type type,
 						int maxAttempts, int numPartitions,
 						DltStrategy dltStrategy,
 						KafkaOperations<?, ?> kafkaOperations,
-						BiPredicate<Integer, Throwable> shouldRetryOn, long timeout) {
+						BiPredicate<Integer, Throwable> shouldRetryOn, long timeout,
+						Integer firstAttemptIndex) {


Why is this Integer and not int? If it can be null it must be marked @Nullable.

garyrussell · 2023-02-27T15:42:18Z

spring-kafka/src/main/java/org/springframework/kafka/retrytopic/DestinationTopic.java

@@ -188,13 +198,15 @@ public Properties(Properties sourceProperties, String suffix, Type type) {
 		 * @param shouldRetryOn the exception classifications.
 		 * @param timeout the timeout.
 		 * @param autoStartDltHandler whether or not to start the DLT handler.
+		 * @param firstAttemptIndex the first attempt this topic is used with.
 		 * @since 2.8
 		 */
 		public Properties(long delayMs, String suffix, Type type,


Ditto (old CTOR must remain).

garyrussell · 2023-02-27T15:47:46Z

spring-kafka/src/main/java/org/springframework/kafka/retrytopic/DestinationTopic.java

@@ -280,6 +293,6 @@ public boolean isMainEndpoint() {
 	}

 	enum Type {
-		MAIN, RETRY, SINGLE_TOPIC_RETRY, DLT, NO_OPS
+		MAIN, RETRY, REUSABLE_RETRY_TOPIC, DLT, NO_OPS


We can't do this in a patch release; need to keep (and deprecate) the old value and make it an alias of the new one.

We can't do this in a patch release; need to keep (and deprecate) the old value and make it an alias of the new one.

I did not push this now. This was in the first commit at Dec 4, 2022.

It is one of the basis of the logic reuse for both kinds of backoff. `DefaultDestinationTopicResolver' will not have a single concept anymore.

?? I only see 5 commits total, and none of the subsequent patches reverted it - this is still the state of the code in the PR.

What I meant is that the original change was not pushed in the last commits. It was in the PR since the beginning.

I am working to revert it right now.

garyrussell · 2023-02-27T15:53:54Z

spring-kafka/src/main/java/org/springframework/kafka/retrytopic/FixedDelayStrategy.java

@@ -22,8 +22,10 @@
 *
 * @author Tomaz Fernandes
 * @since 2.7
+ * @deprecated in a future release, will be replaced by {@link SameIntervalTopicReuseStrategy}.


I agree with @tomazfernandes here.

…RETRY.

…irar_removal

jgslima · 2023-02-28T03:25:09Z

Pushed requested changes

jgslima · 2023-03-01T14:57:41Z

...c/test/java/org/springframework/kafka/retrytopic/DestinationTopicPropertiesFactoryTests.java

+		assertThat(firstRetryProperties.suffix()).isEqualTo(retryTopicSuffix);
+		DestinationTopic retryDestinationTopic = destinationTopicList.get(1);
+		assertThat(retryDestinationTopic.isSingleTopicRetry()).isTrue();
+		assertThat(retryDestinationTopic.isReusableRetryTopic()).isFalse();


Here, it is a matter of convention. We might consider every singleTopicRetry to also be a reusableRetryTopic. Right now, these methods are just based on the internal Type attribute.

garyrussell

LGTM; just some minor polishing needed.

One other thing I'd like to verify is that the enum changes are binary compatible - can users that explicitly use the enum drop in the new jar and use it without recompiling. We always avoid API changes at the compiler level, but we strive to maintain binary compatibility too.

Most likely, we'll have to move the new value to the end of the enum but I am not sure if even that will work.

garyrussell · 2023-03-01T20:01:36Z

spring-kafka-docs/src/main/asciidoc/retrytopic.adoc

@@ -683,7 +651,7 @@ IMPORTANT: Note that the blocking retries behavior is allowlist - you add the ex

 IMPORTANT: The non-blocking exception classification behavior also depends on the specific topic's configuration.

-==== Topic Naming
+==== Topic Amount


Suggested change

==== Topic Amount

==== Topic Naming

garyrussell · 2023-03-01T20:01:55Z

spring-kafka-docs/src/main/asciidoc/retrytopic.adoc

@@ -693,6 +661,12 @@ Examples:

 "my-other-topic" -> "my-topic-myRetrySuffix-1000", "my-topic-myRetrySuffix-2000", ..., "my-topic-myDltSuffix".

+NOTE: The default behavior is to create separate retry topics for each attempt, appended with an index value: retry-0, retry-1, ..., retry-n.
+Therefore, by default the amount of retry topics is the configured `maxAttempts` minus 1.


Suggested change

Therefore, by default the amount of retry topics is the configured `maxAttempts` minus 1.

Therefore, by default the number of retry topics is the configured `maxAttempts` minus 1.

garyrussell · 2023-03-01T20:06:07Z

spring-kafka-docs/src/main/asciidoc/retrytopic.adoc

+
+NOTE: By opting to use a single topic for the retries with the `maxInterval` delay, it may become more viable to configure an exponential retry policy that keeps retrying for a long time, because in this approach you do not need a large amount of topics.
+
+The default behavior is to work with an amount of retry topics equal to the configured `maxAttempts` minus 1, and when using exponential backoff, the retry topics are suffixed with the delay values, with the last retry topics (corresponding to the `maxInterval` delay) being suffixed with an additional index.


Suggested change

The default behavior is to work with an amount of retry topics equal to the configured `maxAttempts` minus 1, and when using exponential backoff, the retry topics are suffixed with the delay values, with the last retry topics (corresponding to the `maxInterval` delay) being suffixed with an additional index.

The default behavior is to work with the number of retry topics equal to the configured `maxAttempts` minus 1 and, when using exponential backoff, the retry topics are suffixed with the delay values, with the last retry topic (corresponding to the `maxInterval` delay) being suffixed with an additional index.

garyrussell · 2023-03-01T20:07:03Z

spring-kafka-docs/src/main/asciidoc/retrytopic.adoc

+* -retry-4000
+* -retry-8000
+* -retry-16000
+


Suggested change

This will be the default in a future release.

garyrussell · 2023-03-01T20:14:55Z

...kafka/src/main/java/org/springframework/kafka/retrytopic/RetryTopicConfigurationBuilder.java

@@ -71,6 +71,7 @@ public class RetryTopicConfigurationBuilder {
 	@Nullable
 	private BinaryExceptionClassifierBuilder classifierBuilder;

+	@SuppressWarnings("deprecation")
 	private FixedDelayStrategy fixedDelayStrategy = FixedDelayStrategy.MULTIPLE_TOPICS;


This needs to be fully qualified (and remove the import). Otherwise the compiler will emit a deprecation warning for the import.

Well, there is no import in the top of the class. It is in the same package.

Doh - sorry.

garyrussell · 2023-03-01T21:12:48Z

Correction DestinationTopic.Type is not a public API (not visible).

garyrussell

I think we can get this into tomorrow's 3.0.4 release (late day US EST).

I tested it with a Boot app I wrote when you submitted the original discussion #2484 and it works nicely.

Thanks for all this work; sorry it took so long to get through the review process.

garyrussell · 2023-03-01T21:13:52Z

...kafka/src/main/java/org/springframework/kafka/retrytopic/SameIntervalTopicReuseStrategy.java

+ * interval in the middle of the retry chain).
+ *
+ * @author João Lima
+ * @since 3.1


Suggested change

* @since 3.1

* @since 3.0.4

garyrussell · 2023-03-01T21:14:25Z

spring-kafka/src/main/java/org/springframework/kafka/annotation/RetryableTopic.java

+	 * <p>Note: for fixed backoffs, when this is configured as
+	 * {@link SameIntervalTopicReuseStrategy#SINGLE_TOPIC}, it has precedence over
+	 * the configuration in {@link #fixedDelayTopicStrategy()}.
+	 * @return the strategy.


Suggested change

* @return the strategy.

* @return the strategy.

* @since 3.0.4

garyrussell · 2023-03-01T21:15:23Z

spring-kafka/src/main/java/org/springframework/kafka/retrytopic/DestinationTopic.java

+		 */
+		@Deprecated
+		SINGLE_TOPIC_RETRY,
+		REUSABLE_RETRY_TOPIC, DLT, NO_OPS


Please make them all one per line and add @since 3.0.4 to the new one.

garyrussell · 2023-03-01T21:16:36Z

...kafka/src/main/java/org/springframework/kafka/retrytopic/RetryTopicConfigurationBuilder.java

+	 * the configuration done through
+	 * {@link #useSingleTopicForFixedDelays(FixedDelayStrategy)}.
+	 * @param sameIntervalTopicReuseStrategy the strategy.
+	 * @return the builder.


Suggested change

* @return the builder.

* @return the builder.

* since 3.0.4

garyrussell · 2023-03-01T21:16:53Z

...kafka/src/main/java/org/springframework/kafka/retrytopic/RetryTopicConfigurationBuilder.java

+	 * and in a future release this will dictate whether to use
+	 * a single retry topic for fixed backoff.
+	 *
+	 * @return the builder.


Suggested change

* @return the builder.

* @return the builder.

* @since 3.0.4

garyrussell · 2023-03-02T21:10:33Z

...kafka/src/main/java/org/springframework/kafka/retrytopic/RetryTopicConfigurationBuilder.java

+	 * Currently used only for the last retries of exponential backoff,
+	 * and in a future release this will dictate whether to use
+	 * a single retry topic for fixed backoff.


@jgslima Looks like we missed cleaning this up - I just tested it and the SameIntervalTopicReuseStrategy works fine for a fixed delay too.

It's true that the other strategy will be removed next release, but they can switch to the new one today (or am I missing something?).

You are right. :(
One of the last changes was indeed to allow the new strategy to be used for fixed delays.
My mistake.
Do you want me to open a PR?

Thanks for confirming; no; I'll just push a fix.

Thanks again for implementing this.

spring-projectsGH-2496: Reuse retry topic for maxInterval delay

bdbc580

Resolves spring-projects#2496 Created SameIntervalTopicReuseStrategy, used to keep retrying in the same retry topic when the delays of the last retries are the same.

jgslima commented Dec 5, 2022

View reviewed changes

spring-projectsGH-2496 Reference documentation typos.

c5becce

tomazfernandes reviewed Jan 28, 2023

View reviewed changes

Merge branch 'main' into GH-2496_reuse_retry_topic_maxDelay

d0d5c23

garyrussell suggested changes Feb 1, 2023

View reviewed changes

jgslima added 2 commits February 6, 2023 22:39

spring-projectsGH-2496: Deprecation of FixedDelayStrategy.

4c6cf75

RetryTopic docs and code fixes and small refactorings.

spring-projectsGH-2496: Restoring DestinationTopic.isSingleTopicRetry()

20865a6

tomazfernandes reviewed Feb 25, 2023

View reviewed changes

Merge branch 'main' into GH-2496_reuse_retry_topic_maxDelay_ajustes_t…

500d7dc

…irar_removal

garyrussell suggested changes Feb 27, 2023

View reviewed changes

jgslima added 3 commits February 27, 2023 23:12

spring-projectsGH-2496: Restoring DestinationTopic.Type.SINGLE_TOPIC_…

d466fe5

…RETRY.

spring-projectsGH-2496: Removal of "forRemoval" for FixedDelayStrategy

f7ff884

spring-projectsGH-2496: Supressing deprecation warnings

6cd53ef

jgslima added 2 commits February 28, 2023 00:10

Merge branch 'main' into GH-2496_reuse_retry_topic_maxDelay_ajustes_t…

5c2bc89

…irar_removal

spring-projectsGH-2496 Ajusting new method isRetryTopic().

7ccdf16

jgslima commented Mar 1, 2023

View reviewed changes

garyrussell suggested changes Mar 1, 2023

View reviewed changes

spring-projectsGH-2496: Documentation and formatting

c038603

garyrussell merged commit a7af914 into spring-projects:main Mar 2, 2023

garyrussell added this to the 3.0.4 milestone Mar 2, 2023

garyrussell added the type: enhancement label Mar 2, 2023

garyrussell reviewed Mar 2, 2023

View reviewed changes

bky373 mentioned this pull request Jan 12, 2025

Align default behavior of @RetryableTopic and RetryTopicConfigurationBuilder with documentation #3706

Open

	private boolean isSingleTopicFixedDelay() {
	return isFixedDelay() && isSingleTopicStrategy();
	}

	private boolean isSingleTopicStrategy() {
	return FixedDelayStrategy.SINGLE_TOPIC.equals(this.fixedDelayStrategy);
	}


		[[single-topic-maxinterval-delay]]
		===== Single Topic for maxInterval Exponential Delay

		@@ -693,6 +660,11 @@ Examples:

		"my-other-topic" -> "my-topic-myRetrySuffix-1000", "my-topic-myRetrySuffix-2000", ..., "my-topic-myDltSuffix".

		NOTE: The default behavior is creating separate retry topics for each attempt, appended with their index value: retry-0, retry-1, ..., retry-n. Therefore, by default the amount of retry topics is the configured `maxAttempts` minus 1.

	NOTE: The default behavior is creating separate retry topics for each attempt, appended with their index value: retry-0, retry-1, ..., retry-n. Therefore, by default the amount of retry topics is the configured `maxAttempts` minus 1.
	NOTE: The default behavior is to create separate retry topics for each attempt, appended with an index value: retry-0, retry-1, ..., retry-n.
	Therefore, by default the number of retry topics is the configured `maxAttempts` minus 1.

	Therefore, by default the amount of retry topics is the configured `maxAttempts` minus 1.
	Therefore, by default the number of retry topics is the configured `maxAttempts` minus 1.


		NOTE: By opting to use a single topic for the retries with the `maxInterval` delay, it may become more viable to configure an exponential retry policy that keeps retrying for a long time, because in this approach you do not need a large amount of topics.

		The default behavior is to work with an amount of retry topics equal to the configured `maxAttempts` minus 1, and when using exponential backoff, the retry topics are suffixed with the delay values, with the last retry topics (corresponding to the `maxInterval` delay) being suffixed with an additional index.

	The default behavior is to work with an amount of retry topics equal to the configured `maxAttempts` minus 1, and when using exponential backoff, the retry topics are suffixed with the delay values, with the last retry topics (corresponding to the `maxInterval` delay) being suffixed with an additional index.
	The default behavior is to work with the number of retry topics equal to the configured `maxAttempts` minus 1 and, when using exponential backoff, the retry topics are suffixed with the delay values, with the last retry topic (corresponding to the `maxInterval` delay) being suffixed with an additional index.

	* @return the strategy.
	* @return the strategy.
	* @since 3.0.4

GH-2496: Reuse retry topic for maxInterval delay #2497

GH-2496: Reuse retry topic for maxInterval delay #2497

Conversation

jgslima commented Dec 5, 2022

jgslima Dec 5, 2022 • edited Loading

Choose a reason for hiding this comment

tomazfernandes Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garyrussell commented Dec 7, 2022

jgslima commented Dec 7, 2022

garyrussell commented Jan 26, 2023

tomazfernandes left a comment

Choose a reason for hiding this comment

tomazfernandes Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garyrussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgslima commented Feb 2, 2023

jgslima commented Feb 5, 2023

jgslima commented Feb 5, 2023

tomazfernandes commented Feb 5, 2023

jgslima commented Feb 6, 2023

tomazfernandes commented Feb 6, 2023

jgslima commented Feb 6, 2023

jgslima commented Feb 7, 2023

jgslima commented Feb 22, 2023

garyrussell commented Feb 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgslima commented Feb 28, 2023

Choose a reason for hiding this comment

garyrussell left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garyrussell commented Mar 1, 2023

garyrussell left a comment

jgslima Dec 5, 2022 •

edited

Loading

tomazfernandes Jan 28, 2023 •

edited

Loading

tomazfernandes Jan 28, 2023 •

edited

Loading

garyrussell left a comment •

edited

Loading