Add support for resume state in sidelining #79

stanlemon · 2017-11-23T15:22:42Z

Summary

This PR adds a new state to the sideline sequence which we are calling resume.

Current States

Today sidelines are binary, they can be started or stopped. Starting a sideline means that a starting offset is captured for a future virtual spout and a filter is applied to the firehose virtual spout. This is typically done around the concept of a 'tenant', for example you might want to sideline account Foobar for a period of time. Stopping a sideline means that an ending offset is captured for a virtual spout that will be now created using the starting offset saved during the start of the sideline. That new virtual spout is immediately added to the coordinator with a filter that only emits the tenant in question and the filter is removed altogether from the firehose.

Potential Problems

The consequences of the binary approach are that ordering gets wildly thrown off and when a sideline stops we are essentially doubling a tenants emitted messages. This can be controlled with a different MessageBuffer, but the underlying problem remains: if a tenant's operations required sidelining we go from 0-60 (or 65) almost immediately without slowing down the emission of data into the main spout.

New States

Moving forward, and the goal of this PR is to split up the current stopping state into two different states:

Resume: This state will create a virtual spout using the starting offset from the starting state, and it will use a filter that only emits the given tenant. What's different here is that there is no ending state for the new virtual spout and the firehose still has its tenant filter.
Resolve: This state will mark the ending offset of the virtual spout for the sideline and remove the filter from the firehose. When the resolving state is complete (fully resolved) the virtual spout for the sideline should be gone and there should be no filters on the filter chain for the tenant in question.

Why does this matter?

The primary use we want to support here is throttling of traffic for a single tenant without having to know the ending point. So by using this new 'resume' state and a MessageBuffer that emits at a slower rate for sidelines, someone could slow the emission of a tenant into the underlying DynamicSpout, and then at the point at which they are ready to resume full processing they can set that sideline to 'resolve'.

It's worth noting here that by calling resume() and resolve() back to back you should have the functional equivalent of today's stop() and that nothing is changing with the current start() process.

In the Future

Related to this, my suspicion is that this implementation will also give us better mechanics to control the timing of resolving the sideline so that some day in the future we can try to target the sideline once it's near the tail of the firehose (probably using some tolerance level), but that's not the purpose of this PR just an additional thought on why this change makes sense.

Crim · 2017-11-24T00:52:23Z

src/main/java/com/salesforce/storm/spout/sideline/handler/SidelineController.java

