Skip to content

Support adaptive refresh in Searcher Managers. - #14443

Merged
vigyasharma merged 22 commits into
apache:mainfrom
vigyasharma:bite_commit_refresh
May 18, 2025
Merged

Support adaptive refresh in Searcher Managers.#14443
vigyasharma merged 22 commits into
apache:mainfrom
vigyasharma:bite_commit_refresh

Conversation

@vigyasharma

Copy link
Copy Markdown
Contributor

In segment based replication systems, a large replication payload (checkpoint) can induce heavy page faults, cause thrashing for in-flight search requests, and affect overall search performance.

A potential way to handle these bursts, is to leverage multiple commit points in the Lucene index. Instead of refreshing to the latest commit for a large replication payload, searchers can intelligently select the commit point that they can safely absorb. By processing through multiple such points, searchers can eventually get to the latest commit, without incurring too many page faults.

This change lets users define a commit selection strategy, controlling which commit the searcher manager refreshes on. Addresses #14219

Usage:
To incrementally refresh through multiple commit points until searcher is current with its directory:

  • Define a commit selection strategy using the RefreshCommitSupplier interface.
  • Update searcher managers with this strategy via setRefreshCommitSupplier()
  • Invoke maybeRefresh() or maybeRefreshBlocking in a loop until isSearcherCurrent() returns true.

@jpountz

jpountz commented Apr 7, 2025

Copy link
Copy Markdown
Contributor

Thanks for tackling this!

To incrementally refresh through multiple commit points until searcher is current with its directory:

[...]
Invoke maybeRefresh() or maybeRefreshBlocking in a loop until isSearcherCurrent() returns true.

Is this the way we anticipate this to be used? I had imagined that the application would not change the way it refreshes and still call it on a schedule, but commit more frequently and retain multiple commits. E.g. commit every 30 seconds, retain commits for 300 seconds and refresh every 120 seconds (these numbers are just for the sake of the example). So every 120 seconds, SearcherManager would pick the most recent commit that differs by less than X GB (configurable based on the amount of trashing that the app can sustain between consecutive point-in-time views of the index) from the current point-in-time reader, or the commit that differs by the least amount of data if there is no such commit (typically the oldest commit). Most of the time, SearcherManager would pick the newest commit point, but under heavy merging it may decide to lag behind the latest commit point a bit for the sake of smoothing out page cache trashing.

@vigyasharma

Copy link
Copy Markdown
Contributor Author

every 120 seconds, SearcherManager would pick the most recent commit that differs by less than X GB

This is indeed how we anticipate it being used. In NRT style segment replicated setups, if searchers refresh more often than replication frequency, they will eventually catch up to the latest commit. I mentioned the while loop for cases where users want to wait and verify that their searchers are current.

The PR of course supports both patterns, I'll update the description to reflect it as well.

@jpountz

jpountz commented Apr 8, 2025

Copy link
Copy Markdown
Contributor

if searchers refresh more often than replication frequency

OK I think I misunderstood how it would be used. I had assumed that commits would always get replicated immediately, but you are suggesting that replications are infrequent and bring several commits at once to leave time to replica nodes to smoothly absorb the delta.

@vigyasharma vigyasharma changed the title Support incremental refresh in Searcher Managers. Support adaptive refresh in Searcher Managers. Apr 8, 2025
@jpountz

jpountz commented Apr 10, 2025

Copy link
Copy Markdown
Contributor

Sorry I'm still a bit confused: how is this approach better than just committing more frequently, replicating commits as soon as they are created, and refreshing searchers as soon as commits are replicated?

@msokolov

Copy link
Copy Markdown
Contributor

Sorry I'm still a bit confused: how is this approach better than just committing more frequently, replicating commits as soon as they are created, and refreshing searchers as soon as commits are replicated?

One scenario of interest is when replication becomes delayed, for example when working with cross-datacenter replication this is expected. In that case commit points may pile up, even to the extent of completely replacing the entire index. In such a case we'd like to be able to recover without undue impact to searchers.

@vigyasharma

Copy link
Copy Markdown
Contributor Author

