cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sylvain Lebresne (JIRA)" <>
Subject [jira] [Commented] (CASSANDRA-8457) nio MessagingService
Date Mon, 07 Nov 2016 12:06:59 GMT


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):
void enqueue(MessageOut msg, int id)
    QueuedMessage qm = new QueuedMessage(msg, id);
    if (state == State.READY)
        ChannelFuture connectionFuture = connect(); // <- connect would deal with concurrency.
        connectionFuture.addListener(f ->;
** 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|]
(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.
* 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:
>             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

View raw message