Fix Journal without flush

[gaozhangmin]

Motivation

In the given code snippet, a ForceWriteRequest object is created with the flushed flag always set to true when calling the createForceWriteRequest method, reused from a recycle pool. Consequently, when the flushFileToDisk method is invoked, it checks the flushed flag and only performs the file flush if it is false. However, since the flushed flag is always true due to the recycling process, the file flush is skipped.

private ForceWriteRequest createForceWriteRequest(JournalChannel logFile,
                          long logId,
                          long lastFlushedPosition,
                          RecyclableArrayList<QueueEntry> forceWriteWaiters,
                          boolean shouldClose) {
        ForceWriteRequest req = forceWriteRequestsRecycler.get();
        req.forceWriteWaiters = forceWriteWaiters;
        req.logFile = logFile;
        req.logId = logId;
        req.lastFlushedPosition = lastFlushedPosition;
        req.shouldClose = shouldClose;
        journalStats.getForceWriteQueueSize().inc();
        return req;
    }

private void flushFileToDisk() throws IOException {
            if (!flushed) {
                logFile.forceWrite(false);
                flushed = true;
            }
        }

Changes:

To address this issue, the recycle() method should be modified to set the flushed flag to false when recycling a ForceWriteRequest object. This ensures that the flushFileToDisk method will correctly perform the file flush when needed. The updated recycle() method could look like this:

public void recycle() {
    // Reset other fields to their initial state
    forceWriteWaiters = null;
    logFile = null;
    logId = 0;
    lastFlushedPosition = 0;
    shouldClose = false;

    // Reset the flushed flag to false
    flushed = false;

    // Recycle the object back to the pool
    forceWriteRequestsRecycler.recycle(this);
}

By setting flushed to false during recycling, subsequent reuse of the ForceWriteRequest object will correctly trigger the file flush operation in the flushFileToDisk method when necessary.

[horizonzy]

LGTM.

[lhotari]

Good catch! Does this bug mean that Bookkeeper hasn't been fsyncing the journal to disk in certain cases?

[horizonzy]

Good catch! Does this bug mean that Bookkeeper hasn't been fsyncing the journal to disk in certain cases?

Yes.

[wenbingshen]

Good Catch!

[hangc0276]

This is a critical bug, and we need to trigger a new release.

[horizonzy]

This is a critical bug, and we need to trigger a new release.

+1

[michaeljmarshall]

This kind of bug is completely avoided by removing object recycling. I understand that object recycling was added a while ago, but I wonder if it is still necessary on JVM 17. It'd be valuable to run benchmarks to verify the underlying assumption that we get better performance by recycling objects.

[eolivelli]

This is a great point

[merlimat]

It should be easy to test because you can disable the recycler using Netty Java properties

[eolivelli]

LGTM

I wonder if we could follow up and add some tests.

We have tests about force() being called (I remember I added them while working on DEFERED_SYNC)

[horizonzy]

We can add a test to cover the recycle() case in the important place
By comparing new objects with objects retrieved from the pool.

[nicoloboschi]

@gaozhangmin do we know if it's a regression of a particular BookKeeper version?

[tisonkun]

We can add a test to cover the recycle() case in the important place By comparing new objects with objects retrieved from the pool.

Instead of testing the implementation, perhaps testing the performance a good idea - if missing recycling doesn't cause performance regression, that should be fine.

Does BookKeeper have any kind of performance regression coverage now?

[horizonzy]

Does BookKeeper have any kind of performance regression coverage now?

It has, I will test the performance without object recycling. But, I am not sure about the performance impact on low-performance CPUs.