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

["Jason Brown (JIRA)" <ji...@apache.org>]

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

Jason Brown commented on CASSANDRA-8457:
----------------------------------------

Ok, [~slebresne], next round of review is ready :) I'm currently ferreting out a regression with {{TriggersTest}}, but that shouldn't hold up things any further.

bq. {{MessageOutHandler.SwappingByteBufDataOutputStreamPlus}} - I'm wondering if it's not premature optimization

Yeah, you may be correct here. This is one of the first parts I wrote, and clearly I was worried about the "pack multiple messages into a single flush" issue. It's possible that a simpler implementation, each message gets own buffer, might just be best. 

However, one counter argument here could be that when we have a very large message to serialize, think tens of MBs, we would allocate that one large buffer, wait to serialize data into it, and then send/flush it. {{OutboundTcpConnection}} and my current branch will buffer a certain subset of that data (say 64k) and send/flush that, rather than wait for it all to be serialized. I'm not sure if that's compelling enough at this time, or if we consider that followup ticket/work. Either way, I've removed {{MessageOutHandler.SwappingByteBufDataOutputStreamPlus}}.

bq. OMC.enqueue and background queue
Unfortunately, there's a few things that get in our way here of making it that simple, assuming we don't want to block threads. The biggest is that we don't want to write general messages to the channel until the internode messaging handshake completes successfully as it's a three-way handshake and if we start sending general messages in the middle of the handshake, it'll break the handshake. Further, it's not so clean or obvious how to get the ChannelFuture instance without *some* degree of blocking (some of which we might be able to avoid with some clever tricks @normanm showed me). Ultimately, we need a singular {{Channel}} instance, which would require blocking to get only one instance.

Another reason why I opted for the backlog queue was to allow as close to an ordered sending of the messages as possible, to a given peer: in other words, to write to the channel as many backlogged messages as possible before sending the new message. For the vast majority of cassandra's functionality, ordering of messages is irrelevant. However, I'm wondering how sensitive repair and cluster membership changes might be to this potential edge-case of message reordering.

bq. I'd expect {{enqueue()}} (which might warrant a rename) to just do a {{ctx.write()}}

Makes sense, done.

That being said, I've left the code in newly-named {{#sendMessage}} and {{#finishHandshake}} a bit tentative, with a note that you and I should hammer this part out further :)

bq. ... flushing ...

I really like what you've done wrt flushing in {{FlushingHandler}} and {{CoalesingStrategies}}. I think as I was trying to maintain both blocking and non-blocking behaviors in {{CoalesingStrategies}}, that's why that code got twisted and complex. Thus, I've eliminated {{CoalescingMessageOutHandler}} and brought in your {{FlushingHandler}} and {{CoalesingStrategies}} changes. One thing to point out is that when scheduling a task to a netty executor thread, it will execute on the same IO thread as the current context. Thus, in {{FlushingHandler}} you don't have to worry about any concurrency between a new message arrival and the execution of the task, as they execute on the same thread. I've removed your (well-documented!) javadoc comments and the volatile keyword on the {{scheduledFlush}} member field.

I agree we can address "targeted benchmarking ... in a separate ticket", as I think getting the essential behavior of flush and coalesce is most important here.

bq. Regarding dropped messages, the current implementation was going through the {{MessagingService.incrementDroppedMessages()}} ...

Hmm, {{OutboundTcpConnection}} has it's own [counter for connection-specific dropped messages|https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/net/OutboundTcpConnection.java#L136], which I've maintained in {{OutboundMessagingConnection}}. It seems like {{OutboundTcpConnection}} should probably be reporting it's dropped messages to {{MessagingService.incrementDroppedMessages()}} in addition to what it currently does. If you think that's legit, I'll fix it in this patch and I'll open a separate ticket to fix it in 3.0+. wdyt?

bq. InboundHandshakeHandler#handshakeHandlerChannelHandlerName

Removed this. I think I needed it with the earlier version of netty? Either way, killed it.

bq. In {{OutboundMessagingConnection#writeBacklogToChannel}}, we seem to be sending timed out messages if those are retried

This behavior was changed in CASSANDRA-12192, so I've retained it here.

bq. {{MessagingService.MessageSender}}

Removed

bq. InboundHandshakeHandler#setupMessagingPipeline

Luckily the comment is not relevant anymore; too bad I didn't fix that up when I fixed the code :D 

bq. pull out the fix to {{AnticompactionRequest}} into a separate ticket

Done in CASSANDRA-12934.

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