Skip to content

[native] Add support for cache periodic full persistence#23192

Merged
zacw7 merged 1 commit intoprestodb:masterfrom
zacw7:cache-full-persistence
Jul 23, 2024
Merged

[native] Add support for cache periodic full persistence#23192
zacw7 merged 1 commit intoprestodb:masterfrom
zacw7:cache-full-persistence

Conversation

@zacw7
Copy link
Member

@zacw7 zacw7 commented Jul 12, 2024

Description

Currently, maxWriteRatio(AsyncDataCache::Options), the maximum ratio of the number of in-memory cache entries being written to the SSD cache over the total number of cache entries, is set to the default value of 0.7, meaning that we only persist 70% of cache entries to the SSD and upon server restart 30% of cache will be lost.

This change adds an optional periodic daemon to flush cache data to ssd. This can help to make sure all the in-memory data is flushed to ssd.

== RELEASE NOTES ==

General Changes
* Add support for persisting full memory cache to SSD periodically on Presto C++ worker. This can be enabled by setting `cache.velox.full-persistence-interval` to a non-zero value. :pr:`23192`

@zacw7 zacw7 requested a review from a team as a code owner July 12, 2024 06:46
@zacw7 zacw7 force-pushed the cache-full-persistence branch 2 times, most recently from 2c32db5 to d7b351f Compare July 16, 2024 20:49
@zacw7 zacw7 force-pushed the cache-full-persistence branch 2 times, most recently from abc9e4c to d6c42f1 Compare July 16, 2024 21:00
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks @zacw7.

Please can you add a release note for this functionality. Would greatly help users.

@zacw7 zacw7 force-pushed the cache-full-persistence branch from d6c42f1 to c3a7675 Compare July 17, 2024 00:28
@zacw7
Copy link
Member Author

zacw7 commented Jul 17, 2024

Thanks @zacw7.

Please can you add a release note for this functionality. Would greatly help users.

release note added. thanks!

@zacw7 zacw7 requested a review from aditi-pandit July 17, 2024 00:33
@steveburnett
Copy link
Contributor

Nit, please add the PR number to the release note entry following the Release Notes Guidelines:

== RELEASE NOTES ==

General Changes
* Add support for persisting full memory cache to SSD periodically on Presto C++ worker. This can be enabled by setting `cache.velox.full-persistence-interval` to a non-zero value. :pr:`23192`

Comment on lines +968 to +976
auto* cache = velox::cache::AsyncDataCache::getInstance();
if (cache == nullptr) {
return;
}

auto* ssdCache = cache->ssdCache();
if (ssdCache == nullptr) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to get ssd cache outside of the periodic func? Also if isCachePeriodicFullPersistenceEnabled() is true shouldn't cache and ssdcache both be non-null? Can we change these conditions to VELOX_CHECKs?

Copy link
Member Author

@zacw7 zacw7 Jul 22, 2024

Choose a reason for hiding this comment

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

If cache and ssdcache both are non-null, shall we just ignore it instead of failing the server hard? Similarly, we still allow the server operation api to be called even when there is no cache:

std::string PrestoServerOperations::serverOperationWriteSsd(

Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant is something like this

if (isCachePeriodicFullPersistenceEnabled()) {
  VELOX_CHECK_NOT_NULL(cache);
  VELOX_CHECK_NOT_NULL(ssdCache);
}

Or are there any scenarios that they can actually be NULL when the if condition is true?

@zacw7 zacw7 force-pushed the cache-full-persistence branch from c3a7675 to 2342987 Compare July 22, 2024 20:26
@zacw7 zacw7 requested a review from tanjialiang July 22, 2024 20:27
Copy link
Contributor

@tanjialiang tanjialiang left a comment

Choose a reason for hiding this comment

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

Thanks @zacw7 for the revision. Some minors

}

void PrestoServer::addServerPeriodicTasks() {
const auto* systemConfig = SystemConfig::instance();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to capture this pointer by copy in the below lambda. Or you can simply put the SystemConfig::instance() call inside lambda. Yeah let's just move it inside lambda. There is not a strong need of having it outside.

Comment on lines +968 to +976
auto* cache = velox::cache::AsyncDataCache::getInstance();
if (cache == nullptr) {
return;
}

auto* ssdCache = cache->ssdCache();
if (ssdCache == nullptr) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant is something like this

if (isCachePeriodicFullPersistenceEnabled()) {
  VELOX_CHECK_NOT_NULL(cache);
  VELOX_CHECK_NOT_NULL(ssdCache);
}

Or are there any scenarios that they can actually be NULL when the if condition is true?

@zacw7 zacw7 force-pushed the cache-full-persistence branch from 2342987 to 23549b8 Compare July 22, 2024 21:42
@zacw7 zacw7 requested a review from tanjialiang July 22, 2024 21:43
tanjialiang
tanjialiang previously approved these changes Jul 22, 2024
Currently, maxWriteRatio(AsyncDataCache::Options), the maximum ratio of the number of in-memory cache entries being written to the SSD cache over the total number of cache entries, is set to the default value of 0.7, meaning that we only persist 70% of cache entries to the SSD and upon server restart 30% of cache will be lost.

This change adds an optional periodic daemon to flush cache data to ssd. This can help to make sure all the in-memory data is flushed to ssd.
@zacw7 zacw7 force-pushed the cache-full-persistence branch from f1b2046 to 610253b Compare July 23, 2024 01:55
@zacw7 zacw7 requested a review from tanjialiang July 23, 2024 01:59
@zacw7 zacw7 merged commit 51d3f69 into prestodb:master Jul 23, 2024
@zacw7 zacw7 deleted the cache-full-persistence branch July 23, 2024 18:04
return;
}
LOG(INFO) << "Persisting full cache to SSD...";
cache->saveToSsd(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

saveToSsd(false)

@tdcmeehan tdcmeehan mentioned this pull request Aug 23, 2024
34 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants