Skip to content

Commit 28e35d8

Browse files
committed
ResourceMonitor only checks the memory health by calculating the memory usage after GC of old gen
Signed-off-by: Lantao Jin <ltjin@amazon.com>
1 parent 7ccdcd1 commit 28e35d8

15 files changed

Lines changed: 237 additions & 31 deletions

File tree

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -112,11 +112,14 @@ public void executeWithCalcite(
112112
log.warn("Fallback to V2 query engine since got exception", t);
113113
executeWithLegacy(plan, queryType, listener, Optional.of(t));
114114
} else {
115-
if (t instanceof Error) {
115+
if (t instanceof Exception) {
116+
listener.onFailure((Exception) t);
117+
} else if (t instanceof VirtualMachineError) {
118+
// throw and fast fail the VM errors such as OOM (same with v2).
119+
throw t;
120+
} else {
116121
// Calcite may throw AssertError during query execution.
117122
listener.onFailure(new CalciteUnsupportedException(t.getMessage(), t));
118-
} else {
119-
listener.onFailure((Exception) t);
120123
}
121124
}
122125
}

integ-test/src/test/java/org/opensearch/sql/calcite/clickbench/PPLClickBenchIT.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ public static void reset() throws IOException {
5353
* down, which will cause ResourceMonitor restriction.
5454
*/
5555
protected Set<Integer> ignored() {
56-
return Set.of(29, 30);
56+
return Set.of(29);
5757
}
5858

5959
@Test
Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package org.opensearch.sql.opensearch.monitor;
7+
8+
import com.sun.management.GarbageCollectionNotificationInfo;
9+
import java.lang.management.GarbageCollectorMXBean;
10+
import java.lang.management.ManagementFactory;
11+
import java.util.Arrays;
12+
import java.util.List;
13+
import java.util.Map;
14+
import java.util.Set;
15+
import java.util.concurrent.atomic.AtomicLong;
16+
import javax.management.Notification;
17+
import javax.management.NotificationEmitter;
18+
import javax.management.NotificationListener;
19+
import javax.management.openmbean.CompositeData;
20+
import org.apache.logging.log4j.LogManager;
21+
import org.apache.logging.log4j.Logger;
22+
23+
/** Get memory usage from GC notification listener, which is used in Calcite engine. */
24+
public class GCedMemoryUsage implements MemoryUsage {
25+
private static final Logger LOG = LogManager.getLogger();
26+
27+
private GCedMemoryUsage() {
28+
registerGCListener();
29+
}
30+
31+
// Lazy initialize the instance to avoid register GCListener in v2.
32+
private static class Holder {
33+
static final MemoryUsage INSTANCE = new GCedMemoryUsage();
34+
}
35+
36+
/**
37+
* Get the singleton instance of GCedMemoryUsage.
38+
*
39+
* @return GCedMemoryUsage instance
40+
*/
41+
public static MemoryUsage getInstance() {
42+
return Holder.INSTANCE;
43+
}
44+
45+
private final AtomicLong usage = new AtomicLong(-1);
46+
47+
@Override
48+
public long usage() {
49+
return usage.get();
50+
}
51+
52+
@Override
53+
public void setUsage(long value) {
54+
usage.set(value);
55+
}
56+
57+
private void registerGCListener() {
58+
List<GarbageCollectorMXBean> gcBeans = ManagementFactory.getGarbageCollectorMXBeans();
59+
for (GarbageCollectorMXBean gcBean : gcBeans) {
60+
if (gcBean instanceof NotificationEmitter) {
61+
NotificationEmitter emitter = (NotificationEmitter) gcBean;
62+
emitter.addNotificationListener(new FullGCListener(), null, null);
63+
}
64+
}
65+
}
66+
67+
private static class FullGCListener implements NotificationListener {
68+
@Override
69+
public void handleNotification(Notification notification, Object handback) {
70+
if (notification
71+
.getType()
72+
.equals(GarbageCollectionNotificationInfo.GARBAGE_COLLECTION_NOTIFICATION)) {
73+
CompositeData cd = (CompositeData) notification.getUserData();
74+
GarbageCollectionNotificationInfo info = GarbageCollectionNotificationInfo.from(cd);
75+
76+
String gcAction = info.getGcAction();
77+
LOG.info("gcAction: {}", gcAction);
78+
if (gcAction.contains("major")
79+
|| gcAction.contains("concurrent")
80+
|| gcAction.contains("old")
81+
|| gcAction.contains("full")) {
82+
Map<String, java.lang.management.MemoryUsage> memoryUsageAfterGc =
83+
info.getGcInfo().getMemoryUsageAfterGc();
84+
String possibleOldGenKey = findOldGenKey(memoryUsageAfterGc.keySet());
85+
if (possibleOldGenKey != null) {
86+
java.lang.management.MemoryUsage oldGenUsage =
87+
memoryUsageAfterGc.get(possibleOldGenKey);
88+
getInstance().setUsage(oldGenUsage.getUsed());
89+
LOG.info("Full GC detected, memory usage after GC is {} bytes.", getInstance().usage());
90+
}
91+
}
92+
}
93+
}
94+
95+
private String findOldGenKey(Set<String> memoryPoolNames) {
96+
LOG.info("Memory pool names: {}", memoryPoolNames);
97+
List<String> possibleOldGenKeys =
98+
Arrays.asList("G1 Old Gen", "PS Old Gen", "CMS Old Gen", "Tenured Gen", "Old Gen");
99+
for (String key : possibleOldGenKeys) {
100+
if (memoryPoolNames.contains(key)) {
101+
return key;
102+
}
103+
}
104+
return null;
105+
}
106+
}
107+
}
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package org.opensearch.sql.opensearch.monitor;
7+
8+
public interface MemoryUsage {
9+
long usage();
10+
11+
void setUsage(long usage);
12+
}

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

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,16 +9,20 @@
99
import java.util.concurrent.ThreadLocalRandom;
1010
import lombok.NoArgsConstructor;
1111
import lombok.extern.log4j.Log4j2;
12+
import org.opensearch.sql.common.setting.Settings;
1213

1314
/** OpenSearch Memory Monitor. */
1415
@Log4j2
1516
public class OpenSearchMemoryHealthy {
1617
private final RandomFail randomFail;
1718
private final MemoryUsage memoryUsage;
1819

19-
public OpenSearchMemoryHealthy() {
20+
public OpenSearchMemoryHealthy(Settings settings) {
2021
randomFail = new RandomFail();
21-
memoryUsage = new MemoryUsage();
22+
memoryUsage =
23+
isCalciteEnabled(settings)
24+
? GCedMemoryUsage.getInstance()
25+
: RuntimeMemoryUsage.getInstance();
2226
}
2327

2428
@VisibleForTesting
@@ -27,6 +31,14 @@ public OpenSearchMemoryHealthy(RandomFail randomFail, MemoryUsage memoryUsage) {
2731
this.memoryUsage = memoryUsage;
2832
}
2933

34+
private boolean isCalciteEnabled(Settings settings) {
35+
if (settings != null) {
36+
return settings.getSettingValue(Settings.Key.CALCITE_ENGINE_ENABLED);
37+
} else {
38+
return false;
39+
}
40+
}
41+
3042
/** Is Memory Healthy. Calculate based on the current heap memory usage. */
3143
public boolean isMemoryHealthy(long limitBytes) {
3244
final long memoryUsage = this.memoryUsage.usage();
@@ -50,14 +62,6 @@ public boolean shouldFail() {
5062
}
5163
}
5264

53-
static class MemoryUsage {
54-
public long usage() {
55-
final long freeMemory = Runtime.getRuntime().freeMemory();
56-
final long totalMemory = Runtime.getRuntime().totalMemory();
57-
return totalMemory - freeMemory;
58-
}
59-
}
60-
6165
@NoArgsConstructor
6266
public static class MemoryUsageExceedFastFailureException extends RuntimeException {}
6367

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package org.opensearch.sql.opensearch.monitor;
7+
8+
/** Get memory usage from runtime, which is used in v2. */
9+
public class RuntimeMemoryUsage implements MemoryUsage {
10+
private RuntimeMemoryUsage() {}
11+
12+
private static class Holder {
13+
static final MemoryUsage INSTANCE = new RuntimeMemoryUsage();
14+
}
15+
16+
public static MemoryUsage getInstance() {
17+
return Holder.INSTANCE;
18+
}
19+
20+
@Override
21+
public long usage() {
22+
final long freeMemory = Runtime.getRuntime().freeMemory();
23+
final long totalMemory = Runtime.getRuntime().totalMemory();
24+
return totalMemory - freeMemory;
25+
}
26+
27+
@Override
28+
public void setUsage(long usage) {
29+
throw new UnsupportedOperationException("Cannot set usage in RuntimeMemoryUsage");
30+
}
31+
}

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,7 @@ public OpenSearchRequestBuilder createRequestBuilder() {
260260
}
261261

262262
public OpenSearchResourceMonitor createOpenSearchResourceMonitor() {
263-
return new OpenSearchResourceMonitor(getSettings(), new OpenSearchMemoryHealthy());
263+
return new OpenSearchResourceMonitor(getSettings(), new OpenSearchMemoryHealthy(settings));
264264
}
265265

266266
public OpenSearchRequest buildRequest(OpenSearchRequestBuilder requestBuilder) {

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchStorageEngine.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ public class OpenSearchStorageEngine implements StorageEngine {
2828
@Override
2929
public Table getTable(DataSourceSchemaName dataSourceSchemaName, String name) {
3030
if (isSystemIndex(name)) {
31-
return new OpenSearchSystemIndex(client, name);
31+
return new OpenSearchSystemIndex(client, settings, name);
3232
} else {
3333
return new OpenSearchIndex(client, settings, name);
3434
}

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexEnumerator.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,13 +120,19 @@ public boolean moveNext() {
120120

121121
@Override
122122
public void reset() {
123-
iterator = Collections.emptyIterator();
123+
OpenSearchResponse response = client.search(request);
124+
if (!response.isEmpty()) {
125+
iterator = response.iterator();
126+
} else {
127+
iterator = Collections.emptyIterator();
128+
}
124129
queryCount = 0;
125130
}
126131

127132
@Override
128133
public void close() {
129-
reset();
134+
iterator = Collections.emptyIterator();
135+
queryCount = 0;
130136
client.cleanup(request);
131137
}
132138
}

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/system/CalciteEnumerableSystemIndexScan.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,9 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
6969
@Override
7070
public Enumerator<Object> enumerator() {
7171
return new OpenSearchSystemIndexEnumerator(
72-
getFieldPath(), sysIndex.getSystemIndexBundle().getRight());
72+
getFieldPath(),
73+
sysIndex.getSystemIndexBundle().getRight(),
74+
sysIndex.createOpenSearchResourceMonitor());
7375
}
7476
};
7577
}

0 commit comments

Comments
 (0)