Skip to content

Conversation

@jeqo
Copy link
Contributor

@jeqo jeqo commented Dec 22, 2023

Indexes are currently cached on broker-side but with a synchronous cache that is not able to source value when interrupted.
This new cache will fetch index asynchronously, and retries would have chances to find the value sourced already.

@jeqo jeqo force-pushed the jeqo/index-cache branch 4 times, most recently from aed6581 to becc24a Compare December 28, 2023 15:28
@jeqo jeqo marked this pull request as ready for review December 28, 2023 15:46
@jeqo jeqo requested a review from a team as a code owner December 28, 2023 15:46
@jeqo jeqo force-pushed the jeqo/index-cache branch 4 times, most recently from 05c66b1 to 399dc2a Compare December 28, 2023 17:41
@jeqo jeqo requested a review from ivanyu December 28, 2023 17:44
@ivanyu ivanyu self-assigned this Jan 2, 2024
Comment on lines 59 to 66
public RemovalListener<SegmentIndexKey, byte[]> removalListener() {
return (key, content, cause) -> log.debug("Deleted cached value for key {} from cache."
+ " The reason of the deletion is {}", key, cause);
}

public Weigher<SegmentIndexKey, byte[]> weigher() {
return (key, value) -> value.length;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these two need to be higher-order functions?

Suggested change
public RemovalListener<SegmentIndexKey, byte[]> removalListener() {
return (key, content, cause) -> log.debug("Deleted cached value for key {} from cache."
+ " The reason of the deletion is {}", key, cause);
}
public Weigher<SegmentIndexKey, byte[]> weigher() {
return (key, value) -> value.length;
}
private static void removalListener(final SegmentIndexKey key, final byte[] value, RemovalCause cause) {
log.debug("Deleted cached value for key {} from cache. The reason of the deletion is {}", key, cause);
}
private static int weigher(final SegmentIndexKey key, byte[] value) {
return value.length;
}

verifyNoMoreInteractions(offsetIndexSupplier);
assertThat(cache.cache.asMap()).hasSize(1);

Thread.sleep(100);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's worth adding a comment that this sleep is needed to spread away in time the creation (and eviction) of the cache entries, or something like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, let's do the latter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AnatolyPopov, falling back to using thread sleep (same as in ChunkCacheTest) as adding a stronger condition (e.g. waiting for cache to be empty) takes unknown amount of time github actions--unless there's a better approach here.

@jeqo jeqo requested a review from ivanyu January 2, 2024 19:36
@jeqo jeqo requested a review from AnatolyPopov January 3, 2024 14:13
@jeqo jeqo force-pushed the jeqo/index-cache branch 2 times, most recently from 973aed6 to c49d3f3 Compare January 4, 2024 20:26
ivanyu
ivanyu previously approved these changes Jan 9, 2024
Copy link
Contributor

@ivanyu ivanyu left a comment

Choose a reason for hiding this comment

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

LGTM

jeqo added 2 commits January 10, 2024 08:51
Indexes are currently cached on broker-side but with a synchronous cache that is not able to source value when interrupted.
This new cache will fetch index asynchronously, and retries would have chances to find the value sourced already.
@jeqo jeqo force-pushed the jeqo/index-cache branch from 6ac772f to 9d260c2 Compare January 10, 2024 13:52
@AnatolyPopov AnatolyPopov merged commit c4b3a51 into main Jan 11, 2024
@AnatolyPopov AnatolyPopov deleted the jeqo/index-cache branch January 11, 2024 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants