YARN-11745: Fix TimSort contract violation in PriorityQueueComparator Class #7278
+57
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
|
Thanks @Hean-Chhinling for the fix!
|
K0K0V0K
suggested changes
Jan 9, 2025
...er/resourcemanager/scheduler/capacity/policy/TestPriorityUtilizationQueueOrderingPolicy.java
Outdated
Show resolved
Hide resolved
|
I think there are some check style error what should be fixed: |
Thank you for pointing this out. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
K0K0V0K
approved these changes
Jan 10, 2025
brumi1024
reviewed
Jan 13, 2025
There was a problem hiding this comment.
Thanks @Hean-Chhinling for the patch! Nice find and clean fix, I only had some smaller styling comments.
Comment on lines
350
to
357
| QueueCapacities mockQueueCapacities = mock(QueueCapacities.class); | ||
| when(mockQueueCapacities.getAbsoluteUsedCapacity(partition)) | ||
| .thenReturn(4.2f); // Could be any number | ||
| when(mockQueueCapacities.getUsedCapacity(partition)) | ||
| .thenReturn(1.0f); // Could be any number | ||
| when(mockQueueCapacities.getAbsoluteCapacity(partition)) | ||
| .thenReturn(6.2f); // Could be any number | ||
|
|
There was a problem hiding this comment.
Nit: why not use the randomQueueCapacities method below? Would make the code a bit cleaner.
There was a problem hiding this comment.
Thank you for the review @brumi1024!
I try using the randomQueueCapacities method but the exception did not occur.
The reason that I am using the this mockQueueCapacities is to have the same values for queues capacities that are generated. So that the PriorityQueueComparator class compare methods will fall into using the configureMinResource value.
There was a problem hiding this comment.
I see, thanks. In that case I think the "// Could be any number" comment is a bit misleading, maybe we could specify that it could be any specific number, so that there are equal values.
Comment on lines
366
to
370
| QueueResourceQuotas mockResourceQuotas = mock(QueueResourceQuotas.class); | ||
| when(mockResourceQuotas.getConfiguredMinResource(partition)) | ||
| .thenReturn(resource); | ||
| when(mockQueue.getQueueResourceQuotas()) | ||
| .thenReturn(mockResourceQuotas); |
There was a problem hiding this comment.
Same here, randomResourceQuotas seems to be similar functionality. AFAICS the (0,0) case is the reason this was needed, that probably could be solved by creating a "zeroResourceQuotas" or something similar which ensures a few queues with zero minResource are present.
There was a problem hiding this comment.
Thank you for the suggestion here, I have refactored my code to use randomResourceQuotas. And randomly override queue resource to zero.
boolean isZeroResource = ThreadLocalRandom.current().nextBoolean();
if (isZeroResource) {
resourceQuotas.setConfiguredMinResource(partition, Resource.newInstance(0, 0));
}
...er/resourcemanager/scheduler/capacity/policy/TestPriorityUtilizationQueueOrderingPolicy.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
brumi1024
reviewed
Jan 16, 2025
Comment on lines
350
to
357
| QueueCapacities mockQueueCapacities = mock(QueueCapacities.class); | ||
| when(mockQueueCapacities.getAbsoluteUsedCapacity(partition)) | ||
| .thenReturn(4.2f); // Could be any number | ||
| when(mockQueueCapacities.getUsedCapacity(partition)) | ||
| .thenReturn(1.0f); // Could be any number | ||
| when(mockQueueCapacities.getAbsoluteCapacity(partition)) | ||
| .thenReturn(6.2f); // Could be any number | ||
|
|
There was a problem hiding this comment.
I see, thanks. In that case I think the "// Could be any number" comment is a bit misleading, maybe we could specify that it could be any specific number, so that there are equal values.
| @@ -331,6 +386,7 @@ private QueueResourceQuotas randomResourceQuotas(String partition) { | |||
| QueueResourceQuotas qr = new QueueResourceQuotas(); | |||
| qr.setConfiguredMinResource(partition, | |||
| Resource.newInstance(randInt(1, 10) * 1024, randInt(1, 10))); | |||
| // System.out.println("Quotas: " + qr.getConfiguredMinResource(partition)); | |||
There was a problem hiding this comment.
It is not needed, I will delete it. It was used during my testing that I forgot to remove it.
Comment on lines
321
to
323
| queues.add(createMockPriorityQueueResourcesForSorting(partition)); | ||
| queues.add(createMockPriorityQueueResourcesForSorting(partition)); | ||
| queues.add(createMockPriorityQueueResourcesForSorting(partition)); |
There was a problem hiding this comment.
Since there are no values supplied in the params the 3 calls seems unnecessary, you could simply increate the limit from 300 to 900.
There was a problem hiding this comment.
Thank you, I am sorry for the oversight.
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
This PR addresses the TimSort contract violation issue in the
PriorityQueueComparatorclass, which was identified when sorting queue with resource (0, 0) and queue resource(any number, any number). The comparator previously failed to maintain transitivity of TimSort, leading tojava.lang.IllegalArgumentException: Comparison method violates its general contract!during sorting.Root Cause
The issue occurred due to inconsistent comparison logic that violated the transitivity rules required by TimSort.
Specifically, at the following code lines the AND condition that only compare the resources if both queues' resources are not none. However, when one of the queue resource is
none or (0, 0)and the other queue resource isnot noneit skips this condition and go to compare based on theabsoluteCapacityand when both of the queues'absoluteCapacityare the same, it leads to both queues equal each other even though their resources are different.For more detail example of how this behaviour break the TimSort algorithm please see this attachment. ExampleZeroQueueResourceProblem
Solution
Instead of checking if both queues' resource are not none. We should only check if one of the queue's resource is not none. This way to avoid skipping the queue resource comparison when we have one queue resource is not none and the other one is none. Specifically, change from AND condition to OR condition at the following codes:
if (!minEffRes1.equals(Resources.none()) || !minEffRes2.equals( Resources.none())) { return minEffRes2.compareTo(minEffRes1); }Testing
Added a unit test
testPriorityQueueComparatorClassDoesNotViolateTimSortContractto verify that sorting no longer throwsjava.lang.IllegalArgumentException: Comparison method violates its general contract!.The test includes setting resource instance (0, 0) and resource(any number, any number) then shuffle the repeated queues that were created and then sort in using the
PriorityQueueComparatorclass. It also mock the necessary elements, for example,prioritylabel,absoluteUsedCapacity,usedCapacityandabsoluteCapacity. These elements can be of any number.Impact
TimSortviolation, ensuring stable and predictable sorting forPriorityQueueComparatorclass.PriorityQueueComparatorsorting algorithm may change in some behaviour when the queue resource is (0, 0).For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?