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

tomazfernandes · 2022-03-17T18:08:25Z

Please let me know if there's a more adequate way of solving this, or if there's anything to be changed. Thanks.

Non-blocking delayed retries feature's beans were being registered lazily, when the first @KafkaListener annotated bean had a configuration associated with it in the KafkaListenerAnnotationBeanPostProcessor.

With that, if any bean autowired one of the feature's components before such bootstrapping it would throw a NoSuchBeanDefinitionException.

Now the components are registered in the RetryTopicRegistryPostProcessor before singletons are instantiated, so autowiring the feature's components in beans works as expected.

Adds a few adjustments to related classes and integration and unit tests.

garyrussell · 2022-03-17T18:35:55Z

Thanks @tomazfernandes . I don't think I can get to review this before Monday's release.

It's a pretty big change - I wonder if it needs to be back-ported or whether the work around in the discussion is sufficient for the current releases?

artembilan

I agree with, @garyrussell , that this is too much.
We may only consider this for the current 3.0 version. Back-port is too risky.

Thanks

artembilan · 2022-03-17T18:25:54Z

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

+		this.clock = Clock.systemUTC();
+	}
+
+	@SuppressWarnings("deprecated")


Why do we need this suppression here?
Doesn't look like you use some other deprecated API...

I thought my IDE was complaining, I'll remove it, thanks.

artembilan · 2022-03-17T18:26:04Z

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

