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

Dropping requestStop() from the DelegateSpout #68

Closed stanlemon closed 6 years ago

stanlemon commented 6 years ago

I would contend that the tiers of SpoutRunner -> VirtualSpout around isStopRequested() are a bit of a smell. I think what we were doing in VirtualSpout was actually the isCompleted() method, but we hadn't exposed it. Basically, we wanted a way for the VirtualSpout to indicate it was done processing and that it could be automatically removed from the coordinator.

Additionally, I think the addition of removeVirtualSpout() elsewhere handles the scenario of "kill this VirtualSpout early" and is a much cleaner interface for doing so.

stanlemon commented 6 years ago

@Crim This was what I was trying to articulate the other night. I'd love some feedback on this one.

Crim commented 6 years ago

Fixed a couple things. Made a note of a place that I think needs additional test coverage about knowing when to complete a VSpout.

Overall I think this looks good. I believe we used to use isCompleted on VirtualSpout to determine if we could cleanup the persisted state. From poking around it looks like we no longer do that? Probably un-related question, but how/where/when do we cleanup state after a sideline is complete? Both consumer state and sideline state.

Crim commented 6 years ago

Ah this looks like the place we do that.

https://github.com/salesforce/storm-dynamic-spout/blob/lemon/request-stop/src/main/java/com/salesforce/storm/spout/dynamic/VirtualSpout.java#L227-L237

stanlemon commented 6 years ago

Correct @Crim and then when we cleanup sideline state we let the handler do it here https://github.com/salesforce/storm-dynamic-spout/blob/lemon/request-stop/src/main/java/com/salesforce/storm/spout/dynamic/VirtualSpout.java#L242

Apologies it's not clear to me, did you add the additional test coverage you wanted or am I misunderstanding?

stanlemon commented 6 years ago

@Crim ping

Crim commented 6 years ago

I did not add test coverage in think I left a comment somewhere about it tho

stanlemon commented 6 years ago

@Crim I backed out the conversion of the primitives to objects, which was the area you asked for an additional test case. I can no longer reproduce whatever issue I was having and don't see any reason to change it. I've also rebased this branch off of master. Can you review this again so we can get it merged in?