Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -328,14 +328,14 @@ LogSegment createAndDeleteSegment(
if (newOffset == segmentToDelete.getBaseOffset()) {
deleteSegmentFiles(Collections.singletonList(segmentToDelete), reason);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When newOffset equals the segment's base offset, this deletes the old segment before its replacement is opened. deleteIfExists() closes the log channel, but the old object remains the active entry in segments; an unsynchronized read can therefore select a closed segment, and if deletion or LogSegment.open() fails this LocalLog instance is left pointing at an unusable active segment. Please rename the old files to the .deleted suffix to free the original paths, install the replacement, and only then physically delete the old segment.

}
reason.logReason(Collections.singletonList(segmentToDelete));

// open a new segment.
LogSegment newSegment = LogSegment.open(logTabletDir, newOffset, config, logFormat);
segments.add(newSegment);

if (newOffset != segmentToDelete.getBaseOffset()) {
segments.remove(segmentToDelete.getBaseOffset());
deleteSegmentFiles(Collections.singletonList(segmentToDelete), reason);
}
return newSegment;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1554,7 +1554,13 @@ private static void loadWritersFromRecords(
Map<Long, WriterAppendInfo> loadedWriters = new HashMap<>();
for (LogRecordBatch batch : records.batches()) {
if (batch.hasWriterId()) {
updateWriterAppendInfo(writerStateManager, batch, loadedWriters, false);
long writerId = batch.writerId();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we keep recovery on the existing updateWriterAppendInfo/WriterAppendInfo.append path and parameterize only the sequence-validation policy (for example, ENFORCE versus WARN_AND_ACCEPT)? This block duplicates the writer lookup and aggregation logic, while appendForRecovery duplicates sequence handling and writer-state advancement. Future fixes to batch metadata or writer-state updates would then need to be maintained in both paths and could easily be missed.

WriterAppendInfo appendInfo =
loadedWriters.computeIfAbsent(
writerId, id -> writerStateManager.prepareUpdate(id));
// The records have already been accepted and persisted. Recovery rebuilds writer
// state without applying online client sequence validation.
appendInfo.appendForRecovery(batch);
}
}
loadedWriters.values().forEach(writerStateManager::update);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,18 @@
import org.apache.fluss.metadata.TableBucket;
import org.apache.fluss.record.LogRecordBatch;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import static org.apache.fluss.record.LogRecordBatchFormat.NO_BATCH_SEQUENCE;

/**
* This class is used to validate the records appended by a given writer before they are written to
* log. It's initialized with writer's state after the last successful append.
*/
public class WriterAppendInfo {
private static final Logger LOG = LoggerFactory.getLogger(WriterAppendInfo.class);

private final long writerId;
private final TableBucket tableBucket;
private final WriterStateEntry currentEntry;
Expand Down Expand Up @@ -56,6 +61,26 @@ public void append(
batch.commitTimestamp());
}

void appendForRecovery(LogRecordBatch batch) {
int currentLastSeq = currentLastBatchSequence();
if (!inSequence(currentLastSeq, batch.batchSequence(), false, false)) {
LOG.warn(
"Detected discontinuous batch sequence while recovering writer {} at offset {} "
+ "in table-bucket {}: incoming sequence {}, current sequence {}. "
+ "Accepting the persisted batch.",
writerId,
batch.lastLogOffset(),
tableBucket,
batch.batchSequence(),
currentLastSeq);
}
appendDataBatch(
batch.batchSequence(),
new LogOffsetMetadata(batch.baseLogOffset()),
batch.lastLogOffset(),
batch.commitTimestamp());
}

public void appendDataBatch(
int batchSequence,
LogOffsetMetadata firstOffsetMetadata,
Expand All @@ -64,6 +89,14 @@ public void appendDataBatch(
boolean isAppendAsLeader,
long batchTimestamp) {
maybeValidateDataBatch(batchSequence, isWriterInBatchExpired, lastOffset, isAppendAsLeader);
appendDataBatch(batchSequence, firstOffsetMetadata, lastOffset, batchTimestamp);
}

private void appendDataBatch(
int batchSequence,
LogOffsetMetadata firstOffsetMetadata,
long lastOffset,
long batchTimestamp) {
updatedEntry.addBath(
batchSequence,
lastOffset,
Expand All @@ -76,10 +109,7 @@ private void maybeValidateDataBatch(
boolean isWriterInBatchExpired,
long lastOffset,
boolean isAppendAsLeader) {
int currentLastSeq =
!updatedEntry.isEmpty()
? updatedEntry.lastBatchSequence()
: currentEntry.lastBatchSequence();
int currentLastSeq = currentLastBatchSequence();
// must be in sequence, even for the first batch should start from 0
if (!inSequence(currentLastSeq, appendFirstSeq, isWriterInBatchExpired, isAppendAsLeader)) {
throw new OutOfOrderSequenceException(
Expand All @@ -90,6 +120,12 @@ private void maybeValidateDataBatch(
}
}

private int currentLastBatchSequence() {
return !updatedEntry.isEmpty()
? updatedEntry.lastBatchSequence()
: currentEntry.lastBatchSequence();
}

public WriterStateEntry toEntry() {
return updatedEntry;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ void testCreateAndDeleteSegment() throws Exception {
assertThat(localLog.getSegments().activeSegment()).isEqualTo(newActiveSegment);
assertThat(localLog.getSegments().activeSegment()).isNotEqualTo(oldActiveSegment);
assertThat(localLog.getSegments().activeSegment().getBaseOffset()).isEqualTo(newOffset);
assertThat(oldActiveSegment.deleted()).isTrue();
assertThat(localLog.getRecoveryPoint()).isEqualTo(0L);
assertThat(localLog.getLocalLogEndOffset()).isEqualTo(newOffset);
FetchDataInfo read =
Expand Down Expand Up @@ -337,6 +338,19 @@ void testTruncateFullyAndStartAt() throws Exception {
assertThat(read.getRecords().sizeInBytes()).isEqualTo(0);
}

@Test
void testTruncateFullyAndStartAtDeletesOldActiveSegmentFile() throws Exception {
LogSegment oldActiveSegment = localLog.getSegments().activeSegment();
File oldLogFile = oldActiveSegment.getFileLogRecords().file();
assertThat(oldLogFile).exists();

localLog.truncateFullyAndStartAt(10L);

assertThat(localLog.getSegments().baseOffsets()).containsExactly(10L);
assertThat(oldActiveSegment.deleted()).isTrue();
assertThat(oldLogFile).doesNotExist();
}

@Test
void testTruncateTo() throws Exception {
for (int i = 0; i <= 11; i++) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,37 @@ void testWriterSnapshotRecoveryFromDiscontinuousBatchSequence() throws Exception
.isEqualTo(13);
}

@Test
void testWriterStateRecoveryAcceptsBatchSequenceGap() throws Exception {
LogTablet log = createLogTablet(true);
long writerId = 1L;

log.appendAsFollower(
genMemoryLogRecordsWithWriterId(
Collections.singletonList(new Object[] {1, "a"}), writerId, 10, 0L));
log.appendAsFollower(
genMemoryLogRecordsWithWriterId(
Collections.singletonList(new Object[] {2, "b"}), writerId, 11, 1L));
log.roll(Optional.empty());

MemoryLogRecords recordsWithSequenceGap =
genMemoryLogRecordsWithWriterId(
Collections.singletonList(new Object[] {3, "c"}), writerId, 100, 2L);
log.activeLogSegment().append(2L, clock.milliseconds(), 2L, recordsWithSequenceGap);
log.close();

log = createLogTablet(false);
assertThat(log.localLogEndOffset()).isEqualTo(3L);
assertThat(log.writerStateManager().activeWriters().get(writerId).lastBatchSequence())
.isEqualTo(100);

// The recovered state should be persisted in the new snapshot and survive another restart.
log.close();
log = createLogTablet(false);
assertThat(log.writerStateManager().activeWriters().get(writerId).lastBatchSequence())
.isEqualTo(100);
}

@Test
void testWriterSnapshotsRecoveryAfterCleanShutdown() throws Exception {
LogTablet log = createLogTablet(true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,46 @@ void testWriterStateTruncateFullyAndStartAt() throws Exception {
assertThat(latestWriterSnapshotOffset(log).get()).isEqualTo(29);
}

@Test
void testTruncateToBeforeFirstSegmentDeletesHigherOffsetSegment() throws Exception {
logTablet.truncateFullyAndStartAt(10L);
logTablet.appendAsLeader(
genMemoryLogRecordsByObject(Collections.singletonList(new Object[] {1, "a"})));
LogSegment oldActiveSegment = logTablet.activeLogSegment();
assertThat(oldActiveSegment.getBaseOffset()).isEqualTo(10L);

logTablet.truncateTo(5L);

assertThat(oldActiveSegment.deleted()).isTrue();
assertThat(logTablet.logSegments())
.extracting(LogSegment::getBaseOffset)
.containsExactly(5L);
assertThat(logTablet.localLogEndOffset()).isEqualTo(5L);

logTablet.close();
logTablet =
LogTablet.create(
tempDir,
PhysicalTablePath.of(DATA1_TABLE_PATH),
logDir,
conf,
new AtomicBoolean(
conf.get(ConfigOptions.LOG_RETENTION_ROLL_ACTIVE_SEGMENT_ENABLED)),
TestingMetricGroups.TABLET_SERVER_METRICS,
0,
scheduler,
LogFormat.ARROW,
1,
false,
SystemClock.getInstance(),
false);

assertThat(logTablet.logSegments())
.extracting(LogSegment::getBaseOffset)
.containsExactly(5L);
assertThat(logTablet.localLogEndOffset()).isEqualTo(5L);
}

@Test
void testWriterIdExpirationOnSegmentDeletion() throws Exception {
long writerId1 = 1L;
Expand Down
Loading