Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

YARN-11745: Fix TimSort contract violation in PriorityQueueComparator Class #7278

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -91,12 +91,12 @@ public static int compare(double relativeAssigned1, double relativeAssigned2,
/**
* Comparator that both looks at priority and utilization
*/
final private class PriorityQueueComparator
final public class PriorityQueueComparator
implements Comparator<PriorityQueueResourcesForSorting> {

final private String partition;

private PriorityQueueComparator(String partition) {
public PriorityQueueComparator(String partition) {
this.partition = partition;
}

Expand Down Expand Up @@ -164,7 +164,7 @@ private int compare(PriorityQueueResourcesForSorting q1Sort,
q1Sort.configuredMinResource;
Resource minEffRes2 =
q2Sort.configuredMinResource;
if (!minEffRes1.equals(Resources.none()) && !minEffRes2.equals(
if (!minEffRes1.equals(Resources.none()) || !minEffRes2.equals(
Resources.none())) {
return minEffRes2.compareTo(minEffRes1);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
import java.util.Iterator;
import java.util.List;
import java.util.Set;
import java.util.Collections;

import java.util.concurrent.ThreadLocalRandom;

import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
Expand Down Expand Up @@ -309,6 +311,59 @@ public void testComparatorDoesNotValidateGeneralContract() {
assertDoesNotThrow(() -> policy.getAssignmentIterator(partition));
}

@Test
public void testComparatorClassDoesNotViolateTimSortContract() {
String partition = "testPartition";

List<PriorityUtilizationQueueOrderingPolicy.
PriorityQueueResourcesForSorting> queues = new ArrayList<>();
for (int i = 0; i < 300; i++) {
queues.add(createMockPriorityQueueResourcesForSorting(partition));
queues.add(createMockPriorityQueueResourcesForSorting(partition));
queues.add(createMockPriorityQueueResourcesForSorting(partition));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there are no values supplied in the params the 3 calls seems unnecessary, you could simply increate the limit from 300 to 900.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I am sorry for the oversight.

}

Collections.shuffle(queues);
// java.lang.IllegalArgumentException: Comparison method violates its general contract!
assertDoesNotThrow(() -> queues.sort(new PriorityUtilizationQueueOrderingPolicy(true)
.new PriorityQueueComparator(partition)));

}

private PriorityUtilizationQueueOrderingPolicy.
PriorityQueueResourcesForSorting createMockPriorityQueueResourcesForSorting(
String partition) {
QueueResourceQuotas resourceQuotas = randomResourceQuotas(partition);

boolean isZeroResource = ThreadLocalRandom.current().nextBoolean();
if (isZeroResource) {
resourceQuotas.setConfiguredMinResource(partition, Resource.newInstance(0, 0));
}

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: why not use the randomQueueCapacities method below? Would make the code a bit cleaner.

Copy link
Author

@Hean-Chhinling Hean-Chhinling Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

CSQueue mockQueue = mock(CSQueue.class);
when(mockQueue.getQueueCapacities())
.thenReturn(mockQueueCapacities);
when(mockQueue.getPriority())
.thenReturn(Priority.newInstance(7)); // Could be any number
when(mockQueue.getAccessibleNodeLabels())
.thenReturn(Collections.singleton(partition));
when(mockQueue.getQueueResourceQuotas())
.thenReturn(resourceQuotas);

return new PriorityUtilizationQueueOrderingPolicy.PriorityQueueResourcesForSorting(
mockQueue, partition
);

}

private QueueCapacities randomQueueCapacities(String partition) {
QueueCapacities qc = new QueueCapacities(false);
qc.setAbsoluteCapacity(partition, (float) randFloat(0.0d, 100.0d));
Expand All @@ -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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not needed, I will delete it. It was used during my testing that I forgot to remove it.

return qr;
}

Expand Down
Loading