[native] Expose an API to clean up async data cache on node by agrawalreetika · Pull Request #24530 · prestodb/presto

agrawalreetika · 2025-02-11T10:07:55Z

Description

Adding e2e tests for existing cache APIs

Motivation and Context

Adding e2e tests for existing cache APIs

Impact

Adding e2e tests for existing cache APIs -

curl -X GET "http://localhost:7777/v1/operation/server/clearCache?type=memory"
   
curl -X GET "http://localhost:7777/v1/operation/server/clearCache?type=ssd"

curl -X GET "http://localhost:7777/v1/operation/server/writeSsd"

Test Plan

Test Added

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

== NO RELEASE NOTE ==

yingsu00 · 2025-02-11T23:20:04Z

+    if (nodeState() == NodeState::kActive) {
+      auto* asyncDataCache = velox::cache::AsyncDataCache::getInstance();
+      if (asyncDataCache != nullptr) {
+        asyncDataCache->clear();


There is a VELOX_CHECK in clear() that might fail. Shall we catch it, log the failure and rethrow?

void AsyncDataCache::clear() { for (auto& shard : shards_) { memory::Allocation unused; shard->evict(std::numeric_limits<uint64_t>::max(), true, 0, unused); VELOX_CHECK(unused.empty()); } }

yingsu00 · 2025-02-11T23:22:44Z

+        LOG(INFO) << "async data cache clean up is successful";
+      }
+      else {
+        LOG(ERROR) << "Issue in async data cache clean up";


Let's be more specific, and just say "Cannot acquire the AsyncDataCache instance"

yingsu00 · 2025-02-11T23:35:43Z

+                nativeQueryRunnerParameters.workerCount,
+                cacheMaxSize,
+                DEFAULT_STORAGE_FORMAT,
+                true,


Why are we setting addStorageFormatToPath to true here?

I just added this to add storage name in the data folder path.

yingsu00 · 2025-02-11T23:50:34Z

+        assertEquals(0, finalMetrics.entries, "Cache should be empty after cleanup.");
+    }
+
+    private Metrics collectCacheMetrics(Set<InternalNode> workerNodes, DistributedQueryRunner distributedQueryRunner, String endpoint)


distributedQueryRunner is not used

yingsu00 · 2025-02-12T00:01:53Z

+        int hits = 0;
+        int entries = 0;
+        for (InternalNode worker : workerNodes) {
+            Map<String, Long> metrics = fetchMetrics(worker.getInternalUri().toString(), endpoint, "GET");


Seems this function only supports scalar integer type metrics. How do you plan to collect histogram type metics in the future? Will you add new logic to this method or create another method? If it's the latter, maybe it'll be clearer to rename this one to fetchScalarLongMetrics?

yingsu00 · 2025-02-12T00:05:25Z

        defaultQueryRunner.close();

-        return createNativeQueryRunner(dataDirectory.get().toString(), prestoServerPath.get(), workerCount, cacheMaxSize, true, Optional.empty(), storageFormat, addStorageFormatToPath, false, isCoordinatorSidecarEnabled, false);
+        return createNativeQueryRunner(dataDirectory.get().toString(), prestoServerPath.get(), workerCount, cacheMaxSize, true, Optional.empty(), storageFormat, addStorageFormatToPath, false, isCoordinatorSidecarEnabled, false, enableRuntimeMetricsCollection);


This line is too long

jaystarshot · 2025-02-12T18:21:25Z

What is the use case to clear the cache? we already have a pushback mechanism now?

pramodsatya

Thanks @agrawalreetika.

pramodsatya · 2025-02-13T16:13:42Z


  void reportNodeStatus(proxygen::ResponseHandler* downstream);

+  void cleanAsynDataCache(


nit: cleanAsyncDataCache

pramodsatya · 2025-02-13T16:22:39Z

+                false);
+    }
+
+    public static QueryRunner createQueryRunner(boolean enableRuntimeMetricsCollection)


would be better to add this parameter to the existing createQueryRunner function:

public static QueryRunner createQueryRunner(boolean addStorageFormatToPath, boolean isCoordinatorSidecarEnabled)

pramodsatya · 2025-02-13T16:28:40Z

+        int entries = 0;
+        for (InternalNode worker : workerNodes) {
+            Map<String, Long> metrics = fetchScalarLongMetrics(worker.getInternalUri().toString(), endpoint, "GET");
+            hits += metrics.getOrDefault("velox_memory_cache_num_hits", 0L);


Where are the configs velox_memory_cache_num_hits and velox_memory_cache_num_entries defined?

These counters should be defined on Velox side

pramodsatya · 2025-02-13T16:33:30Z

          proxygen::ResponseHandler* downstream) {
        server->reportMemoryInfo(downstream);
      });
