Skip to content

[native] Make periodic cache flush incremental#23626

Merged
zacw7 merged 1 commit intoprestodb:masterfrom
zacw7:periodic-flush-incremental
Sep 12, 2024
Merged

[native] Make periodic cache flush incremental#23626
zacw7 merged 1 commit intoprestodb:masterfrom
zacw7:periodic-flush-incremental

Conversation

@zacw7
Copy link
Member

@zacw7 zacw7 commented Sep 11, 2024

When flushing the in-memory cache to SSD, perform an incremental flush instead of saving everything forcefully. Additionally, explicit checkpointing is unnecessary, as it will automatically trigger when the written bytes exceed the checkpoint threshold and during shutdown.

== RELEASE NOTES ==

General Changes
* Add incremental periodic cache persistence for Presto C++ worker. :pr:`23626`
* Replace configuration property `async-cache-full-persistence-interval` with `async-cache-persistence-interval`. :pr:`23626`

@steveburnett
Copy link
Contributor

Suggest the following revision of the release note entry to follow the Order of Changes in the Release Notes Guidelines. Perhaps something like:

== RELEASE NOTES ==

General Changes
* Add incremental periodic cache persistence for Presto C++ worker. :pr:`23626`
* Replace configuration property `async-cache-full-persistence-interval` with `cache-persistence-interval`. :pr:`23626`

Also suggest adding documentation for the new cache-persistence-interval configuration property, probably to Presto C++ Properties Reference.

@zacw7 zacw7 force-pushed the periodic-flush-incremental branch 3 times, most recently from c1df974 to a0586c3 Compare September 11, 2024 18:10
steveburnett
steveburnett previously approved these changes Sep 11, 2024
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

Pull branch, local docs build, looks good. Thanks!

@zacw7 zacw7 marked this pull request as ready for review September 11, 2024 18:39
@zacw7 zacw7 requested review from a team and elharo as code owners September 11, 2024 18:39
@zacw7 zacw7 requested a review from presto-oss September 11, 2024 18:39
@zacw7 zacw7 force-pushed the periodic-flush-incremental branch from a0586c3 to 821f631 Compare September 11, 2024 18:41
@zacw7 zacw7 force-pushed the periodic-flush-incremental branch from 821f631 to 3e4ede0 Compare September 11, 2024 18:50
steveburnett
steveburnett previously approved these changes Sep 11, 2024
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

Pull updated branch, new local docs build, looks good. Thanks!

xiaoxmeng
xiaoxmeng previously approved these changes Sep 12, 2024
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@zacw7 LGTM. Thanks!

@zacw7 zacw7 dismissed stale reviews from xiaoxmeng and steveburnett via a5a960f September 12, 2024 18:21
@zacw7 zacw7 force-pushed the periodic-flush-incremental branch from 3e4ede0 to a5a960f Compare September 12, 2024 18:21
@zacw7 zacw7 requested a review from xiaoxmeng September 12, 2024 18:21
When flushing the in-memory cache to SSD, perform an incremental flush
instead of saving everything forcefully. Additionally, explicit
checkpointing is unnecessary, as it will automatically trigger when
the written bytes exceed the checkpoint threshold and during shutdown.
@zacw7 zacw7 force-pushed the periodic-flush-incremental branch from a5a960f to 80e6035 Compare September 12, 2024 18:32
@zacw7 zacw7 merged commit 4bedf5e into prestodb:master Sep 12, 2024
@zacw7 zacw7 deleted the periodic-flush-incremental branch September 12, 2024 20:59
@jaystarshot jaystarshot mentioned this pull request Nov 1, 2024
25 tasks
@tdcmeehan tdcmeehan added the from:Meta PR from Meta label Dec 13, 2024
@prestodb-ci
Copy link
Contributor

Saved that user @zacw7 is from Meta

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

Labels

from:Meta PR from Meta

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Comments