tillrohrmann commented on a change in pull request #12520:
URL: https://github.com/apache/flink/pull/12520#discussion_r436560717
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/util/config/memory/ProcessMemoryUtils.java
##########
@@ -158,6 +156,25 @@ public JvmMetaspaceAndOverhead
deriveJvmMetaspaceAndOverheadFromTotalFlinkMemory
return jvmMetaspaceAndOverhead;
}
+ private MemorySize
deriveJvmOverheadFromTotalFlinkMemoryAndOtherComponents(
+ Configuration config,
+ MemorySize totalFlinkMemorySize) {
+ MemorySize totalProcessMemorySize =
getMemorySizeFromConfig(config, options.getTotalProcessMemoryOption());
+ MemorySize jvmMetaspaceSize = getMemorySizeFromConfig(config,
options.getJvmOptions().getJvmMetaspaceOption());
+ MemorySize totalFlinkAndJvmMetaspaceSize =
totalFlinkMemorySize.add(jvmMetaspaceSize);
Review comment:
It seems as if we are doing these computations twice. Once here and in
`deriveJvmMetaspaceAndOverheadFromTotalFlinkMemory` before this method here is
called.
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/util/config/memory/ProcessMemoryUtilsTestBase.java
##########
@@ -350,4 +368,35 @@ public MemorySize getTotalProcessMemorySize() {
throw new UnsupportedOperationException();
}
}
+
+ private static class DummyFlinkMemoryUtils implements
FlinkMemoryUtils<DummyFlinkMemory> {
+ @Override
+ public DummyFlinkMemory
deriveFromRequiredFineGrainedOptions(Configuration config) {
+ return new DummyFlinkMemory();
+ }
+
+ @Override
+ public DummyFlinkMemory
deriveFromTotalFlinkMemory(Configuration config, MemorySize
totalFlinkMemorySize) {
+ throw new UnsupportedOperationException();
+ }
+ }
+
+ private static class DummyFlinkMemory implements FlinkMemory {
+ private static final long serialVersionUID = 1L;
+
+ @Override
+ public MemorySize getJvmHeapMemorySize() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public MemorySize getJvmDirectMemorySize() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public MemorySize getTotalFlinkMemorySize() {
+ throw new UnsupportedOperationException();
+ }
+ }
Review comment:
I don't really understand why we need these helper classes. Looking at
the method bodies it looks as if they are not really needed. The fact that we
need to provide them for the test is an indicator that there is something wrong
with the code abstraction as the actual change is not easily testable.
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/util/config/memory/ProcessMemoryUtilsTestBase.java
##########
@@ -350,4 +368,35 @@ public MemorySize getTotalProcessMemorySize() {
throw new UnsupportedOperationException();
}
}
+
+ private static class DummyFlinkMemoryUtils implements
FlinkMemoryUtils<DummyFlinkMemory> {
Review comment:
Meta comment: `FlinkMemoryUtils` does not seem to be documented well
enough. I would suggest that every interface method should get a JavaDoc
explaining what it does. Also the JavaDocs for the implementations of
`FlinkMemoryUtils` are scarce at most.
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/util/config/memory/ProcessMemoryUtils.java
##########
@@ -158,6 +156,25 @@ public JvmMetaspaceAndOverhead
deriveJvmMetaspaceAndOverheadFromTotalFlinkMemory
return jvmMetaspaceAndOverhead;
}
+ private MemorySize
deriveJvmOverheadFromTotalFlinkMemoryAndOtherComponents(
Review comment:
I think the method `deriveProcessSpecWithTotalProcessMemory` could also
be susceptible to the same problem.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
