Add APIs to control packing/repacking algorithm and change instance resources #2059

objmagic · 2017-07-13T20:50:33Z

In Twitter, customers wants the ability to change packing algorithm, and tune instance ram usage without passing --config-property to Heron client. This PR adds API to change them.

Yes, the PackingAlgorithmType.java and RepackingAlgorithmType.java are pretty ugly. Please free feel to advise better solutions.

huijunwu · 2017-07-13T21:10:40Z

These packing.algorithm configs are set in yaml as well. What happens if config in yaml conflicts with setting in topology writter's code?
Does this PR violate 'Neither system- nor component-level configurations can be overridden by topology developers.' in document https://twitter.github.io/heron/docs/operators/configuration/config-intro/

objmagic · 2017-07-13T21:20:57Z

This argument sounds strange to me. If this needs to be true all the time, how shall we change packing algorithm? The only way is to pass --config-property?

billonahill · 2017-07-13T22:28:44Z

@huijunw these are not system or component level configs as defined on that page. They're not set in heron_internal.yaml. These should be defaulted but overridable by the topology.

billonahill · 2017-07-13T22:30:37Z

heron/api/src/java/com/twitter/heron/api/Config.java

+   * In bytes.
+   */
+  public static final String TOPOLOGY_INSTANCE_RAM_REQUESTED =
+      "heron.resources.instance.ram";


please append with .bytes to make units explicit. same for disk settings.

billonahill · 2017-07-13T22:31:02Z

heron/api/src/java/com/twitter/heron/api/Config.java

