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

Jump to bottom

Enhance Parent circuit breaker error message #32056

Merged
merged 4 commits into from
Jul 20, 2018
Merged

Enhance Parent circuit breaker error message #32056

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
90add0c
Enhance Parent circuit breaker error message
dakrone Jul 13, 2018
980030e
Merge remote-tracking branch 'origin/master' into enhance-parent-brea…
dakrone Jul 16, 2018
81bda09
Merge remote-tracking branch 'origin/master' into enhance-parent-brea…
dakrone Jul 19, 2018
ceec2ea
Only call currentMemoryUsage once by returning structured object
dakrone Jul 19, 2018
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
29 changes: 26 additions & 3 deletions server/src/main/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.atomic.AtomicLong;
import java.util.stream.Collectors;

/**
* CircuitBreakerService that attempts to redistribute space between breakers
Expand Down Expand Up @@ -250,11 +251,33 @@ public void checkParentLimit(long newBytesReserved, String label) throws Circuit
long parentLimit = this.parentSettings.getLimit();
if (totalUsed > parentLimit) {
this.parentTripCount.incrementAndGet();
final String message = "[parent] Data too large, data for [" + label + "]" +
final StringBuilder message = new StringBuilder("[parent] Data too large, data for [" + label + "]" +
" would be [" + totalUsed + "/" + new ByteSizeValue(totalUsed) + "]" +
", which is larger than the limit of [" +
parentLimit + "/" + new ByteSizeValue(parentLimit) + "]";
throw new CircuitBreakingException(message, totalUsed, parentLimit);
parentLimit + "/" + new ByteSizeValue(parentLimit) + "]");
if (this.trackRealMemoryUsage) {
final long realUsage = currentMemoryUsage();
Copy link
Member

Choose a reason for hiding this comment

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

Calling currentMemoryUsage() again here is tricky because the value is likely different than the one that cause the breaker to trip. To get the correct value, we could calculate it as final long realUsage = totalUsed - newBytesReserved which would basically revert the calculation in #parentUsed(long) but that also doesn't feel entirely right. Alternatively, we could also return a more structured representation which returns the memory usage as a dedicated property. Wdyt?

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 thought about this, but I thought that giving the real memory usage at exception construction time was the least-worst option (as part as plain-text is concerned).

For a structured error, I do like the idea of making this exception more structured, it's just a matter of coming up with the structure.

We currently have:

  • bytesWanted
  • bytesLimit

How about adding

  • currentUsage (we could always retrieve it even if "track real memory" is disabled, just for message purposes)
  • breakerUsages - a map of the current breakers to their usages

The only downside for this is that we already have logic for serializing the CircuitBreakingException, and to make this structured we'd need a new exception (ParentCircuitBreakingException) since it doesn't make sense to add this additional structure to every circuit breaker trip (or maybe it does? what do you think?)

Copy link
Member

Choose a reason for hiding this comment

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

For a structured error, I do like the idea of making this exception more structured, it's just a matter of coming up with the structure.

My point was not to change CircuitBreakingException but rather to have #parentUsed(long) return a more structured representation, something along the lines of:

private static class ParentMemUsage {
    private final long baseUsage;
    private final long reservation;
    private final long totalUsage;
    
    public ParentMemUsage(long baseUsage, long reservation, long totalUsage) {
        // ...
    }
    // ... getters here ...
}

private long parentUsed(long newBytesReserved) {
    if (this.trackRealMemoryUsage) {
        long current = currentMemoryUsage();
        return new ParentMemUsage(current, newBytesReserved,  current + newBytesReserved);
    } else {
        long parentEstimated = 0;
        for (CircuitBreaker breaker : this.breakers.values()) {
            parentEstimated += breaker.getUsed() * breaker.getOverhead();
        }
        return new ParentMemUsage(parentEstimated, newBytesReserved, parentEstimated);
    }
}

We could then just call ParentMemUsage#getBaseUsage() in order to retrieve the original value again in case an exception occurs. A downside of this is that we allocate an additional object but it might be possible that the compiler is able to optimize it away (by inlining #parentUsed(long) and then eliminating the allocation via escape analysis).

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I gave this a shot, let me know what you think

message.append(", real usage: [");
message.append(realUsage);
message.append("/");
message.append(new ByteSizeValue(realUsage));
message.append("], new bytes reserved: [");
message.append(newBytesReserved);
message.append("/");
message.append(new ByteSizeValue(newBytesReserved));
message.append("]");
} else {
message.append(", usages [");
message.append(String.join(", ",
this.breakers.entrySet().stream().map(e -> {
final CircuitBreaker breaker = e.getValue();
final long breakerUsed = (long)(breaker.getUsed() * breaker.getOverhead());
return e.getKey() + "=" + breakerUsed + "/" + new ByteSizeValue(breakerUsed);
})
.collect(Collectors.toList())));
message.append("]");
}
throw new CircuitBreakingException(message.toString(), totalUsed, parentLimit);
}
}

Expand Down
5 changes: 5 additions & 0 deletions .../src/test/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,8 @@ public void testBorrowingSiblingBreakerMemory() throws Exception {
.addEstimateBytesAndMaybeBreak(new ByteSizeValue(50, ByteSizeUnit.MB).getBytes(), "should break"));
assertThat(exception.getMessage(), containsString("[parent] Data too large, data for [should break] would be"));
assertThat(exception.getMessage(), containsString("which is larger than the limit of [209715200/200mb]"));
assertThat(exception.getMessage(),
containsString("usages [request=157286400/150mb, fielddata=54001664/51.5mb, in_flight_requests=0/0b, accounting=0/0b]"));
}
}

Expand Down Expand Up @@ -239,6 +241,9 @@ long currentMemoryUsage() {
// it was the parent that rejected the reservation
assertThat(exception.getMessage(), containsString("[parent] Data too large, data for [request] would be"));
assertThat(exception.getMessage(), containsString("which is larger than the limit of [200/200b]"));
assertThat(exception.getMessage(),
containsString("real usage: [181/181b], new bytes reserved: [" + (reservationInBytes * 2) +
"/" + new ByteSizeValue(reservationInBytes * 2) + "]"));
assertEquals(0, requestBreaker.getTrippedCount());
assertEquals(1, service.stats().getStats(CircuitBreaker.PARENT).getTrippedCount());

Expand Down