nwangtw commented on a change in pull request #3142: Validate resource
constraint (RAM and CPU) in RoundRobinPacking
URL: https://github.com/apache/incubator-heron/pull/3142#discussion_r245445543
##########
File path:
heron/packing/src/java/org/apache/heron/packing/roundrobin/RoundRobinPacking.java
##########
@@ -133,16 +142,36 @@ private PackingPlan packInternal(int numContainer,
Map<String, Integer> parallel
// Get the RAM map for every instance
Map<Integer, Map<InstanceId, ByteAmount>> instancesRamMap =
- getInstancesRamMapInContainer(roundRobinAllocation);
+ calculateInstancesResourceMapInContainer(
+ roundRobinAllocation,
+ TopologyUtils.getComponentRamMapConfig(topology),
+ getContainerRamHint(roundRobinAllocation),
+ instanceRamDefault,
+ containerRamPadding,
+ ByteAmount.ZERO,
+ NOT_SPECIFIED_BYTE_AMOUNT,
+ RAM);
+
+ // Get the CPU map for every instance
+ Map<Integer, Map<InstanceId, CPUShare>> instancesCpuMap =
+ calculateInstancesResourceMapInContainer(
+ roundRobinAllocation,
+
CPUShare.convertDoubleMapToCpuShareMap(TopologyUtils.getComponentCpuMapConfig(topology)),
+ CPUShare.fromDouble(getContainerCpuHint(roundRobinAllocation)),
+ CPUShare.fromDouble(instanceCpuDefault),
+ CPUShare.fromDouble(containerCpuPadding),
+ CPUShare.fromDouble(0.0),
+ CPUShare.fromDouble(NOT_SPECIFIED_CPU_SHARE),
+ CPU);
ByteAmount containerDiskInBytes =
getContainerDiskHint(roundRobinAllocation);
- double containerCpu = getContainerCpuHint(roundRobinAllocation);
+ double containerCpuHint = getContainerCpuHint(roundRobinAllocation);
Review comment:
That is true. Assuming user configures:
- container cpu to be 5
- 1 cpu per instance
- 3 instances in container
Then:
cpu hint is 5,
instance cpu is valid (enough for padding and instances)
In the old calculation, container cpu is 5 because user configured it.
In the new calculation, container cpu is 4 (1 * 3 + 1 padding).
With "max(containerCpuHint, containerCpu)", container cpu is the same as
before.
I think it is important to make the result consistent. Some users might rely
on setting container cpu to allocate more processing power to their instances
instead of configuring per instance cpu (although it is technically correct).
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
