-
Notifications
You must be signed in to change notification settings - Fork 7
refactor(inkless): SharedState to own StorageBackends #469
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
base: main
Are you sure you want to change the base?
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.
Pull request overview
This PR refactors the SharedState class to properly own and manage the lifecycle of storage backends, separating them into three dedicated instances for fetch, produce, and background operations. The refactoring also simplifies the MetadataView.getTopicConfig method to directly return LogConfig instead of Properties, eliminating the need for manual config merging in consuming code.
Key changes:
- SharedState now creates and owns three separate StorageBackend instances (fetchStorage, produceStorage, backgroundStorage) and is responsible for closing all of them
- Reader and Writer components no longer close storage backends; SharedState handles all storage lifecycle management
- Background components (FileMerger, FileCleaner) now have package-visible constructors for easier testing
- MetadataView.getTopicConfig() now returns LogConfig directly, with default config merging happening internally
Reviewed changes
Copilot reviewed 19 out of 19 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/common/SharedState.java | Refactored to own three separate StorageBackend instances, added comprehensive resource cleanup in close() and exception handlers |
| storage/inkless/src/main/java/io/aiven/inkless/produce/Writer.java | Removed storage parameter and close logic from internal constructor |
| storage/inkless/src/main/java/io/aiven/inkless/produce/AppendHandler.java | Simplified to use Function<String, LogConfig> instead of manually merging configs |
| storage/inkless/src/main/java/io/aiven/inkless/merge/FileMerger.java | Added package-visible constructor for testing, uses backgroundStorage from SharedState |
| storage/inkless/src/main/java/io/aiven/inkless/delete/FileCleaner.java | Updated to use backgroundStorage from SharedState |
| storage/inkless/src/main/java/io/aiven/inkless/delete/RetentionEnforcer.java | Simplified to directly use getTopicConfig without manual config merging |
| storage/inkless/src/main/java/io/aiven/inkless/delete/DeleteRecordsInterceptor.java | Refactored to accept ControlPlane and MetadataView directly instead of SharedState |
| storage/inkless/src/main/java/io/aiven/inkless/consume/Reader.java | Removed storage close logic |
| storage/inkless/src/main/java/io/aiven/inkless/consume/FetchHandler.java | Updated to use fetchStorage from SharedState |
| storage/inkless/src/main/java/io/aiven/inkless/control_plane/MetadataView.java | Changed getTopicConfig return type from Properties to LogConfig |
| core/src/main/scala/kafka/server/metadata/InklessMetadataView.scala | Implemented new getTopicConfig signature, merging default and topic configs internally |
| Test files | Updated to reflect new constructor signatures and simplified mocking requirements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c13d67f to
43e2c35
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.
Pull request overview
Copilot reviewed 16 out of 16 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/main/java/io/aiven/inkless/common/SharedState.java
Outdated
Show resolved
Hide resolved
storage/inkless/src/test/java/io/aiven/inkless/common/SharedStateTest.java
Show resolved
Hide resolved
43e2c35 to
b3b48e9
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.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
storage/inkless/src/test/java/io/aiven/inkless/common/SharedStateTest.java
Show resolved
Hide resolved
storage/inkless/src/main/java/io/aiven/inkless/common/SharedState.java
Outdated
Show resolved
Hide resolved
storage/inkless/src/main/java/io/aiven/inkless/common/SharedState.java
Outdated
Show resolved
Hide resolved
b3b48e9 to
ae9e37c
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.
Pull request overview
Copilot reviewed 18 out of 18 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/main/java/io/aiven/inkless/common/SharedState.java
Outdated
Show resolved
Hide resolved
storage/inkless/src/main/java/io/aiven/inkless/common/SharedState.java
Outdated
Show resolved
Hide resolved
ae9e37c to
ed1c0b1
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.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
storage/inkless/src/main/java/io/aiven/inkless/common/SharedState.java
Outdated
Show resolved
Hide resolved
ed1c0b1 to
1d233d8
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.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
storage/inkless/src/test/java/io/aiven/inkless/produce/AppendHandlerTest.java
Outdated
Show resolved
Hide resolved
storage/inkless/src/test/java/io/aiven/inkless/produce/AppendHandlerTest.java
Outdated
Show resolved
Hide resolved
storage/inkless/src/test/java/io/aiven/inkless/produce/AppendHandlerTest.java
Outdated
Show resolved
Hide resolved
storage/inkless/src/test/java/io/aiven/inkless/produce/AppendHandlerTest.java
Outdated
Show resolved
Hide resolved
storage/inkless/src/test/java/io/aiven/inkless/produce/AppendHandlerTest.java
Outdated
Show resolved
Hide resolved
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
8baf5de to
71ac969
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.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
storage/inkless/src/main/java/io/aiven/inkless/common/SharedState.java
Outdated
Show resolved
Hide resolved
71ac969 to
18871fb
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.
Pull request overview
Copilot reviewed 21 out of 21 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/produce/AppendHandlerTest.java
Outdated
Show resolved
Hide resolved
Properly separating storage back-end clients between read/write and background jobs. SharedState now owns the StorageBackends lifecycle, properly closing all of them. So, Reader and Writer component do not have to close storages anymore. Alongside, making background components internal constructors visible for easier testing. This allows to make internal SharedState constructor private.
18871fb to
c5c3f2f
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.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Refactoring follow up from #467
Properly separating storage back-end clients between read/write and background jobs.
SharedState now owns the StorageBackends lifecycle, properly closing all of them.
So, Reader and Writer component do not have to close storages anymore.
Alongside, making background components internal constructors visible for easier testing.
This allows to make internal SharedState constructor private.