Skip to content

Concurrent loading segment cache#18489

Merged
GWphua merged 27 commits into
apache:masterfrom
GWphua:concurrent-loading-segment-cache
Jan 12, 2026
Merged

Concurrent loading segment cache#18489
GWphua merged 27 commits into
apache:masterfrom
GWphua:concurrent-loading-segment-cache

Conversation

@GWphua

@GWphua GWphua commented Sep 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Concurrent loading of cached segments during startup

Previously, we are using 1 thread to run SegmentLocalCacheManager#getCachedSegments. We may take quite a bit of time if the number of segments is large.

We can do better by multithreading using loadOnBootstrapExec, and speed up segment retrieval.

Refactoring

  • Create segmentsToLoad array via extracted method: retrieveSegmentMetadataFiles.
  • Extract logic to load segments into addFilesToCachedSegments

Log Changes

  • Add stopwatch to report time taken to load cached segments.
  • Removed log "Loading segment cache file [%d/%d][%s]." to remove log clutter, stopwatch will help to handle this better. (It was annoying to navigate ~100k lines of this when looking through logs)

Release note

Speed up load of cached segments during Historical startup.

Key changed/added classes in this PR
  • SegmentLocalCacheManager
  • SegmentCacheManager + Relevant implementations.

This PR has:

  • been self-reviewed.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • been tested in a test Druid cluster.

kfaraz
kfaraz reviewed Sep 5, 2025
View reviewed changes

@kfaraz kfaraz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@GWphua , thanks for the PR! This can be very useful.
For the most part, this PR is moving code from the original method into a new one.

Do you think it is possible to modify the PR such that the diff is minimal?
Otherwise, we might miss some minor step in the review that can lead to bugs later.

Comment thread server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java Outdated
@GWphua GWphua mentioned this pull request Sep 7, 2025
Merged
1 task
@GWphua

GWphua commented Sep 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hi @kfaraz, I have tried to changes, but the need to add a ExecutorService and use exec.submit() introduces another layer of indentation -- meaning the reviewing process will still be tedious.

I have created #18494 to help with reviewing this change, please take a look at that. I hope that after merging #18494, diff changes in this PR will be clearer. Thanks!

clintropolis
clintropolis reviewed Sep 9, 2025
View reviewed changes
Comment thread server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java Outdated
Comment on lines +202 to +204
AtomicInteger ignoredfileCounter = new AtomicInteger(0);
CountDownLatch latch = new CountDownLatch(segmentsToLoad.length);
ExecutorService exec = Objects.requireNonNullElseGet(loadOnBootstrapExec, MoreExecutors::newDirectExecutorService);

@clintropolis clintropolis Sep 9, 2025

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.

drive by comment since i haven't had a chance to do a full review yet:

given that the actual load of these segments is already happening concurrently on a pool (by the thing that calls getCachedSegments), I'm wondering if instead of putting deserializing these info files on a thread pool to instead combine reading the info file and mounting the segment into a single thread, instead of threads to load all the infos then more threads to do all the mounts.

That said, it is a bit more involved of a refactor, because SegmentCacheBootstrapper is what currently collects all the metadata today since it blends both the stuff on disk and the separate 'bootstrap' segments to then put onto the pool. Need to think a bit more about what that would look like.

@GWphua GWphua Oct 1, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If I'm understanding you correctly, then the flow will be something like:

  1. Put getBootstrapSegments() into array.
  2. Retrieve File[] segmentsToLoad from getEffectiveInfoDir()
  3. Parallelization, each thread does the following:
  • Load N cached segments
  • loadSegmentOnBootstrap for N cached segments
  • Announce the N cached segments.
  1. Some of the threads that finished working can load the bootstrap segments. (This can happen anytime after (1))

This will indeed involve some refactoring... But I guess we can have the above steps? Let me know if i am missing anything.

@GWphua
GWphua requested a review from kfaraz October 6, 2025 08:57
@GWphua GWphua mentioned this pull request Oct 22, 2025
Closed
7 tasks
FrankChen021
FrankChen021 approved these changes Oct 24, 2025
View reviewed changes
Comment thread server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java Outdated
Comment thread server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java Outdated
@GWphua

GWphua commented Dec 18, 2025

Copy link
Copy Markdown
Contributor Author

Hi @kfaraz, hopefully the changes now are more reviewer-friendly. Can take a look again. Thanks!

kfaraz
kfaraz approved these changes Dec 18, 2025
View reviewed changes

@kfaraz kfaraz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, @GWphua !
I have left minor suggestions, otherwise looks good.

Comment thread server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java Outdated
Comment thread server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java Outdated
Comment thread server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java Outdated
Comment thread server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java Outdated
Comment thread server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java Outdated
Comment thread server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java Outdated
@GWphua

GWphua commented Dec 18, 2025

Copy link
Copy Markdown
Contributor Author

Thanks @kfaraz for the review! Comments are resolved, feel free to take a look again. :)

kfaraz
kfaraz approved these changes Dec 18, 2025
View reviewed changes
Comment thread server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java Outdated
@GWphua
GWphua requested a review from clintropolis December 19, 2025 07:56
@GWphua

GWphua commented Jan 2, 2026

Copy link
Copy Markdown
Contributor Author

Hi @clintropolis are you still looking at this PR?

If there are no further comments, I will be merging this PR soon (maybe by next Friday). Thanks!

@GWphua

GWphua commented Jan 12, 2026

Copy link
Copy Markdown
Contributor Author

Thanks all for the reviews!

I will merge this in first, if we need to do further refactoring, new PR's can be created.

@GWphua
GWphua merged commit 74dee84 into apache:master Jan 12, 2026
82 of 83 checks passed
@GWphua
GWphua deleted the concurrent-loading-segment-cache branch January 12, 2026 09:47
@kgyrtkirk kgyrtkirk added this to the 37.0.0 milestone Jan 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kfaraz kfaraz kfaraz approved these changes
@FrankChen021 FrankChen021 FrankChen021 approved these changes
@clintropolis clintropolis Awaiting requested review from clintropolis

Assignees

No one assigned

Labels

Design Review Performance

Projects

None yet

Milestone

37.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@GWphua @clintropolis @FrankChen021 @kfaraz @kgyrtkirk @cryptoe