Check all queries returned memory on query runner teardown#11102
Check all queries returned memory on query runner teardown#11102losipiuk merged 1 commit intotrinodb:masterfrom
Conversation
losipiuk
force-pushed
the
lo/dist-que-run-check-pools
branch
from
ae531ba to
ee82260
Compare
There was a problem hiding this comment.
io.airlift.testing.Closeables#closeAllSuppress ?
There was a problem hiding this comment.
close() it not an awesome place to assert.
we could perhaps have a @AfterMethod check in TestDistributedEngineOnlyQueries, using queryRunner.getExclusiveLock()?
There was a problem hiding this comment.
@AfterMethod might not work great as we run tests in multiple threads. How about putting it in the AbstractTestQueryFramework#close annotated with @AfterClass? (or maybe have a dedicated @AfterClass method that does just that?)
There was a problem hiding this comment.
Yeah - I agree place is not great.
TestDistributedEngineOnlyQueries is not the only place we want this. Specifically, we want to test in Test*QueryFailureRecoveryTest.
There was a problem hiding this comment.
@AfterClass feels less invasive. Having @AfterMethod with lock may make test execution significantly longer, due to threads waiting one for another.
Not sure we can have two @AfterClass methods (would would be the ordering). But putting that in AbstractTestQueryFramework.close() looks good to me.
There will still be some places where query runner is constructed directly (not via AbstractTestQueryFramework) which will not be subject to validation - but we can live with that I guess.
There was a problem hiding this comment.
Having @AfterMethod with lock may make test execution significantly longer, due to threads waiting one for another.
i thought about that, but was more optimistic.
is it hard to measure?
note that pools should be empty immediately after query finishes, so the "assert eventually" should end real quick
There was a problem hiding this comment.
But putting that in
AbstractTestQueryFramework.close()looks good to me.
Looks good to me too, let's do this
There was a problem hiding this comment.
@AfterMethod might not work great as we run tests in multiple threads. How about putting it in the AbstractTestQueryFramework#close annotated with @AfterClass? (or maybe have a dedicated @AfterClass method that does just that?)
There was a problem hiding this comment.
if(exception != e) as self suppression is not allowed
There was a problem hiding this comment.
Not needed as exception cannot be IOException (IOException is checked).
losipiuk
force-pushed
the
lo/dist-que-run-check-pools
branch
from
ee82260 to
7e81ac7
Compare
losipiuk
force-pushed
the
lo/dist-que-run-check-pools
branch
from
7e81ac7 to
7bc06f9
Compare
|
I temporarily cherry-picked #11097 here. Should fix the CI failures. |
There was a problem hiding this comment.
Should this go to finally?
There was a problem hiding this comment.
no. It would mess up exception in case closer would throw. I want to get exception from checkQueryMemoryReleased if there is one.
There was a problem hiding this comment.
It's still feels like it is worth calling close. If close is not called the in memory cluster will remain active while the testing framework is running other test cases. Maybe put it in finally and wrap it in a try-catch?
There was a problem hiding this comment.
i agree. afterClassCloser must be invoked even if checkQueryMemoryReleased fals.
There was a problem hiding this comment.
But it is. In catch clause
There was a problem hiding this comment.
right, missed that.
you could also use try() for same effect
There was a problem hiding this comment.
I totally missed that the close was called. The current state looks good to me.
losipiuk
force-pushed
the
lo/dist-que-run-check-pools
branch
from
7bc06f9 to
7793b00
Compare
There was a problem hiding this comment.
i agree. afterClassCloser must be invoked even if checkQueryMemoryReleased fals.
losipiuk
force-pushed
the
lo/dist-que-run-check-pools
branch
from
7793b00 to
1724f93
Compare
There was a problem hiding this comment.
Wow. 🤯 . Was that always possible?
There was a problem hiding this comment.
No, not always. Initially wasn't.
losipiuk
force-pushed
the
lo/dist-que-run-check-pools
branch
from
1724f93 to
a971cac
Compare
|
Follow-up issue: #11275 |
3 participants
Description
Check all queries returned memory on query runner teardown
testing improvement
tests
Related issues, pull requests, and links
fixes: #11093
Documentation
(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.
Release notes
(x) No release notes entries required.
( ) Release notes entries required with the following suggested text: