zhuzhurk commented on a change in pull request #14955:
URL: https://github.com/apache/flink/pull/14955#discussion_r578191731
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/types/ResourceProfile.java
##########
@@ -453,10 +456,15 @@ public ResourceProfile multiply(final int multiplier) {
return UNKNOWN;
}
- Map<String, Resource> resultExtendedResource = new
HashMap<>(extendedResources.size());
- for (Map.Entry<String, Resource> entry : extendedResources.entrySet())
{
- resultExtendedResource.put(entry.getKey(),
entry.getValue().multiply(multiplier));
- }
+ Map<String, Resource> resultExtendedResource =
Review comment:
Maybe we simply returns ResourceProfile.ZERO when multiplier == 0?
##########
File path:
flink-core/src/main/java/org/apache/flink/api/common/operators/ResourceSpec.java
##########
@@ -175,9 +177,7 @@ public ResourceSpec subtract(final ResourceSpec other) {
resource,
(v1, v2) -> {
final Resource subtracted = v1.subtract(v2);
- return
subtracted.getValue().compareTo(BigDecimal.ZERO) == 0
- ? null
- : subtracted;
+ return subtracted.isZero() ? null : subtracted;
Review comment:
maybe simplify it because we already have the filter in constructor?
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/clusterframework/types/ResourceProfileTest.java
##########
@@ -514,6 +514,33 @@ public void
includesCPUAndMemoryInToStringIfTheyAreBelowThreshold() {
assertThat(resourceProfile.toString(), allOf(containsCPUCores(),
containsTaskHeapMemory()));
}
+ @Test
+ public void testZeroExtendedResourceFromConstructor() {
+ final ResourceProfile resourceProfile =
+ ResourceProfile.newBuilder()
+ .addExtendedResource("gpu", new GPUResource(0.0))
+ .build();
+ assertEquals(resourceProfile.getExtendedResources().size(), 0);
+ }
+
+ @Test
+ public void testZeroExtendedResourceFromSubtract() {
+ final ResourceProfile resourceProfile =
+ ResourceProfile.newBuilder()
+ .addExtendedResource("gpu", new GPUResource(1.0))
+ .build();
+
assertEquals(resourceProfile.subtract(resourceProfile).getExtendedResources().size(),
0);
+ }
+
+ @Test
+ public void testZeroExtendedResourceFromMultiply() {
+ final ResourceProfile resourceProfile =
+ ResourceProfile.newBuilder()
+ .addExtendedResource("gpu", new GPUResource(1.0))
+ .build();
+
assertEquals(resourceProfile.multiply(0).getExtendedResources().size(), 0);
Review comment:
maybe check whether the result is ResourceProfile.ZERO?
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/types/ResourceProfile.java
##########
@@ -427,9 +432,7 @@ public ResourceProfile subtract(final ResourceProfile
other) {
name,
(ignored, oldResource) -> {
Resource resultResource =
oldResource.subtract(resource);
- return
resultResource.getValue().compareTo(BigDecimal.ZERO) == 0
- ? null
- : resultResource;
+ return resultResource.isZero() ? null :
resultResource;
Review comment:
I think we no longer need to filter it in ahead after introducing the
filter in constructor?
----------------------------------------------------------------
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]
