-
Notifications
You must be signed in to change notification settings - Fork 21
CNDB-15360 Chunk Cache inspection #2140
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
base: main
Are you sure you want to change the base?
Conversation
Checklist before you submit for review
|
eolivelli
left a comment
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.
I have left a couple of suggestions, you are on your way.
Let's add unit tests, there are some tests about ChunkCache, you can add them there.
We don't really care about the implementation of Caffeine, you will hardly have a deterministic behavior for ordering, for the unit tests it is just enough to ensure that if there is something that it is returned correctly, in any order
1fa7663 to
1c9a057
Compare
| private long assignFileId(File file) | ||
| { | ||
| return nextFileId.getAndIncrement(); | ||
| long id = nextFileId.getAndIncrement(); |
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.
nit: revert unneeded change
| * @param limit maximum number of entries to inspect | ||
| * @param consumer consumer to process each entry | ||
| */ | ||
| public void inspectHotEntries(int limit, java.util.function.Consumer<ChunkCacheInspectionEntry> consumer) |
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.
why do we have 2 methods ?
can't we keep the boolean parameter ?
if you have two methods I guess that on the caller site you will have some "if (hottest) inspectHotEntries else inspectColdEntries" and you have to unroll this
| // We need to shift right to extract just the File ID portion by discarding the lower bits | ||
| int shift = CHUNK_SIZE_LOG2_BITS + READER_TYPE_BITS; | ||
|
|
||
| synchronousCache.policy().eviction().ifPresent(policy -> { |
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.
if there is no eviction policy we should throw and exception, because there is no concept of "hot" or "cold"
This it not going to happen, so you can simply add a precondition
b236ad5 to
abf97b6
Compare
| } | ||
|
|
||
| @Test | ||
| public void testInspectEntriesWithLimit() throws IOException |
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.
you can put this into the parametrizedtest
public static Stream<Argument> testInspectEntriesValues() {
return ..... Arguments.of(CacheOrder.HOTTEST, 10), Arguments.of(CacheOrder.HOTTEST, 2)....
}
@ParametrizedTest
vood testInspectEntries(CacheOrder order, int limit)
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.
Had to put tests in separate class because in Junit4 for test's to be parameterised the entire class has to run with Parameterised.class
| implements RemovalListener<ChunkCache.Key, ChunkCache.Chunk>, CacheSize | ||
| import java.util.Map; | ||
|
|
||
| public class ChunkCache implements RemovalListener<ChunkCache.Key, ChunkCache.Chunk>, CacheSize |
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.
let's not reformat code we don't touch, if not really useful
| /** | ||
| * Defines the ordering strategy for cache entries during inspection. | ||
| */ | ||
| public enum CacheOrder |
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.
InspectEntriesOrder ?
| */ | ||
| public enum CacheOrder | ||
| { | ||
| /** Orders cache entries from most frequently accessed to least */ |
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.
style ? missing new lines
| public void testInspectEntriesWithEmptyCache() | ||
| { | ||
| logger.info("Testing inspect entries with empty cache for order={}", order); | ||
| ChunkCache.instance.clear(); |
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.
is there any particular reason to not using a "new ChunkCache" in each test ?
|
|
||
| try (FileHandle.Builder builder1 = new FileHandle.Builder(file).withChunkCache(ChunkCache.instance)) | ||
| { | ||
| try (FileHandle handle1 = builder1.complete(); |
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.
is this only removing blank lines?
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.
Yes, I've overlooked this
…e a new chunkCache for every test
eolivelli
left a comment
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.
Overall LGTM
I have left a minor comment about Files.write.
| { | ||
| File file = FileUtils.createTempFile("test" + i, null); | ||
| file.deleteOnExit(); | ||
| writeBytes(file, new byte[RandomAccessReader.DEFAULT_BUFFER_SIZE]); |
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.
nit: you can use Files.write(file.toPath() .... )
|
❌ Build ds-cassandra-pr-gate/PR-2140 rejected by Butler2 regressions found Found 2 new test failures
Found 3 known test failures |
|
|
||
| // Look up the File by searching through fileIdMap entries | ||
| File file = null; | ||
| for (Map.Entry<File, Long> entry : fileIdMap.entrySet()) |
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 overall time complexity would be: O (N * F) where N is the num of cache entries and F is num of files in cache..
Should we build a temporary Map<Long, File> above to cache the reversed mappings once? This will save some CPU cycles. Time complexity would be O(N + F)



What is the issue
https://github.com/riptano/cndb/issues/15360
What does this PR fix and why was it fixed
This PR adds the ability to inspect hot and cold entries in ChunkCache to better understand cache dynamics and identify which files and sections are most useful for caching.
Changes
New ChunkCache methods:
inspectEntries()method to iterate over cache entries ordered by access frequencyInspectEntriesOrderenum (HOTTEST/COLDEST) to specify ordering by access frequencyChunkCacheInspectionEntryclass containing file, position, and size metadata for each cached chunkpolicy().eviction().hottest()/coldest()APIs to efficiently retrieve ordered entries without materializing full cache in memoryUse Cases
Testing
Added comprehensive parameterized unit tests covering: