upgrade yarn scheduler driver memory to ByteAmount #1728

mycFelix · 2017-02-20T08:10:13Z

As #1727 mentioned and #1707 refactored, this PR upgraded driverMemory to use ByteAmount.

The key changes are following:

add getByteAmountValueMB() in Config class
change driverMemory from int to ByteAmount
refactor YarnKey class
remove unused variables in HeronMasterDriver

ashvina · 2017-02-21T19:52:04Z

heron/common/src/java/com/twitter/heron/common/basics/TypeUtils.java

@@ -87,6 +87,17 @@ public static ByteAmount getByteAmount(Object o) {
    }
  }

+  public static ByteAmount getByteAmountMB(Object o) {


Instead of adding a utility method for each unit, I think adding a unit to the existing method would be better. For e.g. getByteAmount(Object o) will become getByteAmount(Object o, ByteAmountUnit unit). This is same as the usage pattern of java.util.concurrent.Delayed.getDelay(TimeUnit unit). What do you think?

Good point~

@billonahill - It seems we don't have ByteAmountUnit class currently. Do you think it is a good idea if we add a new class in c.t.h.common.basics in this PR?

@mycFelix Bill is on vacation so he may not have spare time to reply you. We can definitely add ByteAmountUnit (and as a enum class to keep things simple). Another approach is to make 2048 into 2048 * 1024. We sacrifice some clarity here but can avoid extending current APIs. Thoughts? @ashvina

@mycFelix actually, now I think @ashvina's suggestion is better. It would be nice to add a enum class ByteAmountUnit in the same style of SystemConfigKey to expose MB and GB in a typed manner.

@objmagic - Understood. Thank you for your suggestion.

ashvina · 2017-02-21T19:53:17Z

heron/schedulers/src/java/com/twitter/heron/scheduler/yarn/YarnContext.java

-  public static int heronDriverMemoryMb(Config cfg) {
-    return cfg.getIntegerValue(YarnKey.YARN_SCHEDULER_DRIVER_MEMORY_MB.value(),
-        YarnKey.YARN_SCHEDULER_DRIVER_MEMORY_MB.getDefaultInt());
+  public static ByteAmount heronDriverMemoryMb(Config cfg) {


Remove mb from method name since ByteAmount is generic?

ashvina · 2017-02-21T19:54:09Z

heron/schedulers/src/java/com/twitter/heron/scheduler/yarn/YarnContext.java

-    return cfg.getIntegerValue(YarnKey.YARN_SCHEDULER_DRIVER_MEMORY_MB.value(),
-        YarnKey.YARN_SCHEDULER_DRIVER_MEMORY_MB.getDefaultInt());
+  public static ByteAmount heronDriverMemoryMb(Config cfg) {
+    return cfg.getByteAmountValueMB(YarnKey.YARN_SCHEDULER_DRIVER_MEMORY_MB.value(),


Same as above, remove MB from method name?

mycFelix · 2017-02-22T02:51:34Z

@ashvina @objmagic I've addressed all comments. Would you please take a look again?

objmagic · 2017-02-22T03:10:10Z

@mycFelix Looks good to me. I think putting ByteAmountUnit inside of ByteAmount might be better, but it's totally up to you. We can merge it if @ashvina gives a ship-it.

mycFelix · 2017-02-22T03:27:07Z

@objmagic - Thanks for reviewing. I'm agree with you. At first, I think putting ByteAmountUnit inside of ByteAmount is a better approach and using enum to instead static MB and GB inside of ByteAmount seems nice too. But It might trigger a large refactor in ByteAmount. So, I decided to put ByteAmountUnit outside of ByteAmount temporarily.

billonahill

I don't think we should introduce ByteAmountUnit. The point of ByteAmount is that config users will know how to create a ByteAmount (because they know the units), and put it in the config for others to leverage. Those others should not need to know what the unit in the config is, hence the ByteAmount class exists. It is a representation of bytes that consumers can get in whatever unit they need to, without knowledge of how the units at play when the config value was added. Creating ByteAmountUnit just allows the ability for the lazy to read a long and convert it to a ByteAmount if they happen to know the correct unit. This is undesirable and is exactly what we're trying to prevent with the ByteAmount class. It implies that the config consumer knows the unit that the config producer used when adding the config, which should not be the case (this pattern is super bug-prone). Instead, the agent that puts the size into the config should know the units, and create a ByteAmount up front.

objmagic · 2017-02-22T18:37:04Z

heron/schedulers/src/java/com/twitter/heron/scheduler/yarn/YarnLauncher.java

@@ -161,7 +162,7 @@ Configuration getHMDriverConf() {
        .set(HeronDriverConfiguration.HTTP_PORT, 0)
        .set(HeronDriverConfiguration.VERBOSE, false)
        .set(YarnDriverConfiguration.QUEUE, queue)
-        .set(DriverConfiguration.DRIVER_MEMORY, driverMemory)
+        .set(DriverConfiguration.DRIVER_MEMORY, driverMemory.asMegabytes())


I'm confused here. Shouldn't DRIVER_MEMORY accept value of type int, according to reef?

To be honest, it make me confused too. I didn't notice that...

@objmagic - cast it to int

I don't think you can just cast here...?

@mycFelix I now think I am not the right person to review this PR. You can think about Bill's review and wait for @ashvina's comment.

@objmagic - Sure~

mycFelix · 2017-03-07T09:09:21Z

Considering @billonahill 's comment, creating ByteAmountUnit enum class might not be a proper way.

@ashvina @billonahill - How about we make 2048 into 2048 * 1024 here？

ashvina · 2017-03-07T23:15:50Z

heron/common/src/java/com/twitter/heron/common/basics/TypeUtils.java

+    }
+  }
+
+  public static ByteAmount getByteAmount(Object o, ByteAmountUnit unit) {


I think it would be useful to add javadocs for these overloaded methods.

objmagic · 2017-03-14T20:50:00Z

is there any update on this? seems we have very few things left to do for this PR? @mycFelix @billonahill

upgrade yarn scheduler driver memory to ByteAmount

32513d5

kramasamy requested review from ashvina and billonahill February 20, 2017 19:09

remove unused import

085e878

ashvina suggested changes Feb 21, 2017

View reviewed changes

adding enum ByteAmountUnit

afe4367

fix typo

8e98314

billonahill suggested changes Feb 22, 2017

View reviewed changes

objmagic reviewed Feb 22, 2017

View reviewed changes

cast to int

55f0376

objmagic added the enhancement label Mar 3, 2017

objmagic assigned mycFelix Mar 3, 2017

ashvina reviewed Mar 7, 2017

View reviewed changes

ashvina approved these changes Mar 7, 2017

View reviewed changes

huijunw added the Yarn label Jul 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upgrade yarn scheduler driver memory to ByteAmount #1728

upgrade yarn scheduler driver memory to ByteAmount #1728

mycFelix commented Feb 20, 2017 •

edited

Loading

ashvina Feb 21, 2017

mycFelix Feb 22, 2017

mycFelix Feb 22, 2017

objmagic Feb 22, 2017 •

edited

Loading

objmagic Feb 22, 2017

mycFelix Feb 22, 2017

ashvina Feb 21, 2017

mycFelix Feb 22, 2017

ashvina Feb 21, 2017

mycFelix Feb 22, 2017

mycFelix commented Feb 22, 2017

objmagic commented Feb 22, 2017

mycFelix commented Feb 22, 2017 •

edited

Loading

billonahill left a comment

objmagic Feb 22, 2017

mycFelix Feb 23, 2017 •

edited

Loading

mycFelix Feb 23, 2017

objmagic Feb 23, 2017

objmagic Feb 23, 2017

mycFelix Feb 23, 2017

mycFelix commented Mar 7, 2017 •

edited

Loading

ashvina Mar 7, 2017

objmagic commented Mar 14, 2017

upgrade yarn scheduler driver memory to ByteAmount #1728

Are you sure you want to change the base?

upgrade yarn scheduler driver memory to ByteAmount #1728

Conversation

mycFelix commented Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

objmagic Feb 22, 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

mycFelix commented Feb 22, 2017

objmagic commented Feb 22, 2017

mycFelix commented Feb 22, 2017 • edited Loading

billonahill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mycFelix Feb 23, 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

mycFelix commented Mar 7, 2017 • edited Loading

Choose a reason for hiding this comment

objmagic commented Mar 14, 2017

mycFelix commented Feb 20, 2017 •

edited

Loading

objmagic Feb 22, 2017 •

edited

Loading

mycFelix commented Feb 22, 2017 •

edited

Loading

mycFelix Feb 23, 2017 •

edited

Loading

mycFelix commented Mar 7, 2017 •

edited

Loading