Retryable with exponential backoff not working with delayExpression #399

R-Zer0 · 2023-11-08T16:01:05Z

No description provided.

pivotal-cla · 2023-11-08T16:01:09Z

@R-Zer0 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-cla · 2023-11-08T16:01:53Z

@R-Zer0 Thank you for signing the Contributor License Agreement!

artembilan

Any chances to have this covered by some unit test?
See ExponentialBackOffPolicyTests as a candidate for addition.
Thanks

R-Zer0 · 2023-11-08T17:17:48Z

Any chances to have this covered by some unit test? See ExponentialBackOffPolicyTests as a candidate for addition. Thanks

Added

src/main/java/org/springframework/retry/backoff/ExponentialBackOffPolicy.java

artembilan · 2023-11-08T18:42:17Z

src/main/java/org/springframework/retry/backoff/ExponentialBackOffPolicy.java

+				return this.interval;
+			}
+			else {
+				return this.interval == DEFAULT_INITIAL_INTERVAL ? this.initialIntervalSupplier.get() : this.interval;


Don't we need to call intervalSupplier every time?
Looks like with your change we do that only once.

Only first time when we need initial value, further iteration store multiplayed value in the interval variable here

artembilan

The build against your PR has failed.
Would you mind double checking it locally running ./mvnw clean verify ?

R-Zer0 · 2023-11-08T20:12:41Z

The build against your PR has failed. Would you mind double checking it locally running ./mvnw clean verify ?

I found discrepancy between annotation default delay value 1000 and here 1000 too but here it is 100 I suspect this value also should be 1000, pushing this change. Let me know if my understanding correct and this change acceptable.

artembilan · 2023-11-08T21:19:14Z

src/main/java/org/springframework/retry/backoff/ExponentialBackOffPolicy.java

 	 * 'multiplier' value this gives a useful initial spread of pauses for 1-5 retries.
 	 */
-	public static final long DEFAULT_INITIAL_INTERVAL = 100L;
+	public static final long DEFAULT_INITIAL_INTERVAL = 1000L;


This is some kind of breaking change for the current point release, but I indeed see the reasoning behind consistency.
Plus it looks like the @Backoff annotation is always contributing exactly that 1000 value.
So, we really can bite a bullet and let this pass through to the next release in a couple weeks.

artembilan

LGTM.

Let's hear yet from @garyrussell !

R-Zer0 · 2023-11-09T12:29:43Z

@artembilan pushed minor update to if else logic, to make it more readable

garyrussell · 2023-11-13T17:23:14Z

src/main/java/org/springframework/retry/backoff/ExponentialBackOffPolicy.java

+				return (long) (this.interval * getMultiplier());
+			}
+			else {
+				return (long) (this.initialIntervalSupplier.get() * getMultiplier());


This is incorrect - the multiplier must be applied to the previous interval; this always applies it to the initial interval.

Yes, that's why there is that logic in the if: || this.interval != DEFAULT_INITIAL_INTERVAL.
So, the initialIntervalSupplier is called only once, for the first time. The next interval is really based on the previous one from now on.

I've made this by purpose: during first iteration we should check if supplier provided or not, if provided, first iteration should use value provided by initialIntervalSupplier(if present), further it will use interval value since it will be assigned to interval here

garyrussell · 2023-11-13T17:23:57Z

src/test/java/org/springframework/retry/backoff/ExponentialBackOffPolicyTests.java

+		BackOffContext context = strategy.start(null);
+		for (int x = 0; x < 5; x++) {
+			strategy.backOff(context);
+			assertThat(sleeper.getLastBackOff()).isEqualTo(seed);


We need to test that we still have exponential back offs (which we no longer have with the current change).

Could you suggest any example here?

Actually current test checking following, validated in debug.

Last backoff Seed

40 40

48 48

57 57

68 68

81 81

I was thinking it is sufficient for current issue.

garyrussell · 2023-11-14T14:39:22Z

src/main/java/org/springframework/retry/backoff/ExponentialBackOffPolicy.java

 		}

 		public double getMultiplier() {
 			return this.multiplierSupplier != null ? this.multiplierSupplier.get() : this.multiplier;
 		}

 		public long getInterval() {
-			return this.intervalSupplier != null ? this.intervalSupplier.get() : this.interval;
+			if (initialIntervalSupplier == null || this.interval != DEFAULT_INITIAL_INTERVAL) {


Why are we changing the logic to make the interval superior to the intervalSupplier?

Perhaps a description of exactly what you are trying to "fix" in the PR description would have been helpful.

It is not superior, actually It is fix to use intervalSupplier as initial value only, as this code was written. Because it was always incrementing interval on every iteration, regardless supplier provided or not.

Sorry adding description here, this my first time I am pushing to spring repo :)

So idea was to make interval multiplication work properly. And use supplier only to obtain initial value, to keep the same logic to operate interval variable

So I updated code to use correct saved multiplied value on every iteration(previously it was always multiplying interval but using intervalSuplier for sleep value which was always constant initial value)
So I changed it that on first iteration we check if initial value provided by supplier or interval, than multiplied and stored in the interval variable and on the second and others iterations we are switching to interval variable.

Hope this explanation is clear

Sorry, I am still missing something; with the existing code...

return this.intervalSupplier != null ? this.intervalSupplier.get() : this.interval;

...the supplier, if present, takes precedence over the interval, regardless of whether it is the default value or not.

Also, it is not at all clear to me why we are testing against DEFAULT_INITIAL_INTERVAL - this value could be reached at some point after the initial delay - e.g. if the initial interval returned by the supplier is DEFAULT_INITIAL_INTERVAL / 2 and the multiplier is 2.0.

Perhaps you could provide a test case that fails with the existing code, it would help me understand what you are trying to fix.

Sorry didn't understand line of code your referring, is it before my change?

You can try test I added in this PR it is failing without my change.

@Test public void testMultiBackOffWithInitialDelaySupplier() { ExponentialBackOffPolicy strategy = new ExponentialBackOffPolicy(); long seed = 40; double multiplier = 1.2; strategy.initialIntervalSupplier(() -> 40L); strategy.setMultiplier(multiplier); strategy.setSleeper(sleeper); BackOffContext context = strategy.start(null); for (int x = 0; x < 5; x++) { strategy.backOff(context); assertThat(sleeper.getLastBackOff()).isEqualTo(seed); seed *= multiplier; } }

If we are talking about old code and you referring to getInterval() method we calling it at line 280 so it will always returns supplier. But in the line 286 we calling getNextInterval() and assigning value to interval (not supplier) . Next iteration supplier will provide same initial value instead of incremented stored in interval on previous iteration. So we end up, when backoff always have initial value and interval always multiplied and never actually used.

The problem, @garyrussell , that getSleepAndIncrement() when calling this.interval = getNextInterval(); does not take into account intervalSupplier.
And this one we need to call there only once: for initial interval value since this.interval is not set in case of intervalSupplier.

Ok; I see the problem now, thanks; but I think this is a much simpler fix (and doesn't require any tests against the constant which, as I stated, may cause undesirable side effects).

public long getInterval() { if (this.intervalSupplier != null) { this.interval = this.intervalSupplier.get(); this.intervalSupplier = null; } return this.interval; }

... and I guess we need to set its result into this.interval since getNextInterval() is going to use that one only.

Yes, 3rd line in the above code.

updated according @garyrussell suggestion.

artembilan

Add your name to @author list, please, of the affected classes.

R-Zer0 · 2023-11-15T07:20:44Z

Add your name to @author list, please, of the affected classes.

Requested update pushed

artembilan · 2023-11-15T13:57:07Z

@R-Zer0 ,

thank you for contribution; looking forward for more!

Retryable with exponential backoff not working with delayExpression

58e70e6

R-Zer0 mentioned this pull request Nov 8, 2023

Retryable with exponential backoff not working with delayExpression #397

Closed

Anton Aharkau added 3 commits November 8, 2023 17:18

Retryable with exponential backoff not working with delayExpression

0dae5b5

Retryable with exponential backoff not working with delayExpression

b2be988

Retryable with exponential backoff not working with delayExpression

aa0bac8

artembilan requested changes Nov 8, 2023

View reviewed changes

Retryable with exponential backoff not working with delayExpression

33af265

Retryable with exponential backoff not working with delayExpression

903640e

artembilan requested changes Nov 8, 2023

View reviewed changes

Retryable with exponential backoff not working with delayExpression

c4f14f4

artembilan requested changes Nov 8, 2023

View reviewed changes

Retryable with exponential backoff not working with delayExpression

6e018d2

artembilan reviewed Nov 8, 2023

View reviewed changes

artembilan approved these changes Nov 8, 2023

View reviewed changes

updated logical conditions

27011f1

artembilan approved these changes Nov 13, 2023

View reviewed changes

garyrussell suggested changes Nov 13, 2023

View reviewed changes

garyrussell reviewed Nov 14, 2023

View reviewed changes

Anton Aharkau added 3 commits November 14, 2023 20:58

addressing comment

06cd6d5

addressing comment

e8715bc

addressing comment

1835f54

artembilan requested changes Nov 14, 2023

View reviewed changes

added author

cac9236

R-Zer0 requested a review from artembilan November 15, 2023 07:26

R-Zer0 requested a review from garyrussell November 15, 2023 07:26

artembilan added the bug label Nov 15, 2023

artembilan modified the milestone: 2.0.5 Nov 15, 2023

artembilan removed the bug label Nov 15, 2023

artembilan merged commit d01f25b into spring-projects:main Nov 15, 2023
1 check passed

R-Zer0 deleted the fix-retryable-exceptional-backoff-with-delay-expression branch November 17, 2023 11:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retryable with exponential backoff not working with delayExpression #399

Retryable with exponential backoff not working with delayExpression #399

R-Zer0 commented Nov 8, 2023

pivotal-cla commented Nov 8, 2023

pivotal-cla commented Nov 8, 2023

artembilan left a comment

R-Zer0 commented Nov 8, 2023

artembilan Nov 8, 2023

R-Zer0 Nov 8, 2023 •

edited

Loading

artembilan left a comment

R-Zer0 commented Nov 8, 2023 •

edited

Loading

artembilan Nov 8, 2023

artembilan left a comment

R-Zer0 commented Nov 9, 2023

garyrussell Nov 13, 2023

artembilan Nov 13, 2023

R-Zer0 Nov 13, 2023 •

edited

Loading

garyrussell Nov 13, 2023

R-Zer0 Nov 13, 2023 •

edited

Loading

R-Zer0 Nov 13, 2023

garyrussell Nov 14, 2023

R-Zer0 Nov 14, 2023

garyrussell Nov 14, 2023

R-Zer0 Nov 14, 2023 •

edited

Loading

R-Zer0 Nov 14, 2023 •

edited

Loading

artembilan Nov 14, 2023

garyrussell Nov 14, 2023

artembilan Nov 14, 2023

garyrussell Nov 14, 2023

R-Zer0 Nov 14, 2023

artembilan left a comment

R-Zer0 commented Nov 15, 2023

artembilan commented Nov 15, 2023

Retryable with exponential backoff not working with delayExpression #399

Retryable with exponential backoff not working with delayExpression #399

Conversation

R-Zer0 commented Nov 8, 2023

pivotal-cla commented Nov 8, 2023

pivotal-cla commented Nov 8, 2023

artembilan left a comment

Choose a reason for hiding this comment

R-Zer0 commented Nov 8, 2023

Choose a reason for hiding this comment

R-Zer0 Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

artembilan left a comment

Choose a reason for hiding this comment

R-Zer0 commented Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

artembilan left a comment

Choose a reason for hiding this comment

R-Zer0 commented Nov 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

R-Zer0 Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

R-Zer0 Nov 13, 2023 • edited Loading

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

R-Zer0 Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

R-Zer0 Nov 14, 2023 • edited Loading

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 left a comment

Choose a reason for hiding this comment

R-Zer0 commented Nov 15, 2023

artembilan commented Nov 15, 2023

R-Zer0 Nov 8, 2023 •

edited

Loading

R-Zer0 commented Nov 8, 2023 •

edited

Loading

R-Zer0 Nov 13, 2023 •

edited

Loading

R-Zer0 Nov 13, 2023 •

edited

Loading

R-Zer0 Nov 14, 2023 •

edited

Loading

R-Zer0 Nov 14, 2023 •

edited

Loading