refactor(inkless:consume): only remote fetches to run on data executor #466
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the fetch planning architecture to optimize thread pool usage by executing only remote fetches on the data executor, while cache lookups run on the calling thread. The key changes eliminate the CacheFetchJob component and extend the ObjectCache interface to accept an executor parameter for async operations.
Key Changes:
- Deleted
CacheFetchJoband moved its logic intoFetchPlanner, which now directly interacts with the cache and schedules remote fetches - Modified
ObjectCacheAPI to returnCompletableFutureand accept an executor parameter for controlling where load operations run - Migrated tests from
CacheFetchJobTesttoFetchPlannerTestwith expanded coverage for cache hit/miss scenarios
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
FetchPlannerTest.java |
Migrated and expanded tests from CacheFetchJobTest, adding comprehensive tests for concurrent requests, cache hits/misses, and failure scenarios |
CacheFetchJobTest.java |
Deleted - tests migrated to FetchPlannerTest |
NullCache.java |
Updated to implement new async computeIfAbsent API that returns CompletableFuture and accepts executor parameter |
Reader.java |
Added clarifying comments about execution stages and which thread pools handle different operations |
FetchPlanner.java |
Major refactor: removed CacheFetchJob dependency, integrated cache operations directly, added ObjectFetchRequest and MergedBatchRequest records for internal planning |
CacheFetchJob.java |
Deleted - functionality merged into FetchPlanner |
ObjectCache.java |
Changed computeIfAbsent signature to return CompletableFuture and accept an Executor parameter for controlling async load execution |
MemoryCache.java |
Deleted - test-only implementation no longer needed |
CaffeineCache.java |
Updated to implement new async computeIfAbsent API using Caffeine's async cache with custom executor |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
storage/inkless/src/main/java/io/aiven/inkless/consume/FetchPlanner.java
Outdated
Show resolved
Hide resolved
storage/inkless/src/test/java/io/aiven/inkless/consume/FetchPlannerTest.java
Outdated
Show resolved
Hide resolved
storage/inkless/src/main/java/io/aiven/inkless/consume/FetchPlanner.java
Outdated
Show resolved
Hide resolved
storage/inkless/src/main/java/io/aiven/inkless/cache/ObjectCache.java
Outdated
Show resolved
Hide resolved
storage/inkless/src/test/java/io/aiven/inkless/consume/FetchPlannerTest.java
Outdated
Show resolved
Hide resolved
jeqo
force-pushed
the
jeqo/only-fetch-on-work-thread
branch
from
6f06aff to
d4c4bc4
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
storage/inkless/src/test/java/io/aiven/inkless/consume/FetchPlannerTest.java
Outdated
Show resolved
Hide resolved
storage/inkless/src/test/java/io/aiven/inkless/consume/FetchPlannerTest.java
Outdated
Show resolved
Hide resolved
jeqo
force-pushed
the
jeqo/only-fetch-on-work-thread
branch
2 times, most recently
from
c64c4aa to
31b9aa9
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Refactor fetch planning by changing how fetch operations are scheduled on the data executor. It only assigns the data executor for remote fetch operations and let the calling thread to run the cache.get(), avoiding the scenario where calls to cache are blocked by all data executor threads being used. By doing this, the CacheFetchJob component becomes irrelevant as most of the logic is already on the planning, even the scheduling decision. Changes: - Delete CacheFetchJob - Extend ObjectCache to allow passing the executor to run the load function - Refactor the FetchPlanner to use the new approach - Update documentation on FetchPlanner and Reader to clarify how stages are ran - Move CacheFetchJobTest to FetchPlannerTest to keep and improve coverage
jeqo
force-pushed
the
jeqo/only-fetch-on-work-thread
branch
from
31b9aa9 to
f6be2a5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor fetch planning by changing how fetch operations are scheduled on the data executor. It only assigns the data executor for remote fetch operations and let the calling thread to run the cache.get(), avoiding the scenario where calls to cache are blocked by all data executor threads being used.
By doing this, the CacheFetchJob component got irrelevant as most of the logic is already on the planning, even the scheduling.
Changes: