You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2021/01/13 04:41:26 UTC
[GitHub] [flink] xintongsong commented on a change in pull request #14612: [FLINK-20864][runtime] Introduce the DEFAULT resource profile for the…
xintongsong commented on a change in pull request #14612:
URL: https://github.com/apache/flink/pull/14612#discussion_r556238763
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/types/ResourceProfile.java
##########
@@ -271,13 +271,41 @@ public boolean isMatching(final ResourceProfile required) {
return true;
}
- if (cpuCores.getValue().compareTo(required.cpuCores.getValue()) >= 0
- && taskHeapMemory.compareTo(required.taskHeapMemory) >= 0
- && taskOffHeapMemory.compareTo(required.taskOffHeapMemory) >= 0
- && managedMemory.compareTo(required.managedMemory) >= 0
- && networkMemory.compareTo(required.networkMemory) >= 0) {
+ return false;
+ }
Review comment:
I wonder if we can remove the `if (this.equals(UNKNOWN))` branch now, and add `throwUnsupportedOperationExecptionIfUnknown` at the beginning of this method.
Ideally, the caller of this method should always represent a resource capacity rather than a requirement, thus should never be `UNKNOWN`. I suspect the reason we didn't do that previously is because this method was also used as `isBiggerThan` in some test cases.
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/slot/TaskSlotTableImpl.java
##########
@@ -319,7 +321,7 @@ public boolean allocateSlot(
taskSlot =
new TaskSlot<>(
index,
- resourceProfile,
+ effectiveResourceProfile,
Review comment:
I think changes to this file are less related to the matching rules.
Basically, for static slot requests, we now use the default profile rather than `UNKNOWN` for creating the `TaskSlot`.
* Could you explain why is this change needed?
* Even if this change is indeed needed, we should make it a separate commit.
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/clusterframework/types/ResourceProfileTest.java
##########
@@ -102,23 +102,61 @@ public void testMatchRequirement() {
.setManagedMemoryMB(100)
.setNetworkMemoryMB(100)
.build();
- assertFalse(rp4.isMatching(rp5));
+ assertFalse(rp4.isBiggerThan(rp5));
ResourceSpec rs1 = ResourceSpec.newBuilder(1.0, 100).setGPUResource(2.2).build();
ResourceSpec rs2 = ResourceSpec.newBuilder(1.0, 100).setGPUResource(1.1).build();
- assertFalse(rp1.isMatching(ResourceProfile.fromResourceSpec(rs1)));
+ assertFalse(rp1.isBiggerThan(ResourceProfile.fromResourceSpec(rs1)));
assertTrue(
ResourceProfile.fromResourceSpec(rs1)
- .isMatching(ResourceProfile.fromResourceSpec(rs2)));
+ .isBiggerThan(ResourceProfile.fromResourceSpec(rs2)));
assertFalse(
ResourceProfile.fromResourceSpec(rs2)
- .isMatching(ResourceProfile.fromResourceSpec(rs1)));
+ .isBiggerThan(ResourceProfile.fromResourceSpec(rs1)));
}
@Test
- public void testUnknownMatchesUnknown() {
Review comment:
Why is this test case removed? I think we still need to cover `UNKNOWN >= UNKNOWN`.
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/types/ResourceProfile.java
##########
@@ -271,13 +271,41 @@ public boolean isMatching(final ResourceProfile required) {
return true;
}
- if (cpuCores.getValue().compareTo(required.cpuCores.getValue()) >= 0
- && taskHeapMemory.compareTo(required.taskHeapMemory) >= 0
- && taskOffHeapMemory.compareTo(required.taskOffHeapMemory) >= 0
- && managedMemory.compareTo(required.managedMemory) >= 0
- && networkMemory.compareTo(required.networkMemory) >= 0) {
+ return false;
+ }
+
+ /**
+ * Check whether this resource profile is bigger than the given resource profile.
+ *
+ * @param other the other resource profile
+ * @return true if this resource profile is bigger, otherwise false
+ */
+ public boolean isBiggerThan(final ResourceProfile other) {
Review comment:
I'm not sure about the name `isBiggerThan`.
* First of all, the semantic here is `>=` rather than `>`
* I'm not sure `bigger` is the proper phrase for describing resource amount. Maybe `greater` or `larger`.
* I think we should emphasize the semantic that **every** field should be `>= other`, to avoid confusing this with comparing the **total** resource.
To sum up, I would suggest `allFieldsGreaterThanOrEqualTo`, `allFieldsLargerThanOrEqualTo`, or `allFieldsNoLessThan`.
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/types/ResourceProfile.java
##########
@@ -271,13 +271,41 @@ public boolean isMatching(final ResourceProfile required) {
return true;
}
- if (cpuCores.getValue().compareTo(required.cpuCores.getValue()) >= 0
- && taskHeapMemory.compareTo(required.taskHeapMemory) >= 0
- && taskOffHeapMemory.compareTo(required.taskOffHeapMemory) >= 0
- && managedMemory.compareTo(required.managedMemory) >= 0
- && networkMemory.compareTo(required.networkMemory) >= 0) {
+ return false;
+ }
+
+ /**
+ * Check whether this resource profile is bigger than the given resource profile.
+ *
+ * @param other the other resource profile
+ * @return true if this resource profile is bigger, otherwise false
+ */
+ public boolean isBiggerThan(final ResourceProfile other) {
Review comment:
And we should also explain in the `JavaDoc` what is the definition of `bigger than`, maybe with a few examples if necessary.
----------------------------------------------------------------
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