gyfora commented on code in PR #670:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/670#discussion_r1324028361
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/config/FlinkConfigBuilder.java:
##########
@@ -420,13 +421,24 @@ private void setResource(Resource resource, Configuration
effectiveConfig, boole
? JobManagerOptions.TOTAL_PROCESS_MEMORY
: TaskManagerOptions.TOTAL_PROCESS_MEMORY;
if (resource.getMemory() != null) {
- effectiveConfig.setString(memoryConfigOption.key(),
resource.getMemory());
+ effectiveConfig.setString(
+ memoryConfigOption.key(),
parseResourceMemoryString(resource.getMemory()));
}
configureCpu(resource, effectiveConfig, isJM);
}
}
+ // Using the K8s units specification for the JM and TM memory settings
+ private String parseResourceMemoryString(String memory) {
+ try {
+ return MemorySize.parse(memory).toString();
Review Comment:
I am completely confused here. You force pushed the branch with a completely
different logic which I think is not fully backward compatible in the sense
that the same spec will lead to now different memory setting.
In this case it would have been nice not to force push it so we can see the
2 alternatives side by side (in 2 commits).
In any case I thought the cleanest solution would be:
```
try {
// Parse like before with Flink memory utils
} catch (parse error) {
// Parse remaining cases that were previously uncovered
}
```
(and I think this was the original code in this PR)
In your example with `2g` your current code will silently reduce the
available memory by 7%. I think 7% reduction may have some critical prod
impact. I understand that it would be better to have this consistent with k8s
than with Flink but this is not something we can afford to do in a minor
release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
