ResourceMonitor only checks the memory health by calculating the memory usage after GC of old gen in v3 #3983

LantaoJin · 2025-08-05T10:35:37Z

Description

In v2, ResourceMonitor gets the memory usage by using total memory - free memory of runtime.
But in v3, Calcite will produce lots of recyclable memory garbage during its planning and implementation process. For Clickbench q30, it produces around 70 mb in optimizing + 30 mb in implementation on local test, which can all be GC, ref #3971 (comment)

This PR changes the behaviour of ResourceMonitor for v3:
ResourceMonitor only checks the memory health by calculating the memory usage after GC of old gen.

Related Issues

Also can improve/resolve the issue of #3750

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…ry usage after GC of old gen Signed-off-by: Lantao Jin <ltjin@amazon.com>

LantaoJin · 2025-08-05T10:39:22Z

core/src/main/java/org/opensearch/sql/executor/QueryService.java

+        if (t instanceof Exception) {
+          listener.onFailure((Exception) t);
+        } else if (t instanceof VirtualMachineError) {
+          // throw and fast fail the VM errors such as OOM (same with v2).
+          throw t;
+        } else {


To align with behaviour of v2, OOM error will fail the whole JVM. We caught OOM error in previous implementation.

It think it's OK to keep align with v2. But will it be better to trigger GC and then check memory usage again before making the service crash? Anyway, the primary purpose of this PR should not be blocked by this.

But will it be better to trigger GC and then check memory usage again before making the service crash?

Maybe more discussion is required and track in a follow-up issue.

LantaoJin · 2025-08-05T10:43:00Z

.../main/java/org/opensearch/sql/opensearch/storage/system/OpenSearchSystemIndexEnumerator.java

+    if (!this.monitor.isHealthy()) {
+      throw new NonFallbackCalciteException("insufficient resources to run the query, quit.");
+    }


Healthy checking is missed in access OpenSearchSystemIndex.

I think we discussed this previously. Can we do this in non-intrusive way? Maybe AOP/wrap on operator level?

I think we discussed this previously. Can we do this in non-intrusive way? Maybe AOP/wrap on operator level?

Yes, technically we could wrap it on operator level. But it will introduce

A new logical node LogicalMonitor

A ConverterRule to convert a logical node to physical node

A new physical node such as EnumerableMonitor similar to ResourceMonitorPlan in v2.

Optimization Skipping codegen and compile for Scan only plan #3853 has to find another way to workaround because the root.rel (physical plan) won't be a Scannable any more.

Not quite sure it's still worth to do the AOP way now.

Okay, probably in future we need this for other operators that possibly generate more records, such as cross/lateral join, right?

opensearch/src/main/java/org/opensearch/sql/opensearch/monitor/GCedMemoryUsage.java

opensearch/src/main/java/org/opensearch/sql/opensearch/monitor/OpenSearchMemoryHealthy.java

dai-chen

I just want to understand the idea more deeply: the assumption is that most objects in Young Gen—such as Calcite’s internal structures—are short-lived, meaning they die after GC and do not survive into the Old Gen, right? As a result, the memory usage calculated from this approach reflects a lower bound of the actual memory pressure?

opensearch/src/main/java/org/opensearch/sql/opensearch/monitor/MemoryUsage.java

opensearch/src/main/java/org/opensearch/sql/opensearch/monitor/GCedMemoryUsage.java

dai-chen · 2025-08-05T17:39:27Z

.../main/java/org/opensearch/sql/opensearch/storage/system/OpenSearchSystemIndexEnumerator.java

+    if (!this.monitor.isHealthy()) {
+      throw new NonFallbackCalciteException("insufficient resources to run the query, quit.");
+    }


I think we discussed this previously. Can we do this in non-intrusive way? Maybe AOP/wrap on operator level?

LantaoJin · 2025-08-06T00:29:11Z

I just want to understand the idea more deeply: the assumption is that most objects in Young Gen—such as Calcite’s internal structures—are short-lived, meaning they die after GC and do not survive into the Old Gen, right? As a result, the memory usage calculated from this approach reflects a lower bound of the actual memory pressure?

Not actually, the objects can survive into the Old Gen but can be easily GCed by concurrent GC (not YGC).

Signed-off-by: Lantao Jin <ltjin@amazon.com>

LantaoJin · 2025-08-08T08:15:40Z

The IT could be still flaky in GitHub CI sometimes. But I cannot reproduce the resource limit issue in local with current implementation. But without this patch, resource limit issue fails easily in CalcitePPLClickBenchIT.test.

…stener

Signed-off-by: Lantao Jin <ltjin@amazon.com>

…stener

Signed-off-by: Lantao Jin <ltjin@amazon.com>

LantaoJin · 2025-08-15T08:25:49Z

The IT could be still flaky in GitHub CI sometimes. But I cannot reproduce the resource limit issue in local with current implementation. But without this patch, resource limit issue fails easily in CalcitePPLClickBenchIT.test.

Not see it again in latest two runs of CI. @dai-chen @penghuo can you check this PR again?

penghuo · 2025-08-20T15:52:35Z

opensearch/src/main/java/org/opensearch/sql/opensearch/monitor/OpenSearchMemoryHealthy.java

+      return isCalciteEnabled(settings)
+          ? GCedMemoryUsage.getInstance()
+          : RuntimeMemoryUsage.getInstance();
+    } catch (MemoryUsageException e) {


fallback to RuntimeMemoryUsage will not trigger; A runtime exception thrown during static field initialization is wrapped in ExceptionInInitializerError.

Signed-off-by: Lantao Jin <ltjin@amazon.com>

…stener

Signed-off-by: Lantao Jin <ltjin@amazon.com>

opensearch-trigger-bot · 2025-08-22T03:24:29Z

The backport to 2.19-dev failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.19-dev 2.19-dev
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.19-dev
# Create a new branch
git switch --create backport/backport-3983-to-2.19-dev
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0398f6852bd3aed3c79b88c2885934cf18e6f71b
# Push it to GitHub
git push --set-upstream origin backport/backport-3983-to-2.19-dev
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.19-dev

Then, create a pull request where the base branch is 2.19-dev and the compare/head branch is backport/backport-3983-to-2.19-dev.

…ry usage after GC of old gen in v3 (opensearch-project#3983) * ResourceMonitor only checks the memory health by calculating the memory usage after GC of old gen Signed-off-by: Lantao Jin <ltjin@amazon.com> * address comments Signed-off-by: Lantao Jin <ltjin@amazon.com> * address comments Signed-off-by: Lantao Jin <ltjin@amazon.com> * Add memory usage fallback Signed-off-by: Lantao Jin <ltjin@amazon.com> * Fix IT Signed-off-by: Lantao Jin <ltjin@amazon.com> * Generate script code lazily to avoid unnecessary usage in Calcite Signed-off-by: Lantao Jin <ltjin@amazon.com> * typo Signed-off-by: Lantao Jin <ltjin@amazon.com> * Address the comments of 3929 followup Signed-off-by: Lantao Jin <ltjin@amazon.com> * Catch throwable instead of MemoryUsageException Signed-off-by: Lantao Jin <ltjin@amazon.com> * Fix unit test when the code in script query expression generated lazily Signed-off-by: Lantao Jin <ltjin@amazon.com> --------- Signed-off-by: Lantao Jin <ltjin@amazon.com> (cherry picked from commit 0398f68)

…calculating the memory usage after GC of old gen in v3 (#3983) (#4105) * ResourceMonitor only checks the memory health by calculating the memory usage after GC of old gen in v3 (#3983) * ResourceMonitor only checks the memory health by calculating the memory usage after GC of old gen Signed-off-by: Lantao Jin <ltjin@amazon.com> * address comments Signed-off-by: Lantao Jin <ltjin@amazon.com> * address comments Signed-off-by: Lantao Jin <ltjin@amazon.com> * Add memory usage fallback Signed-off-by: Lantao Jin <ltjin@amazon.com> * Fix IT Signed-off-by: Lantao Jin <ltjin@amazon.com> * Generate script code lazily to avoid unnecessary usage in Calcite Signed-off-by: Lantao Jin <ltjin@amazon.com> * typo Signed-off-by: Lantao Jin <ltjin@amazon.com> * Address the comments of 3929 followup Signed-off-by: Lantao Jin <ltjin@amazon.com> * Catch throwable instead of MemoryUsageException Signed-off-by: Lantao Jin <ltjin@amazon.com> * Fix unit test when the code in script query expression generated lazily Signed-off-by: Lantao Jin <ltjin@amazon.com> --------- Signed-off-by: Lantao Jin <ltjin@amazon.com> (cherry picked from commit 0398f68) * Fix IT: disable GCedMemoryUsage if no concurrent GC MXBean Signed-off-by: Lantao Jin <ltjin@amazon.com> * Fix IT Signed-off-by: Lantao Jin <ltjin@amazon.com> * Ignore q30 when GCedMemoryUsage not initialized Signed-off-by: Lantao Jin <ltjin@amazon.com> --------- Signed-off-by: Lantao Jin <ltjin@amazon.com>

ResourceMonitor only checks the memory health by calculating the memo…

28e35d8

…ry usage after GC of old gen Signed-off-by: Lantao Jin <ltjin@amazon.com>

LantaoJin added enhancement New feature or request performance Make it fast! calcite calcite migration releated labels Aug 5, 2025

LantaoJin commented Aug 5, 2025

View reviewed changes

LantaoJin marked this pull request as ready for review August 5, 2025 12:05

LantaoJin requested review from GumpacG, MaxKsyunz, Swiddis, YANG-DB, Yury-Fridlyand, acarbonetto, anirudha, dai-chen, derek-ho, forestmvey, joshuali925, kavithacm, mengweieric, noCharger, penghuo, ps48, qianheng-aws, seankao-az and ykmr1224 as code owners August 5, 2025 12:05

LantaoJin mentioned this pull request Aug 5, 2025

SUM aggregation enhancement on operations with literal #3971

Merged

7 tasks

penghuo reviewed Aug 5, 2025

View reviewed changes

opensearch/src/main/java/org/opensearch/sql/opensearch/monitor/GCedMemoryUsage.java Outdated Show resolved Hide resolved

opensearch/src/main/java/org/opensearch/sql/opensearch/monitor/OpenSearchMemoryHealthy.java Outdated Show resolved Hide resolved

dai-chen reviewed Aug 5, 2025

View reviewed changes

address comments

24e330e

Signed-off-by: Lantao Jin <ltjin@amazon.com>

Fix IT

46a147d

Signed-off-by: Lantao Jin <ltjin@amazon.com>

LantaoJin added 3 commits August 11, 2025 10:53

Merge remote-tracking branch 'upstream/main' into issues/3750_2_gc_li…

9621cdd

…stener

Generate script code lazily to avoid unnecessary usage in Calcite

87eaba3

Signed-off-by: Lantao Jin <ltjin@amazon.com>

typo

66b75c3

Signed-off-by: Lantao Jin <ltjin@amazon.com>

qianheng-aws previously approved these changes Aug 11, 2025

View reviewed changes

LantaoJin added 2 commits August 13, 2025 17:56

Merge remote-tracking branch 'upstream/main' into issues/3750_2_gc_li…

d3f9c8f

…stener

Address the comments of 3929 followup

cc8941c

Signed-off-by: Lantao Jin <ltjin@amazon.com>

LantaoJin dismissed qianheng-aws’s stale review via cc8941c August 13, 2025 10:04

qianheng-aws previously approved these changes Aug 14, 2025

View reviewed changes

LantaoJin added the backport 2.19-dev label Aug 15, 2025

penghuo reviewed Aug 20, 2025

View reviewed changes

Catch throwable instead of MemoryUsageException

dbf362a

Signed-off-by: Lantao Jin <ltjin@amazon.com>

LantaoJin dismissed qianheng-aws’s stale review via dbf362a August 21, 2025 03:12

LantaoJin requested review from vamsimanohar and yuancu as code owners August 21, 2025 03:12

LantaoJin added 2 commits August 21, 2025 11:13

Merge remote-tracking branch 'upstream/main' into issues/3750_2_gc_li…

36dbfe1

…stener

Fix unit test when the code in script query expression generated lazily

60a596b

Signed-off-by: Lantao Jin <ltjin@amazon.com>

penghuo approved these changes Aug 22, 2025

View reviewed changes

qianheng-aws approved these changes Aug 22, 2025

View reviewed changes

qianheng-aws merged commit 0398f68 into opensearch-project:main Aug 22, 2025
23 checks passed

opensearch-trigger-bot bot added the backport-failed label Aug 22, 2025

LantaoJin added the backport-manually Filed a PR to backport manually. label Aug 22, 2025

ResourceMonitor only checks the memory health by calculating the memory usage after GC of old gen in v3 #3983

ResourceMonitor only checks the memory health by calculating the memory usage after GC of old gen in v3 #3983

Uh oh!

Conversation

LantaoJin commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

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!

dai-chen Aug 7, 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!

Uh oh!

Uh oh!

dai-chen left a comment • 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LantaoJin commented Aug 6, 2025

Uh oh!

LantaoJin commented Aug 8, 2025

Uh oh!

LantaoJin commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

opensearch-trigger-bot bot commented Aug 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LantaoJin commented Aug 5, 2025 •

edited

Loading

dai-chen Aug 7, 2025 •

edited

Loading

dai-chen left a comment •

edited

Loading

LantaoJin commented Aug 15, 2025 •

edited

Loading