CASSANALYTICS-87: Fix LEAK DETECTED errors during bulk read #138
Conversation
Patch by Yifan Cai, James Berragan; Reviewed by TBD for CASSANALYTICS-87
| .removalListener(notification -> { | ||
| // The function is to eliminate the LEAK DETECTED errors. | ||
| // How it happens: | ||
| // 1. AutoCloseable objects (e.g. IndexSummary and BloomFilter) are evicted from cache | ||
| // 2. JVM GC and the close method is not called explicitly to reduce the reference count | ||
| // 3. Reference-Reaper thread release the object and print the LEAK DETECTED error | ||
| // The function fixes it by closing the object when evicting. | ||
| Object val = notification.getValue(); | ||
| if (val instanceof AutoCloseable) | ||
| { | ||
| String typeLiteral = val.getClass().getName(); | ||
| try | ||
| { | ||
| LOGGER.debug("Evicting auto-closable of type: {}", typeLiteral); | ||
| ((AutoCloseable) val).close(); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| LOGGER.error("Exception closing cached instance of {}", typeLiteral, e); | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
If commenting it out, the new test fails with "LEAK DETECTED" errors
| @Override // The method is expected to be called when evicting the object from sstable cache; do not call it explicitly. | ||
| public void close() throws Exception | ||
| { | ||
| indexSummary.close(); |
There was a problem hiding this comment.
Be careful with this, I remember trying this once before but it resulted in seg. faults (SIGSEGV).
There was a problem hiding this comment.
I think it is ref. counted in the org.apache.cassandra.io.util.Memory class
There was a problem hiding this comment.
Ack. I will double check. Thank you!
| assertThat(SSTableCache.INSTANCE.containsSummary(sstable)).isFalse(); | ||
| // trigger GC and wait a bit before asserting LEAK DETECTED is not logged. | ||
| System.gc(); | ||
| Thread.sleep(1000); |
There was a problem hiding this comment.
NIT: We could use Awaitility, if we consider more places that need waiting for asynchronous condition. Kafka developers do not pull the dependency (even though only for test), but implemented a simple function to do the same here.
There was a problem hiding this comment.
Changed to Uninterruptibles. I was under the impression that we removed guava from the repo. But looks like we still have them as test dependency.
|
|
||
| public class IndexSummaryComponent implements AutoCloseable | ||
| { | ||
| private final IndexSummary indexSummary; |
There was a problem hiding this comment.
Extracted the IndexSummaryComponent out from SummaryDbUtils, since IndexSummary is version specific.
| * @throws IOException io exception | ||
| */ | ||
| @Nullable | ||
| static IndexSummaryComponent readSummary(InputStream summaryStream, |
There was a problem hiding this comment.
The read method implementation is version specific too.
| */ | ||
| public IndexSummary summarySharedCopy() | ||
| { | ||
| return indexSummary.sharedCopy(); |
There was a problem hiding this comment.
get a shared copy, so that if IndexSummaryComponent is closed, the underlying indexSummary is still not freed (since reference count > 0)
…much as all logs are stored and synchronized.
|
slf4j-test is too slow. So I have to remove the test in 4190d40 |
3 participants
Patch by Yifan Cai, James Berragan; Reviewed by TBD for CASSANALYTICS-87