Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

upgrade yarn scheduler driver memory to ByteAmount #1728

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

upgrade yarn scheduler driver memory to ByteAmount #1728

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
32513d5
upgrade yarn scheduler driver memory to ByteAmount
mycFelix Feb 20, 2017
085e878
remove unused import
mycFelix Feb 21, 2017
afe4367
adding enum ByteAmountUnit
mycFelix Feb 22, 2017
8e98314
fix typo
mycFelix Feb 22, 2017
55f0376
cast to int
mycFelix Feb 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions heron/common/src/java/com/twitter/heron/common/basics/TypeUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,17 @@ public static ByteAmount getByteAmount(Object o) {
}
}

public static ByteAmount getByteAmountMB(Object o) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point~

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

@objmagic objmagic Feb 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@objmagic - Understood. Thank you for your suggestion.

if (o != null && o instanceof ByteAmount) {
return (ByteAmount) o;
}
try {
return ByteAmount.fromMegabytes(getLong(o));
} catch (IllegalArgumentException e) {
throw new IllegalArgumentException("Don't know how to convert " + o + " to ByteAmount", e);
}
}

public static Boolean getBoolean(Object o) {
if (o instanceof Boolean) {
return (Boolean) o;
Expand Down
2 changes: 0 additions & 2 deletions heron/schedulers/src/java/com/twitter/heron/scheduler/yarn/HeronMasterDriver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,7 @@
*/
@Unit
public class HeronMasterDriver {
static final int TM_MEM_SIZE_MB = 1024;
static final int TMASTER_CONTAINER_ID = 0;
static final int MB = 1024 * 1024;
private static final Logger LOG = Logger.getLogger(HeronMasterDriver.class.getName());
private final String topologyPackageName;
private final String heronCorePackageName;
Expand Down
7 changes: 4 additions & 3 deletions heron/schedulers/src/java/com/twitter/heron/scheduler/yarn/YarnContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.twitter.heron.scheduler.yarn;

import com.twitter.heron.common.basics.ByteAmount;
import com.twitter.heron.spi.common.Config;
import com.twitter.heron.spi.common.Context;

Expand All @@ -27,8 +28,8 @@ public static String heronYarnQueue(Config cfg) {
YarnKey.HERON_SCHEDULER_YARN_QUEUE.getDefaultString());
}

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove mb from method name since ByteAmount is generic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree

return cfg.getByteAmountValueMB(YarnKey.YARN_SCHEDULER_DRIVER_MEMORY_MB.value(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, remove MB from method name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

(ByteAmount) YarnKey.YARN_SCHEDULER_DRIVER_MEMORY_MB.getDefault());
}
}
16 changes: 5 additions & 11 deletions heron/schedulers/src/java/com/twitter/heron/scheduler/yarn/YarnKey.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.twitter.heron.scheduler.yarn;

import com.twitter.heron.common.basics.ByteAmount;
import com.twitter.heron.spi.common.Key;

/**
Expand All @@ -23,7 +24,8 @@ public enum YarnKey {
// yarn queue for submitting and launching the topology
HERON_SCHEDULER_YARN_QUEUE("heron.scheduler.yarn.queue", "default"),
// the amount of memory topology's driver (yarn application master) needs
YARN_SCHEDULER_DRIVER_MEMORY_MB("heron.scheduler.yarn.driver.memory.mb", 2048);
YARN_SCHEDULER_DRIVER_MEMORY_MB("heron.scheduler.yarn.driver.memory.mb",
ByteAmount.fromMegabytes(2048));

private final String value;
private final Key.Type type;
Expand All @@ -35,9 +37,9 @@ public enum YarnKey {
this.defaultValue = defaultValue;
}

YarnKey(String value, Integer defaultValue) {
YarnKey(String value, ByteAmount defaultValue) {
this.value = value;
this.type = Key.Type.INTEGER;
this.type = Key.Type.BYTE_AMOUNT;
this.defaultValue = defaultValue;
}

Expand All @@ -56,12 +58,4 @@ public String getDefaultString() {
}
return (String) this.defaultValue;
}

public int getDefaultInt() {
if (type != Key.Type.INTEGER) {
throw new IllegalAccessError(String.format(
"Config Key %s is type %s, getDefaultInt() not supported", this.name(), this.type));
}
return (Integer) this.defaultValue;
}
}
5 changes: 3 additions & 2 deletions heron/schedulers/src/java/com/twitter/heron/scheduler/yarn/YarnLauncher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.apache.reef.tang.annotations.Unit;
import org.apache.reef.tang.exceptions.InjectionException;

import com.twitter.heron.common.basics.ByteAmount;
import com.twitter.heron.scheduler.yarn.HeronMasterDriver.ContainerAllocationHandler;
import com.twitter.heron.scheduler.yarn.HeronMasterDriver.FailedContainerHandler;
import com.twitter.heron.scheduler.yarn.HeronMasterDriver.HeronSchedulerLauncher;
Expand Down Expand Up @@ -63,7 +64,7 @@ public class YarnLauncher implements ILauncher {
private String role;
private String env;
private String queue;
private int driverMemory;
private ByteAmount driverMemory;
private ArrayList<String> libJars = new ArrayList<>();

@Override
Expand Down Expand Up @@ -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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mycFelix mycFelix Feb 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@objmagic - cast it to int

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@objmagic - Sure~

.build();
}

Expand Down
8 changes: 8 additions & 0 deletions heron/spi/src/java/com/twitter/heron/spi/common/Config.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,14 @@ public ByteAmount getByteAmountValue(Key key) {
return TypeUtils.getByteAmount(value);
}

public ByteAmount getByteAmountValueMB(String key, ByteAmount defaultValue) {
Object value = get(key);
if (value != null) {
return TypeUtils.getByteAmountMB(value);
}
return defaultValue;
}

DryRunFormatType getDryRunFormatType(Key key) {
return (DryRunFormatType) get(key);
}
Expand Down