[jira] [Commented] (TWILL-241) Allow specifying reserved off-heap memory and extra JVM options per runnable
[ https://issues.apache.org/jira/browse/TWILL-241?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16117509#comment-16117509 ] ASF GitHub Bot commented on TWILL-241: -- Github user chtyim commented on the issue: https://github.com/apache/twill/pull/59 @anew Thanks for the review. > Allow specifying reserved off-heap memory and extra JVM options per runnable > > > Key: TWILL-241 > URL: https://issues.apache.org/jira/browse/TWILL-241 > Project: Apache Twill > Issue Type: Improvement > Components: api, yarn >Affects Versions: 0.11.0 >Reporter: Andreas Neumann >Assignee: Terence Yim > Fix For: 0.12.0 > > > Sometimes, a particular runnable needs a lot more off-heap memory than > others. It would therefore be useful to specify the amount of reserved > non-heap memory per runnable. > Similarly, for example, for debugging purposes, it may be necessary to add a > JVM option for one of the runnables without affecting the other runnables of > the same application. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] twill issue #59: (TWILL-241) Added per runnable configuration and jvm option...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[jira] [Commented] (TWILL-241) Allow specifying reserved off-heap memory and extra JVM options per runnable
[ https://issues.apache.org/jira/browse/TWILL-241?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16117383#comment-16117383 ] ASF GitHub Bot commented on TWILL-241: -- Github user anew commented on the issue: https://github.com/apache/twill/pull/59 LGTM > Allow specifying reserved off-heap memory and extra JVM options per runnable > > > Key: TWILL-241 > URL: https://issues.apache.org/jira/browse/TWILL-241 > Project: Apache Twill > Issue Type: Improvement > Components: api, yarn >Affects Versions: 0.11.0 >Reporter: Andreas Neumann >Assignee: Terence Yim > Fix For: 0.12.0 > > > Sometimes, a particular runnable needs a lot more off-heap memory than > others. It would therefore be useful to specify the amount of reserved > non-heap memory per runnable. > Similarly, for example, for debugging purposes, it may be necessary to add a > JVM option for one of the runnables without affecting the other runnables of > the same application. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] twill issue #59: (TWILL-241) Added per runnable configuration and jvm option...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events
[
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16117272#comment-16117272
]
ASF GitHub Bot commented on TWILL-240:
--
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131761743
--- Diff:
twill-api/src/main/java/org/apache/twill/api/EventHandlerContext.java ---
@@ -22,5 +22,9 @@
*/
public interface EventHandlerContext {
+ String getTwillAppName();
--- End diff --
Should be called `getApplicationName()`.
> Improve EventHandler to handle more application lifecycle events
>
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
> Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when
> certain lifecycle events happen by calling methods from EventHandler. For
> instance, when the app first starts, completes, aborts and etc.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events
[
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16117276#comment-16117276
]
ASF GitHub Bot commented on TWILL-240:
--
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131761787
--- Diff:
twill-api/src/main/java/org/apache/twill/api/EventHandlerContext.java ---
@@ -22,5 +22,9 @@
*/
public interface EventHandlerContext {
+ String getTwillAppName();
+
+ String getTwillAppRunId();
--- End diff --
Just call it `getRunId()`.
> Improve EventHandler to handle more application lifecycle events
>
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
> Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when
> certain lifecycle events happen by calling methods from EventHandler. For
> instance, when the app first starts, completes, aborts and etc.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events
[
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16117277#comment-16117277
]
ASF GitHub Bot commented on TWILL-240:
--
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131762365
--- Diff:
twill-api/src/main/java/org/apache/twill/api/EventHandlerContext.java ---
@@ -22,5 +22,9 @@
*/
public interface EventHandlerContext {
+ String getTwillAppName();
+
+ String getTwillAppRunId();
--- End diff --
The run id should be of type `RunId`.
> Improve EventHandler to handle more application lifecycle events
>
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
> Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when
> certain lifecycle events happen by calling methods from EventHandler. For
> instance, when the app first starts, completes, aborts and etc.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events
[
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16117271#comment-16117271
]
ASF GitHub Bot commented on TWILL-240:
--
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131761843
--- Diff:
twill-api/src/main/java/org/apache/twill/api/EventHandlerContext.java ---
@@ -22,5 +22,9 @@
*/
public interface EventHandlerContext {
+ String getTwillAppName();
--- End diff --
Also please add javadoc.
> Improve EventHandler to handle more application lifecycle events
>
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
> Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when
> certain lifecycle events happen by calling methods from EventHandler. For
> instance, when the app first starts, completes, aborts and etc.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events
[
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16117274#comment-16117274
]
ASF GitHub Bot commented on TWILL-240:
--
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131762894
--- Diff:
twill-yarn/src/test/java/org/apache/twill/yarn/ProvisionTimeoutTestRun.java ---
@@ -85,13 +105,29 @@ public TimeoutAction
launchTimeout(Iterable timeoutEvents) {
return TimeoutAction.recheck(10, TimeUnit.SECONDS);
}
}
+
+@Override
+public void aborted() {
+ try {
+new
File(context.getSpecification().getConfigs().get("parentFolderPath") +
File.separator
--- End diff --
Also, we are only testing abort? How about other lifecycle methods?
> Improve EventHandler to handle more application lifecycle events
>
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
> Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when
> certain lifecycle events happen by calling methods from EventHandler. For
> instance, when the app first starts, completes, aborts and etc.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[GitHub] twill pull request #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131762189
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
---
@@ -291,6 +300,24 @@ public void run() {
// Since all the runnables are now stopped, it is okay to stop the
poller.
stopPoller.shutdownNow();
cleanupDir();
+if (eventHandler != null) {
+ if (stopStatus == null) {
+// if finalStatus is not set, the application must be stopped by a
SystemMessages#STOP_COMMAND
+eventHandler.killed(twillRuntimeSpec.getTwillAppName(),
twillRuntimeSpec.getTwillAppRunId());
+return;
+ }
+ switch (stopStatus) {
+case COMPLETED:
+ eventHandler.completed();
+ break;
+case ABORTED:
+eventHandler.aborted();
--- End diff --
Misalignment.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events
[
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16117273#comment-16117273
]
ASF GitHub Bot commented on TWILL-240:
--
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131762189
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
---
@@ -291,6 +300,24 @@ public void run() {
// Since all the runnables are now stopped, it is okay to stop the
poller.
stopPoller.shutdownNow();
cleanupDir();
+if (eventHandler != null) {
+ if (stopStatus == null) {
+// if finalStatus is not set, the application must be stopped by a
SystemMessages#STOP_COMMAND
+eventHandler.killed(twillRuntimeSpec.getTwillAppName(),
twillRuntimeSpec.getTwillAppRunId());
+return;
+ }
+ switch (stopStatus) {
+case COMPLETED:
+ eventHandler.completed();
+ break;
+case ABORTED:
+eventHandler.aborted();
--- End diff --
Misalignment.
> Improve EventHandler to handle more application lifecycle events
>
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
> Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when
> certain lifecycle events happen by calling methods from EventHandler. For
> instance, when the app first starts, completes, aborts and etc.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events
[
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16117275#comment-16117275
]
ASF GitHub Bot commented on TWILL-240:
--
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131762593
--- Diff:
twill-yarn/src/test/java/org/apache/twill/yarn/ProvisionTimeoutTestRun.java ---
@@ -85,13 +105,29 @@ public TimeoutAction
launchTimeout(Iterable timeoutEvents) {
return TimeoutAction.recheck(10, TimeUnit.SECONDS);
}
}
+
+@Override
+public void aborted() {
+ try {
+new
File(context.getSpecification().getConfigs().get("parentFolderPath") +
File.separator
--- End diff --
When constructing file, usually not to use `File.separator`, but instead
use the constructor `File(File parent, String path)`.
> Improve EventHandler to handle more application lifecycle events
>
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
> Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when
> certain lifecycle events happen by calling methods from EventHandler. For
> instance, when the app first starts, completes, aborts and etc.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[GitHub] twill pull request #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131761843
--- Diff:
twill-api/src/main/java/org/apache/twill/api/EventHandlerContext.java ---
@@ -22,5 +22,9 @@
*/
public interface EventHandlerContext {
+ String getTwillAppName();
--- End diff --
Also please add javadoc.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131762593
--- Diff:
twill-yarn/src/test/java/org/apache/twill/yarn/ProvisionTimeoutTestRun.java ---
@@ -85,13 +105,29 @@ public TimeoutAction
launchTimeout(Iterable timeoutEvents) {
return TimeoutAction.recheck(10, TimeUnit.SECONDS);
}
}
+
+@Override
+public void aborted() {
+ try {
+new
File(context.getSpecification().getConfigs().get("parentFolderPath") +
File.separator
--- End diff --
When constructing file, usually not to use `File.separator`, but instead
use the constructor `File(File parent, String path)`.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131762365
--- Diff:
twill-api/src/main/java/org/apache/twill/api/EventHandlerContext.java ---
@@ -22,5 +22,9 @@
*/
public interface EventHandlerContext {
+ String getTwillAppName();
+
+ String getTwillAppRunId();
--- End diff --
The run id should be of type `RunId`.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131762894
--- Diff:
twill-yarn/src/test/java/org/apache/twill/yarn/ProvisionTimeoutTestRun.java ---
@@ -85,13 +105,29 @@ public TimeoutAction
launchTimeout(Iterable timeoutEvents) {
return TimeoutAction.recheck(10, TimeUnit.SECONDS);
}
}
+
+@Override
+public void aborted() {
+ try {
+new
File(context.getSpecification().getConfigs().get("parentFolderPath") +
File.separator
--- End diff --
Also, we are only testing abort? How about other lifecycle methods?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131761787
--- Diff:
twill-api/src/main/java/org/apache/twill/api/EventHandlerContext.java ---
@@ -22,5 +22,9 @@
*/
public interface EventHandlerContext {
+ String getTwillAppName();
+
+ String getTwillAppRunId();
--- End diff --
Just call it `getRunId()`.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[jira] [Commented] (TWILL-241) Allow specifying reserved off-heap memory and extra JVM options per runnable
[
https://issues.apache.org/jira/browse/TWILL-241?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16116858#comment-16116858
]
ASF GitHub Bot commented on TWILL-241:
--
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 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 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 config, int
defaultValue) {
+if (config == null) {
+ return defaultValue;
--- End diff --
similar to getMinHeapRatio
> Allow specifying reserved off-heap memory and extra JVM options per runnable
>
>
> Key: TWILL-241
> URL: https://issues.apache.org/jira/browse/TWILL-241
> Project: Apache Twill
> Issue Type: Improvement
> Components: api, yarn
>Affects Versions: 0.11.0
>Reporter: Andreas Neumann
>Assignee: Terence Yim
> Fix For: 0.12.0
>
>
> Sometimes, a particular runnable needs a lot more off-heap memory than
> others. It would therefore be useful to specify the amount of reserved
> non-heap memory per runnable.
> Similarly, for example, for debugging purposes, it may be necessary to add a
> JVM option for one of the runnables without affecting the other runnables of
> the same application.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[GitHub] twill pull request #59: (TWILL-241) Added per runnable configuration and jvm...
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 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 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 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #59: (TWILL-241) Added per runnable configuration and jvm...
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 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 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
