[
https://issues.apache.org/jira/browse/CASSANDRA-9590?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14716267#comment-14716267
]
Robert Stupp commented on CASSANDRA-9590:
-----------------------------------------
The code looks good so far and configuration and lifecycle look better now.
Thanks for the contribution!
* Unit tests - can you add unit tests for the new {{NativeTransportService}}
class that tests the various combinations (plain/encrypted/both) and lifecycle
(initialize/start/restart/stop/destroy) to ensure it doesn't break anything?
* Dtests - can you also add a dtest that tests the SSL upgrade scenario we
discussed?
* Can you rebase against current cassandra-3.0 branch?
* {{NativeTransportService}}
** License header is missing
** Nit: code formatting in {{NativeTransportService}}
** Nit: Like to replace the {{Callable}} code with a lambda in {{initialize()}}
(not with a stream) ?
** Lifecycle between {{initialize()}} and {{destroy()}} - the first is
synchroninzed and the latter is not. {{destroy()}} also doesn’t reset
{{initialized}} field.
** I’d prefer to have the {{servers}} field being set from {{initialize()}}
using {{Collections.singletonList()}}/{{Arrays.asList()}} - no strong objection
on this, though.
** {{destroy()}} introduces another 2 seconds of shutdown delay
({{eventExecutorGroup.shutdownGracefully();}}). The _reasonable default quiet
period_ (as Netty doc says) is 2 seconds quiet period plus 15 seconds timeout.
But otherwise a good choice to go away from the deprecated {{shutdown()}}
method. Mind to add @Deprecated to {{RequestThreadPoolExecutor.shutdown()}} ?
** {{destroy()}} now “await”s worker and event groups to shutdown. (You might
also use {{awaitUninterruptibly()}} as it is a Netty Future.) Isn’t this
(await) what {{shutdownGracefully()}} already does?
As soon as the unit tests and dtest are in place I’ll continue the review. You
may put a a self-signed cert/keystore for that into {{test/conf}} - maybe with
a short script how to generate the keystore from command line in the comment of
the unit test/dtest.
> Support for both encrypted and unencrypted native transport connections
> -----------------------------------------------------------------------
>
> Key: CASSANDRA-9590
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9590
> Project: Cassandra
> Issue Type: Improvement
> Components: Core
> Reporter: Stefan Podkowinski
> Fix For: 2.1.x
>
>
> Enabling encryption for native transport currently turns SSL exclusively on
> or off for the opened socket. Migrating from plain to encrypted requires to
> migrate all native clients as well and redeploy all of them at the same time
> after starting the SSL enabled Cassandra nodes.
> This patch would allow to start Cassandra with both an unencrypted and ssl
> enabled native port. Clients can connect to either, based whether they
> support ssl or not.
> This has been implemented by introducing a new {{native_transport_port_ssl}}
> config option.
> There would be three scenarios:
> * client encryption disabled: native_transport_port unencrypted, port_ssl not
> used
> * client encryption enabled, port_ssl not set: encrypted native_transport_port
> * client encryption enabled and port_ssl set: native_transport_port
> unencrypted, port_ssl encrypted
> This approach would keep configuration behavior fully backwards compatible.
> Patch proposal (tests will be added later in case people will speak out in
> favor for the patch):
> [Diff
> trunk|https://github.com/apache/cassandra/compare/trunk...spodkowinski:feat/optionalnativessl],
>
> [Patch against
> trunk|https://github.com/apache/cassandra/compare/trunk...spodkowinski:feat/optionalnativessl.patch]
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