+    conf.put(Config.TOPOLOGY_INSTANCE_CPU_REQUESTED, Float.toString(ncpus));
+  }
+
+  public static void setInstanceDiskRequested(Map<String, Object> conf, ByteAmount nbytes) {


s/nbytes/byteAmount/g here and below.

billonahill · 2017-07-13T22:31:19Z

heron/api/src/java/com/twitter/heron/api/Config.java

+    conf.put(Config.TOPOLOGY_REPACKING_ALGORITHM, type.toString());
+  }
+
+  public static void setInstanceCpuRequested(Map<String, Object> conf, float ncpus) {


s/ncpus/cpus/g here and below

billonahill · 2017-07-13T22:36:13Z

heron/api/src/java/com/twitter/heron/api/Config.java

@@ -452,6 +487,28 @@ public static void setTopologyExactlyOnceEnabled(Map<String, Object> conf, boole
    conf.put(Config.TOPOLOGY_EXACTLYONCE_ENABLED, String.valueOf(exactOnce));
  }

+  public static void setTopologyPackingAlgorithm(Map<String, Object> conf,
+                                                 PackingAlgorithmType type) {


The packing type is just a Class. It needs to be extensible so users can implement their own so an enum won't work.

public static void setTopologyPackingClass(Map<String, Object> conf, Class<? extends IPacking> packingClass); public static void setTopologyRepackingClass(Map<String, Object> conf, Class<? extends IRepacking> repackingClass);

I was worried if doing this needs to pull in lots of dependencies from schedulers. But I can try though.

This would cause a problem, since IPacking and IRepacking are in spi, which depends on api, but not the other way around. We could considering moving these interfaces into api. @kramasamy do you have an opinion on this one?

You are right. I was not clear in my message above: yes, I was worried about circular dependencies as well when reading BUILD files.

Regarding packaging of IPacking and IRepacking, is the distribution of interfaces between api and spi packages needed? Given the current structure, moving these interfaces may create confusion. These interfaces are closely linked to other components like IScheduler.

We could avoid future movements of interfaces used by users by keeping all the interfaces implemented by custom code in api package?

I think the intent (which makes sense to me) is that api is where topology-level APIs are that topology developers might implement or specify (i.e., IBolt, ISpout, etc). The spi package is where system-level APIs live that would be implemented by administrating teams for a new scheduler or state manager for example.

I think the packing algorithms could be implemented or at least specified by topology authors at the topology level. Hence it makes sense for those interfaces to move to api, but not others perse.

Thanks @billonahill I agree with the interpretation of the api and spi packages.

I think packing configuration is rarely provided by topology developer. It is rather commonly specified by the administrator or the member deploying the topology via yaml config files or command line arguments. To me this fits the definition of system-level API. WDYT?

I think the packing impls should be defaulted at the system level, but I certainly see the need to be able to override them at the topology level. We've recommended to some topology owners to specify a different packing impl to change the way resources are allocated for their specific job, which seems reasonable to me.

srkukarni

One major feedback from my side.
Heron API should be self contained. It should not be dependent on any other module. I already see dependency on heron.common. This must be removed

billonahill · 2017-07-13T23:20:23Z

@srkukarni that dep already exists, so removing it should not be in scope for this issue, but this patch should not make it worse. Opened PR #2061 to remove dep on TypeUtils and #2062 to move ByteAmount into API.

srkukarni · 2017-07-13T23:22:18Z

@billonahill This pr makes it worse by adding more dep from common.

billonahill · 2017-07-13T23:33:06Z

@srkukarni which I'm saying it shouldn't do. :) That probably wasn't clear in my wording above...

objmagic · 2017-07-14T17:26:16Z

depends on #2061 #2066

nlu90 · 2017-07-17T22:25:59Z

heron/api/src/java/com/twitter/heron/api/Config.java

+  /**
+   * Packing algorithm used to calculate packing plan
+   */
+  public static final String TOPOLOGY_PACKING_ALGORITHM =


I really doubt if we need the packing algorithm to be configurable by users. By the original design, it's a system wide property which should only be set by the heron administrator.

Also, how are the ram, cpu and disk resource configs used? It seems these config doesn't tell the difference for different components. It applies all components' instances for a topology. Correct me if my understanding is wrong here.

I could see having multiple packing implementation supported, where power users could specify a non-default based on their needs. Or even write one of their own.

objmagic · 2017-07-18T19:51:50Z

It seems that we are not going to reach an agreement here soon. I have asked @ttim to add --config-property support in tsar config.

objmagic added 2 commits July 13, 2017 11:09

API to control packing algorithm.

b8fb456

API to control repacking and instance resources.

719b767

objmagic requested a review from billonahill July 13, 2017 20:50

huijunw requested review from srkukarni and maosongfu July 13, 2017 21:17

billonahill suggested changes Jul 13, 2017

View reviewed changes

srkukarni suggested changes Jul 13, 2017

View reviewed changes

nlu90 suggested changes Jul 17, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add APIs to control packing/repacking algorithm and change instance resources #2059

Add APIs to control packing/repacking algorithm and change instance resources #2059

objmagic commented Jul 13, 2017

huijunwu commented Jul 13, 2017 •

edited

Loading

objmagic commented Jul 13, 2017

billonahill commented Jul 13, 2017

billonahill Jul 13, 2017

billonahill Jul 13, 2017

billonahill Jul 13, 2017

billonahill Jul 13, 2017

objmagic Jul 13, 2017 •

edited

Loading

billonahill Jul 13, 2017

objmagic Jul 13, 2017

ashvina Jul 14, 2017

billonahill Jul 14, 2017

ashvina Jul 14, 2017

billonahill Jul 14, 2017

srkukarni left a comment

billonahill commented Jul 13, 2017 •

edited

Loading

srkukarni commented Jul 13, 2017

billonahill commented Jul 13, 2017

objmagic commented Jul 14, 2017

nlu90 Jul 17, 2017

billonahill Jul 17, 2017

objmagic commented Jul 18, 2017

Add APIs to control packing/repacking algorithm and change instance resources #2059

Are you sure you want to change the base?

Add APIs to control packing/repacking algorithm and change instance resources #2059

Conversation

objmagic commented Jul 13, 2017

huijunwu commented Jul 13, 2017 • edited Loading

objmagic commented Jul 13, 2017

billonahill commented Jul 13, 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

objmagic Jul 13, 2017 • edited Loading

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

srkukarni left a comment

Choose a reason for hiding this comment

billonahill commented Jul 13, 2017 • edited Loading

srkukarni commented Jul 13, 2017

billonahill commented Jul 13, 2017

objmagic commented Jul 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

objmagic commented Jul 18, 2017

huijunwu commented Jul 13, 2017 •

edited

Loading

objmagic Jul 13, 2017 •

edited

Loading

billonahill commented Jul 13, 2017 •

edited

Loading