[
https://issues.apache.org/jira/browse/CASSANDRA-15170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16901150#comment-16901150
]
Benedict commented on CASSANDRA-15170:
--------------------------------------
Thanks, the patch looks good overall. I've reviewed the 4.0 branch for now.
{{StreamingInboundHandler}}: We probably shouldn’t assume we cannot leak these
objects, particularly just to enable tests - we should probably store only weak
references if retaining references in a {{static}} collection. It might be
easier overall to instead use a {{ThreadGroup}} here, and simply interrupt all
of the threads in the group. In fact, this might be a cleaner way to manage
all errant threads on a per-node basis.
{{InboundSockets}}: Probably not great to {{awaitTermination}} inside the
listener that may be executed on the {{GlobalEventExecutor}} - which is single
threaded, and whose shutdown order we cannot guarantee. Not actually sure the
best solution to this problem. Perhaps accept an {{executor}} parameter to the
{{close}} method, so that the invoker can consciously guarantee that the
executor can both handle committing a thread to this and also ensure the
executor is not shutdown before its completion. It would be nicer to return a
{{Future}} that logically wraps {{awaitTermination}} without committing a
{{Thread}}, but unfortunately there’s seemingly no clean way to do so; it
should be possible to implement a {{java.util.concurrent.Future}} with only
{{awaitTermination}}, but not an {{io.netty.util.concurrent.Future}}. We could
plausibly have the {{Future}} return a function that accepts a {{nanoTime}}
deadline that invokes {{awaitTermination}}, which might be cleaner, if still
far from optimal.
Some nits:
* SSLFactory
** unused imports (4.0)
** isOpenSslAvailable != openSslIsAvailable?
* {{Ref}}:
** Ignoring parameters (looks to be pre-existing, but propagated bug)
** Marginally cleaner to {{awaitTermination}} of both items together?
Particularly given parameter indicating timeout that we should honour.
* IsolatedExecutor
** unused imports
** unused {{ThreadFactory}} variable
> Reduce the time needed to release in-JVM dtest cluster resources after close
> ----------------------------------------------------------------------------
>
> Key: CASSANDRA-15170
> URL: https://issues.apache.org/jira/browse/CASSANDRA-15170
> Project: Cassandra
> Issue Type: Improvement
> Components: Test/dtest
> Reporter: Jon Meredith
> Assignee: Jon Meredith
> Priority: Normal
>
> There are a few issues that slow the in-JVM dtests from reclaiming metaspace
> once the cluster is closed.
> IsolatedExecutor issues the shutdown on a SingleExecutorThreadPool, sometimes
> this thread was still running 10s after the dtest cluster was closed.
> Instead, switch to a ThreadPoolExecutor with a core pool size of 0 so that
> the thread executing the class loader close executes sooner.
> If an OutboundTcpConnection is waiting to connect() and the endpoint is not
> answering, it has to wait for a timeout before it exits. Instead it should
> check the isShutdown flag and terminate early if shutdown has been requested.
> In 3.0 and above, HintsCatalog.load uses java.nio.Files.list outside of a
> try-with-resources construct and leaks a file handle for the directory. This
> doesn't matter for normal usage, it leaks a file handle for each dtest
> Instance created.
> On trunk, Netty global event executor threads are still running and delay GC
> for the instance class loader.
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
