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

Run tasks in the determinsitic task queue upto a cutoff time #100655

Merged
merged 2 commits into from
Oct 12, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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 @@ -304,8 +304,7 @@ public void testScheduledRecoveryNoOpWhenNewTermBegins() {
elapsed.millis(),
() -> setClusterStateTaskQueue.submitTask(randomAlphaOfLength(5), new SetClusterStateTask(clusterStateOfTerm2), null)
);
deterministicTaskQueue.advanceTime();
deterministicTaskQueue.runAllRunnableTasks();
deterministicTaskQueue.runTasksUpToTimeInOrder(elapsed.millis());
assertThat(gatewayService.currentPendingStateRecovery, not(sameInstance(pendingStateRecoveryOfTerm1)));
// The 1st scheduled recovery is now a no-op
deterministicTaskQueue.advanceTime();
Expand Down Expand Up @@ -388,16 +387,14 @@ public void testScheduledRecoveryWithRecoverAfterNodes() {
elapsed.millis(),
() -> setDataNodeCountTaskQueue.submitTask(randomAlphaOfLength(5), new SetDataNodeCountTask(recoverAfterNodes - 1), null)
);
deterministicTaskQueue.advanceTime();
deterministicTaskQueue.runAllRunnableTasks();
deterministicTaskQueue.runTasksUpToTimeInOrder(elapsed.millis());

// The 2nd scheduled recovery when data nodes are above recoverAfterDataNodes again
deterministicTaskQueue.scheduleAt(
elapsed.millis() * 2,
() -> setDataNodeCountTaskQueue.submitTask(randomAlphaOfLength(5), new SetDataNodeCountTask(recoverAfterNodes), null)
);
deterministicTaskQueue.advanceTime();
deterministicTaskQueue.runAllRunnableTasks();
deterministicTaskQueue.runTasksUpToTimeInOrder(elapsed.millis() * 2);
Copy link
Member Author

Choose a reason for hiding this comment

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

This usage seems to suggest a scheduleAtAndRunUpTo method. Let me know if you think so as well. Happy to iterate.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it's that common a pattern elsewhere, but that still seems like a reasonable thing to add indeed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great. I added the method and used it here and in two other places.


assertThat(gatewayService.currentPendingStateRecovery, sameInstance(pendingStateRecoveryOfInitialTerm));
assertThat(deterministicTaskQueue.getCurrentTimeMillis(), equalTo(elapsed.millis() * 2));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,17 @@ public void runAllTasksInTimeOrder() {
}
}

/**
* Run all {@code runnableTasks} and {@code deferredTasks} that are scheduled to run before or on the given {@code timeInMillis}.
*/
public void runTasksUpToTimeInOrder(long timeInMillis) {
runAllRunnableTasks();
while (nextDeferredTaskExecutionTimeMillis <= timeInMillis) {
advanceTime();
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

runAllRunnableTasks();
}
}

/**
* @return whether there are any runnable tasks.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,14 @@
import org.elasticsearch.threadpool.ThreadPool;

import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Random;
import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsInAnyOrder;
Expand Down Expand Up @@ -229,6 +233,37 @@ public void testRunInTimeOrder() {
assertThat(strings, contains("foo", "bar"));
}

public void testRunTasksUpToTimeInOrder() {
final DeterministicTaskQueue taskQueue = new DeterministicTaskQueue();
// The queue does _not_ have to be a clean slate before test
if (randomBoolean()) {
taskQueue.scheduleAt(randomLongBetween(1, 100), () -> {});
taskQueue.runAllTasksInTimeOrder();
}

final long cutoffTimeInMillis = randomLongBetween(taskQueue.getCurrentTimeMillis(), 1000);
final Set<Integer> seenNumbers = new HashSet<>();

final int nRunnableTasks = randomIntBetween(0, 10);
IntStream.range(0, nRunnableTasks).forEach(i -> taskQueue.scheduleNow(() -> seenNumbers.add(i)));

final int nDeferredTasksUptoCutoff = randomIntBetween(0, 10);
IntStream.range(0, nDeferredTasksUptoCutoff)
.forEach(i -> taskQueue.scheduleAt(randomLongBetween(0, cutoffTimeInMillis), () -> seenNumbers.add(i + nRunnableTasks)));

IntStream.range(0, randomIntBetween(0, 10))
.forEach(
i -> taskQueue.scheduleAt(
randomLongBetween(cutoffTimeInMillis + 1, 2 * cutoffTimeInMillis),
() -> seenNumbers.add(i + nRunnableTasks + nDeferredTasksUptoCutoff)
)
);

taskQueue.runTasksUpToTimeInOrder(cutoffTimeInMillis);
assertThat(seenNumbers, equalTo(IntStream.range(0, nRunnableTasks + nDeferredTasksUptoCutoff).boxed().collect(Collectors.toSet())));
assertThat(taskQueue.getCurrentTimeMillis(), lessThanOrEqualTo(cutoffTimeInMillis));
Copy link
Contributor

Choose a reason for hiding this comment

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

Test looks good apart from this (see previous comment) - can we make this an equality assertion?

}

public void testThreadPoolEnqueuesTasks() {
final DeterministicTaskQueue taskQueue = new DeterministicTaskQueue();
final List<String> strings = new ArrayList<>(2);
Expand Down