maytasm opened a new pull request #10891:
URL: https://github.com/apache/druid/pull/10891
Fix maxBytesInMemory for heap overhead of sinks and hydrants check
### Description
There is currently two bugs with maxBytesInMemory for heap overhead of sinks
and hydrants check.
The first bug is that the check for maxBytesInMemory for heap overhead of
sinks and hydrants check is currently done instead the `persistAll` method. The
`persistAll` method is done in the background and while the `persistAll` method
is running, there could already be another concurrent in-memory ingestion. The
new in-memory ingestion would use the old value of `bytesCurrentlyInMemory`
without accounting for the heap overhead of sinks and hydrants from the latest
persist (since we only calculate the heap overhead of sinks and hydrants in the
`persistAll` method.
The second bug is that the memory overhead of sinks and hydrants check lags
by one persist iteration. In each persist iteration the heap overhead of sinks
and hydrants is calculated and store in a variable called
bytesCurrentlyInMemory. However, bytesCurrentlyInMemory is only check against
the limit in the next iteration of persist. This is because, in the current
iteration we check against `bytesInMemoryBeforePersist` which is a copy of
`bytesCurrentlyInMemory` at the start of the persist (not accounting for the
heap overhead of sinks and hydrants in the current persist). Basically we will
only fails the task one persist iteration after we have already reached the
limit. Most of the time this is fine as our limit is below the real JVM memory
capacity. However, for JVM memory that is set very low, this bug can results in
OOM.
This PR fixes both problem by moving the heap overhead of sinks and hydrants
check outside of the `persistAll` method. It is now done before the background
task `persistAll` method. This also eliminates the needs of
`bytesInMemoryBeforePersist` variable. Hence, we can adds the heap overhead of
sinks and hydrants to `bytesCurrentlyInMemory` and checks
`bytesCurrentlyInMemory` before calling `persistAll`
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency
checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md)
(Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked
related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in
[licenses.yaml](https://github.com/apache/druid/blob/master/licenses.yaml)
- [ ] added comments explaining the "why" and the intent of the code
wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths,
ensuring the threshold for [code
coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md)
is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
