Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Prefetching by the number of bytes #429

Merged
merged 1 commit into from
Oct 31, 2023
Merged

Prefetching by the number of bytes #429

merged 1 commit into from
Oct 31, 2023

Conversation

AnatolyPopov
Copy link
Contributor

No description provided.

@AnatolyPopov AnatolyPopov requested a review from a team as a code owner October 30, 2023 13:25
jeqo
jeqo reviewed Oct 30, 2023
View reviewed changes
core/src/main/java/io/aiven/kafka/tieredstorage/chunkmanager/cache/ChunkCacheConfig.java Outdated
@@ -33,9 +33,9 @@ public class ChunkCacheConfig extends AbstractConfig {
+ "where \"-1\" represents infinite retention";
private static final long DEFAULT_CACHE_RETENTION_MS = 600_000;

private static final String CACHE_PREFETCHING_SIZE_CONFIG = "prefetching.size";
private static final String CACHE_PREFETCHING_SIZE_CONFIG = "prefetching.bytes";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using .size for most bytes configurations. Also, final size is not specifically this value but whatever chunks can make up to this size, right? Finally, ing suffix doesn't add much so we may as well drop it:

Suggested change
private static final String CACHE_PREFETCHING_SIZE_CONFIG = "prefetching.bytes";
private static final String CACHE_PREFETCH_MAX_SIZE_CONFIG = "prefetch.max.size";

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not max size but rather min, since we are prefetching the whole chunk if requested range ends or starts in the middle of the chunk.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh actually you are right, it's max size

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

core/src/main/java/io/aiven/kafka/tieredstorage/manifest/index/AbstractChunkIndex.java Outdated Show resolved Hide resolved
ivanyu
ivanyu reviewed Oct 30, 2023
View reviewed changes
core/src/main/java/io/aiven/kafka/tieredstorage/manifest/index/AbstractChunkIndex.java Outdated
public List<Chunk> chunksForRange(final BytesRange bytesRange) {
Chunk current;
final var result = new ArrayList<Chunk>();
for (int i = bytesRange.from; i < bytesRange.to && i < originalFileSize; i += current.originalSize) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make it explicit: do we calculate the prefetch range on the original or transformed size?

@ivanyu
Copy link
Contributor

ivanyu commented Oct 31, 2023

LGTM. If it's 👍 for @jeqo , let's squash and merge

jeqo
jeqo reviewed Oct 31, 2023
View reviewed changes
Copy link
Contributor

@jeqo jeqo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a minor comment on the constant name

core/src/main/java/io/aiven/kafka/tieredstorage/chunkmanager/cache/ChunkCacheConfig.java Outdated
@@ -33,9 +33,9 @@ public class ChunkCacheConfig extends AbstractConfig {
+ "where \"-1\" represents infinite retention";
private static final long DEFAULT_CACHE_RETENTION_MS = 600_000;

private static final String CACHE_PREFETCHING_SIZE_CONFIG = "prefetching.size";
private static final String CACHE_PREFETCHING_SIZE_CONFIG = "prefetch.max.size";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could we align the constant name?

Suggested change
private static final String CACHE_PREFETCHING_SIZE_CONFIG = "prefetch.max.size";
private static final String CACHE_PREFETCH_MAX_SIZE_CONFIG = "prefetch.max.size";

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

core/src/main/java/io/aiven/kafka/tieredstorage/chunkmanager/cache/ChunkCacheConfig.java Outdated
@@ -33,9 +33,9 @@ public class ChunkCacheConfig extends AbstractConfig {
+ "where \"-1\" represents infinite retention";
private static final long DEFAULT_CACHE_RETENTION_MS = 600_000;

private static final String CACHE_PREFETCHING_SIZE_CONFIG = "prefetching.size";
private static final String CACHE_PREFETCHING_SIZE_CONFIG = "prefetch.max.size";
private static final String CACHE_PREFETCHING_SIZE_DOC =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@jeqo jeqo mentioned this pull request Oct 31, 2023
Closed
@ivanyu ivanyu merged commit fcec879 into main Oct 31, 2023
8 checks passed
@ivanyu ivanyu deleted the anatolii/prefetching branch October 31, 2023 10:41
@HenryCaiHaiying HenryCaiHaiying mentioned this pull request Nov 1, 2023
Open
This was referenced Nov 7, 2023
Closed
Closed
@jeqo jeqo mentioned this pull request Aug 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeqo jeqo jeqo approved these changes

@ivanyu ivanyu ivanyu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AnatolyPopov @ivanyu @jeqo