GH-2967: Some minor improvements in RetryableTopicAnnotationProcessor #2967
Conversation
NathanQingyangXu
force-pushed
the
improvement-on-RetryableTopicAnnotationProcessor
branch
from
626bd95 to
930bbee
Compare
| if (concurrency != null) { | ||
| builder.concurrency(concurrency); | ||
| } | ||
| return builder.create(getKafkaTemplate(resolveExpressionAsString(annotation.kafkaTemplate(), "kafkaTemplate"), topics)); |
There was a problem hiding this comment.
there are many loose ends regarding nullness safety net. Not sure why there is no usage of the following two entries in the package-info.java within the package of annotation:
@org.springframework.lang.NonNullApi
@org.springframework.lang.NonNullFields
if we added them, many NPE issues in this package would be better exposed.
There was a problem hiding this comment.
Sure. Feel free to send another PR with that change if you are up to it.
| @@ -200,7 +210,6 @@ private EndpointHandlerMethod getDltProcessor(Method listenerMethod, Object bean | |||
| .orElse(RetryTopicConfigurer.DEFAULT_DLT_HANDLER); | |||
| } | |||
|
|
|||
| @SuppressWarnings("deprecation") | |||
There was a problem hiding this comment.
seems leftover of previous JDK. In JDK17, I saw no compiling warnings after the removal of it.
| if (this.beanFactory != null && this.beanFactory instanceof ConfigurableBeanFactory) { | ||
| return ((ConfigurableBeanFactory) this.beanFactory).resolveEmbeddedValue(value); | ||
| if (this.beanFactory instanceof ConfigurableBeanFactory cbf) { | ||
| return cbf.resolveEmbeddedValue(value); |
There was a problem hiding this comment.
of course, the original nullness checking is unnecessary for instanceof has included it already.
| if (this.expressionContext != null) { | ||
| String resolved = resolve(value); |
There was a problem hiding this comment.
seems the resolved is only used within the if block, so it makes more sense to move it within, so when expressionContext is null, no unnecessary overhead will incur.
NathanQingyangXu
changed the title
NathanQingyangXu
changed the title
| policy.setMultiplier(multiplier); | ||
| policy.setMaxInterval(max > min ? max : ExponentialBackOffPolicy.DEFAULT_MAX_INTERVAL); | ||
| if (max != null && min != null && max > min) { |
There was a problem hiding this comment.
What will max be if max <= min? In this case, currently, we set it to ExponentialBackOffPolicy.DEFAULT_MAX_INTERVAL. The new code seems to change that semantics.
There was a problem hiding this comment.
Good comment! Below is the relevant code snippet:
public class ExponentialBackOffPolicy implements SleepingBackOffPolicy<ExponentialBackOffPolicy> {
... ...
/**
* The maximum value of the backoff period in milliseconds.
*/
private long maxInterval = DEFAULT_MAX_INTERVAL;
so regardless of whether we set the same default value or not in RetryableTopicAnnotationProcessor above, the end result is the same. I avoided duplication to make it more elegant.
There was a problem hiding this comment.
Let me know whether we need to revert back old behaviour. I am perfectly fine with either way.
There was a problem hiding this comment.
But don't we need to set that value in RetryableTopicAnnotationProcessor? Otherwise, how does this max variable get to that value? I am fine with cleaning it up if it is redundant. I am trying to see the connection.
There was a problem hiding this comment.
As I showed above, the policy class will set the fields to default values already just after the instance was initiated but before constructor is invoked, so after the default constructor was invoked, we have ended up with valid state already.
Am I missing something here?
There was a problem hiding this comment.
Ok, I got it. Thanks for the explanation.
some minor and easily verified code quality improvements.
after the changes, the warnings in IDEA decreased from 16 to 5.