refactor(inkless): improve thread pool lifecycle management#475
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request fixes thread pool resource leaks in three Inkless storage components by ensuring proper shutdown of all ExecutorService instances during broker lifecycle operations.
Key Changes:
- Added missing shutdown call for
executorServiceCacheStorein FileCommitter, preventing leak of 8 threads per broker shutdown - Implemented Closeable interface for DeleteRecordsInterceptor with proper ExecutorService shutdown using 5-second timeout
- Improved FetchOffsetHandler shutdown from basic
shutdown()toshutdownExecutorServiceQuietly()with 5-second timeout
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| storage/inkless/src/main/java/io/aiven/inkless/produce/FileCommitter.java | Added missing executorServiceCacheStore.shutdown() call and documentation about thread pool lifecycle |
| storage/inkless/src/test/java/io/aiven/inkless/produce/FileCommitterTest.java | Added verification for executorServiceCacheStore shutdown |
| storage/inkless/src/main/java/io/aiven/inkless/delete/DeleteRecordsInterceptor.java | Implemented Closeable, changed Executor to ExecutorService, added InklessThreadFactory for thread naming, and proper shutdown with timeout |
| storage/inkless/src/test/java/io/aiven/inkless/delete/DeleteRecordsInterceptorTest.java | Added close() calls to all tests and new test verifying shutdown behavior |
| storage/inkless/src/main/java/io/aiven/inkless/consume/FetchOffsetHandler.java | Changed to shutdownExecutorServiceQuietly with timeout, refactored constructor for testability |
| storage/inkless/src/test/java/io/aiven/inkless/consume/FetchOffsetHandlerTest.java | Added test verifying proper executor shutdown with timeout |
| storage/inkless/src/main/java/io/aiven/inkless/produce/Writer.java | Added documentation about thread pool lifecycle management |
| storage/inkless/src/main/java/io/aiven/inkless/consume/Reader.java | Added documentation about thread pool lifecycle management |
| core/src/main/scala/kafka/server/ReplicaManager.scala | Added close() call for inklessDeleteRecordsInterceptor in shutdown sequence |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
storage/inkless/src/main/java/io/aiven/inkless/delete/DeleteRecordsInterceptor.java
Outdated
Show resolved
Hide resolved
jeqo
force-pushed
the
jeqo/refactor-thread-pools
branch
from
2458994 to
89571b9
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.
jeqo
force-pushed
the
jeqo/refactor-thread-pools
branch
from
89571b9 to
d6f8803
Compare
jeqo
changed the title
jeqo
force-pushed
the
jeqo/refactor-thread-pools
branch
3 times, most recently
from
43da310 to
e871ebb
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.
- **FileCommitter**: Add missing `executorServiceCacheStore` shutdown and implement ordered shutdown sequence (reject → cancel cache → await commits → force-stop uploads) to prevent orphaned files in object storage - **DeleteRecordsInterceptor**: Implement `Closeable`, narrow dependencies from `SharedState` to `ControlPlane` + `MetadataView`, add `InklessThreadFactory` for consistent thread naming, and register in `ReplicaManager.close()` - **FetchOffsetHandler**: Use `shutdownExecutorServiceQuietly()` with 5s timeout, extract constructor for testability - **Reader, Writer**: Add Javadoc documenting thread pool lifecycle
jeqo
force-pushed
the
jeqo/refactor-thread-pools
branch
from
e871ebb to
36a9c65
Compare
viktorsomogyi
previously approved these changes
Mar 16, 2026
Contributor
viktorsomogyi
left a comment
viktorsomogyi
left a comment
There was a problem hiding this comment.
I think overall this is good, had one minor comment. I won't merge yet to give @Mwea a chance to add comments if needed.
storage/inkless/src/test/java/io/aiven/inkless/delete/DeleteRecordsInterceptorTest.java
Outdated
Show resolved
Hide resolved
viktorsomogyi
approved these changes
Mar 16, 2026
AnatolyPopov
pushed a commit
that referenced
this pull request
Mar 23, 2026
- **FileCommitter**: Add missing `executorServiceCacheStore` shutdown and implement ordered shutdown sequence (reject → cancel cache → await commits → force-stop uploads) to prevent orphaned files in object storage - **DeleteRecordsInterceptor**: Implement `Closeable`, narrow dependencies from `SharedState` to `ControlPlane` + `MetadataView`, add `InklessThreadFactory` for consistent thread naming, and register in `ReplicaManager.close()` - **FetchOffsetHandler**: Use `shutdownExecutorServiceQuietly()` with 5s timeout, extract constructor for testability - **Reader, Writer**: Add Javadoc documenting thread pool lifecycle (cherry picked from commit 3daf634)
jeqo
added a commit
that referenced
this pull request
Mar 23, 2026
- **FileCommitter**: Add missing `executorServiceCacheStore` shutdown and implement ordered shutdown sequence (reject → cancel cache → await commits → force-stop uploads) to prevent orphaned files in object storage - **DeleteRecordsInterceptor**: Implement `Closeable`, narrow dependencies from `SharedState` to `ControlPlane` + `MetadataView`, add `InklessThreadFactory` for consistent thread naming, and register in `ReplicaManager.close()` - **FetchOffsetHandler**: Use `shutdownExecutorServiceQuietly()` with 5s timeout, extract constructor for testability - **Reader, Writer**: Add Javadoc documenting thread pool lifecycle
jeqo
added a commit
that referenced
this pull request
Mar 23, 2026
- **FileCommitter**: Add missing `executorServiceCacheStore` shutdown and implement ordered shutdown sequence (reject → cancel cache → await commits → force-stop uploads) to prevent orphaned files in object storage - **DeleteRecordsInterceptor**: Implement `Closeable`, narrow dependencies from `SharedState` to `ControlPlane` + `MetadataView`, add `InklessThreadFactory` for consistent thread naming, and register in `ReplicaManager.close()` - **FetchOffsetHandler**: Use `shutdownExecutorServiceQuietly()` with 5s timeout, extract constructor for testability - **Reader, Writer**: Add Javadoc documenting thread pool lifecycle (cherry picked from commit 3daf634)
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
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.
executorServiceCacheStoreshutdown and implement ordered shutdown sequence (reject → cancel cache → await commits → force-stop uploads) to prevent orphaned files in object storageCloseable, narrow dependencies fromSharedStatetoControlPlane+MetadataView, addInklessThreadFactoryfor consistent thread naming, and register inReplicaManager.close()shutdownExecutorServiceQuietly()with 5s timeout, extract constructor for testabilityTest plan