salesforce / storm-dynamic-spout

A framework for building spouts for Apache Storm and a Kafka based spout for dynamically skipping messages to be processed later.
BSD 3-Clause "New" or "Revised" License
40 stars 13 forks source link

Add missing close implementation, better naming of threads #63

Closed Crim closed 6 years ago

Crim commented 6 years ago

This PR deals with mysterious orphaned Curator threads within tests.

Crim commented 6 years ago

@stanlemon take a look see and merge if you see fit. I think the biggest problem was VirtualSidelineSpoutHandler was using the default close() implementation, and thus leaving Curator instances hanging around.

Side note around CI test times:

Just as an FYI, the number of Curator instances is just about :todamnhigh: For the simple case of just running the Firehose, we now have a minimum of 4 Curator instances running, each spawning several threads themselves.

1 - SidelineSpoutHandler 2 - SidelineSpoutHandler -> ZookeeperTriggerWatcher 3 - Firehose VirtualSpout 4 - SidelineVirtualSpoutHandler

Do you forsee any way we can share instances between the sideline handlers at all? By design curator is threadsafe, and its intended to be shared across the JVM per cluster.