Closed stanlemon closed 6 years ago
@Crim Thinking this through... we probably need to cleanup the state in Zookeeper, otherwise we will attempt to re-start and re-stop sidelines on every load. That shouldn't yield in duplicate sideline spouts, but I'm not sure we want to do that. Maybe it's OK. At the very least we should probably delete the records once the STOP requests have been processed. What do you think?
Makes sense to delete, or move them to a completed subnode or something.
Would now be a good time to move this bit into a separate submodule? sideline-zookeeper-trigger or something?
Somewhat related, indirectly to this pr, I noticed that ZookeeperPersistenceAdapter implements Serializable, but has non-serializable class property CuratorHelper. I think we need to make sure CuratorHelper is serializable.
This applies to both the Sideline and dynamic spout instance.
I'm of the opinion we should remove the Serializable interface from the ZookeeperPersistenceAdapter. We don't actually need it, I think it snuck on over time.
Regarding deletions, the more I think about this the more I'm inclined to add an isProcessed
boolean to the TriggerEvent
and flip it after the sideline has been started/stopped, and leave "cleanup" to the implementer. I've noticed that today when we use this and we do STOP requests you can't see them from the UI and that's not ideal.
Sounds good. Drop the Serializable marker from those classes and maybe the GSON thing and this should be good to go.
@Crim Updated with PR feedback, and I went ahead and added the 'processed' bit to the event. I think this is ready, let me know.
Looks good, made a few comments for your consideration, but feel free to roll w/ what you got.
Addresses #46