GH-2800: Make it possible to route messages to custom DLT based on exceptions thrown #2929

breader124 · 2023-12-10T16:38:36Z

This PR is related to #2800. It introduces changes making it easy for developers to route messages to different DLTs when the previously configured exception is thrown.

sobychacko · 2023-12-12T19:57:27Z

@breader124 Thanks for the PR. I am going to take a look reviewing the changes. One general comment. Could you add some docs snippets related to the change? This seems like a bigger change that deserves some notes in the reference docs.

breader124 · 2023-12-12T21:14:05Z

Sure, I was thinking about modifications in the reference docs as well. The only thing I'd like to wait for until I start working on it is the confirmation of the API part of changes. Having the API stable I'll propose docs changes

sobychacko · 2023-12-12T23:26:24Z

Since this PR is large in terms of the changes, could you provide a brief overview of the changes in a few sentences? Please provide a flow of the changes, the APIs you added, etc. It doesn't have to be very elaborate, but brief.

breader124 · 2023-12-13T09:03:37Z

The main difference that I introduced is the possibility to configure the routing to the custom DLTs based on the exceptions thrown during the message processing. This configuration can be provided in 2 ways (and these are the only new APIs):

Using the org.springframework.kafka.annotation.RetryableTopic#exceptionBasedDltRouting. There you need to pass the array of routing rules (org.springframework.kafka.retrytopic.ExceptionBasedDestinationDlt). Each of them specifies what suffix extension should be used when creating the custom DLT and which exceptions should be handled by it
Using the org.springframework.kafka.retrytopic.RetryTopicConfigurationBuilder#dltRoutingRules method, where you pass the Map describing the custom DLTs routing rules.

When the configuration is provided, then depending on the autoCreateTopics config property new DLTs might be created together with rest of the topic automatically. When these DLTs are being created, then their properties are placed before the properties of the generic purpose DLT inside the org.springframework.kafka.retrytopic.DestinationTopicPropertiesFactory#createPropertiesForDefaultTopicStrategy method. Thanks to this change they are considered earlier for routing than the generic purpose DLT is considered.

Lastly, when it comes to the actual routing part, I've modified the org.springframework.kafka.retrytopic.DefaultDestinationTopicResolver. There I try to match the exception, which has been thrown during the message processing with the ones configured for the particular custom DLT. This process traverse causes and it's not configurable now. When I'm not able to find any custom, dedicated DLT, then finally the processing comes to the generic purpose DLT and message will land there.

Besides all changes described above I also provided tests for the changed behaviour.

artembilan

This sounds reasonable, but unfortunately it introduces some breaking changes to public API. To avoid immediate upgrade in the target projects we cannot accept this change for the current patch version.

We are fully OK to schedule this for the next 3.2 generation, but not earlier.

Therefore your PR will sit here for a while.

Thanks for understanding.

sobychacko · 2023-12-13T16:56:28Z

That's a great point, Artem. @ breader124 If you still want to go into the patch release, could you still make these changes without breaking an existing API? It might still be okay to add new API's as we have done in some other existing PR's.

Update: It is better to postpone this PR for 3.2.0, as Artem mentioned, due to the scope of the changes in the PR.

breader124 · 2023-12-13T19:54:12Z

It's totally fine for me to wait for a while @artembilan, @sobychacko. I'll update the @since tags in the Javadocs then to make them point to the 3.2.0 version instead of the 3.1.1

sobychacko · 2023-12-13T22:16:33Z

...g-kafka/src/main/java/org/springframework/kafka/retrytopic/ExceptionBasedDestinationDlt.java

