KAFKA-9373: Reduce shutdown time by avoiding unnecessary loading of indexes

[ijuma]

KAFKA-7283 enabled lazy mmap on index files by initializing indices
on-demand rather than performing costly disk/memory operations when
creating all indices on broker startup. This helped reducing the startup
time of brokers. However, segment indices are still created on closing
segments, regardless of whether they need to be closed or not.

This is a cleaned up version of #7900, which was submitted by @efeg. It
eliminates unnecessary disk accesses and memory map operations while
deleting, renaming or closing offset and time indexes.

In a cluster with 31 brokers, where each broker has 13K to 20K segments,
@efeg and team observed up to 2 orders of magnitude faster LogManager
shutdown times - i.e. dropping the LogManager shutdown time of each
broker from 10s of seconds to 100s of milliseconds.

To avoid confusion between renameTo and setFile, I replaced the
latter with the more restricted updateParentDir` (it turns out that's
all we need).

Co-authored-by: Adem Efe Gencer agencer@linkedin.com
Co-authored-by: Ismael Juma ismael@juma.me.uk

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[ijuma]

@efeg In #7900, you also had included a change that:

"Prevents illegal accesses to underlying indices of a closed segment, which would lead to memory leaks due to recreation of the underlying memory mapped objects."

Under what scenario could this happen, have you seen it in production? Or is it more of a preventative measure to catch potential bugs?

[junrao]

@ijuma : Thanks for the PR. Looks good overall. Just a couple of comments below.

[junrao]

Identation of *. Also there seems to be tab in front of AbstractIndex.

[junrao]

Hmm, we didn't do that before. Is there a reason that we should hide this exception?

[ijuma]

We may call renameTo when the index file has not been created yet with this new approach. So we catch this case and ignore it. We could alternatively check if the file exists before calling atomicMoveWithFallback.

[junrao]

Interesting. That's true for the active segment.

[ijuma]

Tests passed.

[junrao]

@ijuma : Thanks for the updated PR. LGTM

[ijuma]

One known flake in one job:

kafka.api.ConsumerBounceTest.testRollingBrokerRestartsWithSmallerMaxGroupSizeConfigDisruptsBigGroup

2 in the other:

kafka.api.PlaintextConsumerTest.testLowMaxFetchSizeForRequestAndPartition
kafka.api.SaslSslAdminIntegrationTest.testCreateDeleteTopics

Checked the failures, not related. Merging to trunk.

[efeg]

@efeg In #7900, you also had included a change that:

"Prevents illegal accesses to underlying indices of a closed segment, which would lead to memory leaks due to recreation of the underlying memory mapped objects."

Under what scenario could this happen, have you seen it in production? Or is it more of a preventative measure to catch potential bugs?

@ijuma I saw this in a unit test (please see LogSegmentTest#testTruncateEmptySegment), but not in production. Hence, this change is preventative.

-- Thanks for this PR!