GH-2226: Add RetryTopicConfigurationSupport #2227

tomazfernandes · 2022-04-11T00:36:32Z

Resolves #2226
Also resolves #2174

This is a first draft of a solution with a RetryTopicConfigurationSupport, so we can evaluate if it's going in the right direction before doing a final review, adding docs, etc.

Overall, I liked it better than I thought I would - the bean count has been reduced to 3 or4 beans, and it gave a lot of clarity on the feature's design. It should make it easier for everyone to understand how the feature works. So thanks a lot @artembilan for insisting on this.

To be honest, I still think this might be a bit overwhelming for the average end-user to configure simple features, but if that's a real problem we can look into providing an easier alternative for some features in the future. It's definitely much better than what we have now, and a solid foundation to build upon.

There are a few decisions I made that are not exactly WebMvcSupport-like - I'm ok with rolling any or all of them back if that's better. For example, I:

Created a RetryTopicComponentsFactory to reduce the number of methods to be overridden in the Support class and keep it overall smaller;
Separated the KafkaBackOffManagerConfigurationSupport because the KafkaBackOffManager is useful outside of Retry Topic's scope;
Created the @EnableRetryTopic annotation - while we're using this as an optional add-on, I think it's easier to document adding the annotation than the @Import alternative - afterwards we can move this annotation to @EnableKafka, or maybe kill it in 3.0.0. It also imports the KafkaBackOffManagerConfigurationSupport class;
Created a new RetryTopicBeanNames class and deprecated the RetryTopicInternalBeanNames class - since we're using a lot less beans, there's no need for so many names. Also this enables setting the names to the framework standards. For now the old names should still work - I need to do a more thorough review to ensure that. If that seems to risky, I'm ok with keeping the old names until 3.0.
There are a few constructors that might be improved - for example adding ApplicationContextAware to some beans instead of injecting an ApplicationContext through the constructor. But I think we can review this in the future.

Also, after more consideration, although I don't see any real risk since we're not actually changing any legacy logic besides some optional bean names which we can rollback, I think maybe it's indeed better for us to hold this for 2.9 - time is flying by and we should release it soon enough. Of course, I'm also ok with releasing it as 2.8.x if that's better.

I used this new configuration support for the RetryTopicIntegrationTests and RetryTopicExceptionRoutingIntegrationTests, so these are good places to take a look into how it's like. I also made sure the former works with legacy configuration.

Let me know what you think about this - hopefully this will be a solid step in the right direction for the RT feature.

Thanks!

artembilan

At a glance this is great!
👍

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

.../src/main/java/org/springframework/kafka/config/KafkaBackOffManagerConfigurationSupport.java

...main/java/org/springframework/kafka/annotation/KafkaListenerAnnotationBeanPostProcessor.java

spring-kafka/src/main/java/org/springframework/kafka/config/RetryTopicConfigurationSupport.java

...a/src/main/java/org/springframework/kafka/retrytopic/ListenerContainerFactoryConfigurer.java

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

tomazfernandes · 2022-04-12T15:17:10Z

I've made the suggested changes - please let me know if there's anything else. If code-wise this looks ok, I can start working on tests and reviewing the javadocs.

If we want to make @EnableRetryTopic provide a default configuration, I think we can address that in a separate PR.

I'm working under the assumption that we'll be releasing this in 2.9 - if that's not the case please let me know so I can update the since tags.

Thanks.

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

tomazfernandes · 2022-04-13T16:31:12Z

I was not too happy with the solution for the KafkaBackOffManager part - it might be confusing for users that RetryTopicConfigurationSupport didn't have everything needed for the feature, and also we already have a KafkaBackOffManagerFactory to handle that complexity. So if users want to customize the instance, now they can just override the public method and use the factory for that end.

@artembilan, if you have the time to take a look at these changes, I'd appreciate your input.

Thanks.

artembilan

Some further review.

Thanks

spring-kafka/src/main/java/org/springframework/kafka/config/RetryTopicComponentFactory.java

.../src/main/java/org/springframework/kafka/listener/PartitionPausingBackOffManagerFactory.java

spring-kafka/src/main/java/org/springframework/kafka/config/RetryTopicConfigurationSupport.java

garyrussell · 2022-04-13T17:49:12Z

I am inclined to push out the 2.9 M1 for up to a week, since we are still in review mode here.

WDYT?

tomazfernandes · 2022-04-13T18:06:31Z

I am inclined to push out the 2.9 M1 for up to a week, since we are still in review mode here.

