Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/2897 fix exponential backoff #2903

Merged
merged 7 commits into from
Nov 17, 2023
Merged

Bugfix/2897 fix exponential backoff #2903

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9cc7702
#2897: WIP Fix exponential backoff
ulischulte Nov 17, 2023
f334a32
reduce number of places where defaults can be defined
erikpetzold Nov 17, 2023
e3e834b
use configured backoff in retry
erikpetzold Nov 17, 2023
75a2365
#2897: javaformat
ulischulte Nov 17, 2023
a129416
add Test
erikpetzold Nov 17, 2023
d2c7ddc
add docs
erikpetzold Nov 17, 2023
56438c5
reduce number of places with defaults
erikpetzold Nov 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions spring-boot-admin-docs/src/site/asciidoc/server.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ In addition when the reverse proxy terminates the https connection, it may be ne
| Time interval to check the status of instances.
| 10,000ms

| spring.boot.admin.monitor.status-max-backoff
| The maximal backoff for status check retries (retry after error has exponential backoff, minimum backoff is 1 second).
| 60,000ms

| spring.boot.admin.monitor.status-lifetime
| Lifetime of status. The status won't be updated as long the last status isn't expired.
| 10,000ms
Expand All @@ -30,6 +34,10 @@ In addition when the reverse proxy terminates the https connection, it may be ne
| Time interval to check the info of instances.
| 1m

| spring.boot.admin.monitor.info-max-backoff
| The maximal backoff for info check retries (retry after error has exponential backoff, minimum backoff is 1 second).
| 10m

| spring.boot.admin.monitor.info-lifetime
| Lifetime of info. The info won't be updated as long the last info isn't expired.
| 1m
Expand Down
2 changes: 2 additions & 0 deletions ...r/src/main/java/de/codecentric/boot/admin/server/config/AdminServerAutoConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ public StatusUpdateTrigger statusUpdateTrigger(StatusUpdater statusUpdater, Publ
StatusUpdateTrigger trigger = new StatusUpdateTrigger(statusUpdater, events);
trigger.setInterval(this.adminServerProperties.getMonitor().getStatusInterval());
trigger.setLifetime(this.adminServerProperties.getMonitor().getStatusLifetime());
trigger.setMaxBackoff(this.adminServerProperties.getMonitor().getStatusMaxBackoff());
return trigger;
}

Expand Down Expand Up @@ -132,6 +133,7 @@ public InfoUpdateTrigger infoUpdateTrigger(InfoUpdater infoUpdater, Publisher<In
InfoUpdateTrigger trigger = new InfoUpdateTrigger(infoUpdater, events);
trigger.setInterval(this.adminServerProperties.getMonitor().getInfoInterval());
trigger.setLifetime(this.adminServerProperties.getMonitor().getInfoLifetime());
trigger.setMaxBackoff(this.adminServerProperties.getMonitor().getInfoMaxBackoff());
return trigger;
}

