[jira] [Commented] (CASSANDRA-8457) nio MessagingService

["Sylvain Lebresne (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CASSANDRA-8457?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15643999#comment-15643999 ] 

Sylvain Lebresne commented on CASSANDRA-8457:
---------------------------------------------

Alright, here's a new round of reviews:
* On {{MessageOutHandler}}:
** I'm a bit unclear on the benefits of the {{SwappingByteBufDataOutputStreamPlus}} (whose name I'd btw probably shorten given it's private), compared to the simple solution of just allocating a {{ByteBuf}} per-message. My analysis is this: if we have no coalescing (which may end up being the default back in 4.0, CASSANDRA-12676), then for any message smaller than {{bufferCapacity}}, we end up allocating more than needed since we flush for each message, so it's less optimal. For larger messages, we end up allocating multiple small buffers (instead of a larger), which may be better but not sure how much. With coalescing, I guess we do end up potentially packing multiple messages into a {{ByteBuf}}, but does that make such a big difference (I mean, even if we were to have one {{ByteBuf}} per message, we would still not flush on every message, which feel the most important)? So to sum up, I can buy that it's slightly better in some cases, but it seems also slightly less optimal in other and it's unclear how much it helps when it does, so in the absence of any benchmarking, I'm wondering if it's not premature optimization. Shouldn't we leave that to later and keep it simpler?
** If we do keep {{SwappingByteBufDataOutputStreamPlus}}, I'd at least inline {{WriteState}}. It creates a new indirection which doesn't seem to have much benefit, and I admit the naming confused me (a "state" is not the first place I looked to find the buffer).
* In {{OutboundMessagingConnection}}:
** In {{writeBacklogToChannel}}, we seem to be sending timeouted messages if those are retried: I don't think we want to do that (we should always drop a timeouted message).
** I'm not a fan of the use of the {{backlog}} queue for sending message. I'm no Netty expert but that doesn't seem to fit Netty's "spirit". Once we're in a stable state (the connection is created), I'd expect {{enqueue()}} (which might warrant a rename) to just do a {{ctx.write()}} so it's confusing to me it doesn't. We do need to handle connection creation and retries, but it feels we can handle that easily enough through the future on channel creation. To clarify, I'd expect something along the lines of (it's simplified, just to clarify what I have in mind):
   {noformat}
