search

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

Coordinator Refactor #55

Closed Crim closed 6 years ago

Crim commented 6 years ago

This is the follow on to #53 and while functionally "complete", should be considered a work in progress. The way the PR is structured, it includes the change set from #53 which makes this look more complex than it really is.

Since #53 made SpoutCoordinator essentially pass-through class to SpoutMonitor, this PR attempts to merge the two. Because most of the logic we wanted to keep lived in SpoutMonitor, I choose to remove SpoutCoordinator. We should consider renaming SpoutMonitor to something else, either SpoutCoordinator, SpoutLauncher, VirtualSpoutMonitor, etc.. as well as rename its package namespace.

So what changed?

What was left in SpoutCoordinator

The core logic left in SpoutCoordinator revolved around creating a FixedSingleThreadPool ExecutorService. Using this service we submitted SpoutMonitor to start running within it. If SpoutMonitor ever threw an exception, SpoutCoordinator would simply log an error and re-submit it. Every other method simply delegated/passed thru to SpoutMonitor.

What changed in SpoutMonitor

With removing SpoutCoordinator, I simply re-arranged SpoutMonitor such that it will now catch and log/report any exceptions and just continue to run. Previously SpoutCoordinator was a watch dog for this process, and with it gone we want to prevent this thread from dying. The end result should be identical as before.

What changed in DynamicSpout

SpoutMonitor is now created/started/closed directly from DynamicSpout. Calling addVirtualSpout/removeVirtualSpout/hasVirtualSpout now passes directly through to SpoutMonitor.

Note It may be that instead of passing the entire Spout to Spout handlers for dealing with adding/removing VirtualSpouts, we pass SpoutMonitor instead? If we did that we can remove the add/remove/has methods from DynamicSpout which would be nice. Unfortunately I'm not super up to speed on how the handlers work to say if this really makes sense or not, but wanted to throw it out there.

Crim commented 6 years ago

I think I broke something in this branch/PR. Travis keeps failing on a compilation error, however locally I have no such issues.