Run tasks in the determinsitic task queue upto a cutoff time #100655
Conversation
ywangd
added
>test
elasticsearchmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
| deterministicTaskQueue.scheduleAt( | ||
| elapsed.millis() * 2, | ||
| () -> setDataNodeCountTaskQueue.submitTask(randomAlphaOfLength(5), new SetDataNodeCountTask(recoverAfterNodes), null) | ||
| ); | ||
| deterministicTaskQueue.advanceTime(); | ||
| deterministicTaskQueue.runAllRunnableTasks(); | ||
| deterministicTaskQueue.runTasksUpToTimeInOrder(elapsed.millis() * 2); |
There was a problem hiding this comment.
This usage seems to suggest a scheduleAtAndRunUpTo method. Let me know if you think so as well. Happy to iterate.
There was a problem hiding this comment.
I'm not sure it's that common a pattern elsewhere, but that still seems like a reasonable thing to add indeed.
There was a problem hiding this comment.
Great. I added the method and used it here and in two other places.
| public void runTasksUpToTimeInOrder(long timeInMillis) { | ||
| runAllRunnableTasks(); | ||
| while (nextDeferredTaskExecutionTimeMillis <= timeInMillis) { | ||
| advanceTime(); |
There was a problem hiding this comment.
This might be slightly trappy: if there is no task scheduled at timeInMillis then this method returns leaving the current time earlier than requested. Should we advance currentTime to timeInMillis in that case?
There was a problem hiding this comment.
Yeah I think you are right. It feels more natural the current time is at the specified time once the method runs. I updated accordingly.
| deterministicTaskQueue.scheduleAt( | ||
| elapsed.millis() * 2, | ||
| () -> setDataNodeCountTaskQueue.submitTask(randomAlphaOfLength(5), new SetDataNodeCountTask(recoverAfterNodes), null) | ||
| ); | ||
| deterministicTaskQueue.advanceTime(); | ||
| deterministicTaskQueue.runAllRunnableTasks(); | ||
| deterministicTaskQueue.runTasksUpToTimeInOrder(elapsed.millis() * 2); |
There was a problem hiding this comment.
I'm not sure it's that common a pattern elsewhere, but that still seems like a reasonable thing to add indeed.
|
|
||
| taskQueue.runTasksUpToTimeInOrder(cutoffTimeInMillis); | ||
| assertThat(seenNumbers, equalTo(IntStream.range(0, nRunnableTasks + nDeferredTasksUptoCutoff).boxed().collect(Collectors.toSet()))); | ||
| assertThat(taskQueue.getCurrentTimeMillis(), lessThanOrEqualTo(cutoffTimeInMillis)); |
There was a problem hiding this comment.
Test looks good apart from this (see previous comment) - can we make this an equality assertion?
Relates: #99994 (comment)