gortiz commented on code in PR #11496:
URL: https://github.com/apache/pinot/pull/11496#discussion_r1320010451
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/DataTableHandler.java:
##########
@@ -83,5 +98,12 @@ protected void channelRead0(ChannelHandlerContext ctx,
ByteBuf msg) {
public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
LOGGER.error("Caught exception while handling response from server: {}",
_serverRoutingInstance, cause);
_brokerMetrics.addMeteredGlobalValue(BrokerMeter.RESPONSE_FETCH_EXCEPTIONS, 1);
+ if (cause instanceof java.lang.OutOfMemoryError) {
Review Comment:
> Re the Netty config option: I will test that proposal but I'm not sure
that it will prevent Netty from loading the entire message into Direct Memory.
We'll see with testing.
I'm almost sure it will. Netty does not instantiate a huge buffer by default
but small buffers that are being discarded once they are read. We are the ones
that ask to join all these buffers into a single one before we read it and we
explicitly say that it can grow until 2GBs.
By changing the parameter I referenced, we can fail fast and safe before
merging all the small buffers. In case I'm wrong and Netty actually keeps all
the small byte buffers (which I honestly won't believe), we could use a
ChunkedWriteHandler, which is an even lower level system provided by Netty to
consume bytes in a streaming fashion closer to the tpc protocol that is being
used under the hood.
> One thing is we don't have any message length field in our payload format
(which is unfortunate) which seems to be what this feature is built around.
Yes, we have to have it because we are actually using
`LengthFieldBasedFrameDecoder`, which just... reads it :laughing: . But the
ByteBuf generated by this decoder does not include the size because we actively
ask him to remove it (it is the last argument on the call I referenced in my
previous message).
> I still think we need explicit handling of DM OOMs in Netty
I'm fine with that, but as said earlier, please apply that catch in a
specific channel so we can keep the separation of concerns. The ability to
protect the system against direct OOM in Netty should not be in the handler
that transforms a ByteBuf into a DataTable.
BTW, at medium term it would be ideal to implement that transformation in a
streaming fashion. By doing that wouldn't need to create the large buffer in
the first term.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
