You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2026/02/10 18:36:49 UTC
[PR] Better netty inspection [pinot]
gortiz opened a new pull request, #17674:
URL: https://github.com/apache/pinot/pull/17674
This PR adds a new machinery to inspect, log and monitor the memory used by Netty. As is explained on the NettyInstance javadoc, a Pinot installation has between 2 and 3 copies of the Netty code:
1. The version we shade. This is present in production but not when running Pinot without shading (i.e., tests or a QuickStart started from the IDE or with Maven)
2. The version shade by gRPC
3. Optionally, an unshaded version, which is present when 1 is not present and may also be included by a third-party library
Netty uses static attributes to keep some important information, like whether it can use off-heap memory or not, or how much memory is being allocated. Given we have 2-3 copies of Netty, we have 2-3 **independent** copies of these attributes. The applied shade doesn't just change package of the classes, it also changes the system properties we need to set in order to customize Netty. We always need to set at least one JAVA_OPT in order to let Netty use offheap memory in Java 21: `io.netty.tryReflectionSetAccessible`
As a corollary, for each JAVA_OPT we plan to use to customize Netty we need to provide that option 2-3 times with different prefixes.
Given that different copies of the same classes are involved here, this is a very error-prone process. This is why we introduce a new class, NettyInstance, that offers clean access to the most important Netty properties. This class can be instantiated to access a specific Netty copy.
Another class, NettyInspector, is used to add some checks on the state of important properties of each known NettyInstance. Right now, the only check we have is whether it uses onheap of offheap memory, but more may be added in the future.
This PR also adds a 2 new metrics per NettyInstance: how much memory is being used and how much memory that instance can use. This is important **because by default each independent Netty copy will consume as many direct memory as specified by `XX:MaxDirectMemorySize`, which means we may be consuming a maximum of:
1. `MaxDirectMemorySize` bytes by normal ByteBuffers
2. `MaxDirectMemorySize` bytes by unshade/pinot-shaded Netty (used in SSE)
3. `MaxDirectMemorySize` bytes by grpc-shaded Netty (used in MSE)
Finally, this PR also adds logs when Brokers and Servers start. Specifically:
- For each NettyInstance, we add a warning log if we are using on-heap memory.
- For each NettyInstance, we add an info log indicating how much memory it is using (usually 0) and how much memory it can use.
- A single node indicating the sum of off-heap memory all NettyInstances are using and can use.
--
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] Better netty inspection [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #17674:
URL: https://github.com/apache/pinot/pull/17674#discussion_r2821605382
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/NettyInstance.java:
##########
@@ -0,0 +1,147 @@
+/**
+ * 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.core.transport;
+
+import java.lang.reflect.Constructor;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/// Represents an instance of Netty, allowing access to certain static properties via reflection, with support for
+/// shaded Netty versions.
+///
+/// We know 2 common Netty instances:
+/// - Unshaded Netty, which uses the standard `io.netty` package
+/// - gRPC-shaded Netty, shaded by gRPC and included as a dependency. It uses `io.grpc.netty.shaded.io.netty` package.
+///
+/// This is important because Netty defines is not designed to be shade, and it uses some static attributes to determine
+/// its behavior, specially whether it can use `Unsafe` or not or how much memory to allocate for direct buffers.
+/// These attributes are set using JAVA_OPTs. Each shaded version uses different JAVA_OPT properties. If we forget to
+/// set one of these properties for a shaded version, that Netty _instance_ will fall back to some default behavior that
+/// may not be optimal, and we won't have any indication of that happening.
+///
+/// By using reflection to access these properties, we can log their values at startup and check if they are set to the
+/// expected values, logging warnings if they are not.
+/// This allows us to catch misconfigurations early.
+///
+/// **It is critical to not shade this class**, otherwise the literals used for reflection
+/// (ie `io.netty.util.internal.PlatformDependent`) will be shaded too, so instead of looking for
+/// `io.grpc.netty.shaded.io.netty.util.internal.PlatformDependent`, as you may think reading this class, the shaded
+/// version of this class will look for
+/// `io.grpc.netty.shaded.org.apache.pinot.shaded.io.netty.util.internal.PlatformDependent`.
+/// At the moment this is written, Pinot _does not_ shade Netty, so it is safe. Just to be sure, this class should be
+/// excluded in the maven shade plugin configuration (see pom.xml on the root of the project).
+public abstract class NettyInstance {
+ private static final Logger LOGGER = LoggerFactory.getLogger(NettyInstance.class);
+ private static final Constructor<DummyClass> CONSTRUCTOR;
+
+ static {
+ try {
+ CONSTRUCTOR = DummyClass.class.getConstructor();
+ } catch (NoSuchMethodException e) {
+ throw new RuntimeException("This should never happen, DummyClass has a default constructor", e);
+ }
+ }
+
+ /// The name of the Netty instance. It will be used on logs but also on metric names, so it should be
+ /// something short and without spaces like "Unshaded", "Pinot", "gRPC".
+ /// Add underscores if you need to separate words, but avoid other special characters.
+ public abstract String getName();
+
+ public abstract String getShadePrefix();
+
+ public abstract boolean isExplicitTryReflectionSetAccessible();
+
+ public abstract long getUsedDirectMemory();
+
+ public abstract long getMaxDirectMemory();
+
+ public abstract long getEstimateMaxDirectMemory();
+
+ public static class UnshadedNettyInstance extends NettyInstance {
+ @Override
+ public String getName() {
+ return "unshaded";
+ }
+
+ @Override
+ public String getShadePrefix() {
+ return "";
+ }
+
+ @Override
+ public boolean isExplicitTryReflectionSetAccessible() {
+ return io.netty.util.internal.ReflectionUtil.trySetAccessible(CONSTRUCTOR, true) == null;
Review Comment:
No, but I think that is very unlikely. I can imagine in the future Netty may create an alternative that uses the new Arenas introduced in foreign function and memory (FFM). In that case, they may change this code, but I don't think they are going to do so soon.
Anyway, the alternative is to use reflection, and that is even less stable.
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/NettyInspector.java:
##########
@@ -0,0 +1,158 @@
+/**
+ * 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.core.transport;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.function.Function;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.metrics.AbstractMetrics;
+import org.apache.pinot.spi.utils.DataSizeUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/// Utility class to inspect Netty constants and log their values, with the ability to check for specific conditions
+/// and log warnings if they are not met.
+public class NettyInspector {
+ private static final Logger LOGGER = LoggerFactory.getLogger(NettyInspector.class);
+ /// We use a CopyOnWriteArrayList to allow dynamic addition of checks at runtime, if needed.
+ public static final CopyOnWriteArrayList<Check> CHECKS;
+ /// We use a CopyOnWriteArrayList to allow dynamic addition of Netty instances at runtime,
+ /// if needed (e.g. if we want to support other shaded versions of Netty).
+ public static final CopyOnWriteArrayList<NettyInstance> KNOWN_INSTANCES;
+
+ static {
+ CHECKS = new CopyOnWriteArrayList<>(
+ new Check[] {
+ NettyInspector::checkDirectMemory
+ }
+ );
+ KNOWN_INSTANCES = new CopyOnWriteArrayList<>(new NettyInstance[] {
+ new NettyInstance.UnshadedNettyInstance(),
+ new NettyInstance.GrpcNettyInstance()
+ });
+ }
+
+ private NettyInspector() {
+ // Private constructor to prevent instantiation
+ }
+
+ public static void registerMetrics(AbstractMetrics<?, ?, ?, ?> metrics) {
+ for (NettyInstance instance : KNOWN_INSTANCES) {
+ metrics.setOrUpdateGauge(instance.getName() + "NettyDirectMemoryUsed",
+ instance::getUsedDirectMemory);
+ metrics.setOrUpdateGauge(instance.getName() + "NettyDirectMemoryMax",
+ instance::getMaxDirectMemory);
+ }
+ }
+
+ /// Logs the memory used by each known Netty instance as long as the max memory it can use.
+ /// It also logs the total memory used by all Netty instances and its max memory.
+ public static void logMemory() {
+ long totalUsedMemory = 0;
+ long totalMaxMemory = 0;
+ for (NettyInstance instance : KNOWN_INSTANCES) {
+ long usedMemory = instance.getUsedDirectMemory();
+ long maxMemory = instance.getMaxDirectMemory();
+ totalUsedMemory += usedMemory;
+ totalMaxMemory += maxMemory;
+ LOGGER.info("Netty instance '{}' is using {} of direct memory (max {}).", instance.getName(),
+ DataSizeUtils.fromBytes(usedMemory), DataSizeUtils.fromBytes(maxMemory));
+ }
+ LOGGER.info("Total direct memory used by all Netty instances: {} (max {}).",
+ DataSizeUtils.fromBytes(totalUsedMemory), DataSizeUtils.fromBytes(totalMaxMemory));
+ }
+
+ /// Logs the values of all constants for all Netty instances, and logs warnings if any checks fail.
+ public static void logAllChecks() {
+ for (Map.Entry<NettyInstance, List<CheckResult>> entry : checkAllConstants().entrySet()) {
+ NettyInstance instance = entry.getKey();
+ List<CheckResult> results = entry.getValue();
+ for (CheckResult result : results) {
+ switch (result._status) {
+ case PASS:
+ // Do nothing
Review Comment:
I think it would be useful to expose this check as a REST endpoint so we can call it periodically in the future.
--
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] Better netty inspection [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #17674:
URL: https://github.com/apache/pinot/pull/17674#issuecomment-3884688126
I've rewritten `NettyInstance` in a way we don't have to use reflection to access most properties. I also removed some of the properties we are not using right now. I'm not proud of the way I implemented `isExplicitTryReflectionSetAccessible`, but it is the best solution I found
--
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] Better netty inspection [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #17674:
URL: https://github.com/apache/pinot/pull/17674#discussion_r2821584319
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/NettyInspector.java:
##########
@@ -0,0 +1,158 @@
+/**
+ * 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.core.transport;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.function.Function;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.metrics.AbstractMetrics;
+import org.apache.pinot.spi.utils.DataSizeUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/// Utility class to inspect Netty constants and log their values, with the ability to check for specific conditions
+/// and log warnings if they are not met.
+public class NettyInspector {
+ private static final Logger LOGGER = LoggerFactory.getLogger(NettyInspector.class);
+ /// We use a CopyOnWriteArrayList to allow dynamic addition of checks at runtime, if needed.
+ public static final CopyOnWriteArrayList<Check> CHECKS;
+ /// We use a CopyOnWriteArrayList to allow dynamic addition of Netty instances at runtime,
+ /// if needed (e.g. if we want to support other shaded versions of Netty).
+ public static final CopyOnWriteArrayList<NettyInstance> KNOWN_INSTANCES;
Review Comment:
Third-party distributions may include their own Netty copies as well. In StarTree, we don't do that, but I don't know about other distributors. A copy-on-write structure is thread-safe and very cheap to read (it incurs a cost when writing). Given we mostly only going to use it to read from it (as you said, probably nobody is going to modify it later), I think this is the best solution for our use case.
--
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] Better netty inspection [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17674:
URL: https://github.com/apache/pinot/pull/17674#issuecomment-3880291940
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17674?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `50.00000%` with `63 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 34.05%. Comparing base ([`0f93d52`](https://app.codecov.io/gh/apache/pinot/commit/0f93d52ef0bfa1c2dc367b31f137a63bd25f9d2f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`3332c16`](https://app.codecov.io/gh/apache/pinot/commit/3332c164ca99f969b47319e91d203b4017b4ef2e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:white_check_mark: All tests successful. No failed tests found.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/17674?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...org/apache/pinot/core/transport/NettyInstance.java](https://app.codecov.io/gh/apache/pinot/pull/17674?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Ftransport%2FNettyInstance.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvTmV0dHlJbnN0YW5jZS5qYXZh) | 38.27% | [47 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17674?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...rg/apache/pinot/core/transport/NettyInspector.java](https://app.codecov.io/gh/apache/pinot/pull/17674?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Ftransport%2FNettyInspector.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvTmV0dHlJbnNwZWN0b3IuamF2YQ==) | 73.17% | [8 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17674?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../pinot/server/starter/helix/BaseServerStarter.java](https://app.codecov.io/gh/apache/pinot/pull/17674?src=pr&el=tree&filepath=pinot-server%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fserver%2Fstarter%2Fhelix%2FBaseServerStarter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9CYXNlU2VydmVyU3RhcnRlci5qYXZh) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17674?src=pr&el=tree&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 (0f93d52) and HEAD (3332c16). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (0f93d52) | HEAD (3332c16) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|0|
>|unittests|4|1|
>|temurin|10|1|
>|java-11|5|0|
>|integration|6|0|
>|integration2|2|0|
>|unittests2|2|1|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17674 +/- ##
=============================================
- Coverage 63.25% 34.05% -29.20%
+ Complexity 1499 772 -727
=============================================
Files 3174 3176 +2
Lines 190373 190497 +124
Branches 29089 29100 +11
=============================================
- Hits 120419 64883 -55536
- Misses 60610 120246 +59636
+ Partials 9344 5368 -3976
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17674/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/17674/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/17674/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/17674/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/17674/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/17674/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/17674/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.05% <50.00%> (-29.17%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17674/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.05% <50.00%> (-29.20%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17674/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.05% <50.00%> (-29.20%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17674/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17674/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.05% <50.00%> (-0.02%)` | :arrow_down: |
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/17674?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] Better netty inspection [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #17674:
URL: https://github.com/apache/pinot/pull/17674#issuecomment-3883442699
> Q: Just to understand: the memory stats are per static instance of netty, as in per pool? I'm guessing 1 for SSE and 1 for MSE? any others?
We use Netty to implement SSE communication between brokers and servers and gRPC uses their own shade version of Netty under the hood. We use gRPC in 4 different places:
- The client gRPC protocol, an alternative to the REST API. Here, the gRPC server side is our broker.
- The direct client to server gRPC protocol, used for example by Trino/Presto. Here the gRPC server side is our servers
- The MSE plan protocol, where Pinot servers are the gRPC servers and brokers are gRPC clients.
- The MSE mailbox protocol, where Pinot servers and brokers play the role of gRPC servers (and only Pinot servers play the role of gRPC clients).
SSE, Client gRPC, direct client to serve gRPC and MSE mailbox protocols; expose their own metrics to report memory usage (note we don't have metrics for the plan protocol, but that is super simple and doesn't consume much).
But each of these gRPC servers is also constrained by the max memory Netty can use (where SSE uses the limits of the unshaded Netty instance and all gRPC use the limits defined on the gRPC shaded Netty instance).
So the metric for used memory by SSE (assuming the fix in https://github.com/apache/pinot/pull/17667 is merged) should be similar to the used memory shown by the new metric for unshaded Netty, but for gRPC it will be the sum of the used memory by the MSE mailbox, MSE plan, and client gRPC protocols.
--
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] Better netty inspection [pinot]
Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.
suvodeep-pyne commented on code in PR #17674:
URL: https://github.com/apache/pinot/pull/17674#discussion_r2789750665
##########
pom.xml:
##########
@@ -2929,6 +2929,9 @@
<!-- Solve NoClassDefFoundError. Borrowed from https://github.com/prometheus/jmx_exporter/issues/802 -->
<exclude>META-INF/versions/9/org/yaml/snakeyaml/internal/**</exclude>
+
+ <!-- Exclude NettyInstacne because it includes Netty package literals -->
Review Comment:
This looks a bit fragile. Should we load the strings from somewhere instead? I understand the motivation but thing is 1 rename or package move and it might break.
--
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] Better netty inspection [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #17674:
URL: https://github.com/apache/pinot/pull/17674#discussion_r2819278441
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/NettyInspector.java:
##########
@@ -0,0 +1,158 @@
+/**
+ * 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.core.transport;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.function.Function;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.metrics.AbstractMetrics;
+import org.apache.pinot.spi.utils.DataSizeUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/// Utility class to inspect Netty constants and log their values, with the ability to check for specific conditions
+/// and log warnings if they are not met.
+public class NettyInspector {
+ private static final Logger LOGGER = LoggerFactory.getLogger(NettyInspector.class);
+ /// We use a CopyOnWriteArrayList to allow dynamic addition of checks at runtime, if needed.
+ public static final CopyOnWriteArrayList<Check> CHECKS;
+ /// We use a CopyOnWriteArrayList to allow dynamic addition of Netty instances at runtime,
+ /// if needed (e.g. if we want to support other shaded versions of Netty).
+ public static final CopyOnWriteArrayList<NettyInstance> KNOWN_INSTANCES;
+
+ static {
+ CHECKS = new CopyOnWriteArrayList<>(
+ new Check[] {
+ NettyInspector::checkDirectMemory
+ }
+ );
+ KNOWN_INSTANCES = new CopyOnWriteArrayList<>(new NettyInstance[] {
+ new NettyInstance.UnshadedNettyInstance(),
+ new NettyInstance.GrpcNettyInstance()
+ });
+ }
+
+ private NettyInspector() {
+ // Private constructor to prevent instantiation
+ }
+
+ public static void registerMetrics(AbstractMetrics<?, ?, ?, ?> metrics) {
+ for (NettyInstance instance : KNOWN_INSTANCES) {
+ metrics.setOrUpdateGauge(instance.getName() + "NettyDirectMemoryUsed",
+ instance::getUsedDirectMemory);
+ metrics.setOrUpdateGauge(instance.getName() + "NettyDirectMemoryMax",
+ instance::getMaxDirectMemory);
+ }
+ }
+
+ /// Logs the memory used by each known Netty instance as long as the max memory it can use.
+ /// It also logs the total memory used by all Netty instances and its max memory.
+ public static void logMemory() {
+ long totalUsedMemory = 0;
+ long totalMaxMemory = 0;
+ for (NettyInstance instance : KNOWN_INSTANCES) {
+ long usedMemory = instance.getUsedDirectMemory();
+ long maxMemory = instance.getMaxDirectMemory();
+ totalUsedMemory += usedMemory;
+ totalMaxMemory += maxMemory;
+ LOGGER.info("Netty instance '{}' is using {} of direct memory (max {}).", instance.getName(),
+ DataSizeUtils.fromBytes(usedMemory), DataSizeUtils.fromBytes(maxMemory));
+ }
+ LOGGER.info("Total direct memory used by all Netty instances: {} (max {}).",
+ DataSizeUtils.fromBytes(totalUsedMemory), DataSizeUtils.fromBytes(totalMaxMemory));
+ }
+
+ /// Logs the values of all constants for all Netty instances, and logs warnings if any checks fail.
+ public static void logAllChecks() {
+ for (Map.Entry<NettyInstance, List<CheckResult>> entry : checkAllConstants().entrySet()) {
+ NettyInstance instance = entry.getKey();
+ List<CheckResult> results = entry.getValue();
+ for (CheckResult result : results) {
+ switch (result._status) {
+ case PASS:
+ // Do nothing
Review Comment:
Since this is only being called once on startup, maybe we could include an info log?
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/NettyInspector.java:
##########
@@ -0,0 +1,158 @@
+/**
+ * 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.core.transport;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.function.Function;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.metrics.AbstractMetrics;
+import org.apache.pinot.spi.utils.DataSizeUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/// Utility class to inspect Netty constants and log their values, with the ability to check for specific conditions
+/// and log warnings if they are not met.
+public class NettyInspector {
+ private static final Logger LOGGER = LoggerFactory.getLogger(NettyInspector.class);
+ /// We use a CopyOnWriteArrayList to allow dynamic addition of checks at runtime, if needed.
+ public static final CopyOnWriteArrayList<Check> CHECKS;
+ /// We use a CopyOnWriteArrayList to allow dynamic addition of Netty instances at runtime,
+ /// if needed (e.g. if we want to support other shaded versions of Netty).
+ public static final CopyOnWriteArrayList<NettyInstance> KNOWN_INSTANCES;
Review Comment:
I don't get why these need to be copy on write arrays? They aren't mutated after creation, no?
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/NettyInstance.java:
##########
@@ -0,0 +1,147 @@
+/**
+ * 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.core.transport;
+
+import java.lang.reflect.Constructor;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/// Represents an instance of Netty, allowing access to certain static properties via reflection, with support for
+/// shaded Netty versions.
+///
+/// We know 2 common Netty instances:
+/// - Unshaded Netty, which uses the standard `io.netty` package
+/// - gRPC-shaded Netty, shaded by gRPC and included as a dependency. It uses `io.grpc.netty.shaded.io.netty` package.
+///
+/// This is important because Netty defines is not designed to be shade, and it uses some static attributes to determine
+/// its behavior, specially whether it can use `Unsafe` or not or how much memory to allocate for direct buffers.
+/// These attributes are set using JAVA_OPTs. Each shaded version uses different JAVA_OPT properties. If we forget to
+/// set one of these properties for a shaded version, that Netty _instance_ will fall back to some default behavior that
+/// may not be optimal, and we won't have any indication of that happening.
+///
+/// By using reflection to access these properties, we can log their values at startup and check if they are set to the
+/// expected values, logging warnings if they are not.
+/// This allows us to catch misconfigurations early.
+///
+/// **It is critical to not shade this class**, otherwise the literals used for reflection
+/// (ie `io.netty.util.internal.PlatformDependent`) will be shaded too, so instead of looking for
+/// `io.grpc.netty.shaded.io.netty.util.internal.PlatformDependent`, as you may think reading this class, the shaded
+/// version of this class will look for
+/// `io.grpc.netty.shaded.org.apache.pinot.shaded.io.netty.util.internal.PlatformDependent`.
+/// At the moment this is written, Pinot _does not_ shade Netty, so it is safe. Just to be sure, this class should be
+/// excluded in the maven shade plugin configuration (see pom.xml on the root of the project).
+public abstract class NettyInstance {
+ private static final Logger LOGGER = LoggerFactory.getLogger(NettyInstance.class);
+ private static final Constructor<DummyClass> CONSTRUCTOR;
+
+ static {
+ try {
+ CONSTRUCTOR = DummyClass.class.getConstructor();
+ } catch (NoSuchMethodException e) {
+ throw new RuntimeException("This should never happen, DummyClass has a default constructor", e);
+ }
+ }
+
+ /// The name of the Netty instance. It will be used on logs but also on metric names, so it should be
+ /// something short and without spaces like "Unshaded", "Pinot", "gRPC".
+ /// Add underscores if you need to separate words, but avoid other special characters.
+ public abstract String getName();
+
+ public abstract String getShadePrefix();
+
+ public abstract boolean isExplicitTryReflectionSetAccessible();
+
+ public abstract long getUsedDirectMemory();
+
+ public abstract long getMaxDirectMemory();
+
+ public abstract long getEstimateMaxDirectMemory();
+
+ public static class UnshadedNettyInstance extends NettyInstance {
+ @Override
+ public String getName() {
+ return "unshaded";
+ }
+
+ @Override
+ public String getShadePrefix() {
+ return "";
+ }
+
+ @Override
+ public boolean isExplicitTryReflectionSetAccessible() {
+ return io.netty.util.internal.ReflectionUtil.trySetAccessible(CONSTRUCTOR, true) == null;
Review Comment:
This is a neat hack lol, but there's no way we can be sure that Netty will continue using the same method to decide whether offheap buffers can be used in future versions right?
##########
pom.xml:
##########
@@ -2929,6 +2929,9 @@
<!-- Solve NoClassDefFoundError. Borrowed from https://github.com/prometheus/jmx_exporter/issues/802 -->
<exclude>META-INF/versions/9/org/yaml/snakeyaml/internal/**</exclude>
+
+ <!-- Exclude NettyInstacne because it includes Netty package literals -->
Review Comment:
```suggestion
<!-- Exclude NettyInstance because it includes Netty package literals -->
```
nit
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/NettyInspector.java:
##########
@@ -0,0 +1,158 @@
+/**
+ * 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.core.transport;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.function.Function;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.metrics.AbstractMetrics;
+import org.apache.pinot.spi.utils.DataSizeUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/// Utility class to inspect Netty constants and log their values, with the ability to check for specific conditions
+/// and log warnings if they are not met.
+public class NettyInspector {
+ private static final Logger LOGGER = LoggerFactory.getLogger(NettyInspector.class);
+ /// We use a CopyOnWriteArrayList to allow dynamic addition of checks at runtime, if needed.
+ public static final CopyOnWriteArrayList<Check> CHECKS;
+ /// We use a CopyOnWriteArrayList to allow dynamic addition of Netty instances at runtime,
+ /// if needed (e.g. if we want to support other shaded versions of Netty).
+ public static final CopyOnWriteArrayList<NettyInstance> KNOWN_INSTANCES;
+
+ static {
+ CHECKS = new CopyOnWriteArrayList<>(
+ new Check[] {
+ NettyInspector::checkDirectMemory
+ }
+ );
+ KNOWN_INSTANCES = new CopyOnWriteArrayList<>(new NettyInstance[] {
+ new NettyInstance.UnshadedNettyInstance(),
+ new NettyInstance.GrpcNettyInstance()
+ });
+ }
+
+ private NettyInspector() {
+ // Private constructor to prevent instantiation
+ }
+
+ public static void registerMetrics(AbstractMetrics<?, ?, ?, ?> metrics) {
+ for (NettyInstance instance : KNOWN_INSTANCES) {
+ metrics.setOrUpdateGauge(instance.getName() + "NettyDirectMemoryUsed",
+ instance::getUsedDirectMemory);
+ metrics.setOrUpdateGauge(instance.getName() + "NettyDirectMemoryMax",
+ instance::getMaxDirectMemory);
+ }
+ }
+
+ /// Logs the memory used by each known Netty instance as long as the max memory it can use.
+ /// It also logs the total memory used by all Netty instances and its max memory.
+ public static void logMemory() {
Review Comment:
Unused?
--
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] Better netty inspection [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #17674:
URL: https://github.com/apache/pinot/pull/17674#discussion_r2793478619
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/NettyInstance.java:
##########
@@ -0,0 +1,147 @@
+/**
+ * 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.core.transport;
+
+import java.lang.reflect.Constructor;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/// Represents an instance of Netty, allowing access to certain static properties via reflection, with support for
+/// shaded Netty versions.
+///
+/// We know 2 common Netty instances:
+/// - Unshaded Netty, which uses the standard `io.netty` package
+/// - gRPC-shaded Netty, shaded by gRPC and included as a dependency. It uses `io.grpc.netty.shaded.io.netty` package.
+///
+/// This is important because Netty defines is not designed to be shade, and it uses some static attributes to determine
+/// its behavior, specially whether it can use `Unsafe` or not or how much memory to allocate for direct buffers.
+/// These attributes are set using JAVA_OPTs. Each shaded version uses different JAVA_OPT properties. If we forget to
+/// set one of these properties for a shaded version, that Netty _instance_ will fall back to some default behavior that
+/// may not be optimal, and we won't have any indication of that happening.
+///
+/// By using reflection to access these properties, we can log their values at startup and check if they are set to the
+/// expected values, logging warnings if they are not.
+/// This allows us to catch misconfigurations early.
+///
+/// **It is critical to not shade this class**, otherwise the literals used for reflection
+/// (ie `io.netty.util.internal.PlatformDependent`) will be shaded too, so instead of looking for
+/// `io.grpc.netty.shaded.io.netty.util.internal.PlatformDependent`, as you may think reading this class, the shaded
+/// version of this class will look for
+/// `io.grpc.netty.shaded.org.apache.pinot.shaded.io.netty.util.internal.PlatformDependent`.
+/// At the moment this is written, Pinot _does not_ shade Netty, so it is safe. Just to be sure, this class should be
+/// excluded in the maven shade plugin configuration (see pom.xml on the root of the project).
+public abstract class NettyInstance {
+ private static final Logger LOGGER = LoggerFactory.getLogger(NettyInstance.class);
+ private static final Constructor<DummyClass> CONSTRUCTOR;
+
+ static {
+ try {
+ CONSTRUCTOR = DummyClass.class.getConstructor();
+ } catch (NoSuchMethodException e) {
+ throw new RuntimeException("This should never happen, DummyClass has a default constructor", e);
+ }
+ }
+
+ /// The name of the Netty instance. It will be used on logs but also on metric names, so it should be
+ /// something short and without spaces like "Unshaded", "Pinot", "gRPC".
+ /// Add underscores if you need to separate words, but avoid other special characters.
+ public abstract String getName();
+
+ public abstract String getShadePrefix();
+
+ public abstract boolean isExplicitTryReflectionSetAccessible();
+
+ public abstract long getUsedDirectMemory();
+
+ public abstract long getMaxDirectMemory();
+
+ public abstract long getEstimateMaxDirectMemory();
+
+ public static class UnshadedNettyInstance extends NettyInstance {
+ @Override
+ public String getName() {
+ return "unshaded";
+ }
+
+ @Override
+ public String getShadePrefix() {
+ return "";
+ }
+
+ @Override
+ public boolean isExplicitTryReflectionSetAccessible() {
+ return io.netty.util.internal.ReflectionUtil.trySetAccessible(CONSTRUCTOR, true) == null;
Review Comment:
This is still tricky: instead of calling a getter (which is private in Netty), we call the method they use to check whether they can call the private constructors of ByteBuffers. As long as Netty keeps using the same code (which seems very likely), this should be a safe, reflection-free way to know whether Netty can use offheap buffers or not.
--
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] Better netty inspection [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #17674:
URL: https://github.com/apache/pinot/pull/17674#discussion_r2821617933
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/NettyInspector.java:
##########
@@ -0,0 +1,158 @@
+/**
+ * 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.core.transport;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.function.Function;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.metrics.AbstractMetrics;
+import org.apache.pinot.spi.utils.DataSizeUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/// Utility class to inspect Netty constants and log their values, with the ability to check for specific conditions
+/// and log warnings if they are not met.
+public class NettyInspector {
+ private static final Logger LOGGER = LoggerFactory.getLogger(NettyInspector.class);
+ /// We use a CopyOnWriteArrayList to allow dynamic addition of checks at runtime, if needed.
+ public static final CopyOnWriteArrayList<Check> CHECKS;
+ /// We use a CopyOnWriteArrayList to allow dynamic addition of Netty instances at runtime,
+ /// if needed (e.g. if we want to support other shaded versions of Netty).
+ public static final CopyOnWriteArrayList<NettyInstance> KNOWN_INSTANCES;
+
+ static {
+ CHECKS = new CopyOnWriteArrayList<>(
+ new Check[] {
+ NettyInspector::checkDirectMemory
+ }
+ );
+ KNOWN_INSTANCES = new CopyOnWriteArrayList<>(new NettyInstance[] {
+ new NettyInstance.UnshadedNettyInstance(),
+ new NettyInstance.GrpcNettyInstance()
+ });
+ }
+
+ private NettyInspector() {
+ // Private constructor to prevent instantiation
+ }
+
+ public static void registerMetrics(AbstractMetrics<?, ?, ?, ?> metrics) {
+ for (NettyInstance instance : KNOWN_INSTANCES) {
+ metrics.setOrUpdateGauge(instance.getName() + "NettyDirectMemoryUsed",
+ instance::getUsedDirectMemory);
+ metrics.setOrUpdateGauge(instance.getName() + "NettyDirectMemoryMax",
+ instance::getMaxDirectMemory);
+ }
+ }
+
+ /// Logs the memory used by each known Netty instance as long as the max memory it can use.
+ /// It also logs the total memory used by all Netty instances and its max memory.
+ public static void logMemory() {
Review Comment:
Removed
--
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] Better netty inspection [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #17674:
URL: https://github.com/apache/pinot/pull/17674
--
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] Better netty inspection [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #17674:
URL: https://github.com/apache/pinot/pull/17674#discussion_r2792326041
##########
pom.xml:
##########
@@ -2929,6 +2929,9 @@
<!-- Solve NoClassDefFoundError. Borrowed from https://github.com/prometheus/jmx_exporter/issues/802 -->
<exclude>META-INF/versions/9/org/yaml/snakeyaml/internal/**</exclude>
+
+ <!-- Exclude NettyInstacne because it includes Netty package literals -->
Review Comment:
This is as fragile as the other exclusions, right?
Anyway, I'm going to modify the PR because I'm wrong. I was sure we were shading Netty, but we don't do that, so we are double-safe
--
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