+    void resume(final SidelineRequest sidelineRequest);
+
+    /**
+     * Does a sideline exist in the finished state?


I agree with peek, we should figure out a better name.

I’m open to suggestions.

Bounded Completed Something Or oTher.

I have no good suggestions :/

Crim · 2017-12-14T13:19:12Z

src/main/java/com/salesforce/storm/spout/dynamic/DynamicSpout.java

@@ -437,6 +437,11 @@ public boolean hasVirtualSpout(final VirtualSpoutIdentifier spoutIdentifier) {
        return getSpoutCoordinator().hasVirtualSpout(spoutIdentifier);
    }

+    public DelegateSpout getVirtualSpout(final VirtualSpoutIdentifier virtualSpoutIdentifier) {


Crim · 2017-12-14T13:20:02Z

src/main/java/com/salesforce/storm/spout/dynamic/VirtualSpout.java

@@ -638,7 +638,7 @@ private void attemptToComplete() {

        // If we made it all the way through the above loop, we completed!
        // Lets flip our flag to true.
-        logger.info("Looks like all partitions are complete!  Lets wrap this up.");
+        logger.info("Looks like all partitions are resolve!  Lets wrap this up.");


(wat) refactor did a string replacement on this one?

Crim · 2017-12-14T13:20:24Z

src/main/java/com/salesforce/storm/spout/dynamic/coordinator/SpoutCoordinator.java

@@ -208,6 +208,10 @@ public synchronized boolean hasVirtualSpout(final VirtualSpoutIdentifier virtual
        return runningSpouts.containsKey(virtualSpoutIdentifier);
    }

+    public synchronized DelegateSpout getVirtualSpout(final VirtualSpoutIdentifier virtualSpoutIdentifier) {


ideally add throws and annotation for it.

Crim · 2017-12-14T13:24:49Z

I know you have a doc somewhere, but it may be helpful to updatethe PR description with clear definitions of all the states and what they mean.

stanlemon · 2017-12-14T20:47:36Z

This PR will need a README overhaul, I'd like to work on that in a separate PR.

Crim · 2017-12-15T05:09:14Z

src/main/java/com/salesforce/storm/spout/dynamic/coordinator/SpoutCoordinator.java

+     * @return {@link DelegateSpout} instance
+     */
+    public synchronized DelegateSpout getVirtualSpout(final VirtualSpoutIdentifier virtualSpoutIdentifier) {
+        return runningSpouts.get(virtualSpoutIdentifier).getSpoutRunner().getSpout();


Probably need to handle the case where you ask for a non-existing virtual spout. Right now I'm guessing it would NPE.

You might be saved here by the synchronized method making this a moot point, but it may be safer checking the return result from runningSpouts.get() and if null throw the exception.

Crim · 2017-12-15T05:11:28Z

src/main/java/com/salesforce/storm/spout/sideline/handler/SidelineSpoutHandler.java

     */
-    public void startSidelining(SidelineRequest sidelineRequest) {
+    @Override
+    public void start(SidelineRequest sidelineRequest) {


does there need to be any validation around, what if you attempt to START a sideline thats already in RESUME, etc.. Basically validation that this state machine can't run backwards.

Crim · 2017-12-15T05:13:18Z

src/main/java/com/salesforce/storm/spout/sideline/handler/SidelineSpoutHandler.java

+    public void resolve(SidelineRequest sidelineRequest) {
+        final SidelineRequestIdentifier id = sidelineRequest.id;
+
+        logger.info("Received FINISH sideline request");


comment seems wrong, FiNISHED != resolve?

Crim · 2017-12-15T05:15:48Z

src/main/java/com/salesforce/storm/spout/sideline/trigger/SidelineType.java

@@ -31,5 +31,6 @@
 public enum SidelineType {

    START,


I'd add some quick descriptions here for what each of these mean

Crim · 2017-12-18T00:43:48Z

src/main/java/com/salesforce/storm/spout/dynamic/DynamicSpout.java

@@ -453,7 +453,7 @@ public boolean hasVirtualSpout(final VirtualSpoutIdentifier spoutIdentifier) {
     * @param virtualSpoutIdentifier identifier for teh {@link DelegateSpout} instance to get from the {@link SpoutCoordinator}.
     * @return {@link DelegateSpout} instance
     */


Needs javadoc @throws annotation

stanlemon · 2017-12-19T22:07:15Z

I want to add the validation mentioned by StevieP but I'd like to do it in a separate PR as part of the work on #75 as I think it'll be easier to address when that interface allows saving partitions together.

Crim · 2017-12-20T07:01:05Z

let er rip.

Crim reviewed Nov 24, 2017

View reviewed changes

salesforce deleted a comment from Crim Dec 5, 2017

stanlemon force-pushed the lemon/sideline-states branch 2 times, most recently from ad42c39 to 4850cb7 Compare December 7, 2017 20:14

stanlemon changed the title ~~WIP Add support for resume state in sidelining~~ Add support for resume state in sidelining Dec 7, 2017

stanlemon force-pushed the lemon/sideline-states branch from ffcfd1a to 9225402 Compare December 13, 2017 20:02

Crim reviewed Dec 14, 2017

View reviewed changes

stanlemon force-pushed the lemon/sideline-states branch from f646559 to f2bd47d Compare December 14, 2017 14:20

Split start stop state into resume/resolve

a62ccb9

stanlemon force-pushed the lemon/sideline-states branch from d5f4873 to a62ccb9 Compare December 14, 2017 15:26

stanlemon added 3 commits December 14, 2017 12:14

Add unit test to cover resume behavior

47c4489

Fix missing rename

9ccc09a

Checkstyle fix

5ce6913

Crim reviewed Dec 15, 2017

View reviewed changes

PR feedback

85e3974

Crim reviewed Dec 18, 2017

View reviewed changes

Crim approved these changes Dec 20, 2017

View reviewed changes

stanlemon merged commit 2dc9dbd into master Jan 17, 2018

stanlemon deleted the lemon/sideline-states branch January 17, 2018 19:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for resume state in sidelining #79

Add support for resume state in sidelining #79

stanlemon commented Nov 23, 2017 •

edited

Loading

Crim Nov 24, 2017

stanlemon Nov 24, 2017

Crim Nov 24, 2017

Crim Dec 14, 2017

Crim Dec 14, 2017

Crim Dec 14, 2017

Crim Dec 18, 2017

Crim commented Dec 14, 2017

stanlemon commented Dec 14, 2017

Crim Dec 15, 2017

Crim Dec 18, 2017

Crim Dec 15, 2017

Crim Dec 15, 2017

Crim Dec 15, 2017

Crim Dec 18, 2017

stanlemon commented Dec 19, 2017

Crim commented Dec 20, 2017

Add support for resume state in sidelining #79

Add support for resume state in sidelining #79

Conversation

stanlemon commented Nov 23, 2017 • edited Loading

Summary

Current States

Potential Problems

New States

Why does this matter?

In the Future

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crim commented Dec 14, 2017

stanlemon commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stanlemon commented Dec 19, 2017

Crim commented Dec 20, 2017

stanlemon commented Nov 23, 2017 •

edited

Loading