Add support for custom evictor and trigger policies #2653

dancollins34 · 2018-01-03T03:44:37Z

Hello,

Pursuant to my question in #2647 , I've added support for custom eviction and trigger policies. The modifications that I have made is to:

-Add the ability to add a custom EvictionPolicy and TriggerPolicy to a WindowingConfigs
-Add an if statement to check for the existence of custom policies in the WindowingConfigs passed to a WindowedBoltExecutor
-Add the ability to add a custom trigger or evictor policy to a BaseWindowedBolt
-Add support for a custom trigger and evictor policy WindowConfig and WindowConfigImpl in the streamlet api

Thoughts? This is my first time interacting with the underlying api, but I tried to keep the methods/implementation as similar as possible to what already existed.

Daniel

Add support in both the streamlet and topology apis for the usage of custom eviction policies extending com.twitter.heron.api.windowing.EvictionPolicy and trigger policies extending com.twitter.heron.api.windowing.TriggerPolicy to enable user-defined windowing schemes.

CLAassistant · 2018-01-03T03:44:43Z

All committers have signed the CLA.

srkukarni · 2018-01-03T03:46:42Z

@jerrypeng

dancollins34 · 2018-01-03T04:14:50Z

As an aside, the travis build appears to be failing due to style issues. I can make those changes, but I'm unsure if I'm allowed to sign the CLA, and therefore contribute, given my current terms of employment. I'll find that out tomorrow.

srkukarni · 2018-01-03T04:26:03Z

@dancollins34 I believe the reason why travis is failing is checkstyle issues.
If you are building on mac, please use
bazel build --config=darwin heron/...
this command should also throw the above check style issues

Fixed Checkstyle Issues

dancollins34 · 2018-01-03T04:55:45Z

Thank you for the advice. I've removed the style issues.

jerrypeng · 2018-01-03T07:19:46Z

heron/api/src/java/com/twitter/heron/api/windowing/WindowingConfigs.java

@@ -35,6 +35,9 @@
 import java.util.HashMap;
 import java.util.Map;

+import com.twitter.heron.api.tuple.Tuple;
+
+


nit. Please remove extra line

jerrypeng · 2018-01-03T07:20:06Z

heron/api/src/java/com/twitter/heron/api/bolt/WindowedBoltExecutor.java

+              evictionPolicy, topoConf);
+    }
+
+


Place remove extra line

kramasamy · 2018-01-03T07:36:44Z

It will be nice to provide some examples of how to use this feature.

…

On Tue, Jan 2, 2018 at 11:20 PM Boyang Jerry Peng ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In heron/api/src/java/com/twitter/heron/api/bolt/WindowedBoltExecutor.java <#2653 (comment)>: > + if (topoConf.containsKey(WindowingConfigs.TOPOLOGY_BOLTS_WINDOW_CUSTOM_EVICTOR)) { + evictionPolicy = (EvictionPolicy<Tuple, ?>) + topoConf.get(WindowingConfigs.TOPOLOGY_BOLTS_WINDOW_CUSTOM_EVICTOR); + } else { + evictionPolicy = getEvictionPolicy(windowLengthCount, windowLengthDurationMs); + } + + if (topoConf.containsKey(WindowingConfigs.TOPOLOGY_BOLTS_WINDOW_CUSTOM_TRIGGER)) { + triggerPolicy = (TriggerPolicy<Tuple, ?>) + topoConf.get(WindowingConfigs.TOPOLOGY_BOLTS_WINDOW_CUSTOM_TRIGGER); + } else { + triggerPolicy = getTriggerPolicy(slidingIntervalCount, slidingIntervalDurationMs, manager, + evictionPolicy, topoConf); + } + + Place remove extra line — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2653 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWcRGuVfwFLq2mjTCfxoTMhhmfWrcy3ks5tGyongaJpZM4RRUjm> .

jerrypeng · 2018-01-03T17:56:53Z

heron/api/src/java/com/twitter/heron/api/bolt/WindowedBoltExecutor.java

