junrao commented on code in PR #23316:
URL: https://github.com/apache/kafka/pull/23316#discussion_r3927600556
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/AbstractIndex.java:
##########
@@ -252,12 +241,15 @@ public void renameTo(File f) throws IOException {
}
/**
- * Flush the data in the index to disk
+ * Flush the data in the index and its metadata to disk
*/
- public void flush() {
+ public void flush() throws IOException {
inLock(() -> {
if (mmap != null) {
mmap.force();
+ try (FileChannel channel = FileChannel.open(file.toPath(),
StandardOpenOption.WRITE)) {
Review Comment:
> 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?
##########
storage/src/test/java/org/apache/kafka/storage/internals/log/LogLoaderTest.java:
##########
@@ -1279,6 +1279,8 @@ public void testRecoveryAfterCrashDuringSplitPhase3()
throws IOException {
// Split the segment
List<LogSegment> newSegments =
logAndSegment.log.splitOverflowedSegment(logAndSegment.segment);
+ logAndSegment.log.close();
Review Comment:
Could we add a comment on why this needs to be done earlier?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
