KAFKA-20979: Fix another gap in index metadata sync logic by mimaison · Pull Request #23316 · apache/kafka

mimaison · 2026-08-31T12:51:28Z

Addressing comments from #23258

Reviewers: Chia-Ping Tsai chia7712@gmail.com, Jun Rao
junrao@gmail.com

junrao

@mimaison : Thanks for the PR. Left a comment. Also, do we have existing test cases cover the changes in this PR?

junrao · 2026-08-31T17:45:12Z

    public void close() throws IOException {
-        flush(); // Ensure the index content is flushed to disk as LogSegment.close may append an entry
-        trimToValidSize(true);
+        flush(false); // Ensure the index content is flushed to disk as LogSegment.close may append an entry


This call seems unnecessary. The second flush(true) flushes both the content and the metadata and should be enough.

trimToValidSize replaces mmap with a new mapping, so the first flush ensures force() is invoked on the previous mmap before it gets swapped out.

The javadoc of MappedByteBuffer.force only guarantees that "all changes made to the buffer since it was created" are written to the device, so changes made through the previous buffer are not strictly covered by calling force() on the new one.

WDYT?

Hmm, mmap maps page cache into user memory space. All writes to mmap are reflected in page cache. unmapping destroys the reference in user memory, but doesn't destroy page cache. So, remapping should preserve the data in page cache without flushing.

If that assumption is not true, we have problems elsewhere. When we roll a segment, we call trimToValidSize() on a rolled segment without flushing too. Flushing only happens later in the background job.

If that assumption is not true, we have problems elsewhere. When we roll a segment, we call trimToValidSize() on a rolled segment without flushing too. Flushing only happens later in the background job.

makes sense. I guess we are all betting on the page cache anyway 😄

Yes that's the reason I kept the two flush() calls. I did not want risking changing the behavior, and potentially breaking other things. It seems flushing an already flushed file is relatively cheap.

If both of you agree a single flush(true) after trimToValidSize() is fine, I'm happy to make the change.

If both of you agree a single flush(true) after trimToValidSize() is fine, I'm happy to make the change.

+1

Updated, thanks

junrao

@mimaison : Thanks for the updated PR. One more comment. Also, what about the following comment?

Also, do we have existing test cases cover the changes in this PR?

junrao · 2026-09-02T15:27:47Z

+     * @param metadata true if the metadata should be flushed as well
     */
-    public void flush() {
+    public void flush(boolean metadata) throws IOException {


All callers now set metadata to true. Could we remove metadata?

mimaison · 2026-09-02T17:25:22Z

Also, do we have existing test cases cover the changes in this PR?

There's a test in AbstractIndexTest, added in #23258, that checks AbstractIndex.close() calls flush(). Did you have anything else in mind?

junrao

@mimaison : Thanks for the updated PR. One more comment.

There's a test in AbstractIndexTest, added in #23258, that checks AbstractIndex.close() calls flush(). Did you have anything else in mind?

I was thinking that we should verify that the flush() call forces the metadata.

junrao · 2026-09-03T00:10:13Z

+                    try (FileChannel channel = FileChannel.open(file.toPath(), StandardOpenOption.WRITE)) {
+                        channel.force(true);
+                    }
+                    dirtyMetadata = false;


This seems problematic. During a control shutdown, LogManager first calls flush() on each log segment and then calls close() on each. The flush() call will clear dirtyMetadata. But close() could add an entry to index and needs to call flush() again. Since dirtyMetadata is cleared, the second flush() won't flush the metadata.

I’m fine with removing the dirty flag if my nitpick introduces any risk. However, I may be missing something in Jun’s comment:

Since dirtyMetadata is cleared, the second flush() won't flush the metadata.

In close(), maybeAppend only writes index content through the mmap and does not change the file size, so mmap.force() covers it.

If the file size does change, trimToValidSize sets the dirty flag again, so the second flush() would still force the metadata. Am I missing a case?

junrao

@mimaison : Thanks for the updated PR. A couple of more comments.

junrao · 2026-09-03T18:42:26Z

        inLock(() -> {
            if (mmap != null) {
                mmap.force();
+                try (FileChannel channel = FileChannel.open(file.toPath(), StandardOpenOption.WRITE)) {


I’m fine with removing the dirty flag if my nitpick introduces any risk. However, I may be missing something in Jun’s comment:

Since dirtyMetadata is cleared, the second flush() won't flush the metadata.

In close(), maybeAppend only writes index content through the mmap and does not change the file size, so mmap.force() covers it.

If the file size does change, trimToValidSize sets the dirty flag again, so the second flush() would still force the metadata. Am I missing a case?

@chia7712 : You are right. The earlier dirty flag logic is correct. I overlooked that it's set to true on the resize() call. It does have a minor issue. The contract for flush() is that we need to flush both the data and the metadata of the index if they are dirty. When an index file is initialized, dirtyMetadata is set to false. This is unintuitive since the index initialization changes the file length.

The current PR does introduce an unnecessary file metadata sync on the last segment during close since LogManager first calls flush() on each log segment and then calls close() on each segment, which triggers a second flush() call. The file metadata sync on the first flush() call is unnecessary.

Currently, the issue is that we flush a rolled segment in a slightly different way from the flush during shutdown. When we roll a segment, we call LogSegment.onBecomeInactiveSegment(), which appends the last timeindex entry if necessary. When flushing a rolled segment, we just call segment.flush() since the extra timeindex entry has been added. However, during shutdown, LogManager first calls flush() on each log segment and call close on each segment. It depends on close() to add the last timeindex entry on the active and to call flush again. I am thinking the following approach to fix this issue in a cleaner way.

During shutdown, LogSegment will call LogSegment.onBecomeInactiveSegment() on the active segment and then call flush. LogSegment.close() won't call timeIndex().maybeAppend to add the last index entry and AbstractIndex.close() won't call flush. This way, we can decouple flush and close. The benefits are (1) only flushing the file metadata once during shutdown; (2) consistent approach when flushing a rolled segment and the active segment; and (3) avoiding unnecessary flush call when LogSegment is closed in LogLoader.load() and LocalLog.splitSegment(). What do you think?

junrao · 2026-09-03T18:43:21Z

        // Split the segment
        List<LogSegment> newSegments = logAndSegment.log.splitOverflowedSegment(logAndSegment.segment);

+        logAndSegment.log.close();


Could we add a comment on why this needs to be done earlier?

KAFKA-20979: Fix another gap in index metadata sync logic

725410d

github-actions Bot added storage Pull requests that target the storage module small Small PRs labels Aug 31, 2026

mimaison mentioned this pull request Aug 31, 2026

KAFKA-20979: Ensure indexes resize are fsynced when shutting down #23258

Merged

mimaison requested review from chia7712 and junrao August 31, 2026 15:14

chia7712 reviewed Aug 31, 2026

View reviewed changes

Comment thread storage/src/main/java/org/apache/kafka/storage/internals/log/AbstractIndex.java

junrao reviewed Aug 31, 2026

View reviewed changes

Keep a single call to flush()

f011181

junrao reviewed Sep 2, 2026

View reviewed changes

Remove flush() argument, add dirtyMetadata flag

5b8f8b3

junrao reviewed Sep 3, 2026

View reviewed changes

Remove dirtyMetadata flag

c3ad063

junrao reviewed Sep 3, 2026

View reviewed changes

Add comment in LogLoaderTest

6eabc6d

Uh oh!

Conversation

mimaison commented Aug 31, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimaison commented Sep 2, 2026

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mimaison commented Aug 31, 2026 •

edited by github-actions Bot

Loading