just committing more frequently, replicating commits as soon as they are created, and refreshing searchers as soon as commits are replicated?

This is more or less the setup we have today at Amazon Product Search. We have separate indexing and search fleets that use s3 as a sink. Some fleets replicate across aws data centers. I believe this is a common architecture, for e.g. DoorDash seems to have a similar search architecture.

However, as Mike mentioned, these commits go over network hops and are vulnerable to networking lags. Our searchers periodically pull the latest commit from s3 and refresh. If replication is delayed, searchers can skip a few commits to pull the latest one available. This latest commit can have a very high delta to what searchers are currently on.

With adaptive refresh, we are experimenting with making searchers pull the last N commits and refresh on the newest commit that they can safely absorb. At the extreme, if the entire index has changed, it will be no different than refreshing on the latest commit. But for moderate delay windows, we could find "bite sized" hops for searchers to catch up safely.

@vigyasharma

Copy link
Copy Markdown
Contributor Author

Another scenario where adaptive refresh might be useful is with heterogenous search fleets. Searchers with less memory would benefit from stepping through smaller commit deltas, while high memory searchers can jump ahead.

@github-actions

Copy link
Copy Markdown
Contributor

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!

@github-actions github-actions Bot added the Stale label Apr 27, 2025
@mikemccand

Copy link
Copy Markdown
Member

Sorry I'm still a bit confused: how is this approach better than just committing more frequently, replicating commits as soon as they are created, and refreshing searchers as soon as commits are replicated?

In Amazon's usage, and I would expect other high-rate NRT segment replication systems, it's helpful to strongly decouple the production of the new commit points (triggered by time, or by X GB new segments), from the replication of these commits.

During peace time (happy path), what you're suggesting works well -- commit and replication can always match each other 1 for 1. But under duress (unhappy path), commit point production can be faster (usually, hopefully, temporarily) than the replication can keep up, maybe because crazy high rate of document updates, or slow pipe for replication, or bit errors needing lots of retries, etc. (distributed systems seem to have all sorts of fun ways to become problematic!) ... and for those situations, it's really nice to have this possible decoupling easily accessible.

It's also delightful because Lucene makes it quite simple to keep more than one commit point alive at once (it's just a custom IndexDeletionPolicy) ... building this decoupling on top of that is "relatively" easy heh.

@mikemccand mikemccand left a comment

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.

Looks great, thanks @vigyasharma -- I left some small comments. It would be nice to give this capability (carefully pick which commit point to refresh to, when there are multiple choices, for resilience in distributed NRT segment replication case) to SearcherManager.

Comment thread lucene/core/src/java/org/apache/lucene/search/SearcherManager.java Outdated
Comment thread lucene/core/src/java/org/apache/lucene/search/SearcherManager.java Outdated
*
* @lucene.experimental
*/
public interface RefreshCommitSupplier {

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.

If we don't like the added API surface area of a new interface, we could maybe un-final SearcherManager and allow subclass to override the default of picking the latest IndexCommit? I don't really have a preference... the new interface is @lucene.experimental so we are free to evolve it, even within stable release branch.

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.

I went through original discussions in LUCENE-3761 where SearcherManager was added.. The choice to make it final seems to have been from the beginning, I didn't find any discussion explicitly about this.

Overriding this class safely does require a sound understanding of the ReferenceManager base class which has threading/synchronization code. I feel it's safer to leave this class as final, in line with the original intent.

@github-actions github-actions Bot removed the Stale label May 2, 2025
@vigyasharma
vigyasharma force-pushed the bite_commit_refresh branch from d12d63d to 084a53a Compare May 10, 2025 23:39
@vigyasharma

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @mikemccand . I've updated the PR with changes.

I did a rebase on main since it had moved quite a bit. There was no conflict for the files here, but since rebase rewrote commit history, you may not be able to see just the changes since your last review. Apologies for the inconvenience, hopefully the PR is small enough to manage.

@vigyasharma

Copy link
Copy Markdown
Contributor Author

Thanks everyone for the discussion and feedback. Unless there are concerns, I'll go with lazy consensus and merge this on Friday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants