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.
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
Fix an issue wheras a -1
breaker limit was interpreted wrongly
#13096
Conversation
server/src/main/java/org/elasticsearch/common/breaker/CircuitBreaker.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge this into 5.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a question, if that's already handled lgtm
if (memoryBytesLimit == -1) { | ||
freeSupplier = () -> Long.MAX_VALUE; | ||
} else if (memoryBytesLimit == 0) { | ||
freeSupplier = () -> 0L; | ||
} else { | ||
freeSupplier = () -> getLimit() - getUsed(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it also be set to other negative values, or is that forbidden already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is forbidden already, see https://github.com/crate/crate/blob/master/server/src/main/java/org/elasticsearch/common/unit/ByteSizeValue.java#L43
A `-1` limit on a breaker should not result in breaking but just accounting. This was not taken into account at the breakers `getFree()` method. It will return Long.MAX_VALUE in this case now. Additionally if will return `0` if the limit is set to `0` which indicates that the breaker should refuce any data according to the related breaker implementation. See also ChildMemoryCircuitBreaker.addEstimateBytesAndMaybeBreak().
cf500f1
to
405f2fd
Compare
Summary of the changes / Why this improves CrateDB
A
-1
limit on a breaker should not result in breaking but just accounting. This was not taken into account at the breakersgetFree()
method. It will return Long.MAX_VALUE in this case now.Additionally if will return
0
if the limit is set to0
which indicates that the breaker should refuce any data according to the related breaker implementation.See also
ChildMemoryCircuitBreaker.addEstimateBytesAndMaybeBreak()
.Checklist
CHANGES.txt
for user facing changessql_features
table for user facing changesCHANGES.txt
(E.g. AdminUI)