Uploaded image for project: 'Apache Twill'
  1. Apache Twill
  2. TWILL-241

Allow specifying reserved off-heap memory and extra JVM options per runnable

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.11.0
    • Fix Version/s: 0.12.0
    • Component/s: api, yarn
    • Labels:
      None

      Description

      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.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/twill/pull/59

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/twill/pull/59
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on the issue:

          https://github.com/apache/twill/pull/59

          @anew Thanks for the review.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on the issue: https://github.com/apache/twill/pull/59 @anew Thanks for the review.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user anew commented on the issue:

          https://github.com/apache/twill/pull/59

          LGTM

          Show
          githubbot ASF GitHub Bot added a comment - Github user anew commented on the issue: https://github.com/apache/twill/pull/59 LGTM
          Hide
          githubbot ASF GitHub Bot added a comment -

          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

          Show
          githubbot ASF GitHub Bot added a comment - 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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;
          ```

          Show
          githubbot ASF GitHub Bot added a comment - 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; ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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

          Show
          githubbot ASF GitHub Bot added a comment - 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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

          Show
          githubbot ASF GitHub Bot added a comment - 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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?

          Show
          githubbot ASF GitHub Bot added a comment - 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?
          Hide
          githubbot ASF GitHub Bot added a comment -

          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 <chtyim@apache.org>
          Date: 2017-08-04T22:29:05Z

          (TWILL-241) Added support for per Runnable configuration

          commit 29a7999f45859996595287fb1c28b225a2564ed9
          Author: Terence Yim <chtyim@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

          Show
          githubbot ASF GitHub Bot added a comment - 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 <chtyim@apache.org> Date: 2017-08-04T22:29:05Z ( TWILL-241 ) Added support for per Runnable configuration commit 29a7999f45859996595287fb1c28b225a2564ed9 Author: Terence Yim <chtyim@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

            People

            • Assignee:
              chtyim Terence Yim
              Reporter:
              anew Andreas Neumann
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development