You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2025/09/28 11:26:47 UTC
[PR] Disable OOM protection when IHOP JVM params not active (server/broker) [pinot]
xiangfu0 opened a new pull request, #16912:
URL: https://github.com/apache/pinot/pull/16912
This change adds a JVM capability guard for OOM protection on both Server and Broker.
- During startup, after enabling thread memory allocation tracking, we verify whether the JVM actually has thread-allocated-bytes measurement enabled via `ThreadResourceUsageProvider.isThreadMemoryMeasurementEnabled()`.
- If not enabled (IHOP JVM parameters not active), we proactively set `pinot.query.scheduler.accounting.oom.enable.killing.query=false` in the effective config, disabling OOM query killing to avoid false positives.
Files changed:
- server: `pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java`
- broker: `pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java`
Motivation:
- OOM protection relies on accurate per-thread allocated-bytes telemetry. When the JVM isn’t configured to expose it, keeping OOM protection enabled can lead to incorrect throttling or query killing decisions.
Notes:
- No behavioral change when IHOP JVM params are present and the capability is enabled.
- Only deprecation warnings observed in lints; no new errors introduced.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable OOM protection when IHOP JVM params not active (server/broker) [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 closed pull request #16912: Disable OOM protection when IHOP JVM params not active (server/broker)
URL: https://github.com/apache/pinot/pull/16912
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable OOM protection when IHOP JVM params not active (server/broker) [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #16912:
URL: https://github.com/apache/pinot/pull/16912#discussion_r2385814072
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java:
##########
@@ -686,6 +686,14 @@ public void start()
ThreadResourceUsageProvider.setThreadMemoryMeasurementEnabled(
_serverConf.getProperty(Server.CONFIG_OF_ENABLE_THREAD_ALLOCATED_BYTES_MEASUREMENT,
Server.DEFAULT_THREAD_ALLOCATED_BYTES_MEASUREMENT));
+ // If JVM does not enable thread allocated bytes measurement, disable OOM protection to avoid false actions.
+ if (!ThreadResourceUsageProvider.isThreadMemoryMeasurementEnabled()) {
+ LOGGER.warn("Thread allocated bytes measurement is not enabled by JVM. Disabling OOM protection.");
+ _serverConf.setProperty(
+ CommonConstants.PINOT_QUERY_SCHEDULER_PREFIX + "."
+ + CommonConstants.Accounting.CONFIG_OF_OOM_PROTECTION_KILLING_QUERY,
+ false);
Review Comment:
[nitpick] The configuration key is constructed using string concatenation. Consider extracting this into a constant or using a helper method to improve maintainability and reduce duplication with the broker implementation.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -365,6 +365,14 @@ public void start()
ThreadResourceUsageProvider.setThreadMemoryMeasurementEnabled(
_brokerConf.getProperty(CommonConstants.Broker.CONFIG_OF_ENABLE_THREAD_ALLOCATED_BYTES_MEASUREMENT,
CommonConstants.Broker.DEFAULT_THREAD_ALLOCATED_BYTES_MEASUREMENT));
+ // If JVM does not enable thread allocated bytes measurement, disable OOM protection to avoid false actions.
+ if (!ThreadResourceUsageProvider.isThreadMemoryMeasurementEnabled()) {
+ LOGGER.warn("Thread allocated bytes measurement is not enabled by JVM. Disabling OOM protection.");
+ _brokerConf.setProperty(
+ CommonConstants.PINOT_QUERY_SCHEDULER_PREFIX + "."
+ + CommonConstants.Accounting.CONFIG_OF_OOM_PROTECTION_KILLING_QUERY,
+ false);
Review Comment:
[nitpick] The configuration key is constructed using string concatenation. Consider extracting this into a constant or using a helper method to improve maintainability and reduce duplication with the server implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable OOM protection when IHOP JVM params not active (server/broker) [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16912:
URL: https://github.com/apache/pinot/pull/16912#issuecomment-3343687033
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16912?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 56.43%. Comparing base ([`90eb0c1`](https://app.codecov.io/gh/apache/pinot/commit/90eb0c171198f2bcce01c388570d54c2c29e1e96?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f46640b`](https://app.codecov.io/gh/apache/pinot/commit/f46640bfa3ae242cdb517d1f297dfae02b08672e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (90eb0c1) and HEAD (f46640b). Click for more details.
>
> <details><summary>HEAD has 32 uploads less than BASE</summary>
>
>| Flag | BASE (90eb0c1) | HEAD (f46640b) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|9|1|
>|java-11|4|0|
>|unittests2|2|0|
>|integration|5|0|
>|integration1|2|0|
>|custom-integration1|2|0|
>|integration2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16912 +/- ##
============================================
- Coverage 63.49% 56.43% -7.07%
+ Complexity 1411 701 -710
============================================
Files 3073 2407 -666
Lines 180726 133872 -46854
Branches 27630 21276 -6354
============================================
- Hits 114761 75550 -39211
+ Misses 57114 52028 -5086
+ Partials 8851 6294 -2557
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/16912/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/16912/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/16912/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/16912/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/16912/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/16912/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/16912/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.43% <ø> (-7.04%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/16912/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.43% <ø> (-7.07%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/16912/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.43% <ø> (-7.07%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16912/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.43% <ø> (+<0.01%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16912/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/16912?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable OOM protection when IHOP JVM params not active (server/broker) [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #16912:
URL: https://github.com/apache/pinot/pull/16912#discussion_r2388478680
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/OomProtectionUtils.java:
##########
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.utils;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.RuntimeMXBean;
+import java.util.List;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public final class OomProtectionUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(OomProtectionUtils.class);
+
+ private OomProtectionUtils() {
+ }
+
+ public static boolean hasDisableAdaptiveIhopFlag(List<String> jvmInputArgs) {
+ if (jvmInputArgs == null || jvmInputArgs.isEmpty()) {
+ return false;
+ }
+ for (String arg : jvmInputArgs) {
+ if ("-XX:-G1UseAdaptiveIHOP".equals(arg)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * If the required GC flag is missing, disables OOM protection by flipping the config flag to false.
+ * @return true if the method disabled OOM protection; false otherwise
+ */
+ public static boolean enforceIhopGcOrDisableOom(PinotConfiguration config, List<String> jvmInputArgs) {
+ boolean hasFlag = hasDisableAdaptiveIhopFlag(jvmInputArgs);
+ if (!hasFlag) {
+ LOGGER.warn("Required GC option -XX:-G1UseAdaptiveIHOP not found. Disabling OOM protection.");
+ config.setProperty(CommonConstants.Accounting.FULLY_QUALIFIED_CONFIG_OF_OOM_PROTECTION_KILLING_QUERY, false);
+ return true;
+ }
+ // Ensure IHOP threshold is below all OOM protection levels so GC triggers before protections
+ // Default IHOP threshold is 45% if not explicitly set by `-XX:InitiatingHeapOccupancyPercent=45`
+ double ihopRatio = 0.45;
+ if (jvmInputArgs != null) {
+ for (String arg : jvmInputArgs) {
+ if (arg != null && arg.startsWith("-XX:InitiatingHeapOccupancyPercent=")) {
+ int idx = arg.indexOf('=');
+ if (idx > 0 && idx + 1 < arg.length()) {
+ try {
+ ihopRatio = Double.parseDouble(arg.substring(idx + 1)) / 100.0;
+ } catch (Exception ignore) {
+ // keep default on parse failures
+ }
+ }
+ }
+ }
+ }
+ // We want to ensure IHOP threshold is below all OOM protection levels so GC triggers before protections
+ // So we add 20% to the default IHOP threshold
+ double ihopRatioThreshold = ihopRatio + 0.20;
+ PinotConfiguration accountingConfigs = config.subset(CommonConstants.PINOT_QUERY_SCHEDULER_PREFIX);
+ double alarmingRatio = accountingConfigs.getProperty(
+ CommonConstants.Accounting.CONFIG_OF_ALARMING_LEVEL_HEAP_USAGE_RATIO,
+ CommonConstants.Accounting.DEFAULT_ALARMING_LEVEL_HEAP_USAGE_RATIO);
+ double criticalRatio = accountingConfigs.getProperty(
+ CommonConstants.Accounting.CONFIG_OF_CRITICAL_LEVEL_HEAP_USAGE_RATIO,
+ CommonConstants.Accounting.DEFAULT_CRITICAL_LEVEL_HEAP_USAGE_RATIO);
+ double panicRatio = accountingConfigs.getProperty(
+ CommonConstants.Accounting.CONFIG_OF_PANIC_LEVEL_HEAP_USAGE_RATIO,
+ CommonConstants.Accounting.DFAULT_PANIC_LEVEL_HEAP_USAGE_RATIO);
+
+ if (ihopRatioThreshold > alarmingRatio || ihopRatioThreshold > criticalRatio
+ || ihopRatioThreshold > panicRatio) {
+ int expectedIhopRatio = (int) Math.min(Math.min(alarmingRatio, criticalRatio), panicRatio) * 100 - 20;
+ LOGGER.warn(
+ "InitiatingHeapOccupancyPercent ratio {} is not below 20% of OOM thresholds (alarming={}, critical={}, "
+ + "panic={}). Disabling OOM protection as GC may not be triggerred or fast enough. Please ensure you "
Review Comment:
There's a typo in 'triggerred' - it should be 'triggered'.
```suggestion
+ "panic={}). Disabling OOM protection as GC may not be triggered or fast enough. Please ensure you "
```
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/OomProtectionUtils.java:
##########
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.utils;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.RuntimeMXBean;
+import java.util.List;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public final class OomProtectionUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(OomProtectionUtils.class);
+
+ private OomProtectionUtils() {
+ }
+
+ public static boolean hasDisableAdaptiveIhopFlag(List<String> jvmInputArgs) {
+ if (jvmInputArgs == null || jvmInputArgs.isEmpty()) {
+ return false;
+ }
+ for (String arg : jvmInputArgs) {
+ if ("-XX:-G1UseAdaptiveIHOP".equals(arg)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * If the required GC flag is missing, disables OOM protection by flipping the config flag to false.
+ * @return true if the method disabled OOM protection; false otherwise
+ */
+ public static boolean enforceIhopGcOrDisableOom(PinotConfiguration config, List<String> jvmInputArgs) {
+ boolean hasFlag = hasDisableAdaptiveIhopFlag(jvmInputArgs);
+ if (!hasFlag) {
+ LOGGER.warn("Required GC option -XX:-G1UseAdaptiveIHOP not found. Disabling OOM protection.");
+ config.setProperty(CommonConstants.Accounting.FULLY_QUALIFIED_CONFIG_OF_OOM_PROTECTION_KILLING_QUERY, false);
+ return true;
+ }
+ // Ensure IHOP threshold is below all OOM protection levels so GC triggers before protections
+ // Default IHOP threshold is 45% if not explicitly set by `-XX:InitiatingHeapOccupancyPercent=45`
+ double ihopRatio = 0.45;
+ if (jvmInputArgs != null) {
+ for (String arg : jvmInputArgs) {
+ if (arg != null && arg.startsWith("-XX:InitiatingHeapOccupancyPercent=")) {
+ int idx = arg.indexOf('=');
+ if (idx > 0 && idx + 1 < arg.length()) {
+ try {
+ ihopRatio = Double.parseDouble(arg.substring(idx + 1)) / 100.0;
+ } catch (Exception ignore) {
+ // keep default on parse failures
+ }
+ }
+ }
+ }
+ }
+ // We want to ensure IHOP threshold is below all OOM protection levels so GC triggers before protections
+ // So we add 20% to the default IHOP threshold
+ double ihopRatioThreshold = ihopRatio + 0.20;
+ PinotConfiguration accountingConfigs = config.subset(CommonConstants.PINOT_QUERY_SCHEDULER_PREFIX);
+ double alarmingRatio = accountingConfigs.getProperty(
+ CommonConstants.Accounting.CONFIG_OF_ALARMING_LEVEL_HEAP_USAGE_RATIO,
+ CommonConstants.Accounting.DEFAULT_ALARMING_LEVEL_HEAP_USAGE_RATIO);
+ double criticalRatio = accountingConfigs.getProperty(
+ CommonConstants.Accounting.CONFIG_OF_CRITICAL_LEVEL_HEAP_USAGE_RATIO,
+ CommonConstants.Accounting.DEFAULT_CRITICAL_LEVEL_HEAP_USAGE_RATIO);
+ double panicRatio = accountingConfigs.getProperty(
+ CommonConstants.Accounting.CONFIG_OF_PANIC_LEVEL_HEAP_USAGE_RATIO,
+ CommonConstants.Accounting.DFAULT_PANIC_LEVEL_HEAP_USAGE_RATIO);
Review Comment:
There's a typo in the constant name 'DFAULT_PANIC_LEVEL_HEAP_USAGE_RATIO' - it should be 'DEFAULT_PANIC_LEVEL_HEAP_USAGE_RATIO'.
```suggestion
CommonConstants.Accounting.DEFAULT_PANIC_LEVEL_HEAP_USAGE_RATIO);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable OOM protection when IHOP JVM params not active (server/broker) [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #16912:
URL: https://github.com/apache/pinot/pull/16912#discussion_r2602991192
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/OomProtectionUtils.java:
##########
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.utils;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.RuntimeMXBean;
+import java.util.List;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public final class OomProtectionUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(OomProtectionUtils.class);
+
+ private OomProtectionUtils() {
+ }
+
+ public static boolean hasDisableAdaptiveIhopFlag(List<String> jvmInputArgs) {
+ if (jvmInputArgs == null || jvmInputArgs.isEmpty()) {
+ return false;
+ }
+ for (String arg : jvmInputArgs) {
+ if ("-XX:-G1UseAdaptiveIHOP".equals(arg)) {
Review Comment:
Good point
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable OOM protection when IHOP JVM params not active (server/broker) [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #16912:
URL: https://github.com/apache/pinot/pull/16912#discussion_r2417717773
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/OomProtectionUtils.java:
##########
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.utils;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.RuntimeMXBean;
+import java.util.List;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public final class OomProtectionUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(OomProtectionUtils.class);
+
+ private OomProtectionUtils() {
+ }
+
+ public static boolean hasDisableAdaptiveIhopFlag(List<String> jvmInputArgs) {
+ if (jvmInputArgs == null || jvmInputArgs.isEmpty()) {
+ return false;
+ }
+ for (String arg : jvmInputArgs) {
+ if ("-XX:-G1UseAdaptiveIHOP".equals(arg)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * If the required GC flag is missing, disables OOM protection by flipping the config flag to false.
+ * @return true if the method disabled OOM protection; false otherwise
+ */
+ public static boolean enforceIhopGcOrDisableOom(PinotConfiguration config, List<String> jvmInputArgs) {
+ boolean hasFlag = hasDisableAdaptiveIhopFlag(jvmInputArgs);
+ if (!hasFlag) {
+ LOGGER.warn("Required GC option -XX:-G1UseAdaptiveIHOP not found. Disabling OOM protection.");
Review Comment:
for now just make this for g1gc, we can support it later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable OOM protection when IHOP JVM params not active (server/broker) [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #16912:
URL: https://github.com/apache/pinot/pull/16912#discussion_r2403306208
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/OomProtectionUtils.java:
##########
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.utils;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.RuntimeMXBean;
+import java.util.List;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public final class OomProtectionUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(OomProtectionUtils.class);
+
+ private OomProtectionUtils() {
+ }
+
+ public static boolean hasDisableAdaptiveIhopFlag(List<String> jvmInputArgs) {
+ if (jvmInputArgs == null || jvmInputArgs.isEmpty()) {
+ return false;
+ }
+ for (String arg : jvmInputArgs) {
+ if ("-XX:-G1UseAdaptiveIHOP".equals(arg)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * If the required GC flag is missing, disables OOM protection by flipping the config flag to false.
+ * @return true if the method disabled OOM protection; false otherwise
+ */
+ public static boolean enforceIhopGcOrDisableOom(PinotConfiguration config, List<String> jvmInputArgs) {
+ boolean hasFlag = hasDisableAdaptiveIhopFlag(jvmInputArgs);
+ if (!hasFlag) {
+ LOGGER.warn("Required GC option -XX:-G1UseAdaptiveIHOP not found. Disabling OOM protection.");
Review Comment:
Does this mean our current thread resource usage tracking and query OOM kill feature work only with G1GC and not with any others?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable OOM protection when IHOP JVM params not active (server/broker) [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16912:
URL: https://github.com/apache/pinot/pull/16912#discussion_r2388902886
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/OomProtectionUtils.java:
##########
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.utils;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.RuntimeMXBean;
+import java.util.List;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public final class OomProtectionUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(OomProtectionUtils.class);
+
+ private OomProtectionUtils() {
+ }
+
+ public static boolean hasDisableAdaptiveIhopFlag(List<String> jvmInputArgs) {
+ if (jvmInputArgs == null || jvmInputArgs.isEmpty()) {
+ return false;
+ }
+ for (String arg : jvmInputArgs) {
+ if ("-XX:-G1UseAdaptiveIHOP".equals(arg)) {
Review Comment:
What happens if user is not using `G1GC`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable OOM protection when IHOP JVM params not active (server/broker) [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #16912:
URL: https://github.com/apache/pinot/pull/16912#issuecomment-3621349881
> > Trying to understand the change. When -XX:-G1UseAdaptiveIHOP is not set, does it prevent per-thread memory/CPU utilization from being reported? Per the argument name, seems they are unrelated
>
> @Jackie-Jiang I think the PR description is wrong. `-G1UseAdaptiveIHOP` doesn't affect whether we can estimate the amount of allocated memory. The reason we need it is that if the % of used memory is below IHOP, the JVM won't collect old generation. The OOM protection algorithm assumes the % of used memory is mostly alive memory, but as long as the % of memory is below IHOP, the used memory may be mostly garbage. As a result, if our thresholds are smaller than IHOP, we may be throttling or killing queries while the % of alive memory is actually very low. With adaptive IHOP, the actual % of memory that fires old GCs is undefined.
>
> I think what we should do here is to verify that the IHOP is lower than our thresholds. This means:
>
> * If the JVM uses adaptive IHOP, the check should fail
> * If the JVM doesn't use IHOP:
>
> * If IHOP is higher than our thresholds, the check should fail
> * In other case, the check should succeed.
>
> If the check fails, we should log an error message and continue working as before (ie if the check was executed during startup, run without OOM protection, if the check failed when a config was changed, log an error and continue using the old config)
- If adaptive IHOP is on (missing -XX:-G1UseAdaptiveIHOP), the check fails and we disable OOM protection.
- With static IHOP, we parse -XX:InitiatingHeapOccupancyPercent; if it’s at/above the lowest OOM threshold, the check fails and OOM protection is disabled. Otherwise it passes.
- Failures now log an explicit error with the detected IHOP and the OOM thresholds, but we keep running (startup continues without OOM protection or retains the prior config on change).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org