+  httpServer_->registerPut(


Why is the v1/memory endpoint overloaded? Might be better to add a new endpoint v1/memory/clear

agrawalreetika · 2025-02-16T08:27:23Z

What is the use case to clear the cache? we already have a pushback mechanism now?

Sorry, I missed this.
Our rationale for enabling this was to conduct detailed query execution analysis when there are no plan changes, particularly for in-memory query performance evaluations. To measure each query's effectiveness accurately, we needed a way to clear all caches before executing the next query. Since there is currently no way to run procedures on workers, we are exposing an API to facilitate cache cleanup.

Could you please provide the details around if this is something could be done with pushback mechanism?

yingsu00 · 2025-02-18T00:59:23Z

What is the use case to clear the cache? we already have a pushback mechanism now?

Sorry, I missed this. Our rationale for enabling this was to conduct detailed query execution analysis when there are no plan changes, particularly for in-memory query performance evaluations. To measure each query's effectiveness accurately, we needed a way to clear all caches before executing the next query. Since there is currently no way to run procedures on workers, we are exposing an API to facilitate cache cleanup.

Could you please provide the details around if this is something could be done with pushback mechanism?

@agrawalreetika Can you please add this context in the PR message? Also,
particularly for in-memory query performance evaluations. -> `particularly for single query cold/warm analysis, where "cold" stands for the state where the caches are not populated, while "warm" means the caches are warm and query is largely ran in memory"

yingsu00

@agrawalreetika THere is test failure:

Error: Failures:
Error: TestPrestoNativeAsyncDataCacheCleanupAPI.testAsyncDataCacheCleanup:67->collectCacheMetrics:100 ? FileNotFound http://127.0.0.1:37499/v1/info/metrics

xiaoxmeng

@agrawalreetika we already support to cleanup cache through We already support ssd/memory cache cleanup through v1/operation/server/clearCache?type=memory cc @tanjialiang @zacw7 Thanks!

yingsu00 · 2025-02-21T09:01:13Z

@xiaoxmeng Thanks for the heads up! However we didn't find any tests or documentation for it. Reetika will rearrange this PR to add them if you don't mind.

xiaoxmeng · 2025-02-21T14:10:48Z

@xiaoxmeng Thanks for the heads up! However we didn't find any tests or documentation for it. Reetika will rearrange this PR to add them if you don't mind.

Here is the implementation and it is only used in operation: presto-native-execution/presto_cpp/main/ServerOperation.cpp. The server operation can support different kinds of operations in addition to cache cleanup. We could help adding tests for this instead of duplicating the implementation. cc @zacw7 @tanjialiang

zacw7 · 2025-02-21T16:41:58Z

I'll be adding the tests for the server operations. Thanks!

steveburnett

Thanks for the doc! A couple of nits only.

agrawalreetika · 2025-03-08T06:28:45Z

@zacw7 @yingsu00 I've addressed your comments, and CI is passing. Please review at your convenience. Thanks!

zacw7

LGTM. Thanks again for adding the tests!

xiaoxmeng

@agrawalreetika thanks for e2e test coverage!

yingsu00 · 2025-03-12T01:41:07Z

                workerCount,
                Optional.of(Paths.get(addStorageFormatToPath ? dataDirectory + "/" + storageFormat : dataDirectory)),
-                getExternalWorkerLauncher("hive", prestoServerPath, cacheMaxSize, remoteFunctionServerUds, failOnNestedLoopJoin, isCoordinatorSidecarEnabled),
+                getExternalWorkerLauncher("hive", prestoServerPath, cacheMaxSize, remoteFunctionServerUds, failOnNestedLoopJoin, isCoordinatorSidecarEnabled, enableRuntimeMetricsCollection, enableSsdCache),


nit: this line is too long

yingsu00 · 2025-03-12T01:41:48Z


-    public static Optional<BiFunction<Integer, URI, Process>> getExternalWorkerLauncher(String catalogName, String prestoServerPath, int cacheMaxSize, Optional<String> remoteFunctionServerUds, Boolean failOnNestedLoopJoin, boolean isCoordinatorSidecarEnabled)
+    public static Optional<BiFunction<Integer, URI, Process>> getExternalWorkerLauncher(String catalogName, String prestoServerPath, int cacheMaxSize, Optional<String> remoteFunctionServerUds, Boolean failOnNestedLoopJoin,
+                                                                                        boolean isCoordinatorSidecarEnabled, boolean enableRuntimeMetricsCollection, boolean enableSsdCache)


Make each parameter on a single line

yingsu00 · 2025-03-12T01:44:48Z

+        for (int i = 0; i < 4; i++) {
+            queryRunner.execute(session, "SELECT count(*) FROM customer");
+        }
+        TimeUnit.SECONDS.sleep(60); // Sleep to allow cache updates


Sleeping 60 seconds? Is it too long? This means this test would take at least 5 minutes. Can we change it to 10 sec?

yingsu00 · 2025-03-12T21:17:09Z

@agrawalreetika Which test fails? I see

Error:  com.facebook.presto.nativeworker.TestPrestoNativeAsyncDataCacheCleanupAPI.testAsyncDataCacheCleanup  Time elapsed: 12.27 s  <<< FAILURE!
java.lang.AssertionError: Cache should have hits after queries. did not expect [0] but found [0]
	at org.testng.Assert.fail(Assert.java:110)
	at org.testng.Assert.failEquals(Assert.java:1417)
	at org.testng.Assert.assertNotEqualsImpl(Assert.java:156)
	at org.testng.Assert.assertNotEquals(Assert.java:1986)
	at

I wonder if it's because we reduced the wait time too much. But it surprises me if 10s is not enough to see the cache update

agrawalreetika · 2025-03-14T08:23:01Z

@zacw7 @xiaoxmeng I tried reducing the sleep window to 10s and 30s to facilitate cache updates, but it seems the updates are failing. Do you have any insights on this?

agrawalreetika requested a review from a team as a code owner February 11, 2025 10:07

prestodb-ci added the from:IBM PR from IBM label Feb 11, 2025

prestodb-ci requested review from a team, NivinCS and pramodsatya and removed request for a team February 11, 2025 10:07

agrawalreetika self-assigned this Feb 11, 2025

agrawalreetika requested a review from yingsu00 February 11, 2025 10:08

agrawalreetika changed the title ~~Expose an API to clean up async data cache on node~~ [native] Expose an API to clean up async data cache on node Feb 11, 2025

yingsu00 reviewed Feb 12, 2025

View reviewed changes

agrawalreetika force-pushed the native-memory-cleanup branch from f47d63b to eba0f8a Compare February 12, 2025 17:33

pramodsatya reviewed Feb 13, 2025

View reviewed changes

agrawalreetika force-pushed the native-memory-cleanup branch from eba0f8a to 4f2836e Compare February 16, 2025 09:23

agrawalreetika requested a review from a team as a code owner February 16, 2025 09:23

agrawalreetika requested a review from jaystarshot February 16, 2025 09:23

yingsu00 requested a review from xiaoxmeng February 18, 2025 01:05

yingsu00 reviewed Feb 18, 2025

View reviewed changes

agrawalreetika force-pushed the native-memory-cleanup branch 2 times, most recently from 9aa9f06 to 6a44bb0 Compare February 18, 2025 18:08

xiaoxmeng reviewed Feb 19, 2025

View reviewed changes

agrawalreetika force-pushed the native-memory-cleanup branch from 6a44bb0 to d9e7485 Compare February 21, 2025 19:01

agrawalreetika requested review from elharo and steveburnett as code owners February 21, 2025 19:01

steveburnett requested changes Feb 21, 2025

View reviewed changes

Comment thread presto-docs/src/main/sphinx/presto_cpp/features.rst Outdated

agrawalreetika force-pushed the native-memory-cleanup branch 5 times, most recently from f18a099 to 0039295 Compare March 7, 2025 03:47

agrawalreetika requested a review from czentgr as a code owner March 7, 2025 03:47

agrawalreetika force-pushed the native-memory-cleanup branch 6 times, most recently from 0a73768 to 304fbec Compare March 8, 2025 03:48

zacw7 previously approved these changes Mar 8, 2025

View reviewed changes

xiaoxmeng previously approved these changes Mar 11, 2025

View reviewed changes

yingsu00 previously approved these changes Mar 12, 2025

View reviewed changes

yingsu00 reviewed Mar 12, 2025

View reviewed changes

agrawalreetika dismissed stale reviews from yingsu00, xiaoxmeng, and zacw7 via 312b40d March 12, 2025 05:29

agrawalreetika force-pushed the native-memory-cleanup branch from 304fbec to 312b40d Compare March 12, 2025 05:29

Add test for async data cache API on node

265b439

agrawalreetika force-pushed the native-memory-cleanup branch from 312b40d to 265b439 Compare March 14, 2025 04:56

yingsu00 approved these changes Mar 15, 2025

View reviewed changes

zacw7 merged commit a3966ad into prestodb:master Mar 17, 2025

agrawalreetika deleted the native-memory-cleanup branch May 2, 2025 12:06

amitkdutta mentioned this pull request Jun 24, 2025

[native] TestPrestoNativeAsyncDataCacheCleanupAPI.testAsyncDataCacheCleanup failing #25407

Closed

agrawalreetika mentioned this pull request Jan 20, 2026

fix: Enable TestPrestoNativeAsyncDataCacheCleanupAPI #26923

Merged

7 tasks


		void reportNodeStatus(proxygen::ResponseHandler* downstream);

		void cleanAsynDataCache(

Conversation

agrawalreetika commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaystarshot commented Feb 12, 2025

Uh oh!

pramodsatya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agrawalreetika commented Feb 16, 2025

Uh oh!

yingsu00 commented Feb 18, 2025

Uh oh!

yingsu00 left a comment

Choose a reason for hiding this comment

Uh oh!

xiaoxmeng left a comment

Choose a reason for hiding this comment

Uh oh!

yingsu00 commented Feb 21, 2025

Uh oh!

xiaoxmeng commented Feb 21, 2025

Uh oh!

zacw7 commented Feb 21, 2025

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

agrawalreetika commented Mar 8, 2025

Uh oh!

zacw7 left a comment

Choose a reason for hiding this comment

Uh oh!

xiaoxmeng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yingsu00 Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yingsu00 commented Mar 12, 2025

Uh oh!

agrawalreetika commented Mar 14, 2025

Uh oh!

Reviewers

agrawalreetika commented Feb 11, 2025 •

edited

Loading

yingsu00 Mar 12, 2025 •

edited

Loading