KAFKA-15511: Catch CorruptIndexException instead of CorruptRecordException #14459
Conversation
This file contains 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
…alling index SanityCheck
divijvaidya
reviewed
Sep 27, 2023
core/src/test/scala/unit/kafka/log/remote/RemoteIndexCacheTest.scala
Outdated
Show resolved
Hide resolved
divijvaidya
added
the
tiered-storage
Hangleton
reviewed
Sep 27, 2023
| @@ -310,7 +310,7 @@ private <T> T loadIndexFile(File file, RemoteLogSegmentMetadata remoteLogSegment | |||
| if (Files.exists(indexFile.toPath())) { | |||
| try { | |||
| index = readIndex.apply(indexFile); | |||
| } catch (CorruptRecordException ex) { | |||
There was a problem hiding this comment.
Can CorruptRecordException be thrown in some scenario?
There was a problem hiding this comment.
@Hangleton The RemoteIndexCache caches only index files which needs to be checked for corruption. There is no such record concept in RemoteIndexCache.
@divijvaidya @satishd can add more details here.
There was a problem hiding this comment.
Thanks, I wanted to confirm that it is indeed the case although we are dealing with indexes here. The rationale being that CorruptRecordException may have been introduced purposefully to cover a use case. Was this not covered by a test in the initial PR, and if it was, did the test make an incorrect assumption, or exercised an invalid (that is, impossible) scenario?
There was a problem hiding this comment.
Was this not covered by a test in the initial PR
@Hangleton no we don't have a test case to execute this code. In fact, the bug fixed this PR was discovered while working on the task [1] to add more unit test for remote index cache.
iit2009060
force-pushed
the
KAFKA-15511
branch
2 times, most recently
from
e81ca46
to
5bc38e4
Compare
iit2009060
force-pushed
the
KAFKA-15511
branch
from
5bc38e4
to
e18d6fd
Compare
|
Test failures are unrelated to Tiered Storage code. |
divijvaidya
approved these changes
Sep 28, 2023
|
@Hangleton @satishd since you folks participated in the review for this PR, do you have any additional comments? |
satishd
approved these changes
Sep 29, 2023
kamalcph
approved these changes
Sep 29, 2023
| @@ -598,4 +615,13 @@ class RemoteIndexCacheTest { | |||
| timeIndex.flush() | |||
| } | |||
| } | |||
|
|
|||
| private def createCorruptRemoteIndexCacheOffsetFile(): Unit = { | |||
| val pw = new PrintWriter((remoteOffsetIndexFile(new File(tpDir, RemoteIndexCache.DIR_NAME),rlsMetadata))) | |||
There was a problem hiding this comment.
nit: space after comma and remove extra brackets
|
Unrelated test failures |
divijvaidya
approved these changes
Sep 29, 2023
|
@divijvaidya It seems i don't have permission to merge it even after the approval. |
divijvaidya
pushed a commit
that referenced
this pull request
Sep 29, 2023
A bug in the RemoteIndexCache leads to a situation where the cache does not replace the corrupted index with a new index instance fetched from remote storage. This commit fixes the bug by adding correct handling for `CorruptIndexException`. Reviewers: Divij Vaidya <diviv@amazon.com>, Satish Duggana <satishd@apache.org>, Kamal Chandraprakash <kamal.chandraprakash@gmail.com>, Alexandre Dupriez <duprie@amazon.com>
|
Merged to trunk and 3.6. Thank you for the contribution @iit2009060. For Apache projects, only the committers can merge code into the repository. |
|
Thanks for the fix @iit2009060 ! Nice find! |
rreddy-22
pushed a commit
to rreddy-22/kafka-rreddy
that referenced
this pull request
Oct 3, 2023
…#14459) A bug in the RemoteIndexCache leads to a situation where the cache does not replace the corrupted index with a new index instance fetched from remote storage. This commit fixes the bug by adding correct handling for `CorruptIndexException`. Reviewers: Divij Vaidya <diviv@amazon.com>, Satish Duggana <satishd@apache.org>, Kamal Chandraprakash <kamal.chandraprakash@gmail.com>, Alexandre Dupriez <duprie@amazon.com>
k-wall
pushed a commit
to k-wall/kafka
that referenced
this pull request
Nov 21, 2023
…#14459) A bug in the RemoteIndexCache leads to a situation where the cache does not replace the corrupted index with a new index instance fetched from remote storage. This commit fixes the bug by adding correct handling for `CorruptIndexException`. Reviewers: Divij Vaidya <diviv@amazon.com>, Satish Duggana <satishd@apache.org>, Kamal Chandraprakash <kamal.chandraprakash@gmail.com>, Alexandre Dupriez <duprie@amazon.com>
mjsax
pushed a commit
to confluentinc/kafka
that referenced
this pull request
Nov 22, 2023
…#14459) A bug in the RemoteIndexCache leads to a situation where the cache does not replace the corrupted index with a new index instance fetched from remote storage. This commit fixes the bug by adding correct handling for `CorruptIndexException`. Reviewers: Divij Vaidya <diviv@amazon.com>, Satish Duggana <satishd@apache.org>, Kamal Chandraprakash <kamal.chandraprakash@gmail.com>, Alexandre Dupriez <duprie@amazon.com>
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.
Scenario
The bug occurs when RemoteIndexCache does not have an entry but CorruptedIndex File exists in the desired path.
Current behaviour
It will throw the CorruptIndex Exception and does not recover or replace the CorruptedIndexFile with new File fetch from remote storage.
Desired Behaviour
It should catch the CorruptIndex Exception and should refetch it from remote storage and overwrite the corrupted file.
Testing
Written Unit Test to test the above scenario
Committer Checklist (excluded from commit message)