[GitHub] zhijiangW commented on a change in pull request #7185: [FLINK-10884] [yarn/mesos] adjust container memory param to set a safe margin from offheap memory
zhijiangW commented on a change in pull request #7185: [FLINK-10884] [yarn/mesos] adjust container memory param to set a safe margin from offheap memory URL: https://github.com/apache/flink/pull/7185#discussion_r238955332 ## File path: flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/ContaineredTaskManagerParameters.java ## @@ -158,8 +158,10 @@ public static ContaineredTaskManagerParameters create( // (2) split the remaining Java memory between heap and off-heap final long heapSizeMB = TaskManagerServices.calculateHeapSizeMB(containerMemoryMB - cutoffMB, config); - // use the cut-off memory for off-heap (that was its intention) - final long offHeapSizeMB = containerMemoryMB - heapSizeMB; + // (3) try to compute the offHeapMemory from a safe margin + final long restMemoryMB = containerMemoryMB - heapSizeMB; + final long offHeapCutoffMemory = calculateOffHeapCutoffMB(config, restMemoryMB); Review comment: Sorry I have not got the point of more refactor you mentioned. I think you just need to adjust both parameters of `containerized.memory-cutoff-ratio` and `containerized.memory-cutoff-min` for your project. Unless in your case, modify the existing parameters are not very convenient, and you want to add extra two parameters to avoid affecting existing ones. I think it is better to let another related committer check this option. @GJL what do you think? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] zhijiangW commented on a change in pull request #7185: [FLINK-10884] [yarn/mesos] adjust container memory param to set a safe margin from offheap memory
zhijiangW commented on a change in pull request #7185: [FLINK-10884] [yarn/mesos] adjust container memory param to set a safe margin from offheap memory URL: https://github.com/apache/flink/pull/7185#discussion_r238154920 ## File path: flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/ContaineredTaskManagerParameters.java ## @@ -158,8 +158,10 @@ public static ContaineredTaskManagerParameters create( // (2) split the remaining Java memory between heap and off-heap final long heapSizeMB = TaskManagerServices.calculateHeapSizeMB(containerMemoryMB - cutoffMB, config); - // use the cut-off memory for off-heap (that was its intention) - final long offHeapSizeMB = containerMemoryMB - heapSizeMB; + // (3) try to compute the offHeapMemory from a safe margin + final long restMemoryMB = containerMemoryMB - heapSizeMB; + final long offHeapCutoffMemory = calculateOffHeapCutoffMB(config, restMemoryMB); Review comment: I agree that both ways can work well. If we introduce the parameter `containerized.offheap-cutoff-ratio`, do you think we should also introduce `containerized.offheap-cutoff-min` to keep the same behavior with previous parameters? I suggest naming the current `containerized.heap-cutoff-ratio` to `containerized.memory-cutoff-ratio` to integrate all the memory overhead issues for below reasons: 1. Less parameters seem better sometimes, but not always. If you want to cut off 100 heap memory, and 200 off-heap memory, then you can cut off 300 memory directly. It does not matter and no control how the 300 memory are used by heap or off-heap. And it is actually stolen by any memory usages as long as no exceeding the container physical memory. 2. Minimum change for only refactor the existing parameter name. Of course I can also accept the separate parameters if you insist on it. :) This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] zhijiangW commented on a change in pull request #7185: [FLINK-10884] [yarn/mesos] adjust container memory param to set a safe margin from offheap memory
zhijiangW commented on a change in pull request #7185: [FLINK-10884] [yarn/mesos] adjust container memory param to set a safe margin from offheap memory URL: https://github.com/apache/flink/pull/7185#discussion_r237743628 ## File path: flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/ContaineredTaskManagerParameters.java ## @@ -158,8 +158,10 @@ public static ContaineredTaskManagerParameters create( // (2) split the remaining Java memory between heap and off-heap final long heapSizeMB = TaskManagerServices.calculateHeapSizeMB(containerMemoryMB - cutoffMB, config); - // use the cut-off memory for off-heap (that was its intention) - final long offHeapSizeMB = containerMemoryMB - heapSizeMB; + // (3) try to compute the offHeapMemory from a safe margin + final long restMemoryMB = containerMemoryMB - heapSizeMB; + final long offHeapCutoffMemory = calculateOffHeapCutoffMB(config, restMemoryMB); Review comment: Currently we already have a `containerized.heap-cutoff-ratio` for reserving some memories for other usages. And the `heapSizeMB` is calculated based on `containerMemoryMB - cutoffMB`, so the `heapSizeMB+ offHeapSizeMB` should be `containerMemoryMB-cutoffMB`. You further extend the `cutoff-ratio` to `containerized.offheap-cutoff-ratio`. I think there are two options: 1. Adjust the existing `containerized.heap-cutoff-ratio` to `containerized.cutoff-ratio` which means reserving some physical memories used for both heap and off-heap. 2. Separate into two different parameters as you provide. I am not sure whether it can get extra benefits compared with first option. But it may make the things a little complicated, because the memory can be further divided into heap, direct, native (used by rockdb state backend). The direct and native memories can be both regarded as off-heap in general speaking. If to do so, do we also need `containerized.offheap-cutoff-min` matched with existing `containerized.heap-cutoff-min`? BTW, I think you can increase the current `containerized.heap-cutoff-ratio` and `containerized.heap-cutoff-min` to avoid container killed because of exceeding memories. :) This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
