[
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)
