Add auto-configuration for Kafka Retry Topics

[tomazfernandes]

Closes #28450

This PR adds basic auto-configuration capabilities for Spring Kafka's non-blocking delayed retries feature (https://docs.spring.io/spring-kafka/reference/html/#retry-topic).

I chose not to add exception type classification to it just now as was previously suggested by @garyrussell so I can get feedback on the basic solution first. I can add exception type classification in a separate PR if and when this one is approved.

I wasn't sure whether to add an entry to the Messaging section of the documentation, since it seems to cover more basic use cases. I can do so if that's the case.

Seems like we should add the new properties to the A.8. Integration Properties section of the documentation, but I couldn't really find it in the source code. I can add it if you point me to it.

Please feel free to request any changes you see fit, or to make a polish commit on top of it.

As always, If @garyrussell and @artembilan can take a look into this it'd be great.

Thanks

[snicoll]

Thanks for the PR. I've made some suggestions for your consideration.

[artembilan]

add the new properties to the A.8. Integration Properties section of the documentation

That part of the doc is auto-generated by source code. So, whatever you have added to KafkaProperties is going to be reflected over there as well.

[tomazfernandes]

Thank you very much for the reviews. I've made the changes as suggested.

I removed the BackOff inner class since it'd have the same problem of creating a group with a dash in the name. Also, most of the BackOff creation logic and JavaDocs are from the Spring Retry project, not sure if we should add proper credit there somehow.

Let me know if there are any other suggestions or anything else to change.

Thanks.

[artembilan]

I don't think it is OK to say this: the maximum what end-user can provide are exactly those props you use for BackOffPolicyBuilder.
So, we might need to ensure that only SleepingBackOffPolicy is created. Perhaps just ignore some props which are not sufficient...

[tomazfernandes]

Thanks for looking into this @artembilan. Currently, the BackOffPolicyBuilder only returns instances of SleepingBackOffPolicy, but I didn't want to add this constraint to the builder's method signature.

So this assertion here is just for the case that, if we in the future update the Builder to for example return a NoBackOffPolicy if delay is set to 0, we don't get a class cast exception, but get this error instead.

But I agree that this is not a really useful message to the user. Maybe we should just blindly cast? Not sure what a useful message for the users should be in this case, since it'd not really be their fault.

[artembilan]

If we don't support a delay == 0, then it has to be rejected as a configuration properties validation, before calling this builder.
What I mean, if we don't support anything else in the builder, then we need to ensure the props provided for it are in the valid range.
The message you say now is fully misleading: end-user does not create instances of this, so the message (if any), must really point what is wrong with end-user config.

[tomazfernandes]

Well, currently if the provided delay < 1 it is set to 1 in the FixedBackOffPolicy, and most or all other BackOffPolicy implementations follow that pattern. In RetryTopic's annotation processing and builder I chose to mimic Spring Retry behavior as much as possible, since we relied on the same @BackOff annotation.

I've been meaning to ask if maybe we should change this in Spring Retry to return the NoBackOffPolicy instead if delay=0 is provided, but didn't want to mix things up. Also, that seems like a breaking change, so it should be a good fit only for SR 2.0.

Maybe it'd be worth it to validate user input here to make sure the inputs are > or >= 0. Although it might be confusing having different behaviors for auto configuration and the @RetryableTopic annotation and builder.

[artembilan]

So, why then does RetryTopicConfigurationBuilder support only a SleepingBackOffPolicy?
Why it cannot be that super BackOffPolicy?

[tomazfernandes]

That's because the way we generate the delay values is we inject a Sleeper implementation in the BackOffPolicy that calls backOff and stores the values, so we need that any custom policy the user provides respect the SleepingBackOffPolicy contract.

The builder offers a noBackOff() method if that's what the user desires, and if that's the case we don't need the Sleeper to fetch the values.

[artembilan]

