You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "imply-cheddar (via GitHub)" <gi...@apache.org> on 2023/05/01 04:59:54 UTC
[GitHub] [druid] imply-cheddar commented on a diff in pull request #14184: modify QueryScheduler to lazily acquire lanes when executing queries to avoid leaks
imply-cheddar commented on code in PR #14184:
URL: https://github.com/apache/druid/pull/14184#discussion_r1181390366
##########
server/src/test/java/org/apache/druid/server/QueryResourceTest.java:
##########
@@ -1043,19 +1045,21 @@ public <T> QueryRunner<T> getQueryRunnerForIntervals(Query<T> query, Iterable<In
return (queryPlus, responseContext) -> {
beforeScheduler.forEach(CountDownLatch::countDown);
- return scheduler.run(
- scheduler.prioritizeAndLaneQuery(queryPlus, ImmutableSet.of()),
- new LazySequence<T>(() -> {
- inScheduler.forEach(CountDownLatch::countDown);
- try {
- // pretend to be a query that is waiting on results
- Thread.sleep(500);
- }
- catch (InterruptedException ignored) {
- }
- // all that waiting for nothing :(
- return Sequences.empty();
- })
+ return Sequences.simple(
+ scheduler.run(
+ scheduler.prioritizeAndLaneQuery(queryPlus, ImmutableSet.of()),
+ new LazySequence<T>(() -> {
+ inScheduler.forEach(CountDownLatch::countDown);
+ try {
+ // pretend to be a query that is waiting on results
+ Thread.sleep(500);
+ }
+ catch (InterruptedException ignored) {
+ }
+ // all that waiting for nothing :(
+ return Sequences.empty();
+ })
+ ).toList()
Review Comment:
Why did you need this change in the test? It looks like you are making the test that was doing things semi-lazily to now be explicitly eager, which feels like a behavior change?
##########
server/src/test/java/org/apache/druid/server/QueryResourceTest.java:
##########
@@ -896,7 +896,9 @@ public void testTooManyQuery() throws InterruptedException
assertAsyncResponseAndCountdownOrBlockForever(
SIMPLE_TIMESERIES_QUERY,
waitAllFinished,
- response -> Assert.assertEquals(Response.Status.OK.getStatusCode(), response.getStatus())
+ response -> {
+ Assert.assertEquals(Response.Status.OK.getStatusCode(), response.getStatus());
+ }
Review Comment:
This looks superfluous? I'm guessing you added some lines and then didn't clean up the `{}` or is there actually a change in here?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org