void enqueue(MessageOut msg, int id)
{
    QueuedMessage qm = new QueuedMessage(msg, id);
    if (state == State.READY)
    {
        channel.write(qm);
    }
    else
    {
        ChannelFuture connectionFuture = connect(); // <- connect would deal with concurrency.
        connectionFuture.addListener(f -> f.channel().write(qm));
    }
}
{noformat}
** I'm a bit unclear on the {{flush()}} strategy. The code seems to flush basically on every message (ok, once per emptying of the queue, but if we ignore connection creation, this can be very well on every message or very close to it), but this seems to defeat the coalescing handler since that handler will flush on flush (does that make sense?). I could be getting this wrong, but if I don't, it seems we shouldn't flush in {{OutboundMessagingConnection}} but delegate that task to the coalescing handler. But see next point for a more general remark on flush.
* In general, there is 2 important operations that I think could be better extracted (so it's cleaner when those happen): flushes and timeouting messages. Here my thinking:
** Flushes: it's my understanding that when you flush is pretty important to performance. So it would be great if we could centralize where we flush if at all possible. In particular, and as said above, I feel we should not flush in {{OutboundMessagingConnection}}. What I would do however is add a {{FlushingHandler}} for that task. That handler would mostly be the {{CoalescingMessageOutHandler}} renamed, but the point of the renaming is to make it clear that it's where the decision to flush happens.
** Timeouts: the patch currently look for timeouted message in different places and it would be great to consolidate that. In general, I think it's enough to check if a message is timeouted once in the pipeline: messages will rarely timeout on the sending time by design and if they do, it's likely because the channel is overwhelmed and the message has sit on the Netty event executor queue too long, and I believe we'd catch well enough with a simple {{MessageTimeoutingHandler}}.
* Regarding dropped messages, the current implementation was going through the {{MessagingService.incrementDroppedMessages()}} methods, which is still used by other parts of the code and is distinguishing between cross-node and local messages, but the new code seems to have its own separate count in {{OutboundMessagingConnection}}, which looks wrong.
* I'm currently unconfortable with coalescing. The code doesn't use the coalescing strategy exactly as they were intented and used before (passing only 1 message at a time in particular) and I'm not entirely sure how important that is. Ideally I'd want to do something relatively simple along the line of [this|https://github.com/pcmanus/cassandra/commit/f38fc8abf0299c5dad3f98e5303086017be28bf3] (which does my "rename to FlushHandler" idea from above), which 1) remove and clean up the blocking parts of {{CoalescingStrategies}} since we don't need them and 2) remove the {{PendingWriteQueue}} (I'm not entirely sure how it helps; it allowed to check for timeouts before the flush, but given that coalescing should be done at a much finer granularity than timeouting, I don't think it's worth the complexity). But as said, this does change a bit when coalescing is applied and I don't know if it matters. It feels we'd need some targeted benchmarking, but that's getting to a point where I feel we should discuss this more specifically on a specific ticket. Wdyt?
* In {{MessagingService}}, the {{MessageSender}} naming makes {{messageSender.listen()}} weird (it's receiving, not sending :)). Maybe something more neutral like {{MessageTransport}}? Alternatively, we don't really need that abstraction anymore so get rid of it.
* Could you pull out the fix to {{AnticompactionRequest}} into a separate ticket: this doesn't belong here and is kind of a bug that should be fixed before 4.0 (even though that may well have no real consequences, haven't checked, it's still pretty fishy).
* In {{InboundHandshakeHandler}}:
** I'm not sure I understand the comment on {{setupMessagingPipeline}} and I'm not a fan of {{handshakeHandlerChannelHandlerName}} in general. Testing is important, but it scares me when it feels we're adding subtlety in the code for it.
** Still think {{handshakeResponse}} is mismaned :). It's a future on the timeout, not on the response itself, so I'd just call it {{handshakeTimeout}}. Related, {{handshakeTimeout()}} should really be called {{failHandshake()}} since it's not only called on timeouts.
* In {{MessageInProcessingHandler}}:
** would be nice to have a comment on why we special case {{DecoderException}} (where this can be thrown and why we want to decapsulate). That's the kind of things that can be annoying to track down when you haven't written the code.
** Are we sure we want to log at INFO in {{channelInactive()}}?
* In {{OutboundHandshakeHandler}}:
** In {{decode}}, there is 2 commented line (2 {{close()}}), which looks like "todos".
** I'm not 100% sure to understand how {{FlushConsolidationHandler}} works but it seems it can impact latency and we already have coalescing, so I wonder if hard-coding it isn't premature optimization. Shouldn't we evaluate it's impact in a followup ticket instead, maybe dealing with how we expose it's configuration, rather then shove everything in this ticket?
* In {{NettyFactory}}:
** I believe the {{Mode}} value inside the {{Mode}} enum is a typo and should be removed.
** {{createInboundChannel}} logs twice at INFO (the same thing roughly).
** Nit: In {{createInboundChannel}}, the line {{Throwable failedChannelCause ...}} is badly indented. Would also use brackets on the following if-then (you only have one statement each time, but they are split on multiple line and I find it less readable; very much a nit).
* Would be nice to start clearing some of the TODOs in general.

I'll close by saying that while I the suggestions and remarks above, the overall organization of the new code look good to me. So once what's above is addressed, I feel the main "blockers" become testing and benchmarking (the latter being kind of particularly important on this ticket), and I believe the missing streaming parts are blocking that.


> nio MessagingService
> --------------------
>
>                 Key: CASSANDRA-8457
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-8457
>             Project: Cassandra
>          Issue Type: New Feature
>            Reporter: Jonathan Ellis
>            Assignee: Jason Brown
>            Priority: Minor
>              Labels: netty, performance
>             Fix For: 4.x
>
>
> Thread-per-peer (actually two each incoming and outbound) is a big contributor to context switching, especially for larger clusters.  Let's look at switching to nio, possibly via Netty.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)