I see there public RetryTopicConfigurationBuilder noBackoff() {, so perhaps with some props config we can fall back to this instead of customBackoff() then?

[tomazfernandes]

You mean, if users set delay=0 we could set noBackOff() for them there? That's an interesting thought. My concern would be having different behaviors in auto configuration and annotation processing - would that be confusing for users? I guess since these are kind of corner cases, maybe we can depart from that and also add properties validation.

WDYT?

[artembilan]

Yes, that would be reasonable.

Thanks

[tomazfernandes]

Sure, I’ll add that then. Thanks a lot for your input!

[artembilan]

static

[artembilan]

A description for these configuration properties must be as short as possible: https://docs.spring.io/spring-boot/docs/current/reference/htmlsingle/#appendix.configuration-metadata.format.property

[tomazfernandes]

Sure. This description was extracted from the @BackOff annotation, I'll try to come up with something more succinct. Thanks.

[artembilan]

So, why then does RetryTopicConfigurationBuilder support only a SleepingBackOffPolicy?
Why it cannot be that super BackOffPolicy?

[tomazfernandes]

Implemented @artembilan's suggestions.

I looked into using @Valid to validate the input, but since a couple properties are Duration and there doesn't seem to be a way to properly validate since annotations don't allow such value types, I preferred to keep it simple and validate all properties together in the code.

Please let me know if there's a better way or anything else to change.

Thanks

[tomazfernandes]

I've cleaned up the validation logic and added tests for that. Please let me know if there's anything else. Thanks!

[artembilan]

I won't mind, but I don't know Spring Boot development process in details, so I'll defer a decision for a proper validation and its place to respective Spring Boot team members.
Thanks

[tomazfernandes]

Sure, thanks. Just for reference, it seems we already have some validation in this buildProperties method of the KafkaProperties class, so I thought it seemed ok there. Of course, I'm ok with validating any other way or place.

spring-boot/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/kafka/KafkaProperties.java

Lines 1216 to 1218 in c58e24a

	public Map<String, Object> buildProperties() {
	validate();
	Properties properties = new Properties();

Thanks for looking into this.

[tomazfernandes]

FYI, this unrelated test failed in the PR build:

Found test failures in 1 test task:
[00:00:08](https://ci.spring.io/builds/185388#L62321ef2:5476)

[00:00:08](https://ci.spring.io/builds/185388#L62321ef2:5477)
:spring-boot-project:spring-boot-devtools:test
[00:00:08](https://ci.spring.io/builds/185388#L62321ef2:5478)
    org.springframework.boot.devtools.tunnel.server.HttpTunnelServerTests > disconnectTimeout()`

I'll force-push a no-ops commit to trigger the PR build again. Thanks.

[tomazfernandes]

@garyrussell, if and when possible, I'd like your input on the chosen sensible defaults, since it's something not so easy to change later.

Now we have:

useSingleTopicForFixedDelays()
suffixTopicsWithIndexValues()
doNotAutoCreateRetryTopics()

With these settings, by default users will get 3 topics, independently of the number of attempts or delay value:
myTopic, myTopic-retry and myTopic-dlt

If a multiplier or maxDelay is set, yelding multiple delay values, users will get one topic per attempt plus the dlt:
myTopic, myTopic-retry-0, myTopic-retry-1, myTopic-retry-2 ... myTopic-dlt

The other defaults are the same as for the rest of the feature, maxAttempts = 3 and FixedBackOff with delay=1s.

Thanks.

[tomazfernandes]

Hi @snicoll and @wilkinsona, looks like from the Spring Kafka team's perspective there are no further concerns. Is there anything you'd like to be changed implementation-wise? We can revise any part of this based on user feedback for 2.7.

Thanks

[snicoll]

Thanks for the follow-up and for working with the team.

Time is running out with RC1 happening tomorrow. I will do my best to review it before that happens if possible.

[tomazfernandes]

Sure, no worries. I should be available if there's anything to be changed prior to the release. Thanks.

[snicoll]

@tomazfernandes thank you for making your first contribution to Spring Boot. If you're interested, I've polished your contribution in b3e3581, mostly about using primitive types if possible. I also decided to remove the validation business as we don't really do such a thing in configuration properties usually. We'd rather let the underlying component we configure throw an exception if necessary.

[tomazfernandes]

Thanks a lot @snicoll for finding the time to look into this in such short notice! The retryable topics feature has been gaining a lot of traction with the community and with auto configuration I'm sure more users will be able to benefit from it and in a simpler way.

I had missed that if users had more than one KafkaTemplate it'd throw an exception. If I understand correctly, now with @ConditionalOnSingleCandidate(KafkaTemplate.class) if users have more than one KafkaTemplate bean defined the retry topic configuration will back off. Is that correct?

I wonder if it would make sense to add a kafkaTemplate configuration property that defaults to "kafkaTemplate", and then fetch the template bean by name, so that users can still benefit of the feature by leveraging auto configuration's KafkaTemplate if that's the case, or set a different one to be used in case they have more than one defined. Is this something you usually do in auto configuration?

Or maybe that's expected behavior - if users choose to define multiple KafkaTemplate beans, they might just as well define RetryTopicConfiguration beans. We'd just need to properly document that.

What do you think?

Thanks again!

[snicoll]

That's not what @ConditionalOnSingleCandidate does. A single KafkaTemplate should be defined (so either one in the context, or one flagged with @Primary in case of multiple instnaces). This is is consistent with what we do elsewhere and the recommendation is indeed to configure the feature if a "main" KafkaTemplate cannot be determined. The auto-configuration report will mention why the feature wasn't configured (again, something users rely on elsewhere) so I don't think it needs an explicit mention in the doc.

[tomazfernandes]

I see, makes sense. I've learned a lot about autoconfiguration and what it is and isn't supposed to do in this issue, so thanks a lot everyone involved.

Thanks again @snicoll for looking into this in time for the RC release!