+ * @see org.springframework.kafka.retrytopic.ExceptionBasedDltRouting
+ * @since 3.2.0
+ */
+public @interface ExceptionBasedDestinationDlt {


Maybe ExceptionBasedDltDestination?

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

sobychacko · 2023-12-13T22:32:58Z

@breader124 I did a first pass at the PR. It looks good. One general question: The corresponding issue that was created by the SO thread , originally trying to route records from deserialization exceptions. Is that use case addressed by this PR?

breader124 · 2023-12-15T23:14:28Z

Yes, it's possible to configure the @RetryableTopic the way how it was done in one of the tests. Then messages, which caused DeserializationExceptions will be routed to the different DLT

artembilan · 2024-01-17T23:14:42Z

@breader124 ,

please, rebase your branch to the latest main.

Then we will start looking into your change to incorporate it into a new version.

Thanks

artembilan

I also believe that this feature deserves some docs in the \spring-kafka\spring-kafka-docs\src\main\antora\modules\ROOT\pages\retrytopic\ dir.

But yeah, that can be done in the end when we agree with all the changes from the code perspective.

Thanks

artembilan · 2024-01-18T18:27:37Z

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

@@ -188,13 +196,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 usedForExceptions the exceptions which destination is intended for
 		 * @since 2.8
 		 */
 		public Properties(long delayMs, String suffix, Type type,


I suggest to have one more public ctor.
This way we won't have a breaking change in the point version.

artembilan · 2024-01-18T18:30:09Z

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

 	 * @return The {@link DestinationTopic} instance corresponding to the DLT.
 	 */
 	@Nullable
-	DestinationTopic getDltFor(String mainListenerId, String topicName);
+	DestinationTopic getDltFor(String mainListenerId, String topicName, Exception exc);


Cannot we have another method to avoid a breaking change?
It also would be great to have it as a default to avoid a immediate upgrade compilation problem.
We may have the current method deprecated then.

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

...g-kafka/src/main/java/org/springframework/kafka/retrytopic/ExceptionBasedDltDestination.java

artembilan · 2024-01-18T18:34:45Z

...g-kafka/src/main/java/org/springframework/kafka/retrytopic/ExceptionBasedDltDestination.java

+	 *
+	 * @return the configured suffix extension
+	 */
+	String suffix();


I wonder if this is really required and can be just empty by default...

Also: no blank lines in the method Javadocs: https://github.com/spring-projects/spring-framework/wiki/Code-Style#javadoc-formatting

From my point of view having the suffix required makes this API harder to misuse by design. exceptions is required, but when someone will be allowed to specify exceptions without the suffix, then it means that the general purpose DLT will be used even for this exc. I'm not sure when it could be an intended usage

artembilan · 2024-01-18T18:36:42Z

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

+ * @see ExceptionBasedDltDestination
+ * @since 3.2.0
+ */
+public @interface ExceptionBasedDltRouting {


Why do we need this annotation if @RetryableTopic could just have a property like that below?

It's not needed, I'll delete it

artembilan · 2024-01-18T18:37:37Z

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

+	 * @since 3.2.0
+	 */
+	public RetryTopicConfigurationBuilder dltRoutingRules(Map<String, Set<Class<? extends Throwable>>> dltRoutingRules) {
+		this.dltRoutingRules = dltRoutingRules;


This is not OK. Has to be a copy.
The point is to avoid internal mutation when that external map is edited.
Well, that dltRoutingRules could be final and here you could just use putAll()

artembilan · 2024-01-19T15:01:11Z

Still, rebased is required for this PR.

Thanks

breader124 · 2024-01-19T17:27:14Z

Thanks for the review. I've just pushed the rebased code and will apply the changes you've suggested in the review over the weekend together with the docs changes suggestions

breader124 · 2024-01-25T17:29:33Z

@artembilan, @sobychacko, I've just rebased the code on top of the most recent main and pushed it. All the suggestions you had are now introduced and I think that the code is ready for the final round of the review

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

...ka/src/main/java/org/springframework/kafka/annotation/RetryableTopicAnnotationProcessor.java

artembilan · 2024-01-26T20:12:18Z

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

@@ -96,6 +131,7 @@ public DestinationTopicPropertiesFactory(String retryTopicSuffix, String dltSuff
 		this.sameIntervalTopicReuseStrategy = sameIntervalTopicReuseStrategy;
 		this.timeout = timeout;
 		this.destinationTopicSuffixes = new DestinationTopicSuffixes(retryTopicSuffix, dltSuffix);
+		this.dltRoutingRules = dltRoutingRules;


This is not OK. Has to be a copy of the map.
This way we will avoid the problem when provided map is mutated somewhere outside.

artembilan · 2024-01-26T20:23:14Z

spring-kafka-docs/src/main/antora/modules/ROOT/pages/retrytopic/features.adoc

+Routing customization consists of the specification of the additional destinations.
+Destinations in turn consist of two settings: the `suffix` and `exceptions`.
+When the exception type specified in `exceptions` has been thrown, the DLT containing the `suffix` will be considered as the target topic for the message before the general purpose DLT is considered.
+Take a look at the examples:


I prefer to have an official neutral language in docs where it is depersonalized.
So, no phrases like you, me, take a look etc.

But... That's my personal preferences and I know there are many like that throughout the doc.

Also: this need to me mentioned in the whats-new.adoc

sobychacko · 2024-01-31T17:07:12Z

@breader124 There was a recent change made via another PR that modified DestinationTopicPropertiesFactory. Could you rebase your PR branch against the latest main?

breader124 · 2024-02-01T19:51:19Z

Sure, will do it and push tomorrow the latest

…uting

…ception type

…se causes Closes spring-projects#2800

…ased in version 3.2.0 Closes spring-projects#2800

…ionBasedDltDestination

Fixes spring-projects#2800

breader124 · 2024-02-01T23:02:35Z

@sobychacko, it's now rebased on top of the current main, all previously existing conflicts were solved

sobychacko · 2024-02-01T23:31:01Z

@breader124 Thank you for this excellent PR. It is now merged upstream.

breader124 mentioned this pull request Dec 10, 2023

Non-Blocking Retries: Consider Making it Easier to Route to Different DLTs by Exception Type #2800

Closed

sobychacko self-assigned this Dec 12, 2023

artembilan requested changes Dec 13, 2023

View reviewed changes

sobychacko modified the milestone: 3.2.0-M1 Dec 13, 2023

sobychacko reviewed Dec 13, 2023

View reviewed changes

artembilan requested changes Jan 18, 2024

View reviewed changes

breader124 force-pushed the GH-2800 branch from 19cd845 to f76b2a6 Compare January 19, 2024 17:25

breader124 force-pushed the GH-2800 branch 4 times, most recently from 1777968 to c5dd055 Compare January 25, 2024 17:27

artembilan requested changes Jan 26, 2024

View reviewed changes

breader124 added 3 commits February 1, 2024 23:32

spring-projectsGH-2800: create topics used for exception based DLT ro…

a37a85c

…uting

spring-projectsGH-2800: route messages to specialized dlt based on ex…

0680fc6

…ception type

spring-projectsGH-2800: wire configuration from annotation and traver…

9cad95c

…se causes Closes spring-projects#2800

breader124 and others added 6 commits February 1, 2024 23:34

spring-projectsGH-2800: prepare custom DLT routing changes to be rele…

bba2ebb

…ased in version 3.2.0 Closes spring-projects#2800

spring-projectsGH-2800: rename ExceptionBasedDestinationDlt to Except…

1a0a654

…ionBasedDltDestination

spring-projectsGH-2800: rebase changes on top of the most recent main

420ec9f

spring-projectsGH-2800: amendments after the code review

bd76149

spring-projectsGH-2800: provide doc for the custom DLT routing feature

e46d481

Fixes spring-projects#2800

spring-projectsGH-2800: code review amendments

18220c5

breader124 force-pushed the GH-2800 branch from 474229f to 18220c5 Compare February 1, 2024 23:00

sobychacko merged commit c6d1d76 into spring-projects:main Feb 1, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-2800: Make it possible to route messages to custom DLT based on exceptions thrown #2929

GH-2800: Make it possible to route messages to custom DLT based on exceptions thrown #2929

breader124 commented Dec 10, 2023

sobychacko commented Dec 12, 2023

breader124 commented Dec 12, 2023 •

edited

Loading

sobychacko commented Dec 12, 2023 •

edited

Loading

breader124 commented Dec 13, 2023

artembilan left a comment

sobychacko commented Dec 13, 2023 •

edited

Loading

breader124 commented Dec 13, 2023

sobychacko Dec 13, 2023

sobychacko commented Dec 13, 2023

breader124 commented Dec 15, 2023 •

edited

Loading

artembilan commented Jan 17, 2024

artembilan left a comment

artembilan Jan 18, 2024

artembilan Jan 18, 2024

artembilan Jan 18, 2024

breader124 Jan 19, 2024

artembilan Jan 18, 2024

breader124 Jan 20, 2024

artembilan Jan 18, 2024

artembilan commented Jan 19, 2024

breader124 commented Jan 19, 2024

breader124 commented Jan 25, 2024

artembilan Jan 26, 2024

artembilan Jan 26, 2024

sobychacko commented Jan 31, 2024

breader124 commented Feb 1, 2024

breader124 commented Feb 1, 2024

sobychacko commented Feb 1, 2024

GH-2800: Make it possible to route messages to custom DLT based on exceptions thrown #2929

GH-2800: Make it possible to route messages to custom DLT based on exceptions thrown #2929

Conversation

breader124 commented Dec 10, 2023

sobychacko commented Dec 12, 2023

breader124 commented Dec 12, 2023 • edited Loading

sobychacko commented Dec 12, 2023 • edited Loading

breader124 commented Dec 13, 2023

artembilan left a comment

Choose a reason for hiding this comment

sobychacko commented Dec 13, 2023 • edited Loading

breader124 commented Dec 13, 2023

Choose a reason for hiding this comment

sobychacko commented Dec 13, 2023

breader124 commented Dec 15, 2023 • edited Loading

artembilan commented Jan 17, 2024

artembilan 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

Choose a reason for hiding this comment

artembilan commented Jan 19, 2024

breader124 commented Jan 19, 2024

breader124 commented Jan 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobychacko commented Jan 31, 2024

breader124 commented Feb 1, 2024

breader124 commented Feb 1, 2024

sobychacko commented Feb 1, 2024

breader124 commented Dec 12, 2023 •

edited

Loading

sobychacko commented Dec 12, 2023 •

edited

Loading

sobychacko commented Dec 13, 2023 •

edited

Loading

breader124 commented Dec 15, 2023 •

edited

Loading