[GitHub] [flink] zhuzhurk commented on a change in pull request #10179: [FLINK-14734][runtime] Add a ResourceSpec in SlotSharingGroup to describe its overall resources

[GitBox <gi...@apache.org>]

zhuzhurk commented on a change in pull request #10179: [FLINK-14734][runtime] Add a ResourceSpec in SlotSharingGroup to describe its overall resources
URL: https://github.com/apache/flink/pull/10179#discussion_r346921259
 
 

 ##########
 File path: flink-core/src/main/java/org/apache/flink/api/common/operators/ResourceSpec.java
 ##########
 @@ -149,6 +150,39 @@ public ResourceSpec merge(final ResourceSpec other) {
 		return target;
 	}
 
+	/**
+	 * Subtracts another resource spec from this one.
+	 *
+	 * @param other The other resource spec to subtract.
+	 * @return The subtracted resource spec.
+	 */
+	public ResourceSpec subtract(final ResourceSpec other) {
+		checkNotNull(other, "Cannot subtract null resources");
+
+		if (this.equals(UNKNOWN) || other.equals(UNKNOWN)) {
+			return UNKNOWN;
+		}
+
+		checkArgument(other.lessThanOrEqual(this), "Cannot subtract a larger ResourceSpec from this one.");
+
+		final ResourceSpec target = new ResourceSpec(
+			this.cpuCores.subtract(other.cpuCores),
+			this.taskHeapMemory.subtract(other.taskHeapMemory),
+			this.taskOffHeapMemory.subtract(other.taskOffHeapMemory),
+			this.onHeapManagedMemory.subtract(other.onHeapManagedMemory),
+			this.offHeapManagedMemory.subtract(other.offHeapManagedMemory));
+
+		target.extendedResources.putAll(extendedResources);
+
+		for (Resource resource : other.extendedResources.values()) {
+			target.extendedResources.merge(resource.getName(), resource, (v1, v2) -> {
+				final Resource subtracted = v1.subtract(v2);
+				return subtracted.getValue().compareTo(BigDecimal.ZERO) == 0 ? null : subtracted;
 
 Review comment:
   It's a good question. And I need to consider how we should treat a value 0 Resource.
   
   CPUResource with value 0 is used in production currently, thus Resource must accept value 0. And CPUResource would not be null in ResourceSpec/ResourceProfile (except for the special UNKNOWN case where null means unknown value).
   
   However, 0 value extends Resource is a bit confusing, since 0 and null also mean no such resource. If accepting both of them, we need to consider it in all comparisons(equals/lessThanOrEqual/isMatch) and aggregations(merge/subtract).
   
   To be simple, I think we can change it like this: in `ResourceSpec`/`ResourceProfile` we only keep extended resource with positive values. If it is 0 value, we need to drop it so the resource would always be null. This change is needed in the constructors and `subtract()`.
   
   Any usages of extended resources, must check whether it's null to see whether that extended resource is needed(for task)/available(in slot).
   
   WDYT?

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services