STORM-2497: Let Supervisor enforce memory and add in support for shared memory regions #2113

revans2 · 2017-05-12T18:58:23Z

This is based off of #2112 so that the formatting is simpler.

jerrypeng · 2017-05-29T18:11:27Z

cool feature

kishorvpatil

👍 LGTM.

revans2 · 2017-06-02T18:27:24Z

@kishorvpatil Could you look again. I had to rebase becase of a minor conflict in the DefaultResourceAwareStrategy. It ended up resulting in only some comment changes to this code, but I wanted to be sure you took a look.

srdo

This looks good as far as I can tell. It was pretty big for 1 commit though :)

srdo · 2017-06-06T15:49:38Z

conf/defaults.yaml

+storm.cgroup.memory.limit.tolerance.margin.mb: 0.0
+storm.supervisor.memory.limit.tolerance.margin.mb: 128.0
+storm.supervisor.hard.memory.limit.multiplier: 2.0
+storm.supervisor.hard.memory.limit.overage: 2024


Nitpick: Some of these properties are missing the unit name

srdo · 2017-06-06T16:04:39Z

docs/Resource_Aware_Scheduler_overview.md

+### Shared Memory
+
+In some cases you may have memory that is shared between components. It may be a cache shared within a worker between instances of a bolt, or it might be static data that is memory mapped into a bolt and is shared accross a worker.  In any case you can specify your share memory request by
+creating one of `SharedOffHeapWithinNode`, `SharedOffHeapWithinWorker`, or `SharedOnHeap` and adding it to bolts and spouts that use that shared memory.


Is there a difference between SharedOnHeap and just declaring a static field?

srdo · 2017-06-06T16:12:55Z

examples/storm-starter/src/jvm/org/apache/storm/starter/ResourceAwareExampleTopology.java

+        // The size of a worker is limited by the amount of heap assigned to it and can be overridden by
+        conf.setTopologyWorkerMaxHeapSize(1024.0);
+        // This is to try and balance the time needed to devote to GC against not needing to
+        // serialize/deserialize tuples


Not sure I understand this. Are more executors ending up in a worker if the max heap size is larger, or how does this work?

srdo · 2017-06-06T16:13:44Z

examples/storm-starter/src/jvm/org/apache/storm/starter/ResourceAwareExampleTopology.java

+        if (args != null && args.length > 0) {
+            topoName = args[0];
+        }
+        //Not needed on RAS


This was already mentioned

srdo · 2017-06-06T16:14:10Z

storm-client/src/jvm/org/apache/storm/Config.java

    @NotNull
-    @isImplementationOfClass(implementsClass = IStrategy.class)
+    @isString
+    //TODO @isImplementationOfClass(implementsClass = IStrategy.class)


srdo · 2017-06-06T19:15:40Z

storm-server/src/main/java/org/apache/storm/daemon/supervisor/Container.java

