implements interuption of compactions and adds an IT to verify compaction cancels on tablet migration #5395
Conversation
|
After running this test, see the following in the logs. |
keith-turner
changed the title
| is.addOption(SLEEP_UNINTERRUPTIBLY, Boolean.toString(b)); | ||
| } | ||
|
|
||
| private void sleep(long time) throws IOException { |
There was a problem hiding this comment.
I checked these changes by jstacking a tablet server running the new test and verifying that compaction threads were sleeping both ways.
| .map(ac -> ac.getTablet().getTable()) | ||
| .filter(tid -> tid.equals(tableId1) || tid.equals(tableId2)).count(); | ||
| log.debug("Running compactions {}", runningCompactions); | ||
| return runningCompactions == 40; |
There was a problem hiding this comment.
This test usually passes for me, every once in a while it will get stuck here w/ less than the expected 40 compactions. Probably some issue w/ the propagation of compaction config changes.
| // Limit interrupting compactions to the part that reads and writes files and avoid | ||
| // interrupting the metadata table updates. | ||
| try (var ignored = new CompactionInterrupter()) { | ||
| stats = CompactableUtils.compact(tablet, job, cInfo, compactEnv, compactFiles, tmpFileName); |
There was a problem hiding this comment.
I'm trying to understand how this PR is different than what is happening inside CompactableUtils.compact at:
When CompactableImpl.close is called and sets closed=true, then cenv.isCompactionEnabled() should return false and interrupt the FileCompactor. I'm thinking the difference here is checking the Thread.interrupted state and throwing an InterruptedException in CompactionInterrupter.close. I'm wondering if the code below is functionally equivalent:
try {
stats = CompactableUtils.compact(tablet, job, cInfo, compactEnv, compactFiles, tmpFileName);
} finally {
if (Thread.interrupted()) {
throw new InterruptedException();
}
}
There was a problem hiding this comment.
The main difference is that compactor.interrupt() was not calling Thread.interrupt(). However it could call that, it was already tracking the thread running the compaction. Modified it do that in 7d39c8d moving where Thread.interrupt() is called down a level (this will probably benefit external compactions also). The test still pass w/ this change, however they were a bit slower because the interrupting the thread could take up 10 seconds after calling close(). So changed the scheduled timer to run every 3 seconds. The previous code would immediately interrupt the thread when close() was called.
|
I'm wondering if we should change: to: |
|
For the change to isCompactionEnabled() the way the code is working w/ the latest that changes it would probably not help to check for interrupted there. The interrupt stats will be set on the compaction thread when is compaction isCompactionEnabled() returns false. Also another thead besides the compaction thread could call isCompactionEnabled(), so could not use Thread.currentThread() |
Changed this from adding a test to adding a test and adding functionality. Added the ability to interrupt compaction threads on tablet close. Added an IT that covers the two ways compactions can be interrupted on compaction close.