+	 * @param clock the clock instance
+	 */
+	public void setClock(Clock clock) {
+		this.clock = clock;


Assert.notNull() ?

artembilan · 2022-03-17T18:28:09Z

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


+	@SuppressWarnings("deprecated")


artembilan · 2022-03-17T18:30:23Z

...va/org/springframework/kafka/annotation/RetryTopicRegistryPostProcessorIntegrationTests.java

+ */
+@SpringJUnitConfig
+@DirtiesContext
+@EmbeddedKafka


I guess we don't need these top-level annotation since all your tests are inside those nested classes.
However I wonder what is the reason of those nested classes at all...
Why those tests cannot be in top-level class (or classes) ?

I had to test whether it works with only one configuration in the registry or another, or none, so I had to have a separate context for each test.

Maybe there's a better way to solve this?

tomazfernandes · 2022-03-17T18:50:30Z

Thanks @tomazfernandes . I don't think I can get to review this before Monday's release.

No worries, I don't know why I had in mind that the release was on Tuesday.

It's a pretty big change - I wonder if it needs to be back-ported or whether the work around in the discussion is sufficient for the current releases?

I agree in terms of functionality the workaround should work. But I worry about the codebases of 3.0 and 2.x.x drifting apart and making it harder to add new features and fixes to 2.x.x. WDYT?

Maybe I can break this in two PRs, one with the functionality changes per se for 3.0, and another with some constructor changes other small changes I had to make to be back ported to 2.x.x. to minimize this. Even so, it might be confusing.

About the functionality itself, I did create enough unit and integration tests to be confident it'll work properly. The only thing I think I might be missing is if there's any difference in behavior between the testing runtime and the actual runtime that might make any of the scenarios not work.

tomazfernandes · 2022-03-17T19:13:17Z

What if we add this to 2.9.0-M1 and release it shortly, leaving the Single Container feature for a 2.9.0-M2 release?

garyrussell · 2022-03-17T19:18:54Z

I am ok with releasing a 2.9, but would prefer if we can sync it with Boot 2.7.

I don't think we can do M1 on Monday for Boot's M3; the next window is Boot's RC1 in a month.

That means we would either have to go straight to RC1 on April 18, or we could do an interim M1 when this feature is ready.

I am not sure how receptive the Boot team will be to get an RC out of the blue so 2.9 may end up being a Bootless version that users have to manually override.

tomazfernandes · 2022-03-17T19:48:15Z

I see, I hadn't noticed the Mx versions are also synced to Boot. Do you think having a bootless version would be a good solution?

If you're not worried with the codebases drifting apart, we can merge this only to 3.0.0, or break the PR as I mentioned.

I was planning on adding the DestinationTopicResolver solution as a feature in the docs with some API polishing, but I think it might be messy if it might depend on a workaround, so maybe it'd also be a 3.0.0 or 2.9.x exclusive feature, or at least hold it until this fix is released in those versions.

We might also hold this PR and consider merging it in a later moment if that's better, no worries, I'm ok with whatever works for you.

Thanks

garyrussell · 2022-03-17T19:56:19Z

After further thought; I think a stand-alone 2.9 (if we decide to do one) would be preferable, rather than forcing Boot 2.7 users to go to 2.9, given that it will get little milestone exposure before an RC will be due.

You can see the schedule on the calendar; we generally release Monday before Boot's Thursday releases.

https://calendar.spring.io/

A decoupled 2.9 would give us a bit more flexibility to add more feature content, and we could release it early Summer instead of late Spring.

tomazfernandes · 2022-03-17T20:20:48Z

After further thought; I think a stand-alone 2.9 (if we decide to do one) would be preferable, rather than forcing Boot 2.7 users to go to 2.9, given that it will get little milestone exposure before an RC will be due.

Sure, we can do that if that's better.

You can see the schedule on the calendar; we generally release Monday before Boot's Thursday releases.

Then I probably mixed up with Boot's 😄

https://calendar.spring.io/

That's useful, thanks.

A decoupled 2.9 would give us a bit more flexibility to add more feature content, and we could release it early Summer instead of late Spring.

Ok, let's go this way then. I still have a couple of things to submit for this PR, I should commit it later today or tomorrow.

Thanks for looking into this.

tomazfernandes · 2022-03-21T22:08:22Z

I've made a few adjustments and addressed the review comments.

I'm still not sure I've made the best choices for this PR - for example, I might have left the current bootstrapping as a fallback to the new one - it might help reducing risk and making the PR smaller. But it still wouldn't be risk free, so I'm not sure if it'd be any better considering we'd have more logic to deal with in the end.

Please let me know if you have any suggestions or any further concerns.

Thanks.

Resolves spring-projects#2174 Retry Topic feature's beans were being registered lazily, when the first @KafkaListener annotated bean had a configuration associated with it in the KafkaListenerAnnotationBeanPostProcessor. With that, if any bean autowired one of the feature's components before such bootstrapping it would throw a NoSuchBeanDefinitionException. Now the components are registered in the RetryTopicRegistryPostProcessor before singletons are instantiated, so autowiring of the feature's components in beans works as expected. Adds a few adjustments to related classes and integration and unit tests.

tomazfernandes · 2022-03-31T14:14:40Z

I've made a few adjustments to this.

Reestablished the legacy bootstrapping as a fallback
Rolled back non-essential changes to existing components
Added an IT to assure that users can disable the new bootstrapping method by adding a no-ops RetryTopicRegistryPostProcessor subclass bean
Added a WARN log to let users know if early bootstrapping didn't work and the fallback kicked in - likely if that happens a lot someone will reach out and let us know

I think with this changes this PR will be less risky and easier to review.

Please let me know if you have any further concerns.

Thanks.

Allow disabling new bootstrapping method Warn if early bootstrapping is not successful

tomazfernandes · 2022-03-31T14:57:05Z

Just removed an unnecessary annotation in the last couple force-pushes. Thanks.

tomazfernandes · 2022-04-01T13:14:05Z

See: https://stackoverflow.com/questions/71705876/combining-blocking-and-non-blocking-retries-in-spring-kafka/71706953#71706953

tomazfernandes · 2022-04-01T13:35:23Z

I've deleted my last comment, it was actually a configuration error on my part, sorry about that 😄

artembilan · 2022-04-01T16:45:28Z

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

@@ -524,6 +524,7 @@ private void bootstrapRetryTopicIfNecessary() {
 		BeanDefinitionRegistry registry = (BeanDefinitionRegistry) this.beanFactory;
 		if (!registry.containsBeanDefinition(RetryTopicInternalBeanNames
 				.RETRY_TOPIC_BOOTSTRAPPER)) {
+			this.logger.warn(() -> "Could not bootstrap RetryTopic early. Bootstrapping lazily.");


Why do we need this warn?
Looks like this one is going to happen in 99% application in the field.

What is wrong with just silently registering that bean if it does not present yet?
We do something like this conditional registration all the time and no warnings.

So, this is why I got the impression that you didn't get the context and wrote the comment below. Actually it's the opposite - I expect all of the applications to bootstrap the feature via the RegistryPostProcessor, and only if for some unexpected reason the RPP is not triggered properly this logic should be executed.

Since that's an unexpected path, I'd rather know that this is happening so we can properly fix it, and when we get enough confidence we should be able to remove this logic.

artembilan · 2022-04-01T16:47:42Z

...afka/src/main/java/org/springframework/kafka/annotation/RetryTopicRegistryPostProcessor.java

+
+	private void bootstrapRetryTopic(BeanDefinitionRegistry registry) {
+		BeanDefinition beanDef = getOrRegisterBeanDefinition(registry);
+		this.applicationContext.getBean(RETRY_TOPIC_BOOTSTRAPPER_BEAN_NAME, RetryTopicBootstrapper.class)


we cannot call getBean() in the postProcessBeanDefinitionRegistry(). This is the same "Inversion of Control impedance" I mentioned in other place.
That's too early to deal with beans.

I see. I think I can find a way around this, but it might require more changes than I'd like to.

I mean changes to existing components such as the RetryTopicBootstrapper.

artembilan · 2022-04-01T16:48:43Z

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

+	@Override
+	public void setApplicationContext(ApplicationContext applicationContext) {
+		this.applicationContext = applicationContext;
+		this.clock = applicationContext.getBean(RetryTopicInternalBeanNames.INTERNAL_BACKOFF_CLOCK_BEAN_NAME, Clock.class);


You cannot call getBean() from the setApplicationContext() - too early, may cause some bean initialization problem...

artembilan · 2022-04-01T16:50:57Z

...afka/src/main/java/org/springframework/kafka/annotation/RetryTopicRegistryPostProcessor.java

+	private void bootstrapRetryTopic(BeanDefinitionRegistry registry) {
+		BeanDefinition beanDef = getOrRegisterBeanDefinition(registry);
+		this.applicationContext.getBean(RETRY_TOPIC_BOOTSTRAPPER_BEAN_NAME, RetryTopicBootstrapper.class)
+				.bootstrapRetryTopic();


This better to be done via SmartInitializingSingleton impl on that RetryTopicBootstrapper. So, we don't need to have this getBean() over here and no early BeanFactory access.
Said that it feels like we don't need this RetryTopicRegistryPostProcessor at all...

Again, I think it's about the premise. If we want to keep the component's bootstrapping optional while also enabling their autowiring to the end user, I think the SmartInitializingSingleton hook is too late for that.

Do we really need to have them to be "autowireable" in the end?
Are they not infrastructure beans to support something else which is already those @RetryableTopic or so?

OK. All sounds good.

Here is my take on this: let's make those beans registered unconditionally if there is a requirements to be able to inject them somewhere in the end-user code. Even if these beans are going to be dangling for nothing, doesn't look like they are heavy, active or store some context to exhaust memory or CPU.

We may revise a conditional feature in the future.

Now that I think of it, we have two kinds of components - infrastructure components used at assembly time, and runtime components that are used at, well, runtime.

I think the runtime components are the ones users might be interested in. For example in #2172, the DestinationTopicContainer is injected for the user to figure out what is the first retry-topic name for a given main topic and reprocess DLT messages.

I think it might also be useful for users to have access to the KafkaConsumerBackOffManager so they can know which topic partitions are currently backed off and their due time, for example. But that one might be trickier because it has a more complex instantiation, but that's something fixable, and probably we won't need it now.

So maybe we can register early only DestinationTopicContainer and maybe KafkaBackOffManager and let the infrastructure components be instantiated where they currently are, when KLABPP detects the feature.

How does this sound? Would KafkaBootstrapConfiguration be the right place for that?

Your scan for @RetryableTopic doesn't look bad. Only the problem here that we need opposite injection from these infrastructure beans into end-user stuff. This is kinda anti-pattern and hard to implement and support eventually.

I'm not sure I understand what opposite injection is. You mean restricting users' access to the infrastrucure beans via injection?

Yes. Sounds like a plan.

I mean that you are going to register a bean dynamically, but at the same time it is asked for end-user autowiring.
That's my concern where we may cause circular dependency.
That's where it might not be conditional. At least for now.

Yes. Sounds like a plan.

Great, I'll do that then. I'll also take a closer look if there are any other beans that might be interesting for users.

I mean that you are going to register a bean dynamically, but at the same time it is asked for end-user autowiring. That's my concern where we may cause circular dependency. That's where it might not be conditional. At least for now.

Oh, I see, thanks for the explanation.

Thanks a lot for looking into this!

tomazfernandes · 2022-04-01T18:18:57Z

Hi @artembilan, thanks again for looking into this.

Before I try to address your specific concerns, let me give you some more context - hopefully not as much as in the other PR 😄 - on why this came up. Or maybe you got the context and I didn't get your solution, but at least we can be sure we're in the same page.

Currently, at application bootstrapping, no RetryTopic components' beans are automatically registered, such as in the KafkaBootstrapConfiguration class, or boot's auto configuration. I did it that way so that, if the feature is not being used, the components are simply not registered. You might have noticed this is a rather bean-heavy feature.

So these are registered by the RetryTopicBootstrapper only if KafkaListenerAnnotationBeanPostProcessor process a bean that has and topic associated with a RetryTopicConfiguration or @RetryableTopic annotation.

The problem with this is that when users try to autowire any of the components, such as the DestinationTopicContainer as requested in #2172, depending on the order things happen, the component is not registered yet and we get a NoSuchBeanDefinitionException.

So, maybe my premise is wrong, and we should just register the BeanDefinitions whether or not the feature is being used - that would certainly be an easier path.

If we stick with the current premise, though, we need an earlier point in application bootstrapping to make this check and register the components. This is how I got to this solution using this RegistryPostProcessor, since it's executed before singletons are instantiated and its dependencies autowired.

Makes sense overall? I'll look into the specific concerns now.

Thanks

artembilan · 2022-04-01T18:44:43Z

OK. All sounds good.

Here is my take on this: let's make those beans registered unconditionally if there is a requirements to be able to inject them somewhere in the end-user code.
Even if these beans are going to be dangling for nothing, doesn't look like they are heavy, active or store some context to exhaust memory or CPU.

We may revise a conditional feature in the future.

Your scan for @RetryableTopic doesn't look bad. Only the problem here that we need opposite injection from these infrastructure beans into end-user stuff.
This is kinda anti-pattern and hard to implement and support eventually.

garyrussell · 2022-04-05T16:11:51Z

A decoupled 2.9 would give us a bit more flexibility to add more feature content,

I had forgotten about this discussion; I will change the milestone dates.

tomazfernandes · 2022-04-05T16:22:04Z

I'm wondering, since we're adding quite a few improvements that we both want to reduce risk, but also get it in the hands of the users quickly since these are actual issues they're facing, how about we schedule a 2.10.0 after 2.9.0?

This way we could release 2.9.0 early, after the milestone exposure we think it's necessary, but don't have to wait until all features are done, or postpone everything else to 3.0.0.

garyrussell · 2022-04-05T16:28:23Z

I'd rather not have 5 active branches; although 2.7.x officially goes out of OSS support in May, realistically, we will probably have a release or two after that.

I do not anticipate much of a delay for 2.9; we can release it in June; I just don't want to cram things into a new release in a couple of weeks by tying it to the Boot release train.

tomazfernandes · 2022-04-05T17:23:05Z

I see, makes sense. I think June is a good timeline. After that, we can work with the 2.9.x versions for small changes and leave bigger ones to 3.0.0.

artembilan

I think we need to revise this after we done with the #2204 .

Perhaps all of these "earlier" concerns will go away by themself when we have one single central configuration for infrastructure beans.

tomazfernandes · 2022-04-23T20:02:30Z

Closed in favor of #2227

tomazfernandes force-pushed the GH-2174 branch from 464f750 to c916663 Compare March 17, 2022 18:18

artembilan requested changes Mar 17, 2022

View reviewed changes

tomazfernandes force-pushed the GH-2174 branch from 7ca82bf to 1d6892a Compare March 21, 2022 21:35

tomazfernandes added 3 commits March 31, 2022 10:36

Accept user-provided RetryTopicBootstrapper bean

93daee7

Fix test deprecation warning

699414c

tomazfernandes force-pushed the GH-2174 branch from 9ae0f18 to 227c6c2 Compare March 31, 2022 14:07

tomazfernandes force-pushed the GH-2174 branch from 227c6c2 to 7397847 Compare March 31, 2022 14:49

Reestablish legacy bootstrapping as a fallback

d7865da

Allow disabling new bootstrapping method Warn if early bootstrapping is not successful

tomazfernandes force-pushed the GH-2174 branch from 7397847 to d7865da Compare March 31, 2022 14:53

artembilan requested changes Apr 1, 2022

View reviewed changes

artembilan requested changes Apr 6, 2022

View reviewed changes

tomazfernandes closed this Apr 23, 2022

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

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

Conversation

tomazfernandes commented Mar 17, 2022

garyrussell commented Mar 17, 2022

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

tomazfernandes commented Mar 17, 2022 • edited

tomazfernandes commented Mar 17, 2022

garyrussell commented Mar 17, 2022

tomazfernandes commented Mar 17, 2022

garyrussell commented Mar 17, 2022

tomazfernandes commented Mar 17, 2022

tomazfernandes commented Mar 21, 2022

tomazfernandes commented Mar 31, 2022

tomazfernandes commented Mar 31, 2022

tomazfernandes commented Apr 1, 2022

tomazfernandes commented Apr 1, 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

Choose a reason for hiding this comment

tomazfernandes commented Apr 1, 2022

artembilan commented Apr 1, 2022

garyrussell commented Apr 5, 2022

tomazfernandes commented Apr 5, 2022

garyrussell commented Apr 5, 2022

tomazfernandes commented Apr 5, 2022

artembilan left a comment

Choose a reason for hiding this comment

tomazfernandes commented Apr 23, 2022

tomazfernandes commented Mar 17, 2022 •

edited