-
Notifications
You must be signed in to change notification settings - Fork 21
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
Prefetching a configurable amount of data. #394
Conversation
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.
Thanks, @AnatolyPopov .
This looks promising. I just have some comments about config definition, and some suggestions to refactor the prefetching logic.
Overall, the approach looks sound to me: Pre-fetching once per fetch request.
For this to be effective, we should consider the prefetched size to be at least higher than the chunk size (else no chunk will be cached) and, better, to be equal or larger than the max fetch bytes per partition, so at least all the get requests to e.g. S3 will be done at once. If double, we can expect the next fetch request to be sourced directly from local cache.
Also, cache size will be affected by this. If 30MB is prefetched per partition, then a broker to prefetch from 10 partitions concurrently will require at least 300MB to cache all required chunks.
core/src/main/java/io/aiven/kafka/tieredstorage/RemoteStorageManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/aiven/kafka/tieredstorage/chunkmanager/cache/ChunkCacheConfig.java
Show resolved
Hide resolved
core/src/main/java/io/aiven/kafka/tieredstorage/chunkmanager/cache/ChunkCacheConfig.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/aiven/kafka/tieredstorage/chunkmanager/cache/ChunkCache.java
Outdated
Show resolved
Hide resolved
d7cce31
to
41f01af
Compare
if (chunkManager instanceof ChunkCache) { | ||
((ChunkCache<?>) chunkManager).startPrefetching(segmentKey, segmentManifest, range.from); | ||
} |
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.
Any reason this is happening here and not in ChunkCache#getChunk
? Looks like a leaky abstraction.
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 are right, fixed.
41f01af
to
4808dcd
Compare
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.
Looks good, a couple of minor things
No description provided.