Expand Down
12 changes: 12 additions & 0 deletions ...n-server/src/main/java/de/codecentric/boot/admin/server/config/AdminServerProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,12 +103,24 @@ public static class MonitorProperties {
@DurationUnit(ChronoUnit.MILLIS)
private Duration statusLifetime = Duration.ofMillis(10_000L);

/**
* The maximal backoff for status check retries (retry after error has exponential backoff, minimum backoff is 1 second).
*/
@DurationUnit(ChronoUnit.MILLIS)
private Duration statusMaxBackoff = Duration.ofMillis(60_000L);

/**
* Time interval to check the info of instances,
*/
@DurationUnit(ChronoUnit.MILLIS)
private Duration infoInterval = Duration.ofMinutes(1L);

/**
* The maximal backoff for info check retries (retry after error has exponential backoff, minimum backoff is 1 second).
*/
@DurationUnit(ChronoUnit.MILLIS)
private Duration infoMaxBackoff = Duration.ofMinutes(10);

/**
* Lifetime of info. The info won't be updated as long the last info isn't
* expired.
Expand Down
7 changes: 6 additions & 1 deletion ...min-server/src/main/java/de/codecentric/boot/admin/server/services/InfoUpdateTrigger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ public class InfoUpdateTrigger extends AbstractEventHandler<InstanceEvent> {
public InfoUpdateTrigger(InfoUpdater infoUpdater, Publisher<InstanceEvent> publisher) {
super(publisher, InstanceEvent.class);
this.infoUpdater = infoUpdater;
this.intervalCheck = new IntervalCheck("info", this::updateInfo, Duration.ofMinutes(5), Duration.ofMinutes(1));
this.intervalCheck = new IntervalCheck("info", this::updateInfo, Duration.ofMinutes(5), Duration.ofMinutes(1),
Duration.ofMinutes(10));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that we have defaults here (because we already have them in the AdminServerProperties).
In the AdminServerAutoConfiguration we call the setters to override these values anyways.

Can't we just pass the values to this constructor here? Then we could get rid of these defaults here and also get rid of the setters which are just passing the value to the next object.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, the setters are also used in tests, makes them easier

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed this, but kept the setters for tests

}

@Override
Expand Down Expand Up @@ -79,4 +80,8 @@ public void setLifetime(Duration infoLifetime) {
this.intervalCheck.setMinRetention(infoLifetime);
}

public void setMaxBackoff(Duration maxBackoff) {
this.intervalCheck.setMaxBackoff(maxBackoff);
}

}
11 changes: 6 additions & 5 deletions ...t-admin-server/src/main/java/de/codecentric/boot/admin/server/services/IntervalCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ public class IntervalCheck {

private final Function<InstanceId, Mono<Void>> checkFn;

@Setter
private Duration maxBackoff;

@Getter
@Setter
private Duration interval;
Expand All @@ -65,16 +68,13 @@ public class IntervalCheck {
@Nullable
private Scheduler scheduler;

public IntervalCheck(String name, Function<InstanceId, Mono<Void>> checkFn) {
this(name, checkFn, Duration.ofSeconds(10), Duration.ofSeconds(10));
}

public IntervalCheck(String name, Function<InstanceId, Mono<Void>> checkFn, Duration interval,
Duration minRetention) {
Duration minRetention, Duration maxBackoff) {
this.name = name;
this.checkFn = checkFn;
this.interval = interval;
this.minRetention = minRetention;
this.maxBackoff = maxBackoff;
}

public void start() {
Expand All @@ -85,6 +85,7 @@ public void start() {
.subscribeOn(this.scheduler)
.concatMap((i) -> this.checkAllInstances())
.retryWhen(Retry.backoff(Long.MAX_VALUE, Duration.ofSeconds(1))
.maxBackoff(maxBackoff)
.doBeforeRetry((s) -> log.warn("Unexpected error in {}-check", this.name, s.failure())))
.subscribe(null, (error) -> log.error("Unexpected error in {}-check", name, error));
}
Expand Down
7 changes: 6 additions & 1 deletion ...n-server/src/main/java/de/codecentric/boot/admin/server/services/StatusUpdateTrigger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ public class StatusUpdateTrigger extends AbstractEventHandler<InstanceEvent> {
public StatusUpdateTrigger(StatusUpdater statusUpdater, Publisher<InstanceEvent> publisher) {
super(publisher, InstanceEvent.class);
this.statusUpdater = statusUpdater;
this.intervalCheck = new IntervalCheck("status", this::updateStatus);
this.intervalCheck = new IntervalCheck("status", this::updateStatus, Duration.ofSeconds(10),
Duration.ofSeconds(10), Duration.ofSeconds(60));
}

@Override
Expand Down Expand Up @@ -81,4 +82,8 @@ public void setLifetime(Duration statusLifetime) {
this.intervalCheck.setMinRetention(statusLifetime);
}

public void setMaxBackoff(Duration maxBackoff) {
this.intervalCheck.setMaxBackoff(maxBackoff);
}

}
16 changes: 15 additions & 1 deletion ...min-server/src/test/java/de/codecentric/boot/admin/server/services/IntervalCheckTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public class IntervalCheckTest {
private final Function<InstanceId, Mono<Void>> checkFn = mock(Function.class, (i) -> Mono.empty());

private final IntervalCheck intervalCheck = new IntervalCheck("test", this.checkFn, Duration.ofMillis(10),
Duration.ofMillis(10));
Duration.ofMillis(10), Duration.ofSeconds(1));

@Test
public void should_check_after_being_started() throws InterruptedException {
Expand Down Expand Up @@ -81,6 +81,20 @@ public void should_recheck_after_retention_period() throws InterruptedException
verify(this.checkFn, atLeast(2)).apply(INSTANCE_ID);
}

@Test
public void should_not_wait_longer_than_maxBackoff() throws InterruptedException {
this.intervalCheck.setInterval(Duration.ofMillis(10));
this.intervalCheck.setMinRetention(Duration.ofMillis(10));
this.intervalCheck.setMaxBackoff(Duration.ofSeconds(2));
this.intervalCheck.markAsChecked(INSTANCE_ID);

when(this.checkFn.apply(any())).thenReturn(Mono.error(new RuntimeException("Test")));

this.intervalCheck.start();
Thread.sleep(1000 * 10);
verify(this.checkFn, atLeast(7)).apply(INSTANCE_ID);
}

@Test
public void should_check_after_error() throws InterruptedException {
this.intervalCheck.markAsChecked(INSTANCE_ID);
Expand Down
Loading