@@ -237,9 +237,23 @@ protected void validate(Map<String, Object> topoConf, Count windowLengthCount, L
    // validate
    validate(topoConf, windowLengthCount, windowLengthDurationMs, slidingIntervalCount,


If a custom trigger or evictor, validate should not be called. Arguments would be directly passed into the custom evictor and trigger when they are intialized

jerrypeng · 2018-01-03T18:10:21Z

Generally looks good, but I would like to bring a of point for discussion:

Even when using custom triggers and evictors, should users still be be required to use the existing API (e.g. setTopologyBoltsWindowLengthDurationMs(long value) in WindowingConfigs) to set window lengths and sliding intervals? or should such parameters be passed directly into the custom trigger and evictors objects e.g. passing these parameter via their respective constructors? I can see pros and cons in each approach

dancollins34 · 2018-01-05T00:01:22Z

I don’t think there’s any way you could prescribe the universe of all possible windowing parameters in that way. I think it makes a lot more sense to make the extent of the requirements to be “subclass triggerPolicy/evictionPolicy” instead of trying to restrict the subset of what each of those could be.

To your comment about validate, I think it might make sense to make this a 3 condition if statement. I.E. if eviction and trigger policy are defined in the config, use them. If neither eviction nor trigger policy are defined, run validate and run the code currently in the master branch, and if only one of them is defined, raise an exception.

I’ll fix the blank lines, make this above change and try to write some example code tonight.

…utor to require both Policies to be provided if one of them is

kramasamy · 2018-01-06T20:17:13Z

@jerrypeng - can you please re-review again?

dancollins34 · 2018-01-07T19:25:53Z

So, I've noticed something else about TriggerPolicies that I had missed on my first go round. There is no way for the API user to actually instantiate an instance of a custom TriggerPolicy to put it into the WindowConfig object because they would first need to have a reference to the WindowManager to act as a TriggerHandler. I'm writing a commit attempting to address this now.

…ush then pull clean copy)

…ndowManager, Config object, TriggerHandler and EvictionPolicy

kramasamy · 2018-01-08T08:11:53Z

@dancollins34 - some errors have been occurring in the unit tests. Can you please fix them?

dancollins34 · 2018-01-11T02:04:35Z

Built on my machine that’s weird. I’ll fix it tonight.

dancollins34 · 2018-01-11T02:09:58Z

Looks like it just timed out. Re running build

dancollins34 · 2018-01-11T07:15:50Z

I see the failures now. Attempting to address them now.

…ning "started" again for CountTriggerPolicy

dancollins34 · 2018-01-11T14:12:40Z

It looks like this one failed on a c++ test? I’m not sure how that’s possible I haven’t modified the c++ code in any way.

dancollins34 · 2018-01-12T02:28:37Z

Example usage

public class RandomTriggerPolicy<T extends Serializable> extends AbstractBaseTriggerPolicy<T, Boolean> {
    Float triggerChance;
    Random sampler;

    public RandomTriggerPolicy(Float triggerChance){
        super(false, false, false);

        if(triggerChance > 1.0 || triggerChance < 0.0){
            throw new IllegalArgumentException("Cannot have a likelihood outside of 0 to 1 range");
        }

        this.triggerChance = triggerChance;
        this.sampler = new Random();
    }

    @Override
    public void track(Event<T> event) {
        if (started && !event.isWatermark()) {
            if (sampler.nextDouble() < triggerChance){
                this.handler.onTrigger();
            }
        }
    }

    @Override
    public void reset() {
        // NOOP
    }

    @Override
    public void shutdown() {
        // NOOP
    }

    @Override
    public Boolean getState() {
        return false;
    }

    @Override
    public void restoreState(Boolean state) {
        // NOOP
    }
}

public class AllEvictionPolicy<T extends Serializable> implements EvictionPolicy<T, Boolean> {
    private EvictionContext context;

    @Override
    public Action evict(Event<T> event) {
        return Action.PROCESS;
    }

    @Override
    public void track(Event<T> event) {
        // NOOP
    }

    @Override
    public void setContext(EvictionContext context) {
        this.context = context;
    }

    @Override
    public EvictionContext getContext() {
        return context;
    }

    @Override
    public void reset() {
        // NOOP
    }

    @Override
    public Boolean getState() {
        return true;
    }

    @Override
    public void restoreState(Boolean state) {
        // NOOP
    }
}

public class Main{
    public static void main(String[] args){
        Builder builder = Builder.newBuilder();

        builder.newSource(() -> 1).reduceByKeyAndWindow(
            value -> value,
            value -> value,
            WindowConfig.CustomWindow(new RandomTriggerPolicy<>(.5f), new AllEvictionPolicy<>()),
            (x, y) -> x + y
        ).log();
    }
}

srkukarni · 2018-01-12T03:22:49Z

Not sure what the build failure was. Have kicked the build again.

kramasamy · 2018-01-13T16:55:09Z

@dancollins34 - can you add examples of how to use these under examples/ and storm-compatibility-examples? This will help developers give some code that they can understand and also quickly copy and modify if needed.

jerrypeng · 2018-01-14T20:30:05Z

heron/api/src/java/com/twitter/heron/api/windowing/TriggerPolicy.java

+   *
+   * @param windowManager the window manager
+   */
+  void setWindowManager(WindowManager<T> windowManager);


I think we should remove this from the interface. Having this interface is confusing to users since WindowManager implements the TriggerHandler interface. WindowManager is also not a interface that users can implement thus it shouldn't be part of an interface that users will have to implement. I think having "setTriggerHandler" should suffice. For build in triggers (e.g. WatermarkCountTriggerPolicy, WatermarkTimeTriggerPolicy), we can just pass in the WindowManager via their constructor.

The reasoning behind this was that the WindowManager, TriggerHandler (which is the WindowManager) and TopologyConfig are three things the users may not have access to directly. I was aiming to provide access to the same structures used in the internal TriggerPolicies to custom TriggerPolicies, should users wish to use them. The idea was that, if someone wanted to, it should be possible to use the existing trigger policies in this manner.

Looking more closely at the code I am confused how the a custom trigger policy will work. All the existing trigger policies require the WindowManager to actually trigger windows. How will a user implementing a custom TriggerPolicy do this. They have no reference to the WindowManager. Shouldn't the windowManager be passed into the custom trigger implicitly? As of right now, trigger polices can't trigger windows without the WindowManager. That is why I suggested removing the setWindowManager interface, since trigger policies need the window manager regardless of what kind of trigger policy it is.

Take a look at the changes i’ve made to windowedBoltExecutor. Essentially, in order to allow these to attach to arbitrary triggerPolicies, Ive replaced their inclusion in the constructor with the setters you see in the interface for all trigger policies.

jerrypeng · 2018-01-14T20:31:29Z

heron/api/src/java/com/twitter/heron/api/windowing/triggers/AbstractBaseTriggerPolicy.java

+import com.twitter.heron.api.windowing.WindowManager;
+
+
+public abstract class AbstractBaseTriggerPolicy<T extends Serializable, S>


Not sure if this interface is necessary, if we remove WindowManager from the TriggerPolicy interface

Regardless, you would still need a way to get the TriggerHandler (WindowPolicy) attached to the custom TriggerPolicy that was passed to the config. AbstractBaseTriggerPolicy was just made as a way to standardize how these attachments were handled for the internal classes, and provide a way to use those same attachment variables in custom classes.

jerrypeng · 2018-01-14T20:32:06Z

@dancollins34 thanks for your work! Generally looks good. I just have couple of comments for you.

dancollins34 · 2018-01-15T16:39:25Z

Will address @kramasamy comments and add to /examples following resolution of concerns raised by @jerrypeng

kramasamy · 2018-01-15T16:56:34Z

@dancollins34 - it will be nice if you could add some documentation on how to use this in a separate PR.

jerrypeng · 2018-01-15T23:01:00Z

@dancollins34 thanks for replying to my comments. I had a question regarding the example you provided above for a custom trigger. The TriggerHandler is never set but you call the handler in the method track: "this.handler.onTrigger();" How is that going to work? or am I missing something?

dancollins34 · 2018-01-16T14:22:42Z

Yes it is- in the windowedBoltExecutor. Take a look at those changes.

dancollins34 · 2018-01-16T14:22:55Z

@jerrypeng

jerrypeng · 2018-01-16T18:43:52Z

heron/api/src/java/com/twitter/heron/api/windowing/triggers/AbstractBaseTriggerPolicy.java

+  /**
+  * Set the requirements in the constructor
+  */
+  public AbstractBaseTriggerPolicy(boolean requiresEvictionPolicy,


Not sure if we need these checks. I think it should be up to the user to make sure to pass in the correct objects he or she needs for a custom trigger. Having to pass these booleans into the constructor also makes the interface confusing to some reading to code. If you really want these checks in place, I would suggest using setters to set these. Then it is clear to the reader of the code what is being set.

@jerrypeng These checks were in place to keep the existing trigger policies as restrictive as they were before wrt which of the objects (evictionPolicy, windowManager) were attached. I could provide an alternate no-arg constructor that sets them all to false, then use setters instead? However, since this is the abstract class which already partially implements the TriggerPolicy interface, I would think it would be okay to perform some checks, but I have no issue with removing them either.

lets remove them to keep the interface as simple and clean as possible

@jerrypeng will do

jerrypeng · 2018-01-16T18:45:29Z

@dancollins34 thanks for the clarification. I am +1 on this PR

kramasamy · 2018-01-17T19:45:55Z

@jerrypeng - Can I merge this?

jerrypeng · 2018-01-17T19:53:07Z

@kramasamy we can once the last comment is addressed

jerrypeng · 2018-01-18T05:35:37Z

+1

@kramasamy we can merge

* Add support for custom evictor and trigger policies Add support in both the streamlet and topology apis for the usage of custom eviction policies extending com.twitter.heron.api.windowing.EvictionPolicy and trigger policies extending com.twitter.heron.api.windowing.TriggerPolicy to enable user-defined windowing schemes. * Fixed Checkstyle Issues Fixed Checkstyle Issues * Fixed extra line errors, reorganized if statement in WIndowedBoltExecutor to require both Policies to be provided if one of them is * Reversed policy order to fix null pointer exception * Trial push (local compilation not working for some reason, going to push then pull clean copy) * Added Changes necessary to allow custom trigger policies to attach WindowManager, Config object, TriggerHandler and EvictionPolicy * Fixed test errors due to lack of setting necessary paramters and defining "started" again for CountTriggerPolicy * Remove flags and checks on AbstractBaseTriggerPolicy

Fixed Checkstyle Issues

44da17b

Fixed Checkstyle Issues

jerrypeng reviewed Jan 3, 2018

View reviewed changes

dancollins34 added 2 commits January 4, 2018 22:18

Fixed extra line errors, reorganized if statement in WIndowedBoltExec…

ef1357a

…utor to require both Policies to be provided if one of them is

Reversed policy order to fix null pointer exception

3184617

dancollins34 added 2 commits January 7, 2018 17:22

Trial push (local compilation not working for some reason, going to p…

6a50ee7

…ush then pull clean copy)

Added Changes necessary to allow custom trigger policies to attach Wi…

1ef4279

…ndowManager, Config object, TriggerHandler and EvictionPolicy

Fixed test errors due to lack of setting necessary paramters and defi…

5431c76

…ning "started" again for CountTriggerPolicy

jerrypeng reviewed Jan 14, 2018

View reviewed changes

jerrypeng reviewed Jan 16, 2018

View reviewed changes

Remove flags and checks on AbstractBaseTriggerPolicy

0098110

srkukarni merged commit 2d86042 into apache:master Jan 18, 2018

		@@ -237,9 +237,23 @@ protected void validate(Map<String, Object> topoConf, Count windowLengthCount, L
		// validate
		validate(topoConf, windowLengthCount, windowLengthDurationMs, slidingIntervalCount,

		import com.twitter.heron.api.windowing.WindowManager;


		public abstract class AbstractBaseTriggerPolicy<T extends Serializable, S>

Add support for custom evictor and trigger policies #2653

Add support for custom evictor and trigger policies #2653

Conversation

dancollins34 commented Jan 3, 2018

CLAassistant commented Jan 3, 2018 • edited

srkukarni commented Jan 3, 2018

dancollins34 commented Jan 3, 2018

srkukarni commented Jan 3, 2018

dancollins34 commented Jan 3, 2018

jerrypeng Jan 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kramasamy commented Jan 3, 2018 via email

Choose a reason for hiding this comment

jerrypeng commented Jan 3, 2018

dancollins34 commented Jan 5, 2018

kramasamy commented Jan 6, 2018

dancollins34 commented Jan 7, 2018 • edited

kramasamy commented Jan 8, 2018

dancollins34 commented Jan 11, 2018

dancollins34 commented Jan 11, 2018

dancollins34 commented Jan 11, 2018 • edited

dancollins34 commented Jan 11, 2018

dancollins34 commented Jan 12, 2018

srkukarni commented Jan 12, 2018

kramasamy commented Jan 13, 2018 • edited

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

jerrypeng commented Jan 14, 2018 • edited

dancollins34 commented Jan 15, 2018

kramasamy commented Jan 15, 2018

jerrypeng commented Jan 15, 2018

dancollins34 commented Jan 16, 2018

dancollins34 commented Jan 16, 2018

Choose a reason for hiding this comment

dancollins34 Jan 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerrypeng commented Jan 16, 2018

kramasamy commented Jan 17, 2018

jerrypeng commented Jan 17, 2018

jerrypeng commented Jan 18, 2018

CLAassistant commented Jan 3, 2018 •

edited

jerrypeng Jan 3, 2018 •

edited

dancollins34 commented Jan 7, 2018 •

edited

dancollins34 commented Jan 11, 2018 •

edited

kramasamy commented Jan 13, 2018 •

edited

jerrypeng commented Jan 14, 2018 •

edited

dancollins34 Jan 17, 2018 •

edited