-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Prioritize non-speculative tasks in scheduler and node allocator #17465
Conversation
The allocatedMemory memory map can be easily computed based on fulfilledAcquires in BinPackingSimulation constructor. It does not add significant amount of computation as we are iterating fulfilledAcquires there anyway. Removal of allocatedMemory simplify state of BinPackingNodeAllocatorService making it less prone to concurrency related issues.
…ltTolerantQueryScheduler
CI: #17158 |
IndexedPriorityQueue.Prioritized<ScheduledTask> task = queue.peekPrioritized(); | ||
checkState(task != null, "queue is empty"); | ||
// negate priority to reverse operation we do in addOrUpdate | ||
return new PrioritizedScheduledTask(task.getValue(), toIntExact(-task.getPriority())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change IndexedPriorityQueue
to sort things in reserved order? The fact that we need to always negate priority here feels really error-prone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we can - will add commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will send separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NodeAllocator.NodeLease acquireSpeculative1 = nodeAllocator.acquire(REQ_NONE, DataSize.of(64, GIGABYTE), true); | ||
assertAcquired(acquireSpeculative1, NODE_1); | ||
NodeAllocator.NodeLease acquireSpeculative2 = nodeAllocator.acquire(REQ_NONE, DataSize.of(32, GIGABYTE), true); | ||
assertAcquired(acquireSpeculative2, NODE_2); | ||
|
||
// non-speculative tasks should still get node | ||
NodeAllocator.NodeLease acquireNonSpeculative1 = nodeAllocator.acquire(REQ_NONE, DataSize.of(64, GIGABYTE), false); | ||
assertAcquired(acquireNonSpeculative1, NODE_2); | ||
NodeAllocator.NodeLease acquireNonSpeculative2 = nodeAllocator.acquire(REQ_NONE, DataSize.of(32, GIGABYTE), false); | ||
assertAcquired(acquireNonSpeculative2, NODE_1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my understanding, two nodes have a total of 128GB, but now we have 192GB scheduled (96GB speculative, 96GB non-speculative), right? How does this work out later, scheduler will preempt speculative tasks in favor of non-speculative tasks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work out later, scheduler will preempt speculative tasks in favor of non-speculative tasks
They will work side by side until we run out of memory on node. In such case speculative task will be killed by low memory killer. If they will fit in available memory (task does not alway use all reserved memory) both should complete succesfully.
Release notes
(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: