Mailing-List: contact commits-help@cassandra.apache.org; run by ezmlm
Precedence: bulk
Date: Sat, 24 Jun 2017 22:03:00 +0000 (UTC)
From: "Jason Brown (JIRA)" <jira@apache.org>
To: commits@cassandra.apache.org
Message-ID: <JIRA.12760861.1418247068000.99208.1498341780672@Atlassian.JIRA>
In-Reply-To: <JIRA.12760861.1418247068000@Atlassian.JIRA>
References: <JIRA.12760861.1418247068000@Atlassian.JIRA> <JIRA.12760861.1418247068598@jira-lw-us.apache.org>
Subject: [jira] [Commented] (CASSANDRA-8457) nio MessagingService
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
archived-at: Sat, 24 Jun 2017 22:03:11 -0000


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

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

bq. Nit: there has been a few Netty minor released since the one in the branch, maybe worth upgrading (since we change it anyway).
there were a few unstable netty releases so I skipped those. 4.1.12 is good, but I need a fix in {{Lz4FrameEncoder}} which should be in 4.1.13.

bq. MessageOutHandler.AUTO_FLUSH_THRESHOLD feels like a magic constants. At least a comment on why it's a reasonable value would be nice.
I've added a comment to the constant, but the value was the value that should have been for the {{Lz4FrameEncoder}} (16k). The value from the [existing code|https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/net/OutboundTcpConnection.java#L85] is 64k, so I've reinstated that. Note that the previous {{OTC.BUFFER_SIZE}} can be seen also as a flush threshold, as well, as it is a [parameter to the output stream|https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/net/OutboundTcpConnection.java#L471]. Also discovered that {{OTC.BUFFER_SIZE}} is configurable, and made the new version configurable, as well.

bq. OutboundHandshakeHandler.WRITE_IDLE_MS: what's the rational behind 10 seconds? Not saying it feels wrong per-se, but wondering if there is more than a gut-feeling to it, and I'd kind of suggest exposing a system property to change that default.
I believe 10 seconds was a value [~aweisberg] and I came up with, but I've made it configurable now. 

bq. There is still a few TODO:JEB in the code: would be good to resolve/clear them if we're going to a more final branch.
Done, most of them I had already done on the CASSANDRA-12229 branch, but now backported to this one.

bq. Largely a nit, but its a tad confusing that OutboundConnectionParams contains a OutboundMessagingConnection. It feels like saying "The parameters you need for an outbound connection is ... an outbound connection". Maybe a simple renaming would make this clearer, though it feels maybe this could be cleanup up a bit further.
Hmm, yeah. I need the {{OutboundMessagingConnection}} for {{ChannelWriter#handleMessageFuture()}} so it can react to the outcome of attempting to send the message. wrt a cleanup, the least-worst idea I have is to move the error handling functionality of {{ChannelWriter#handleMessageFuture()}} to {{OutboundMessageConnection}}, as that's the only thing in {{ChannelWriter}} that requires {{OutboundMessageConnection}}. In {{OutboundConnectionParams}} we can pass a reference to an "error consumer" (not fully thought out but something like {{BiConsumer<QueuedMessage, Throwable>}}, that's a function on {{OutboundMessageConncetion}}), and {{ChannelWriter#handleMessageFuture()}} can invoke that instead of having a reference to a {{OutboundMessageConncetion}}. wdyt?

bq. The handling of "backlogged" messages and channel writability feels a bit complex. For instance, it looks like MessageOutHandler.channelWritabilityChanged can potentially silently drop a message every time it's called.
Good catch, fixed.

bq. The CI links listed with the branch a bunch of comments ago are completely out of dates. 
updated the ticket with a pointer to the circleci utests.

bq. Some basic benchmark results wouldn't hurt either ...
Attaching to this ticket, as it's a non-trivial martrix to test all the combinations of compression, coalescing, jdk-based TLS, and openssl-based TLS. 

bq. Maybe you could create a parent ticket of which this and CASSANDRA-12229 would be sub-tasks where we could focus the testing/benchmarking/final discussions on the whole thing?
done as CASSANDRA-13628


> 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.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org