+    /**
+     * Get the topology id this is a part of.
+     */
+    public String getTopoogyId() {


topoogy -> topology

srdo · 2017-06-06T19:24:34Z

storm-server/src/test/java/org/apache/storm/daemon/nimbus/NimbusTest.java

+import org.junit.Test;
+
+public class NimbusTest {
+    @Test(expected=IllegalArgumentException.class)


Nit: Could reduce the scope of this assertion more local with the ExpectedException rule

srdo · 2017-06-06T19:25:28Z

storm-server/src/test/java/org/apache/storm/scheduler/resource/TestResourceAwareScheduler.java

-        defaultTopologyConf.put(Config.TOPOLOGY_COMPONENT_CPU_PCORE_PERCENT, 10.0);
-        defaultTopologyConf.put(Config.TOPOLOGY_COMPONENT_RESOURCES_ONHEAP_MEMORY_MB, 128.0);
-        defaultTopologyConf.put(Config.TOPOLOGY_COMPONENT_RESOURCES_OFFHEAP_MEMORY_MB, 0.0);
+      //TODO clean this up some more


srdo · 2017-06-06T19:33:17Z

storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java

+                    Map<String, Object> topoConf = ConfigUtils.readSupervisorStormConf(_conf, _topologyId);
+                    _fsOps.setupStormCodeDir(topoConf, _stormRoot);
+                    if (_assignment.is_has_node_shared_memory()) {
+                        File sharedMemoryDir = new File(_stormRoot, "shared_by_topology");


I might have missed it somewhere, but how does the topology code access the off heap shared memory?

That is up to the topology. It could be JNI code or even something that a ShellBolt or ShellSpout does. The point is to provide a place for them to store the data if they need it. We have a group doing this in /tmp right now and causing all kinds of issues if things crash/get rescheduled leaking things.

Okay. I was just thinking it might be helpful if the topology could get the directory path (or _stormRoot), so users don't have to figure out how to path to this directory themselves.

I can add it to the documentation. But I didn't spend a lot of time documenting it on purpose. shared memory off heap is hard to get right and I would rather discourage people from using it.

srdo · 2017-06-06T19:37:12Z

storm-server/src/main/java/org/apache/storm/container/cgroup/CgroupManager.java

+                    (int)
+                        (Math.ceil(
+                            ObjectReader.getDouble(
+                                this.conf.get(DaemonConfig.STORM_CGROUP_MEMORY_LIMIT_TOLERANCE_MARGIN_MB),


I'm wondering if having defaults here is necessary? Isn't defaults.yaml always expected to be present?

Except in some unit tests.

Makes sense

revans2 · 2017-06-07T16:16:01Z

I think I addressed all of the review comments

srdo

LGTM, thanks for addressing the comments.

srdo · 2017-06-07T16:27:33Z

storm-client/src/jvm/org/apache/storm/scheduler/SchedulerAssignment.java

    /**
-     * get the topology-id this assignment is for.
-     * @return
+     * Return the ID of the topolgoy.


I even typo my fixes.

srdo · 2017-06-07T16:30:17Z

storm-client/src/jvm/org/apache/storm/trident/operation/DefaultResourceDeclarer.java


-    private Map<String, Number> resources = new HashMap<>();
-    private static Map<String, Object> conf = Utils.readStormConfig();
+    //@{link org.apache.storm.trident.planner.Node} and several other tirdent classes inherit from DefaultResourceDeclarer


srdo · 2017-06-07T16:30:28Z

storm-client/src/jvm/org/apache/storm/trident/operation/DefaultResourceDeclarer.java

-    private static Map<String, Object> conf = Utils.readStormConfig();
+    //@{link org.apache.storm.trident.planner.Node} and several other tirdent classes inherit from DefaultResourceDeclarer
+    // These classes are serialized out as part of the bolts and spouts of a topology, often for each bolt/spout in the topology.
+    // The following are marked as transiant because they are never used after the topology is created so keeping them around just wasts


srdo · 2017-06-07T16:32:41Z

storm-server/src/test/java/org/apache/storm/daemon/nimbus/NimbusTest.java

 import org.apache.storm.topology.TopologyBuilder;
 import org.junit.Test;

+import static org.junit.Assert.*;


I think checkstyle bans this

Checkstyle does not check tests for some reason, but if you care about it I can change it.

No, don't worry about it. Hadn't noticed this was in a test

srdo · 2017-06-07T16:32:56Z

storm-server/src/test/java/org/apache/storm/daemon/nimbus/NimbusTest.java

-        Nimbus.validateTopologyWorkerMaxHeapSizeConfigs(config1, stormTopology1);
+        try {
+            Nimbus.validateTopologyWorkerMaxHeapSizeConfigs(config1, stormTopology1);
+            fail("Expected exeption not thrown");


revans2 · 2017-06-07T17:13:29Z

@srdo I think I got the rest of them

@kishorvpatil could you take a look at let me know if the rework is good for you?

…ed memory regions

revans2 · 2017-07-25T20:30:50Z

@srdo @kishorvpatil @jerrypeng

I just rebased this on the latest master and added in a few caches in some of the internal data structures to help with performance issues we had seen in production. Please take a look again, I really would like to get this in sooner rather then later as we have some other work we want to do on the RAS scheduler to add in generic resource support (we have a goal to officially support GPUs)

kishorvpatil

Still +1

srdo · 2017-07-25T22:02:07Z

+1, though I'll admit to just skimming it this time. It would be easier to tell what changed if the non-merge-conflict-resolution changes were in separate commits.

It might make sense to drop a line to @roshannaik before merging, I think he asked on the mailing list to be made aware of changes to storm-client due to #2241

HeartSaVioR

+1

revans2 force-pushed the STORM-2497 branch from d8903b8 to d0fb999 Compare May 15, 2017 15:57

revans2 force-pushed the STORM-2497 branch 2 times, most recently from 7c22715 to d45c562 Compare May 23, 2017 16:16

revans2 mentioned this pull request May 23, 2017

Leave checkstyle violation to each module's target directory #2135

Merged

revans2 force-pushed the STORM-2497 branch from d45c562 to ac378fb Compare May 24, 2017 15:57

kishorvpatil approved these changes Jun 2, 2017

View reviewed changes

revans2 force-pushed the STORM-2497 branch from ac378fb to 63a5b8d Compare June 2, 2017 18:26

srdo reviewed Jun 6, 2017

View reviewed changes

srdo approved these changes Jun 7, 2017

View reviewed changes

STORM-2497: Let Supervisor enforce memory and add in support for shar…

b4d3395

…ed memory regions

revans2 force-pushed the STORM-2497 branch from fe8441b to b4d3395 Compare July 25, 2017 20:27

kishorvpatil approved these changes Jul 25, 2017

View reviewed changes

HeartSaVioR approved these changes Jul 30, 2017

View reviewed changes

asfgit merged commit b4d3395 into apache:master Jul 31, 2017

STORM-2497: Let Supervisor enforce memory and add in support for shared memory regions #2113

STORM-2497: Let Supervisor enforce memory and add in support for shared memory regions #2113

Uh oh!

Conversation

revans2 commented May 12, 2017

Uh oh!

jerrypeng commented May 29, 2017

Uh oh!

kishorvpatil left a comment

Choose a reason for hiding this comment

Uh oh!

revans2 commented Jun 2, 2017

Uh oh!

srdo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

revans2 commented Jun 7, 2017

Uh oh!

srdo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

revans2 commented Jun 7, 2017

Uh oh!

revans2 commented Jul 25, 2017

Uh oh!

kishorvpatil left a comment

Choose a reason for hiding this comment

Uh oh!

srdo commented Jul 25, 2017

Uh oh!

HeartSaVioR left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers