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

SidelineSpoutHandler::loadSidelines fails to relaunch the Firehose VirtualSpout #64

Closed Crim closed 6 years ago

Crim commented 6 years ago

This behavior is shown in test: SidelineSpoutHandlerTest::testLoadSidelines

More specifically in these code lines the test removes running VirtualSpouts. It then calls loadSidelines() which triggers these lines to fire:

SidelineSpoutHandler.java

        // After altering the filter chain is complete, lets NOW start the fire hose
        // This keeps a race condition where the fire hose could start consuming before filter chain
        // steps get added.
        if (!spout.hasVirtualSpout(fireHoseIdentifier)) {
            spout.addVirtualSpout(fireHoseSpout);
        }

What is happening here the Firehose VirtualSpout instance already exists, but its not "running" so it attempts to re-add it to the Coordinator. The coordinator fires it up, but when it calls open() on the VirtualSpout it explodes complaining that it's already been opened. You end up with exceptions like these:

15:03:31.841 ERROR c.s.s.s.d.c.SpoutRunner [[DynamicSpout:SpoutCoordinator] VirtualSpout Pool 3 on Mock:0 VirtualSpoutPrefix:main]: SpoutRunner for VirtualSpoutPrefix:main threw an exception Cannot call open more than once for VirtualSpoutId:VirtualSpoutPrefix:main
java.lang.IllegalStateException: Cannot call open more than once for VirtualSpoutId:VirtualSpoutPrefix:main
    at com.salesforce.storm.spout.dynamic.VirtualSpout.open(VirtualSpout.java:186) ~[classes/:?]
    at com.salesforce.storm.spout.dynamic.coordinator.SpoutRunner.run(SpoutRunner.java:115) [classes/:?]
    at java.util.concurrent.CompletableFuture$AsyncRun.run(CompletableFuture.java:1626) [?:1.8.0_144]

I'm unsure if this is a bug in SidelineSpoutHandler, in that it shouldn't try to re-start VirtualSpouts that are already opened, but instead re-create them? Or if its a bug in the test case scenario?

Crim commented 6 years ago

@stanlemon assigning this one to you, I'm not sure what the best approach here is, but this feels suspect in either assumptions SidelineSpoutHandler is making, or the test case is fubar.

Perhaps onSpoutClose() needs to null the firehose reference in SidelineSpoutHandler if its closed?

stanlemon commented 6 years ago

@Crim I think #73 resolves this. I actually think the test is sound, the state handling in the handler is just wonky.

stanlemon commented 6 years ago

Closing as resolved.