You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@twill.apache.org by chtyim <gi...@git.apache.org> on 2017/08/05 01:08:26 UTC
[GitHub] twill pull request #59: (TWILL-241) Added per runnable configuration and jvm...
GitHub user chtyim opened a pull request:
https://github.com/apache/twill/pull/59
(TWILL-241) Added per runnable configuration and jvm options support
This PR has two commits, one for adding per runnable configuration, the other for adding per runnable jvm options.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/chtyim/twill feature/TWILL-241-per-runnable-opts
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/twill/pull/59.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #59
----
commit f1931deb128dda8b3c4da76486ab1b443318496e
Author: Terence Yim <ch...@apache.org>
Date: 2017-08-04T22:29:05Z
(TWILL-241) Added support for per Runnable configuration
commit 29a7999f45859996595287fb1c28b225a2564ed9
Author: Terence Yim <ch...@apache.org>
Date: 2017-08-04T23:19:32Z
(TWILL-241) Added support for per runnable JVM options
- Also removed JvmOptionsCodec since JvmOptions only uses simple types
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #59: (TWILL-241) Added per runnable configuration and jvm...
Posted by anew <gi...@git.apache.org>.
Github user anew commented on a diff in the pull request:
https://github.com/apache/twill/pull/59#discussion_r131708495
--- Diff: twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java ---
@@ -171,9 +168,27 @@ public String getKafkaZKConnect() {
/**
* Returns the minimum heap ratio ({@link Configs.Keys#HEAP_RESERVED_MIN_RATIO}) based on the given configuration.
*/
- private double getMinHeapRatio(Map<String, String> config) {
- return config.containsKey(Configs.Keys.HEAP_RESERVED_MIN_RATIO) ?
- Double.parseDouble(config.get(Configs.Keys.HEAP_RESERVED_MIN_RATIO)) :
- Configs.Defaults.HEAP_RESERVED_MIN_RATIO;
+ private double getMinHeapRatio(@Nullable Map<String, String> config, double defaultValue) {
+ if (config == null) {
--- End diff --
I think this would be easier to read, and it would give better errors, as follows:
```
if (config == null || !config.containsKey(...)) {
return defaultValue;
}
try {
double ratio = Double.parseDouble(config.get(...));
} catch (NumberFormatException) {
// either warn or throw a more informative exn (including runnable name and the bad value)
}
if (ratio <= 0d) {
// either warn or throw a more informative exn (including runnable name and the bad value)
}
return ratio;
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #59: (TWILL-241) Added per runnable configuration and jvm...
Posted by anew <gi...@git.apache.org>.
Github user anew commented on a diff in the pull request:
https://github.com/apache/twill/pull/59#discussion_r131511023
--- Diff: twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java ---
@@ -87,19 +86,46 @@ public String getTwillAppName() {
return twillAppName;
}
- public int getReservedMemory() {
- return reservedMemory;
+ /**
+ * Returns the minimum heap ratio for the application master.
+ */
+ public double getAMMinHeapRatio() {
+ return getMinHeapRatio(config);
+ }
+
+ /**
+ * Returns the minimum heap ratio for the given runnable.
+ */
+ public double getMinHeapRatio(String runnableName) {
+ return getMinHeapRatio(runnableConfigs.containsKey(runnableName) ? runnableConfigs.get(runnableName) : config);
--- End diff --
I think this logic is incorrect. It might be that the runnable has its own config but does specify a min heap ratio. In that case the min heap ratio should come from the (appllication) config, and only if that does not specify a min heap ratio, use the default.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #59: (TWILL-241) Added per runnable configuration and jvm...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/59#discussion_r131511384
--- Diff: twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillPreparer.java ---
@@ -183,6 +184,13 @@ public TwillPreparer withConfiguration(Map<String, String> config) {
}
@Override
+ public TwillPreparer withConfiguration(String runnableName, Map<String, String> config) {
--- End diff --
Should default to the app setting first, then to the constants default
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill issue #59: (TWILL-241) Added per runnable configuration and jvm option...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on the issue:
https://github.com/apache/twill/pull/59
@anew Thanks for the review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #59: (TWILL-241) Added per runnable configuration and jvm...
Posted by anew <gi...@git.apache.org>.
Github user anew commented on a diff in the pull request:
https://github.com/apache/twill/pull/59#discussion_r131511104
--- Diff: twill-yarn/src/test/java/org/apache/twill/yarn/ContainerSizeTestRun.java ---
@@ -67,20 +69,37 @@ public void testContainerSize() throws InterruptedException, TimeoutException, E
@Test
public void testMaxHeapSize() throws InterruptedException, TimeoutException, ExecutionException {
TwillRunner runner = getTwillRunner();
+ String runnableName = "sleep";
+
TwillController controller = runner.prepare(new MaxHeapApp())
- // Alter the AM container size
- .withConfiguration(Collections.singletonMap(Configs.Keys.YARN_AM_MEMORY_MB, "256"))
+ // Alter the AM container size and heap ratio
+ .withConfiguration(ImmutableMap.of(Configs.Keys.YARN_AM_MEMORY_MB, "256",
+ Configs.Keys.HEAP_RESERVED_MIN_RATIO, "0.65"))
+ // Use a different heap ratio and reserved memory size for the runnable
+ .withConfiguration(runnableName,
+ ImmutableMap.of(Configs.Keys.HEAP_RESERVED_MIN_RATIO, "0.8",
--- End diff --
can you add a test that, if this config for the runnable does not have HEAP_RESERVED_MIN_RATIO, it defaults to the one above (0.65) and not the system default.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #59: (TWILL-241) Added per runnable configuration and jvm...
Posted by anew <gi...@git.apache.org>.
Github user anew commented on a diff in the pull request:
https://github.com/apache/twill/pull/59#discussion_r131707378
--- Diff: twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java ---
@@ -171,9 +168,27 @@ public String getKafkaZKConnect() {
/**
* Returns the minimum heap ratio ({@link Configs.Keys#HEAP_RESERVED_MIN_RATIO}) based on the given configuration.
*/
- private double getMinHeapRatio(Map<String, String> config) {
- return config.containsKey(Configs.Keys.HEAP_RESERVED_MIN_RATIO) ?
- Double.parseDouble(config.get(Configs.Keys.HEAP_RESERVED_MIN_RATIO)) :
- Configs.Defaults.HEAP_RESERVED_MIN_RATIO;
+ private double getMinHeapRatio(@Nullable Map<String, String> config, double defaultValue) {
+ if (config == null) {
+ return defaultValue;
+ }
+
+ double ratio = config.containsKey(Configs.Keys.HEAP_RESERVED_MIN_RATIO) ?
+ Double.parseDouble(config.get(Configs.Keys.HEAP_RESERVED_MIN_RATIO)) : defaultValue;
+ // Ratio can't be <= 0
+ return ratio <= 0d ? defaultValue : ratio;
--- End diff --
It would be good to log a warning if it is <0, because that is a configuration error.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #59: (TWILL-241) Added per runnable configuration and jvm...
Posted by anew <gi...@git.apache.org>.
Github user anew commented on a diff in the pull request:
https://github.com/apache/twill/pull/59#discussion_r131708600
--- Diff: twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java ---
@@ -171,9 +168,27 @@ public String getKafkaZKConnect() {
/**
* Returns the minimum heap ratio ({@link Configs.Keys#HEAP_RESERVED_MIN_RATIO}) based on the given configuration.
*/
- private double getMinHeapRatio(Map<String, String> config) {
- return config.containsKey(Configs.Keys.HEAP_RESERVED_MIN_RATIO) ?
- Double.parseDouble(config.get(Configs.Keys.HEAP_RESERVED_MIN_RATIO)) :
- Configs.Defaults.HEAP_RESERVED_MIN_RATIO;
+ private double getMinHeapRatio(@Nullable Map<String, String> config, double defaultValue) {
+ if (config == null) {
+ return defaultValue;
+ }
+
+ double ratio = config.containsKey(Configs.Keys.HEAP_RESERVED_MIN_RATIO) ?
+ Double.parseDouble(config.get(Configs.Keys.HEAP_RESERVED_MIN_RATIO)) : defaultValue;
+ // Ratio can't be <= 0
+ return ratio <= 0d ? defaultValue : ratio;
+ }
+
+ /**
+ * Returns the reserved memory size ({@link Configs.Keys#HEAP_RESERVED_MIN_RATIO}) based on the given configuration.
+ */
+ private int getReservedMemory(@Nullable Map<String, String> config, int defaultValue) {
+ if (config == null) {
+ return defaultValue;
--- End diff --
similar to getMinHeapRatio
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #59: (TWILL-241) Added per runnable configuration and jvm...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/59#discussion_r131511349
--- Diff: twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java ---
@@ -87,19 +86,46 @@ public String getTwillAppName() {
return twillAppName;
}
- public int getReservedMemory() {
- return reservedMemory;
+ /**
+ * Returns the minimum heap ratio for the application master.
+ */
+ public double getAMMinHeapRatio() {
+ return getMinHeapRatio(config);
+ }
+
+ /**
+ * Returns the minimum heap ratio for the given runnable.
+ */
+ public double getMinHeapRatio(String runnableName) {
+ return getMinHeapRatio(runnableConfigs.containsKey(runnableName) ? runnableConfigs.get(runnableName) : config);
--- End diff --
Agree. Get it wrong after some refactoring
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #59: (TWILL-241) Added per runnable configuration and jvm...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/twill/pull/59
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill issue #59: (TWILL-241) Added per runnable configuration and jvm option...
Posted by anew <gi...@git.apache.org>.
Github user anew commented on the issue:
https://github.com/apache/twill/pull/59
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #59: (TWILL-241) Added per runnable configuration and jvm...
Posted by anew <gi...@git.apache.org>.
Github user anew commented on a diff in the pull request:
https://github.com/apache/twill/pull/59#discussion_r131510954
--- Diff: twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillPreparer.java ---
@@ -183,6 +184,13 @@ public TwillPreparer withConfiguration(Map<String, String> config) {
}
@Override
+ public TwillPreparer withConfiguration(String runnableName, Map<String, String> config) {
--- End diff --
If the config contains only one key, say reserved memory, will the remaining keys default to the values configured for the entire app? Or will they default to the system default?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---