[server] Recover log and index file for unclean shutdown#1749
Conversation
LiebingYu
force-pushed
the
fix-corrupt-index-new
branch
2 times, most recently
from
b9b5f2d to
ceaf704
Compare
|
Ready for CR @wuchong @swuferhong |
swuferhong
left a comment
swuferhong
left a comment
There was a problem hiding this comment.
@LiebingYu Thanks for your work. I left some comments.
LiebingYu
force-pushed
the
fix-corrupt-index-new
branch
from
ceaf704 to
1e8a582
Compare
| // can potentially skip over more segments. | ||
| LogTablet.rebuildWriterState( | ||
| writerStateManager, logSegments, 0, segment.getBaseOffset(), false); | ||
| int bytesTruncated = segment.recover(); |
There was a problem hiding this comment.
Do we need to pass writerStateManager into segment.recover() to update writer state like how kafka does? cc @swuferhong
There was a problem hiding this comment.
Kafka use writerStateManager in segment.recover() mainly for txn operations. I think Fluss doen't need here. What's your opinion @swuferhong?
There was a problem hiding this comment.
After discussing offline with @LiebingYu , we confirmed that it‘s necessary to rebuild the writerState1. Although it is not used during recovery, this code path ensures that when writerStateManager.takeSnapshot()` is called, a correct snapshot up to the current segment is written to disk. This way, during recovery, each segment can be restored based on a fully accurate writerState snapshot, without requiring a full global reconstruction.
| segment.getFileLogRecords().file().getAbsoluteFile()); | ||
| } | ||
| recoverSegment(segment); | ||
| } |
There was a problem hiding this comment.
Should we catch CorruptIndexException and recover segment for this case like how kafka does? And we should rethrow other exceptions in else branch.
There was a problem hiding this comment.
- Kafka throw
CorruptIndexExceptionbecause Kafka will do sanity check for transaction file which may throwCorruptIndexException. But here in fluss we don't do sanity check of index files insegment.sanityCheck. So I think we don't need to catchCorruptIndexExceptionhere.
|
I added a commit to fix the |
…che#1749)" (apache#2036) This reverts commit b41fa53.
3 participants
Purpose
Linked issue: close #1716
Brief change log
Tests
API and Format
Documentation