gortiz commented on code in PR #19864:
URL: https://github.com/apache/pinot/pull/19864#discussion_r4280580098
##########
pinot-spi/src/test/java/org/apache/pinot/spi/executor/HardLimitExecutorTest.java:
##########
@@ -127,4 +133,83 @@ public void testGetMultiStageExecutorHardLimit() {
PinotConfiguration config5 = new PinotConfiguration(configMap5);
assertEquals(HardLimitExecutor.getMultiStageExecutorHardLimit(config5),
-1);
}
+
+ /// A task that passes the submit-time check must run even if the limit is
reached before it starts. The limit used
+ /// to be re-checked at start time, which silently dropped the task: the
exception was captured by the FutureTask
+ /// created by AbstractExecutorService.submit and never reached the
submitter.
+ @Test
+ public void testAcceptedTaskRunsEvenIfLimitIsReachedBeforeItStarts()
+ throws Exception {
+ DeferredExecutor deferred = new DeferredExecutor();
+ HardLimitExecutor ex = new HardLimitExecutor(1, deferred);
+ CountDownLatch firstStarted = new CountDownLatch(1);
+ CountDownLatch firstRelease = new CountDownLatch(1);
+ CountDownLatch secondRan = new CountDownLatch(1);
+
+ // Both submissions pass the synchronous check because nothing has started
yet.
+ ex.execute(() -> {
+ firstStarted.countDown();
+ try {
+ firstRelease.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ });
+ ex.execute(secondRan::countDown);
+ assertEquals(deferred._queued.size(), 2);
+
+ Thread first = new Thread(deferred._queued.get(0));
+ first.start();
+ assertTrue(firstStarted.await(10, TimeUnit.SECONDS), "first task did not
start");
+
+ // The running counter is at the limit now, but the second task was
already accepted and must run.
+ deferred._queued.get(1).run();
+ assertTrue(secondRan.await(10, TimeUnit.SECONDS), "accepted task was
dropped at start time");
+
+ // A third submission is rejected synchronously because the first task is
still running.
+ try {
+ ex.execute(() -> {
+ });
+ fail("Should not allow more than 1 task");
+ } catch (IllegalStateException e) {
Review Comment:
Thanks! I'll leave it as-is: pinot-query-runtime doesn't depend on the
pinot-spi test-jar today, and adding that dependency for a ~20-line helper
doesn't seem worth it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
