refactor(inkless): SharedState to own StorageBackend lifecycle#562
Open
refactor(inkless): SharedState to own StorageBackend lifecycle#562
Conversation
SharedState now creates and manages 4 dedicated StorageBackend instances (fetch, laggingFetch, produce, background) instead of exposing a buildStorage() factory method. This centralizes resource lifecycle, eliminates double-close risks, and makes ownership explicit. - Private constructor with typed storage backend parameters - initialize() creates backends with try/catch cleanup on partial failure - close() shuts down all backends in reverse creation order - Consumers (Reader, Writer, FileCleaner, FileMerger) no longer close storage backends — SharedState owns their lifecycle Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors Inkless SharedState to centrally own the lifecycle of storage backends by eagerly creating dedicated StorageBackend instances for fetch/produce/background use, and updating consumers to stop creating/closing their own backends.
Changes:
SharedState.initialize()now creates and stores 4 backend instances (fetchStorage, optionalmaybeLaggingFetchStorage,produceStorage,backgroundStorage) andclose()shuts them down.- Consumers (
FetchHandler/Reader,AppendHandler/Writer,FileCleaner,FileMerger) are updated to use the new accessors and no longer close storage. - Adds
SharedStateTestand updates multiple tests to reflect the new constructor/lifecycle behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| storage/inkless/src/main/java/io/aiven/inkless/common/SharedState.java | Centralizes backend creation and lifecycle; adds dedicated backend accessors and new close behavior. |
| storage/inkless/src/main/java/io/aiven/inkless/consume/FetchHandler.java | Switches from buildStorage() to fetchStorage() / maybeLaggingFetchStorage(). |
| storage/inkless/src/main/java/io/aiven/inkless/consume/Reader.java | Accepts optional lagging backend and stops closing storage backends on close(). |
| storage/inkless/src/main/java/io/aiven/inkless/produce/AppendHandler.java | Uses produceStorage() instead of building a backend per consumer. |
| storage/inkless/src/main/java/io/aiven/inkless/produce/Writer.java | Removes direct storage ownership/closing; relies on SharedState lifecycle. |
| storage/inkless/src/main/java/io/aiven/inkless/delete/FileCleaner.java | Uses backgroundStorage() and stops closing storage in close(). |
| storage/inkless/src/main/java/io/aiven/inkless/merge/FileMerger.java | Uses backgroundStorage() and stops closing storage in close(). |
| storage/inkless/src/test/java/io/aiven/inkless/common/SharedStateTest.java | New tests for shutdown ordering, lagging-disabled path, and init-failure cleanup. |
| storage/inkless/src/test/java/io/aiven/inkless/delete/DeleteRecordsInterceptorTest.java | Updates test construction to avoid SharedState dependency. |
| storage/inkless/src/test/java/io/aiven/inkless/merge/FileMergerMockedTest.java | Adds config stubs needed for eager SharedState.initialize() backend creation. |
| storage/inkless/src/test/java/io/aiven/inkless/produce/WriterMockedTest.java | Updates Writer construction to reflect removed storage parameter in test ctor. |
| storage/inkless/src/test/java/io/aiven/inkless/produce/WriterPropertyTest.java | Updates Writer construction to reflect removed storage parameter in test ctor. |
💡 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/test/java/io/aiven/inkless/common/SharedStateTest.java
Outdated
Show resolved
Hide resolved
…ize() catch block The catch block in initialize() was closing fetchStorage before laggingFetchStorage, inconsistent with close() which correctly uses reverse creation order. Fixed to: background → produce → lagging → fetch in both paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix ReplicaManagerTest$Inkless: stub maybeLaggingFetchStorage() to return Optional.empty() and set lagging consumer thread pool size to 0 so Reader validation passes (fetcher and executor must both be null when feature is disabled) - Fix initialize() catch block to close in reverse creation order (background → produce → lagging → fetch), consistent with close() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 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/merge/FileMergerMockedTest.java
Show resolved
Hide resolved
Prevents resource leaks (storage backends, metrics) between tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
On init failure, controlPlane was not closed even though SharedState takes ownership of it on the success path. This could leak control-plane resources (threads/connections) when initialization fails. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n close() close() was closing objectCache before batchCoordinateCache, but batchCoordinateCache is created after objectCache in initialize(), so reverse order should close batchCoordinateCache first. Now both close() and the catch block use the same reverse-creation order. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 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.
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.
Summary
SharedStatenow creates and manages 4 dedicatedStorageBackendinstances (fetchStorage,maybeLaggingFetchStorage,produceStorage,backgroundStorage) instead of exposing abuildStorage()factory methodReader,Writer,FileCleaner,FileMerger) no longer close storage backends —SharedStateowns their lifecycleChanges
initialize()creates backends with try/catch cleanup,close()shuts down all backends in reverse creation order usingUtils.closeQuietlyfetchStorage()andmaybeLaggingFetchStorage()(Optional) from SharedStateproduceStorage(), Writer no longer stores or closes a StorageBackend fieldbackgroundStorage()(shared, thread-safe)SharedStateTest: verifies close ordering, lagging-disabled path, and failure cleanupMotivation
Previously each consumer called
buildStorage()to create its own backend and was responsible for closing it. This led to:Test plan
SharedStateTestcovers close ordering, disabled lagging path, and partial init failure cleanup🤖 Generated with Claude Code