WDYT?

TBH, I wasn't aware we had a release date for M1 😄 Yeah, I think the 25th should be doable. Anything happens, I'll let you know and we can either push the date a bit further or leave this for M2, if that works for you.

garyrussell · 2022-04-13T18:09:41Z

I scheduled it for the 18th because it's easier for me to do all the releases the same day.

Moving to the 25th (which doesn't have to be firm).

AFAICT, the only one we need for Monday is #2223

tomazfernandes · 2022-04-13T18:25:32Z

I scheduled it for the 18th because it's easier for me to do all the releases the same day.

Moving to the 25th (which doesn't have to be firm).

Sure, sounds good, thanks.

AFAICT, the only one we need for Monday is #2223

Yes, I'm working on that right now. I noticed since the EndpointCustomizerFactory was extracted from the RetryTopicConfigurer class at some point in time, it doesn't have such great unit test coverage - actually the coverage it had from the RetryTopicConfigurerTest was removed, probably at the same time, so I'm working on that.

Also, I couldn't really figure out an worthwhile way to adjust the integration tests so that they'd fail with the current logic - I think I'd need a way of setting the partition's offset to 1 before the container is created, and make sure that when the container starts it resets back to 0. If you have any pointers on that please let me know, probably on that PR so we don't hijack this one.

Thanks

Resolves spring-projects#2226 Add RetryTopicConfigurationSupport Add @EnableRetryTopic Add KafkaBackOffManagerConfigurationSupport

* Change deprecated constants for strings * Remove deprecation suppressions * Remove ApplicationContext and BeanFactory from constructors * Change TaskExecutor shutdown logic to DisposableBean

tomazfernandes · 2022-04-15T00:38:52Z

Added unit tests, if you care to take a look. It gives a nice perspective on how configurations can be achieved.

Also, happy to say that on aggregate (UT + IT), the new classes have 100% test coverage, and the retrytopic package has 94% test coverage on class and lines. I know that test coverage doesn't mean it's infallible, but that's certainly a nice safety net to have.

Thanks a lot for the support and guidance from you both! Next up, docs and javadocs.

tomazfernandes · 2022-04-15T00:42:13Z

As a note, we're already using this new Support approach in two main integration test suites, that's why I'm not adding any new ones.

...afka/src/main/java/org/springframework/kafka/listener/WakingKafkaConsumerTimingAdjuster.java

...g-kafka/src/main/java/org/springframework/kafka/listener/PartitionPausingBackoffManager.java

Create defaultFatalExceptionsList method in `ExceptionClassifier` Change RetryTopicConfigurationSupport logic to provide a list instead of a configurer for non-blocking retries

Add KafkaBackOffManagerConfigurer to RetryTopicConfigurationSupport

tomazfernandes · 2022-04-16T18:58:16Z

Considering what was discussed with @artembilan, I've changed the TaskExecutor to a @Bean - this way Spring manages its lifecycle so we don't need to. This is a minor breaking change - users of the PartitionPausingBackOffManagerFactory class might expect a ThreadPoolTaskExecutor to be created there. I've documented that in the class' javadocs. But we'd have a breaking change either way by moving the TaskExecutor creation and management to the WakingKafkaConsumerTimingAdjuster, and those seems like niche use cases, so it seems it's a lesser problem - please let me know if otherwise.

I also created a KafkaBackOffManagerConfigurer to let users easily manage this. If timingAdjustment is disabled by the user, the ThreadPoolTaskExecutor is not created.

@garyrussell, I've also added a defaultFatalExceptionsList() method to the ExceptionClassifier that returns a new list with the default classifications. With that, when configuring the exceptions for the RT feature, we can provide a populated list for the user to manage, and then set the classifications back to the classifier. Please let me know if that looks ok - I can work around that if you think that's an unwelcome change or have a better idea.

Thanks!

tomazfernandes · 2022-04-19T00:05:07Z

I've added documentation, changed the code examples to the new approach and reviewed the javadocs. Of course, that's a whole lot of text 😄 - there are probably some mistakes around we can fix as we go through the milestone cycle. But I think overall we can see in the code example diffs the improvements achieved with this approach.

As far as I'm concerned this is good to go, so I'm removing the Draft status - it should be a matter of addressing any further concerns you might have.

I think we look good for releasing this next week, so thank you very much for the support and guidance so far.

As usual, let me know if there's anything else to be changed, or any further concerns.

Thanks!

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

spring-kafka/src/main/java/org/springframework/kafka/config/RetryTopicConfigurationSupport.java

tomazfernandes · 2022-04-19T20:27:50Z

This is a big step forward; thanks! Just some minor formatting nits and one comment.

I appreciate that @garyrussell, thanks. Merit goes to @artembilan for insisting on this approach, and then seeing it through - turned out a lot better than I expected! Also, thanks for your patience, some of this work is sometimes still a bit overwhelming for me. I appreciate the guidance and support from you both.

I've addressed the review comments - please let me know if there's anything else.

Thanks!

artembilan · 2022-04-20T16:29:03Z

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

@@ -87,5 +82,6 @@
 @Target(ElementType.TYPE)
 @Documented
 @Import(RetryTopicConfigurationSupport.class)
-public @interface EnableRetryTopic {
+@EnableKafka


I don't think this has to be there. Or at least it has to be documented properly:
"For convenience the @EnableKafkaRetryTopic is marked also with the @EnableKafka since former cannot work without secondary"
(Or some other proper English language 😄 )

Sure. Now I'm thinking that since Boot's auto configuration already provides an @EnableKafka by default, maybe we'd run into the same issue of duplicated beans if users used this @EnableKafkaRetryTopic annotation?

I think that, considering this is a temporary solution until we add this as default in 3.0, maybe it'd be simpler to stick with the @EnableRetryTopic annotation, and then either move it to @EnableKafka or simply kill it and import the Support class directly for 3.0. WDYT?

No, there is no harm: the @EnableKafka does a decent work to not duplicate beans. See its target KafkaBootstrapConfiguration.
I'm not sure that retry topics should be a default feature.
Therefore keep that @EnableKafkaRetryTopic is the way to go from now on.

Well, if we're introducing a new annotation then, and probably 99% of the users will be using it in a Spring Boot application anyway, I'd stick with the simpler @EnableRetryTopic annotation that should be enough for them, and just add a note in the documentation that if users are not in a Spring Boot app they should also add @EnableKafka.

But no worries, I'll keep it as is and we can revise this down the road if needed.

Thanks.

Just as a thought, of course we can look into this later, but what if for 3.0 we added this class to @EnableKafka, but also added a custom @Condition that would perform that annotation / bean lookup we had in the other PR, to only add this beans it if there's at least one RetryTopicConfiguration or @RetryableTopic present?

This way we'd keep the automatic feature enabling users currently have, while not encumbering other users with this if they don't use it.

Thanks.

Well, that's not how other @Enable... features work. So, my vote for "No": if you are going to use @RetryableTopic, then add respective @Enable....
The condition could be considered in Spring Boot which indeed has some opinion.
The framework by itself has to be as straightforward as possible.
In the end it is just a library with minimal opinion 😉

Sure, makes sense. As you said in another post, eventually it'll be a good thing to better integrate this with Spring Boot's auto configuration in the future. But since this approach won't be available for Spring Boot prior to 3.0, we don't really need to worry about it now.

Thanks.

I still want to see some mentioning of this meta-annotation in the Javadocs of this EnableKafkaRetryTopic class.

artembilan · 2022-04-20T16:37:21Z

.../src/main/java/org/springframework/kafka/listener/PartitionPausingBackOffManagerFactory.java

+		ThreadPoolTaskExecutor taskExecutor = new ThreadPoolTaskExecutor();
+		JavaUtils.INSTANCE.acceptIfNotNull(this.maxThreadPoolSize, taskExecutor::setMaxPoolSize);
+		taskExecutor.initialize();
+		super.addApplicationListener((ApplicationListener<ContextClosedEvent>) event -> taskExecutor.shutdown());


I still in doubts for all of this stuff around internal executor...
Can you, please, bring the bean back and let's revise it in a separate issue?

Sure, no worries, I'll bring it back that and open the issue. Thanks.

I've moved it back - how grateful I am for git in these moments 😄

Also opened the issue: #2239

Please let me know if there's anything else.

Thanks.

tomazfernandes · 2022-04-22T16:45:42Z

@artembilan, just a friendly reminder, if nothing has changed, this is supposed to be on Monday's 2.9.0-M1 release, so I wonder if there's anything else to change here or we're good.

Of course, if you're not available to look at this soon enough, or if there's anything else, no worries.

The two points I'm not 100% confident about this are:

The annotation name - I still think @EnableRetryTopic would be cleaner
An IMPORTANT: I added to PartitionPausingBackOffManagerFactory's javadoc regarding the TaskExecutor breaking change, since it does not provide one by default anymore. Not sure if it's appropriate to have IMPORTANT on a javadoc.

I'm ok with leaving both as is, if you're ok with that. Please let me know if there's anything to be changed.

Thanks.

artembilan · 2022-04-23T01:56:04Z

It is holiday season here for us, but I definitely will look shortly again on Monday if Gary insists on that release.

Nevertheless I agree with Gary that annotation name has to be @EnableKafkaRetryTopic.

tomazfernandes · 2022-04-23T02:41:01Z

It is holiday season here for us, but I definitely will look shortly again on Monday if Gary insists on that release.

Nevertheless I agree with Gary that annotation name has to be @EnableKafkaRetryTopic.

Sure, sorry, I didn't know you were on holidays. Also, after some more consideration, I think I'd be ok with this being merged to 3.0 instead - if you think this is good enough - and we let go of 2.9.

I'll much unfortunately need to change some work arrangements around here and I don't think I'll be able to keep up the pace of involvement with the project I've had for the past few months, so maybe it'd be better to have a longer cycle for this - at least as far as I'm concerned. Of course, that's @garyrussell's call.

It's been a blast though, and thank you both so very much for everything! Of course, feel free to ping me if anything comes up that I can help with - I should be able to at least provide some input.

Thanks again, and happy holidays!

garyrussell · 2022-04-23T13:44:57Z

We can be flexible when we release M1 since nothing depends on it; Monday is just a placeholder.

tomazfernandes · 2022-04-23T17:00:21Z

FYI: https://stackoverflow.com/questions/71977113/when-retryabletopic-annotation-is-used-serializationexception-exception-is-not/71981820#71981820

tomazfernandes · 2022-04-23T19:50:31Z

We can be flexible when we release M1 since nothing depends on it; Monday is just a placeholder.

Sure. As I mentioned though, I'll really need to shift my focus elsewhere beginning next week - I was counting on friday's deadline for this. It's been more than I month since I've started looking into these configuration issues, so I hope you consider that enough effort on my part. If there's anything else to be done, it would be great if you could just add a polish commit to the PR like our friends from Boot do - hope that's ok with you.

TBH, I still think we could have come with some simple fix we could merge to 2.8.x, so we could help out users such as this one on today's SO and everyone else on Boot's train, and left this larger change for closer to 3.0 so we could do any breaking changes we wanted to. Anyway, I've learned a lot from it, and I think it'll make it easier for you and everyone else to reason about the feature.

As I said before, feel free to ping me if there's anything I can help with - I should be able to at least provide some input on retryable topic issues - and I will try to help out all around if and when possible.

Thanks a lot for everything, you're both really great software engineers and this has been both an honor and a blast!

artembilan · 2022-04-25T15:05:17Z

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

@@ -48,6 +48,11 @@ NOTE: You can have separate `ListenerContainerFactory` instances for the main an

 ==== Configuration

+Since 2.9, the `@EnableKafkaRetryTopic` annotation should be used in a `@Configuration` annotated class instead of the regular `@EnableKafka` annotation, which it contains.


That's not good say like this: brings an impression that we must not use an @EnableKafka anymore.
And you even didn't introduce what is @EnableKafkaRetryTopic. The side effect with the @EnableKafka could be explained in the end of this paragraph. Probably in the simple "NOTE: When @EnableKafkaRetryTopic is used, the @EnableKafka can be omitted: the former is meta-annotated with the second."

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

artembilan · 2022-04-25T15:11:12Z

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

@@ -87,5 +82,6 @@
 @Target(ElementType.TYPE)
 @Documented
 @Import(RetryTopicConfigurationSupport.class)
-public @interface EnableRetryTopic {
+@EnableKafka


I still want to see some mentioning of this meta-annotation in the Javadocs of this EnableKafkaRetryTopic class.

artembilan · 2022-04-25T15:12:45Z

spring-kafka/src/main/java/org/springframework/kafka/config/RetryTopicComponentFactory.java

+	 * @return the task executor.
+	 */
+	public TaskExecutor taskExecutor() {
+		return new ThreadPoolTaskExecutor();


Don't we provide a bean now?
What is the point of this factory method if end-user will have to just provide a respective bean to override a default one?

artembilan · 2022-04-25T15:14:30Z

spring-kafka/src/main/java/org/springframework/kafka/config/RetryTopicConfigurationSupport.java

+ * is to extend directly from this class and override methods as necessary, remembering
+ * to add {@link Configuration @Configuration} to the subclass and {@link Bean @Bean}
+ * to overridden {@link Bean @Bean} methods. For more details see the javadoc of
+ * {@link EnableKafkaRetryTopic @EnableRetryTopic}.


Some typo. Or missed renaming...

artembilan · 2022-04-25T15:17:26Z

spring-kafka/src/main/java/org/springframework/kafka/config/RetryTopicConfigurationSupport.java

+		return configurer.timingAdjustmentEnabled
+				? this.componentFactory.taskExecutor()
+				: task -> {
+				};


You can return null here instead if we just don't need such a bean.

We can't do that because it is a required dependency in the kafkaConsumerBackoffManager bean.

artembilan · 2022-04-25T15:19:31Z

spring-kafka/src/main/java/org/springframework/kafka/config/RetryTopicConfigurationSupport.java

+
+		private Consumer<CommonErrorHandler> errorHandlerCustomizer;
+		private Consumer<ConcurrentMessageListenerContainer<?, ?>> listenerContainerCustomizer;
+		private Consumer<DeadLetterPublishingRecoverer> deadLetterPublishingRecovererCustomizer;


Every member of the class has to be surrounded with blank lines (including the last one in the class).

tomazfernandes · 2022-04-25T16:43:27Z

Hi. Can anyone be so kind to explain to me again why we need the @EnableKafkaRetryTopic instead of @EnableRetryTopic, considering most users will be on Boot and will already have @EnableKafka by default? Aren't we optimizing for the few rather than for the many? I'm sure I'm missing something - would like to know what though.

Thanks.

artembilan · 2022-04-25T16:47:07Z

Because this is more Apache Kafka feature and it has nothing to do with JMS or AMQP (or even Hazelcast) topics. So, such a generic name might lead to the confusion for someone who is new to Kafka, but would rather expect a feature for his/her non-Kafka topics.

tomazfernandes · 2022-04-25T16:55:59Z

Because this is more Apache Kafka feature and it has nothing to do with JMS or AMQP (or even Hazelcast) topics. So, such a generic name might lead to the confusion for someone who is new to Kafka, but would rather expect a feature for his/her non-Kafka topics.

Sure, that sounds reasonable, thanks.

tomazfernandes · 2022-04-25T17:16:11Z

Also, still counting on your kindness, if you could bear with me this one more time, my heart is still not at ease with our plan for (not) solving the configuration problems for 2.8.x. Again, I'm sure I'm missing something and would just like to know what.

As far as I can see, in our current plan, we'll go throughout the year with users on the latest Boot (2.x), then they'll refer to the proper Spring Kafka documentation for that version (2.8.x). Then they will see an interesting retry topic feature, and will do exactly what's in the docs - and will get random, non-intelligible errors.

Then, if they don't just give up trying that, they'll look around on Stack Overflow and see either the workaround or our recommendation to manually upgrade to 2.9.

So, overall, looks to me like a very frustrating path for our users, throughout the whole year and likely for much longer since most existing applications probably won't be able to upgrade to Spring 6.0 and Java 17 so easily to get a proper functional feature by default.

If we had any plans to get back to boot's 2.x train, that would be less of a concern to me.

What am I missing from this picture, please? Is being on the latest Boot version, doing exactly what's in the docs and getting random errors acceptable for such an important Spring library, for so long?

Thanks.

garyrussell · 2022-04-25T18:01:06Z

@tomazfernandes My apologies; I have not been closely involved with this PR; I thought it was generally to make it easier to customize the configuration of the feature. Which random, non-intelligible errors do you mean?

tomazfernandes · 2022-04-25T19:17:48Z

@tomazfernandes My apologies; I have not been closely involved with this PR; I thought it was generally to make it easier to customize the configuration of the feature. Which random, non-intelligible errors do you mean?

Sure Gary, no worries.

I think the proverbial case is as in this question:
https://stackoverflow.com/questions/71705876/combining-blocking-and-non-blocking-retries-in-spring-kafka/71706953#71706953

The 2.8.x user will do exactly what's in the documentation to configure, for example, the blocking exceptions. But note that this can happen on any feature configuration where the bean to be configured depends on other feature's beans.

@Bean(name = RetryTopicInternalBeanNames.LISTENER_CONTAINER_FACTORY_CONFIGURER_NAME)
public ListenerContainerFactoryConfigurer lcfc(KafkaConsumerBackoffManager kafkaConsumerBackoffManager,
                                               DeadLetterPublishingRecovererFactory deadLetterPublishingRecovererFactory,
                                               @Qualifier(RetryTopicInternalBeanNames
                                                       .INTERNAL_BACKOFF_CLOCK_BEAN_NAME) Clock clock) {
    ListenerContainerFactoryConfigurer lcfc = new ListenerContainerFactoryConfigurer(kafkaConsumerBackoffManager, deadLetterPublishingRecovererFactory, clock);
    lcfc.setBlockingRetryableExceptions(MyBlockingRetryException.class, MyOtherBlockingRetryException.class);
    lcfc.setBlockingRetriesBackOff(new FixedBackOff(500, 5)); // Optional
    return lcfc;
}}

Depending on the particular order things happen in the user's application, this might work, or it might throw a NoSuchBeanDefinitionException due to the feature not having been bootstrapped yet. Let's say the user tries to provide these dependencies themselves - each will depend on other things. If I was the user, I'd probably not feel confident going forward with this.

Then maybe the user finds that question on SO, and sees this workaround:

@Configuration
    public static class SO71705876Configuration {

        @Bean(name = RetryTopicInternalBeanNames.LISTENER_CONTAINER_FACTORY_CONFIGURER_NAME)
        public ListenerContainerFactoryConfigurer lcfc(KafkaConsumerBackoffManager kafkaConsumerBackoffManager,
                DeadLetterPublishingRecovererFactory deadLetterPublishingRecovererFactory) {
            ListenerContainerFactoryConfigurer lcfc = new ListenerContainerFactoryConfigurer(kafkaConsumerBackoffManager, deadLetterPublishingRecovererFactory, Clock.systemUTC());
            lcfc.setBlockingRetryableExceptions(IllegalArgumentException.class, IllegalStateException.class);
            lcfc.setBlockingRetriesBackOff(new FixedBackOff(500, 5)); // Optional
            return lcfc;
        }

        @Bean(name = RetryTopicInternalBeanNames.KAFKA_CONSUMER_BACKOFF_MANAGER)
        public KafkaConsumerBackoffManager backOffManager(ApplicationContext context) {
            PartitionPausingBackOffManagerFactory managerFactory =
                    new PartitionPausingBackOffManagerFactory();
            managerFactory.setApplicationContext(context);
            return managerFactory.create();
        }

        @Bean(name = RetryTopicInternalBeanNames.DEAD_LETTER_PUBLISHING_RECOVERER_FACTORY_BEAN_NAME)
        public DeadLetterPublishingRecovererFactory dlprFactory(DestinationTopicResolver resolver) {
            return new DeadLetterPublishingRecovererFactory(resolver);
        }

        @Bean(name = RetryTopicInternalBeanNames.DESTINATION_TOPIC_CONTAINER_NAME)
        public DestinationTopicResolver destinationTopicResolver(ApplicationContext context) {
            return new DefaultDestinationTopicResolver(Clock.systemUTC(), context);
        }
}

I wouldn't be too confident about this either TBH. But then, let's say we release 2.9, so the user is asked to manually override a dependency version to use this support approach - which indeed solves the problem. I think many users wouldn't feel so confident about doing that manual override either. And by this moment, probably many of them will have given up already.

Unless I'm missing something, that's the user experience we'll be providing for the entire year, and, as I've stated, for even longer. Note that the user didn't do anything wrong - they're on the latest and greatest Spring Boot version, using the proper spring boot-defined SK dependency version, and just trying to use something exactly as it's documented.

Of course, as I mentioned, maybe I'm missing something, or exaggerating. I just really want to understand why this is not a problem we should give a better solution to.

Thanks.

garyrussell · 2022-04-25T19:52:28Z

To solve the lazy bean creation, we should be able to backport just the @EnableKafkaRetryTopics to force creation of the legacy RetryTopicBootstrapper earlier in the application lifecycle.

Am I missing something?

garyrussell · 2022-04-25T21:03:40Z

You even suggested that here.

tomazfernandes · 2022-04-26T00:02:53Z

I’m not sure I can visualize exactly what’s on your mind. From RT’s perspective this should work. My concerns would be:

Legacy bootstrapper uses a lot of factory.getBean; not sure if that’s a proper moment of the lifecycle
If the bootstrapper bean creation is lazy, it might actually bootstrap at the same time as of now, so it should probably be made eager
It’d still need to be executed before other configuration classes, so maybe it should have a higher priority order

tomazfernandes · 2022-04-26T00:13:55Z

The other options I see would be:

Adding default constructors to configurable components, making them beanFactoryAware, and injecting the dependencies manually afterSingletonsInstantiated. This way users can simply declare the bean with their proper configuration without worrying with dependencies. We can deprecate this on 2.9 and remove on 3.0.
Lightening up this PR a bit by removing new bean names and rolling back task executor related things, and merging it to 2.8.x. I think by doing this there wouldn’t be any changes to existing logic at all, except maybe for some new constructors we can also take out if needed, and since it’s opt in, there should be no risk for users

artembilan · 2022-04-26T13:46:07Z

Right. That was one my biggest concerns for the original solution: end-user beans cannot contribute to the infrastructure if they also depend on some infrastructure.
It is going to be much worse when we come to Spring Boot's auto-configuration which will report for us "circular dependency" or "no bean yet" errors.
And that's why I suggested to look into @EnableWebMvc from the beginning.
Anyway the cat is out of the bag...

I believe we stated somewhere that retriable topics is an experimental feature. So, it is expected that something is not going to work.

Therefore an afterSingletonsInstantiated() might really be a good middle ground for 2.8.x.

I still against pushing this PR back to 2.8.x...

garyrussell · 2022-04-26T14:12:18Z

I agree; that's not up for debate; I was just looking to see if there was some middle ground for 2.8.x users; if not, so be it.

tomazfernandes · 2022-04-27T17:36:04Z

Thanks a lot for your answers, I really appreciate that.

I do think that, if something can be done to reduce user frustration on the 2.8.x branch, which is Boot's official branch for the year, it's worth giving it a shot, even if only out of empathy for them.

Also, I think that this became a bigger issue mostly because I showed up out of the blue a few months ago, started pushing code, and by the time we realized we'd need a new minor version it was too late to get into Boot's train. Otherwise, we'd probably have merged this to 2.9.0-RC1, and it'd all be good. Under different circumstances we'd surely have been able to plan all this better.

Another important thing for us to figure out is whether this is a production-ready, robust and complete feature, as the experimental disclaimer states, and that deserves proper support as such, or if it's a fundamentally flawed experimental feature that the only worthy thing to do is to focus on rebuilding it for 3.0. I'm ok with both views, but being in the middle of them without proper discussion was really frustrating.

I think we're on a good track overall, we did some really nice work so far, and as we get to know each other better we should be able to reduce friction in complex issues such as this. I'll try to look into this PR sometime later this week, or early next. Or, if you prefer, you can add that polish commit to release 2.9-M1 sooner.

As I've stated, I shouldn't be able to focus as much on this project as I had been lately. But I'll try to help out as much as I can. Either way, I'm deeply grateful for all we've been able to do so far, and please let me know if there's anything else I can help with.

Thanks!

garyrussell · 2022-04-27T18:26:28Z

Thanks @tomazfernandes I will take care of @artembilan 's latest comments in a polishing commit; I plan on releasing 2.9.0-M1 tomorrow, now.

Thanks for all your hard work on this.

garyrussell · 2022-04-27T19:13:13Z

Polishing commit: garyrussell@db68ab7

garyrussell · 2022-04-27T19:32:31Z

Merged as b2beec3 and cherry-picked to 2.9.x

artembilan requested changes Apr 11, 2022

View reviewed changes

...a/src/main/java/org/springframework/kafka/retrytopic/ListenerContainerFactoryConfigurer.java Outdated Show resolved Hide resolved

spring-kafka/src/main/java/org/springframework/kafka/annotation/EnableRetryTopic.java Outdated Show resolved Hide resolved

tomazfernandes force-pushed the GH-2226 branch from e5b0cac to 20a292d Compare April 12, 2022 15:09

artembilan requested changes Apr 12, 2022

View reviewed changes

spring-kafka/src/main/java/org/springframework/kafka/retrytopic/RetryTopicConfigurer.java Show resolved Hide resolved

tomazfernandes force-pushed the GH-2226 branch from 20a292d to 9eb9ec4 Compare April 12, 2022 18:15

artembilan requested changes Apr 13, 2022

View reviewed changes

tomazfernandes force-pushed the GH-2226 branch from fde7a74 to b147075 Compare April 14, 2022 03:37

tomazfernandes added 4 commits April 14, 2022 21:24

spring-projectsGH-2226: Add RetryTopicConfigurationSupport

3da9ea9

Resolves spring-projects#2226 Add RetryTopicConfigurationSupport Add @EnableRetryTopic Add KafkaBackOffManagerConfigurationSupport

Address code review comments

06979ef

* Change deprecated constants for strings * Remove deprecation suppressions * Remove ApplicationContext and BeanFactory from constructors * Change TaskExecutor shutdown logic to DisposableBean

Remove KafkaBackOffManagerConfigurationSupport

f10f276

Move Task Executor logic to Timing Adjuster

58bfaea

tomazfernandes force-pushed the GH-2226 branch from d583db3 to 9d8273c Compare April 15, 2022 00:26

Add unit tests

c82fa57

tomazfernandes force-pushed the GH-2226 branch from 9d8273c to c82fa57 Compare April 15, 2022 00:33

artembilan requested changes Apr 15, 2022

View reviewed changes

...afka/src/main/java/org/springframework/kafka/listener/WakingKafkaConsumerTimingAdjuster.java Outdated Show resolved Hide resolved

...g-kafka/src/main/java/org/springframework/kafka/listener/PartitionPausingBackoffManager.java Outdated Show resolved Hide resolved

tomazfernandes added 2 commits April 16, 2022 00:06

Enable managing fatal exceptions with a List

39d6549

Create defaultFatalExceptionsList method in `ExceptionClassifier` Change RetryTopicConfigurationSupport logic to provide a list instead of a configurer for non-blocking retries

Make provided TaskExecutor a bean

a887fba

Add KafkaBackOffManagerConfigurer to RetryTopicConfigurationSupport

tomazfernandes force-pushed the GH-2226 branch from 20a8711 to e36fab7 Compare April 18, 2022 23:58

tomazfernandes marked this pull request as ready for review April 19, 2022 00:05

tomazfernandes force-pushed the GH-2226 branch from e36fab7 to 822dbb1 Compare April 19, 2022 00:20

artembilan requested changes Apr 19, 2022

View reviewed changes

Address PR review comments

c43f159

artembilan requested changes Apr 20, 2022

View reviewed changes

Restore ThreadPoolTaskExecutor bean

dd512b4

tomazfernandes mentioned this pull request Apr 23, 2022

GH-2174: Register Retry Topic's components earlier #2176

Closed

artembilan requested changes Apr 25, 2022

View reviewed changes

garyrussell closed this Apr 27, 2022

		@@ -48,6 +48,11 @@ NOTE: You can have separate `ListenerContainerFactory` instances for the main an

		==== Configuration

		Since 2.9, the `@EnableKafkaRetryTopic` annotation should be used in a `@Configuration` annotated class instead of the regular `@EnableKafka` annotation, which it contains.

GH-2226: Add RetryTopicConfigurationSupport #2227

GH-2226: Add RetryTopicConfigurationSupport #2227

Conversation

tomazfernandes commented Apr 11, 2022 • edited

artembilan left a comment

Choose a reason for hiding this comment

tomazfernandes commented Apr 12, 2022

tomazfernandes commented Apr 13, 2022

artembilan left a comment

Choose a reason for hiding this comment

garyrussell commented Apr 13, 2022

tomazfernandes commented Apr 13, 2022

garyrussell commented Apr 13, 2022 • edited

tomazfernandes commented Apr 13, 2022

tomazfernandes commented Apr 15, 2022

tomazfernandes commented Apr 15, 2022

tomazfernandes commented Apr 16, 2022 • edited

tomazfernandes commented Apr 19, 2022

tomazfernandes commented Apr 19, 2022

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

tomazfernandes commented Apr 22, 2022

artembilan commented Apr 23, 2022

tomazfernandes commented Apr 23, 2022

garyrussell commented Apr 23, 2022

tomazfernandes commented Apr 23, 2022

tomazfernandes commented Apr 23, 2022

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

tomazfernandes commented Apr 25, 2022

artembilan commented Apr 25, 2022

tomazfernandes commented Apr 25, 2022

tomazfernandes commented Apr 25, 2022

garyrussell commented Apr 25, 2022

tomazfernandes commented Apr 25, 2022

garyrussell commented Apr 25, 2022

garyrussell commented Apr 25, 2022

tomazfernandes commented Apr 26, 2022

tomazfernandes commented Apr 26, 2022

artembilan commented Apr 26, 2022

garyrussell commented Apr 26, 2022

tomazfernandes commented Apr 27, 2022

garyrussell commented Apr 27, 2022

garyrussell commented Apr 27, 2022

garyrussell commented Apr 27, 2022

tomazfernandes commented Apr 11, 2022 •

edited

garyrussell commented Apr 13, 2022 •

edited

tomazfernandes commented Apr 16, 2022 •

edited