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

Make ratio between total memory and on-heap memory configurable

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.10.0
    • Component/s: yarn
    • Labels:
      None

      Description

      As of now ratio between on-heap memory and total memory provided to yarn container is hardcoded to 0.7, so if app running in the container needs more reserved memory than on-heap it is not possible to achieve.

      Suggestion is to make it configurable as well as amount of reserved memory

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user yufeldman opened a pull request:

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

          TWILL-216 Make ratio between total memory and on-heap memory configur…

          …able

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/yufeldman/twill branch-TWILL-216

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/twill/pull/33.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 #33


          commit 6f721f696452bb6124cf5b53b285fe60db75ff03
          Author: Yuliya Feldman <yuliya@dremio.com>
          Date: 2017-02-10T21:05:20Z

          TWILL-216 Make ratio between total memory and on-heap memory configurable


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user yufeldman opened a pull request: https://github.com/apache/twill/pull/33 TWILL-216 Make ratio between total memory and on-heap memory configur… …able You can merge this pull request into a Git repository by running: $ git pull https://github.com/yufeldman/twill branch- TWILL-216 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/33.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 #33 commit 6f721f696452bb6124cf5b53b285fe60db75ff03 Author: Yuliya Feldman <yuliya@dremio.com> Date: 2017-02-10T21:05:20Z TWILL-216 Make ratio between total memory and on-heap memory configurable
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hsaputra commented on the issue:

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

          @yufeldman : Could you kindly add in the summary of the PR the summary of the approach you are taking to fix the issue?

          Will help with the review to compare expectation vs implementation.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hsaputra commented on the issue: https://github.com/apache/twill/pull/33 @yufeldman : Could you kindly add in the summary of the PR the summary of the approach you are taking to fix the issue? Will help with the review to compare expectation vs implementation.
          Hide
          yufeldman Yuliya Feldman added a comment - - edited

          Sorry for not adding more details sooner.

          Essentially at the moment Twill decides how much of the total requested memory to allocate to heap based on the hardcoded ratio of 0.7, meaning it will allocate at least 70% to heap.

          There could be applications that use direct memory quite a bit and they want to allocate more then 30% of total memory to be direct memory.

          This is a rational behind this JIRA.

          Show
          yufeldman Yuliya Feldman added a comment - - edited Sorry for not adding more details sooner. Essentially at the moment Twill decides how much of the total requested memory to allocate to heap based on the hardcoded ratio of 0.7, meaning it will allocate at least 70% to heap. There could be applications that use direct memory quite a bit and they want to allocate more then 30% of total memory to be direct memory. This is a rational behind this JIRA.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on the issue:

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

          The approach seems fine. Just couple comments about the naming and not to allow `null` ratio.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on the issue: https://github.com/apache/twill/pull/33 The approach seems fine. Just couple comments about the naming and not to allow `null` ratio.
          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/33#discussion_r101699381

          — Diff: twill-api/src/main/java/org/apache/twill/api/Configs.java —
          @@ -75,6 +82,7 @@ private Keys() {
          */
          public static final int JAVA_RESERVED_MEMORY_MB = 200;

          + public static final double HEAP_RESERVED_MIN_RATIO_DEFAULT = Constants.HEAP_MIN_RATIO;
          — End diff –

          You can remove the `Constants.HEAP_MIN_RATIO` and just define the default here.

          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/33#discussion_r101699381 — Diff: twill-api/src/main/java/org/apache/twill/api/Configs.java — @@ -75,6 +82,7 @@ private Keys() { */ public static final int JAVA_RESERVED_MEMORY_MB = 200; + public static final double HEAP_RESERVED_MIN_RATIO_DEFAULT = Constants.HEAP_MIN_RATIO; — End diff – You can remove the `Constants.HEAP_MIN_RATIO` and just define the default here.
          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/33#discussion_r101699432

          — Diff: twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java —
          @@ -41,11 +41,23 @@
          private final String rmSchedulerAddr;
          private final Map<String, Map<String, String>> logLevels;
          private final Map<String, Integer> maxRetries;
          + private Double heapToReserveRatio;
          — End diff –

          Shouldn't allow null. This is an internal class, you can simply require the caller to pass in valid value.

          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/33#discussion_r101699432 — Diff: twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java — @@ -41,11 +41,23 @@ private final String rmSchedulerAddr; private final Map<String, Map<String, String>> logLevels; private final Map<String, Integer> maxRetries; + private Double heapToReserveRatio; — End diff – Shouldn't allow null. This is an internal class, you can simply require the caller to pass in valid value.
          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/33#discussion_r101699339

          — Diff: twill-core/src/main/java/org/apache/twill/internal/TwillContainerLauncher.java —
          @@ -59,19 +59,21 @@
          private final int instanceCount;
          private final JvmOptions jvmOpts;
          private final int reservedMemory;
          + private final Double heapToReservedRatio;
          private final Location secureStoreLocation;

          public TwillContainerLauncher(RuntimeSpecification runtimeSpec, ContainerInfo containerInfo,
          ProcessLauncher.PrepareLaunchContext launchContext,
          ZKClient zkClient, int instanceCount, JvmOptions jvmOpts, int reservedMemory,

          • Location secureStoreLocation) {
            + Location secureStoreLocation, Double heapToReservedRatio) {
              • End diff –

          Better to have `minHeapRatio` a `double` and the caller need to make sure it is not null.

          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/33#discussion_r101699339 — Diff: twill-core/src/main/java/org/apache/twill/internal/TwillContainerLauncher.java — @@ -59,19 +59,21 @@ private final int instanceCount; private final JvmOptions jvmOpts; private final int reservedMemory; + private final Double heapToReservedRatio; private final Location secureStoreLocation; public TwillContainerLauncher(RuntimeSpecification runtimeSpec, ContainerInfo containerInfo, ProcessLauncher.PrepareLaunchContext launchContext, ZKClient zkClient, int instanceCount, JvmOptions jvmOpts, int reservedMemory, Location secureStoreLocation) { + Location secureStoreLocation, Double heapToReservedRatio) { End diff – Better to have `minHeapRatio` a `double` and the caller need to make sure it is not null.
          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/33#discussion_r101699525

          — Diff: twill-core/src/main/java/org/apache/twill/internal/json/TwillRuntimeSpecificationCodec.java —
          @@ -45,6 +45,7 @@
          private static final String TWILL_RUNID = "twillRunId";
          private static final String TWILL_APP_NAME = "twillAppName";
          private static final String RESERVED_MEMORY = "reservedMemory";
          + private static final String HEAP_TO_RESERVED_RATIO = "heapToReservedRatio";
          — End diff –

          for consistency, just have everywhere call it `minHeapRatio`

          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/33#discussion_r101699525 — Diff: twill-core/src/main/java/org/apache/twill/internal/json/TwillRuntimeSpecificationCodec.java — @@ -45,6 +45,7 @@ private static final String TWILL_RUNID = "twillRunId"; private static final String TWILL_APP_NAME = "twillAppName"; private static final String RESERVED_MEMORY = "reservedMemory"; + private static final String HEAP_TO_RESERVED_RATIO = "heapToReservedRatio"; — End diff – for consistency, just have everywhere call it `minHeapRatio`
          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/33#discussion_r101699224

          — Diff: twill-api/src/main/java/org/apache/twill/api/Configs.java —
          @@ -34,6 +36,11 @@
          public static final String JAVA_RESERVED_MEMORY_MB = "twill.java.reserved.memory.mb";

          /**
          + * Configurable ratio between Heap and Reserved Memory
          + */
          + public static final String HEAP_RESERVED_MIN_RATIO_CONFIG = "twill.java.reserved.memory.ratio";
          — End diff –

          Also, I think the config key should be `twill.java.min.heap.memory.ratio`, as it is the minimum ratio of heap memory.

          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/33#discussion_r101699224 — Diff: twill-api/src/main/java/org/apache/twill/api/Configs.java — @@ -34,6 +36,11 @@ public static final String JAVA_RESERVED_MEMORY_MB = "twill.java.reserved.memory.mb"; /** + * Configurable ratio between Heap and Reserved Memory + */ + public static final String HEAP_RESERVED_MIN_RATIO_CONFIG = "twill.java.reserved.memory.ratio"; — End diff – Also, I think the config key should be `twill.java.min.heap.memory.ratio`, as it is the minimum ratio of heap memory.
          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/33#discussion_r101698978

          — Diff: twill-api/src/main/java/org/apache/twill/api/Configs.java —
          @@ -75,6 +82,7 @@ private Keys() {
          */
          public static final int JAVA_RESERVED_MEMORY_MB = 200;

          + public static final double HEAP_RESERVED_MIN_RATIO_DEFAULT = Constants.HEAP_MIN_RATIO;
          — End diff –

          Inside the `Defaults` class, no need to repeat the variable with `DEFAULT`. Just name it the same as the config key constant.

          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/33#discussion_r101698978 — Diff: twill-api/src/main/java/org/apache/twill/api/Configs.java — @@ -75,6 +82,7 @@ private Keys() { */ public static final int JAVA_RESERVED_MEMORY_MB = 200; + public static final double HEAP_RESERVED_MIN_RATIO_DEFAULT = Constants.HEAP_MIN_RATIO; — End diff – Inside the `Defaults` class, no need to repeat the variable with `DEFAULT`. Just name it the same as the config key constant.
          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/33#discussion_r101699007

          — Diff: twill-api/src/main/java/org/apache/twill/api/Configs.java —
          @@ -34,6 +36,11 @@
          public static final String JAVA_RESERVED_MEMORY_MB = "twill.java.reserved.memory.mb";

          /**
          + * Configurable ratio between Heap and Reserved Memory
          + */
          + public static final String HEAP_RESERVED_MIN_RATIO_CONFIG = "twill.java.reserved.memory.ratio";
          — End diff –

          No need to have `_CONFIG` as it is already defined inside the `Keys` inner class.

          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/33#discussion_r101699007 — Diff: twill-api/src/main/java/org/apache/twill/api/Configs.java — @@ -34,6 +36,11 @@ public static final String JAVA_RESERVED_MEMORY_MB = "twill.java.reserved.memory.mb"; /** + * Configurable ratio between Heap and Reserved Memory + */ + public static final String HEAP_RESERVED_MIN_RATIO_CONFIG = "twill.java.reserved.memory.ratio"; — End diff – No need to have `_CONFIG` as it is already defined inside the `Keys` inner class.
          Hide
          chtyim Terence Yim added a comment -

          Yuliya Feldman I am ok with your approach. If you have time to address the comments by tomorrow, I will include the fix in the coming 0.10.0 release as well.

          Show
          chtyim Terence Yim added a comment - Yuliya Feldman I am ok with your approach. If you have time to address the comments by tomorrow, I will include the fix in the coming 0.10.0 release as well.
          Hide
          yufeldman Yuliya Feldman added a comment -

          Terence Yim Absolutely - I'll try to address your comments ASAP

          Show
          yufeldman Yuliya Feldman added a comment - Terence Yim Absolutely - I'll try to address your comments ASAP
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hsaputra commented on a diff in the pull request:

          https://github.com/apache/twill/pull/33#discussion_r101708864

          — Diff: twill-core/src/main/java/org/apache/twill/internal/TwillContainerLauncher.java —
          @@ -59,19 +59,21 @@
          private final int instanceCount;
          private final JvmOptions jvmOpts;
          private final int reservedMemory;
          + private final Double heapToReservedRatio;
          private final Location secureStoreLocation;

          public TwillContainerLauncher(RuntimeSpecification runtimeSpec, ContainerInfo containerInfo,
          ProcessLauncher.PrepareLaunchContext launchContext,
          ZKClient zkClient, int instanceCount, JvmOptions jvmOpts, int reservedMemory,

          • Location secureStoreLocation) {
            + Location secureStoreLocation, Double heapToReservedRatio) {
              • End diff –

          do we need double here? Would float be ok?

          Show
          githubbot ASF GitHub Bot added a comment - Github user hsaputra commented on a diff in the pull request: https://github.com/apache/twill/pull/33#discussion_r101708864 — Diff: twill-core/src/main/java/org/apache/twill/internal/TwillContainerLauncher.java — @@ -59,19 +59,21 @@ private final int instanceCount; private final JvmOptions jvmOpts; private final int reservedMemory; + private final Double heapToReservedRatio; private final Location secureStoreLocation; public TwillContainerLauncher(RuntimeSpecification runtimeSpec, ContainerInfo containerInfo, ProcessLauncher.PrepareLaunchContext launchContext, ZKClient zkClient, int instanceCount, JvmOptions jvmOpts, int reservedMemory, Location secureStoreLocation) { + Location secureStoreLocation, Double heapToReservedRatio) { End diff – do we need double here? Would float be ok?
          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/33#discussion_r101789313

          — Diff: twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillPreparer.java —
          @@ -17,6 +17,8 @@
          */
          package org.apache.twill.yarn;

          +import joptsimple.OptionSpec;
          — End diff –

          Please don't reorder import.

          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/33#discussion_r101789313 — Diff: twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillPreparer.java — @@ -17,6 +17,8 @@ */ package org.apache.twill.yarn; +import joptsimple.OptionSpec; — End diff – Please don't reorder import.
          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/33#discussion_r101789099

          — Diff: twill-core/src/main/java/org/apache/twill/internal/json/TwillRuntimeSpecificationCodec.java —
          @@ -94,6 +96,8 @@ public TwillRuntimeSpecification deserialize(JsonElement json, Type typeOfT,
          jsonObj.has(RM_SCHEDULER_ADDR) ?
          jsonObj.get(RM_SCHEDULER_ADDR).getAsString() : null,
          logLevels,

          • maxRetries);
            + maxRetries,
            + jsonObj.has(HEAP_RESERVED_MIN_RATIO) ?
              • End diff –

          Should use HEAP_RESERVED_MIN_RATIO as the default instead of `null`

          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/33#discussion_r101789099 — Diff: twill-core/src/main/java/org/apache/twill/internal/json/TwillRuntimeSpecificationCodec.java — @@ -94,6 +96,8 @@ public TwillRuntimeSpecification deserialize(JsonElement json, Type typeOfT, jsonObj.has(RM_SCHEDULER_ADDR) ? jsonObj.get(RM_SCHEDULER_ADDR).getAsString() : null, logLevels, maxRetries); + maxRetries, + jsonObj.has(HEAP_RESERVED_MIN_RATIO) ? End diff – Should use HEAP_RESERVED_MIN_RATIO as the default instead of `null`
          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/33#discussion_r101789920

          — Diff: twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java —
          @@ -38,7 +38,6 @@
          import com.google.common.util.concurrent.SettableFuture;
          import com.google.gson.Gson;
          import com.google.gson.GsonBuilder;
          — End diff –

          Please don't update import spaces. If you use IntelliJ or eclipse, you can download the style from the twill site.

          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/33#discussion_r101789920 — Diff: twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java — @@ -38,7 +38,6 @@ import com.google.common.util.concurrent.SettableFuture; import com.google.gson.Gson; import com.google.gson.GsonBuilder; — End diff – Please don't update import spaces. If you use IntelliJ or eclipse, you can download the style from the twill site.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user yufeldman commented on a diff in the pull request:

          https://github.com/apache/twill/pull/33#discussion_r101817212

          — Diff: twill-core/src/main/java/org/apache/twill/internal/json/TwillRuntimeSpecificationCodec.java —
          @@ -94,6 +96,8 @@ public TwillRuntimeSpecification deserialize(JsonElement json, Type typeOfT,
          jsonObj.has(RM_SCHEDULER_ADDR) ?
          jsonObj.get(RM_SCHEDULER_ADDR).getAsString() : null,
          logLevels,

          • maxRetries);
            + maxRetries,
            + jsonObj.has(HEAP_RESERVED_MIN_RATIO) ?
              • End diff –

          sure

          Show
          githubbot ASF GitHub Bot added a comment - Github user yufeldman commented on a diff in the pull request: https://github.com/apache/twill/pull/33#discussion_r101817212 — Diff: twill-core/src/main/java/org/apache/twill/internal/json/TwillRuntimeSpecificationCodec.java — @@ -94,6 +96,8 @@ public TwillRuntimeSpecification deserialize(JsonElement json, Type typeOfT, jsonObj.has(RM_SCHEDULER_ADDR) ? jsonObj.get(RM_SCHEDULER_ADDR).getAsString() : null, logLevels, maxRetries); + maxRetries, + jsonObj.has(HEAP_RESERVED_MIN_RATIO) ? End diff – sure
          Hide
          yufeldman Yuliya Feldman added a comment -

          Terence Yim Thank you for the reviews, I have updated PR with latest changes, also squashed commits

          Show
          yufeldman Yuliya Feldman added a comment - Terence Yim Thank you for the reviews, I have updated PR with latest changes, also squashed commits
          Hide
          yufeldman Yuliya Feldman added a comment -

          Sorry, missed one style change. Will update PR in a minute

          Show
          yufeldman Yuliya Feldman added a comment - Sorry, missed one style change. Will update PR in a minute
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on the issue:

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

          LGTM. Will merge it when build pass.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on the issue: https://github.com/apache/twill/pull/33 LGTM. Will merge it when build pass.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hsaputra commented on the issue:

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

          @chtyim I was asking concern about the selection of double vs float

          Show
          githubbot ASF GitHub Bot added a comment - Github user hsaputra commented on the issue: https://github.com/apache/twill/pull/33 @chtyim I was asking concern about the selection of double vs float
          Hide
          yufeldman Yuliya Feldman added a comment -

          Henry Saputra What is your concern regarding double versus float?

          We already use double for that ratio.

          Show
          yufeldman Yuliya Feldman added a comment - Henry Saputra What is your concern regarding double versus float? We already use double for that ratio.
          Hide
          hsaputra Henry Saputra added a comment -

          Yuliya Feldman Gah, yeah you are right. I thought we had float for the initial ratio and change it to double. My mistake

          Show
          hsaputra Henry Saputra added a comment - Yuliya Feldman Gah, yeah you are right. I thought we had float for the initial ratio and change it to double. My mistake
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hsaputra commented on the issue:

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

          Ah, Yulia just replied on JIRA that we HAD ALREADY used double for the ratio (not sure why) but this PR just continuing the existing data format.

          +1 then

          Show
          githubbot ASF GitHub Bot added a comment - Github user hsaputra commented on the issue: https://github.com/apache/twill/pull/33 Ah, Yulia just replied on JIRA that we HAD ALREADY used double for the ratio (not sure why) but this PR just continuing the existing data format. +1 then
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

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

          Thank you guys for quick turnaround

          Show
          yufeldman Yuliya Feldman added a comment - Thank you guys for quick turnaround

            People

            • Assignee:
              yufeldman Yuliya Feldman
              Reporter:
              yufeldman Yuliya Feldman
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development