-
Notifications
You must be signed in to change notification settings - Fork 21
CNDB-13524: Properly shutdown executors, use test CFS in PendingAntiCompactionTest #1832
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
Conversation
Checklist before you submit for review
|
test/unit/org/apache/cassandra/db/repair/PendingAntiCompactionTest.java
Outdated
Show resolved
Hide resolved
@@ -717,7 +741,7 @@ public boolean apply(SSTableReader sstable) | |||
} | |||
finally | |||
{ | |||
es.shutdown(); | |||
waitForExecutorShutdown(es); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this help to fix the problem for this test? Can you put the reasoning in the commit message or PR description?
As per the checklist
Each commit has a meaningful description
It looks like maybe only makeSSTables
is meaningful here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The primary fix is waiting for the executor to finish shutting down after shutdown() is invoked. When we don't do this it causes interference when the whole class is run.
makeSSTables
gets around another shared state issue with MockSchema.newCFS but that is secondary to the executor problem.
Please see the attached check list, it contains the following point:
|
|
❌ Build ds-cassandra-pr-gate/PR-1832 rejected by Butler1 new test failure(s) in 2 builds Found 1 new test failures
Found 1 known test failures |
What is the issue
In PendingAntiCompactionTest, the executors don't properly wait for completion to shutdown which causes shared state interference when the whole class is run.
What does this PR fix and why was it fixed
Waits for operations to complete, prefer test CFS over mock