[GitHub] [druid] clintropolis commented on pull request #14184: modify QueryScheduler to lazily acquire lanes when executing queries to avoid leaks

["clintropolis (via GitHub)" <gi...@apache.org>]

clintropolis commented on PR #14184:
URL: https://github.com/apache/druid/pull/14184#issuecomment-1529372418

   >Hrm, I think I had expected to see a HavingSpec that threw an exception on `setQuery` in order to repro the behavior that we saw in the wild, but I'm not seeing that in the tests. Just to help avoid regressions in the future, I think it would be good to have a test that throws an exception out of a Having spec and explicitly tests for what has and has not been run so far before that gets thrown (specifically, it is thrown before any actual work has been done).
   
   I guess I tested this indirectly with 
   ```
       Sequence<?> sequence = scheduler.run(report1, Sequences.empty());
       // making the sequence doesn't count, only running it does
       Assert.assertEquals(5, scheduler.getTotalAvailableCapacity());
   ```
   which is asserting that the lock is no longer grabbed eagerly so anything outside of the sequence can freely explode and the right thing will happen. I'm not sure we have any test setup that has the full set of stuff and an observable `QueryScheduler` to do this exact test, will look around and see how much trouble it would be to write a test ensuring that a having filter exploding when making value matchers can't leak locks on the broker.


-- 
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