-
Notifications
You must be signed in to change notification settings - Fork 517
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
HDDS-10488. Datanode OOM due to run out of mmap handler #6690
Changes from 3 commits
e7c412e
cb7664c
4298697
c677266
f530b89
1aac285
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,12 +19,14 @@ | |
|
||
import java.io.IOException; | ||
import java.nio.ByteBuffer; | ||
import java.nio.MappedByteBuffer; | ||
import java.nio.channels.GatheringByteChannel; | ||
import java.util.Collections; | ||
import java.util.Iterator; | ||
import java.util.List; | ||
import java.util.NoSuchElementException; | ||
import java.util.Objects; | ||
import java.util.function.Consumer; | ||
import java.util.function.Function; | ||
|
||
import org.apache.ratis.thirdparty.com.google.protobuf.ByteString; | ||
|
@@ -34,14 +36,24 @@ | |
final class ChunkBufferImplWithByteBuffer implements ChunkBuffer { | ||
private final ByteBuffer buffer; | ||
private final UncheckedAutoCloseable underlying; | ||
private final Consumer<Integer> releaseCallback; | ||
|
||
ChunkBufferImplWithByteBuffer(ByteBuffer buffer) { | ||
this(buffer, null); | ||
this.buffer = Objects.requireNonNull(buffer, "buffer == null"); | ||
this.releaseCallback = null; | ||
this.underlying = null; | ||
} | ||
|
||
ChunkBufferImplWithByteBuffer(ByteBuffer buffer, Consumer<Integer> releaseFunction) { | ||
this.buffer = Objects.requireNonNull(buffer, "buffer == null"); | ||
this.releaseCallback = releaseFunction; | ||
this.underlying = null; | ||
} | ||
|
||
ChunkBufferImplWithByteBuffer(ByteBuffer buffer, UncheckedAutoCloseable underlying) { | ||
this.buffer = Objects.requireNonNull(buffer, "buffer == null"); | ||
this.underlying = underlying; | ||
this.releaseCallback = null; | ||
} | ||
|
||
@Override | ||
|
@@ -163,4 +175,12 @@ public String toString() { | |
return getClass().getSimpleName() + ":limit=" + buffer.limit() | ||
+ "@" + Integer.toHexString(hashCode()); | ||
} | ||
|
||
@Override | ||
protected void finalize() throws Throwable { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ChenSammi , Let's don't override There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see the performance impact. The thing is these MappedByteBuffer are wrapped into ChunkBuffer, then into ByteString, there is not easy to find a better timing other than finalize() and trace back to MappedByteBuffers to release and ummap them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ChenSammi , as @adoroszlai mentioned, the buffer should be released in the BTW, HDDS-7188 ( #3730 ) may be a better solution since Netty has a better buffer management. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ChunkBuffer#close() has been tried before use finalize(), but it's not get called when the buffer is auto reclaimed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A ChunkBuffer is converted as one ByteString(data can be copied, or zero copied, depending on unsafe conversion is enabled or not). Multiple ByteString data for a readChunk request could be concatenated as one whole ByteString or a list of ByteString, depending on request options. The whole response with data is passed to the GPRC call which is async. I could be wrong, but I didn't find any GRPC notification or callback to tell a response has been sent out successfully so that the buffer associated with the response can be safely released, when last time I investigated if it's feasible to adopt the buffer pool in datanode for data read. When the response is finally sent out, the buffer will be auto released by GC some time later. GC will not call ChunkBuffer#close(), but it will call finalize(). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm trying to use WeakReference solution. Will update the patch later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please see LeakDetector (HDDS-9528) @ChenSammi . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GRPC is known to have some limits in the outbound:
This PR may be very out of scope (of this PR discussion), but GRPC is not the right tool for the job. And we will keep making unnatural efforts to cope with it (like we're doing). |
||
if (releaseCallback != null && buffer instanceof MappedByteBuffer) { | ||
releaseCallback.accept(1); | ||
} | ||
super.finalize(); | ||
} | ||
} |
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.
Nit: use
IntConsumer
instead ofConsumer<Integer>
.