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

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

Reply via email to