KAFKA-14490: Throw UncheckedIOException from LazyIndex#22330
Open
Phixsura wants to merge 2 commits into
Open
Conversation
Wraps IOException at the file-IO boundary inside IndexFile, IndexValue and
the index loader so LazyIndex's public methods no longer declare
`throws IOException`, matching the pattern already in place in the raft
module (FileRawSnapshotReader and peers).
Folds in the constructor cleanup called out in the ticket description:
`(IndexWrapper, baseOffset, maxIndexSize, IndexType)` becomes
`(IndexWrapper, IndexLoader<T>)`, removing the private `loadIndex` switch
on `IndexType` and its `@SuppressWarnings("unchecked")` cast. `forOffset`
and `forTime` pass a lambda factory instead.
External callers' signatures are left untouched; their `throws IOException`
clauses become redundant but are safe to keep until the follow-up files in
KAFKA-14490 are converted.
First PR in the KAFKA-14490 series.
github-actions
Bot
added
triage
…dexTest Aligns the wrap sites with the raft module's existing pattern (FileRawSnapshotReader, Snapshots, FileQuorumStateStore): each UncheckedIOException now carries a String.format message naming the operation and the affected index file, so stack traces in production identify the failing path without needing the cause chain. LazyIndexTest covers the public API end-to-end with real files via TestUtils.tempFile, including: - forOffset / forTime return the right index subtype on get - get caches and returns the same instance on subsequent calls - get does not touch disk until first call - get wraps loader IOException as UncheckedIOException carrying "Error loading index file" and the original IOException as cause - renameTo swallows NoSuchFileException when the source has already been deleted (preserves the existing tolerance contract) - deleteIfExists and updateParentDir behave correctly before and after load - close is safe both before and after load
Author
|
@ijuma First PR in the KAFKA-14490 series is up. This batch covers |
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
2 participants
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.
Background
KAFKA-14490 proposes replacing checked
IOExceptionwithUncheckedIOExceptionin the log layer, following the pattern already adopted by the raft module. The ticket description specifically calls outLazyIndex's private constructor as a candidate for simplification onceIOExceptionis unchecked.This is the first PR in the KAFKA-14490 series. Plan posted on the ticket walks through the remaining 17 files in dependency order.
Changes
LazyIndex.get/renameTo/deleteIfExists/closeno longer declarethrows IOException.IOExceptionis caught at the file-IO boundary insideIndexFile,IndexValueand the loader, and re-thrown asUncheckedIOException. This mirrorsFileRawSnapshotReader/FileRawSnapshotWriter/Snapshots/FileQuorumStateStore/KafkaRaftLogin the raft module.(IndexWrapper, long, int, IndexType)to(IndexWrapper, IndexLoader<T>). The newIndexLoader<T>is a small@FunctionalInterfacethat allows the lambda to declarethrows IOException.forOffsetandforTimenow pass a lambda that capturesbaseOffsetandmaxIndexSizedirectly.loadIndexprivate method and its@SuppressWarnings("unchecked")switch onIndexTypeare removed. The remaining@SuppressWarnings("unchecked")ongetis for the(IndexValue<T>) wrappercast, which is structurally required by the<?>wildcard oninstanceof IndexValue<?>and cannot be eliminated without a larger refactor.Scope kept minimal
External callers' method signatures (e.g.
LogSegment.offsetIndex() throws IOException) are left untouched. Theirthrows IOExceptionclauses become redundant sinceLazyIndex.getno longer throws checked, but Java permits over-declaredthrowsand pruning them belongs in the follow-up PRs that convert the surrounding files. This keeps the blast radius of the first PR limited toLazyIndex.java.Verification
:storage:compileJavaclean.:storage:test --tests "*LogSegment*" --tests "*Index*"all green.:core:test --tests "*LogSegment*" --tests "*UnifiedLog*" --tests "*LazyIndex*"all green (covers the ScalaLogTestUtilsandRemoteLogManagerTestcallers).:storage:spotlessCheck,:storage:checkstyleMain,:storage:checkstyleTest,:storage:spotbugsMain --rerun-tasksall clean.Committer review notes
Happy to split out the constructor cleanup into its own PR if reviewers prefer the IOException conversion be reviewed in isolation first.