You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "markrmiller (via GitHub)" <gi...@apache.org> on 2026/06/09 16:09:23 UTC
[PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
markrmiller opened a new pull request, #4514:
URL: https://github.com/apache/solr/pull/4514
Load-average and memory circuit breakers were doing more harm than good under high concurrency.
LoadAverageCircuitBreaker called OperatingSystemMXBean.getSystemLoadAverage() on every request. The OS load average is a one-minute moving average, so re-polling it per-request is wasted work - and when many requests arrived concurrently, the syscall stampede hammered the CPU, which is the condition this breaker exists to prevent rather than cause.
MemoryCircuitBreaker sampled MemoryMXBean.getHeapMemoryUsage().getUsed() on a 30-second moving average. With a generational collector that signal climbs toward max between collections during normal operation; the breaker would trip on transient pre-GC peaks that GC was about to reclaim.
https://issues.apache.org/jira/browse/SOLR-18284
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470130832
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/TtlSampledMetric.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.solr.util.circuitbreaker;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+
+/**
+ * Single-flight, time-bounded cache around an expensive metric sample.
+ *
+ * <ul>
+ * <li><b>Fresh:</b> within the TTL window every caller returns the cached value with one volatile
+ * read.
+ * <li><b>Stale:</b> exactly one thread runs the underlying sampler at a time (chosen by CAS); all
+ * other concurrent callers immediately return the most recent published value rather than
+ * queueing behind the refresh or piling onto the sampler.
+ * <li><b>Cold (no sample yet):</b> guarded by a one-time {@code synchronized (this)} block so
+ * only the first caller computes; the rest see the result. The monitor is held across the
+ * sampler invocation, so concurrent first-callers will block — but this path runs at most
+ * once per instance, and only callers that arrive before any value has been published are
+ * affected.
+ * </ul>
+ *
+ * <p>Used by {@link LoadAverageCircuitBreaker} and {@link MemoryCircuitBreaker} so that high-QPS
+ * admission control cannot stampede the OS load-average syscall or the post-GC heap-pool walk: even
+ * under thousands of concurrent {@code isTripped()} callers, the underlying sampler is invoked at
+ * most once per TTL window.
+ *
+ * <p><b>Exception behavior:</b> if the {@code source} supplier throws, the exception propagates to
+ * the calling thread and no new sample is published. Any previously-published value remains and
+ * other concurrent callers continue to see it; the next caller to find the entry stale will retry
+ * the supplier. The {@code refreshing} flag is always released, so a thrown sampler does not wedge
+ * the single-flight latch.
+ */
+final class TtlSampledMetric<T> {
Review Comment:
Yes, Caffeine can do this: `refreshAfterWrite` provides a single-flight refresh that serves the stale value to other callers while one thread refreshes, which is the behavior I want.
But this is one value, not a keyed map. Caffeine's `refreshAfterWrite` also refreshes async. It uses `ForkJoinPool.commonPool()` unless you provide your own executor. This changes the timing from "at most one sample per window" to "at most one in-flight refresh that completes whenever the pool runs it." A heap-pool walk or load-average syscall ends up on the shared pool, which I didn't like. The reasons are similar to my concerns about using a simple Timer, refreshing async, and writing to a shared volatile. This method instead gives me the exact behavior I want on a critical hot path. I also did not want to allocate cache entries or wrap a key just for a single primitive.
So I considered it and decided to stay with the small class. It is about 50 lines that we have exact control over, with no new dependencies that force our use case onto the hot path. No extra executors, no extra threads, no nondeterministic timing.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470033924
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -17,75 +17,145 @@
package org.apache.solr.util.circuitbreaker;
-import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.lang.management.MemoryUsage;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
-import org.apache.solr.util.RefCounted;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
+import org.apache.solr.common.util.EnvUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Tracks the current JVM heap usage and triggers if a moving heap usage average over 30 seconds
- * exceeds the defined percentage of the maximum heap size allocated to the JVM. Once the average
- * memory usage goes below the threshold, it will start allowing queries again.
+ * Trips when post-collection live data in the JVM heap exceeds a configured percentage of the
+ * maximum heap size.
*
- * <p>The memory threshold is defined as a percentage of the maximum memory allocated -- see
- * memThreshold in <code>solrconfig.xml</code>.
+ * <p>The signal is read from {@link MemoryPoolMXBean#getCollectionUsage()} on the old/tenured heap
+ * pool, which reports memory usage immediately after the most recent collection that affected that
+ * pool. This is the only memory reading that distinguishes "live data" from "garbage waiting to be
+ * collected."
+ *
+ * <p>Earlier versions of this breaker sampled {@link MemoryMXBean#getHeapMemoryUsage()} on a
Review Comment:
Partly, but there is a reason to keep a migration note. This is a plugin point - it's public, configured in `solrconfig.xml`, and subclassable. And it still carries the deprecated `MemoryCircuitBreaker(int, int)` constructor for subclasses of the old version (we are going to break them, but I sided with a warning log message over a hard break, since neither option seemed great). Anyone who tuned a threshold against the 30-second average, or extended it, will see different trip behavior now. "What changed and why" helps them, so it is not pure change history.
I'll update it to make it a present-tense design point. I'll also add details about "earlier versions sampled X on a 30-second average" to the migration note in the changelog and to the javadoc for the deprecated constructor. This removes the retrospective from the class description but still warns users coming from the old version.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470079920
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -17,75 +17,145 @@
package org.apache.solr.util.circuitbreaker;
-import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.lang.management.MemoryUsage;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
-import org.apache.solr.util.RefCounted;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
+import org.apache.solr.common.util.EnvUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Tracks the current JVM heap usage and triggers if a moving heap usage average over 30 seconds
- * exceeds the defined percentage of the maximum heap size allocated to the JVM. Once the average
- * memory usage goes below the threshold, it will start allowing queries again.
+ * Trips when post-collection live data in the JVM heap exceeds a configured percentage of the
+ * maximum heap size.
*
- * <p>The memory threshold is defined as a percentage of the maximum memory allocated -- see
- * memThreshold in <code>solrconfig.xml</code>.
+ * <p>The signal is read from {@link MemoryPoolMXBean#getCollectionUsage()} on the old/tenured heap
+ * pool, which reports memory usage immediately after the most recent collection that affected that
+ * pool. This is the only memory reading that distinguishes "live data" from "garbage waiting to be
+ * collected."
+ *
+ * <p>Earlier versions of this breaker sampled {@link MemoryMXBean#getHeapMemoryUsage()} on a
+ * 30-second moving average, which produced a high signal during normal operation: with a
+ * generational collector, {@code used} climbs toward {@code max} between collections — that's the
+ * steady-state shape, not a problem. The new signal updates only when an old-gen GC runs, which is
+ * the only point at which "how full is the heap really?" has a defined answer.
+ *
+ * <p>Pool selection by collector:
+ *
+ * <ul>
+ * <li><b>G1 / Parallel / Serial / generational ZGC:</b> uses the pool whose name matches the
Review Comment:
Yes, this is the fix, not a side change. I will say that plainly in the JIRA and the class javadoc.
MemoryCircuitBreaker was agnostic to the pool and GC algorithms, but that was because it could do the wrong thing at times. The old breaker read `MemoryMXBean.getHeapMemoryUsage().getUsed()’. That is whole-heap occupancy, including garbage that has not yet been collected. On a generational collector, that number climbs toward `max` between collections during normal operation. That is the steady-state shape of a healthy JVM, not heap pressure to trigger a circuit breaker. Using 99% of the heap is meaningless - that can be 1 second before a harmless GC drops it to 10%. The breaker tripped on pre-GC peaks that the next collection was about to reclaim.
`getCollectionUsage()` on the old-gen pool reports occupancy right after the last collection - live data, not garbage, not live data + garbage. That is the only reading that answers "Is the heap actually full?"
`getCollectionUsage()` is per-pool, so you have to pick a pool, and the one you want is the old/tenured generation where long-lived data is, and each collector names that differently:
- G1 → `G1 Old Gen`
- Parallel → `PS Old Gen`
- Serial → `Tenured Gen`
- generational ZGC → `ZGC Old Generation`
Those match the `\b(Old|Tenured)\b` name pattern, so the circuit breaker reads `getCollectionUsage()` on that pool. Non-generational ZGC and Shenandoah are different, and some yet to be introduced collectors may be too - they have one combined heap pool and no old-gen name to match. When nothing matches the pattern, the fallback sums `getCollectionUsage()` across every `HEAP`-typed pool instead. So the name match plus fallback is there because the pool layout changes depending on the collector.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470038573
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -17,75 +17,145 @@
package org.apache.solr.util.circuitbreaker;
-import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.lang.management.MemoryUsage;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
-import org.apache.solr.util.RefCounted;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
+import org.apache.solr.common.util.EnvUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Tracks the current JVM heap usage and triggers if a moving heap usage average over 30 seconds
- * exceeds the defined percentage of the maximum heap size allocated to the JVM. Once the average
- * memory usage goes below the threshold, it will start allowing queries again.
+ * Trips when post-collection live data in the JVM heap exceeds a configured percentage of the
Review Comment:
Agree, I'll use "post-GC".
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3469930748
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/TtlSampledMetric.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.solr.util.circuitbreaker;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+
+/**
+ * Single-flight, time-bounded cache around an expensive metric sample.
+ *
+ * <ul>
Review Comment:
You are right. get() is the whole surface, and it exposes none of that split. I will drop the taxonomy from the class javadoc and keep those notes as inline comments on each branch in get(), where most of them already are. The class javadoc will just state the contract: a time-bounded single-flight cache; the sampler runs at most once per TTL window, even under a concurrent stampede.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470130832
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/TtlSampledMetric.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.solr.util.circuitbreaker;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+
+/**
+ * Single-flight, time-bounded cache around an expensive metric sample.
+ *
+ * <ul>
+ * <li><b>Fresh:</b> within the TTL window every caller returns the cached value with one volatile
+ * read.
+ * <li><b>Stale:</b> exactly one thread runs the underlying sampler at a time (chosen by CAS); all
+ * other concurrent callers immediately return the most recent published value rather than
+ * queueing behind the refresh or piling onto the sampler.
+ * <li><b>Cold (no sample yet):</b> guarded by a one-time {@code synchronized (this)} block so
+ * only the first caller computes; the rest see the result. The monitor is held across the
+ * sampler invocation, so concurrent first-callers will block — but this path runs at most
+ * once per instance, and only callers that arrive before any value has been published are
+ * affected.
+ * </ul>
+ *
+ * <p>Used by {@link LoadAverageCircuitBreaker} and {@link MemoryCircuitBreaker} so that high-QPS
+ * admission control cannot stampede the OS load-average syscall or the post-GC heap-pool walk: even
+ * under thousands of concurrent {@code isTripped()} callers, the underlying sampler is invoked at
+ * most once per TTL window.
+ *
+ * <p><b>Exception behavior:</b> if the {@code source} supplier throws, the exception propagates to
+ * the calling thread and no new sample is published. Any previously-published value remains and
+ * other concurrent callers continue to see it; the next caller to find the entry stale will retry
+ * the supplier. The {@code refreshing} flag is always released, so a thrown sampler does not wedge
+ * the single-flight latch.
+ */
+final class TtlSampledMetric<T> {
Review Comment:
Yes, Caffeine can do this: `refreshAfterWrite` gives single-flight refresh that serves the stale value to other callers while one thread refreshes, and that is the behavior I want.
But this is one value, not a keyed map. Caffeine's `refreshAfterWrite` also refreshes asynchronously — on `ForkJoinPool.commonPool()` unless you wire in your own executor — so the timing shifts from "at most one sample per window" to "at most one in-flight refresh that lands whenever the pool runs it," and a heap-pool walk or load-average syscall ends up on a shared pool. I didn't like that for similar reasons to why I didn't like the idea of a simple Timer refreshing async and writing to a shared volatile - this instead give me the exact behavior I want on a critical hot path - and I also did not want to allocate cache entries or wrap a key for a single primitive.
So I considered it and stayed with the small class. It is about 50 lines that we have exact control over and no new dependency on the hot path.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3469925581
##########
changelog/unreleased/SOLR-18284-load-and-memory-circuit-breakers.yml:
##########
@@ -0,0 +1,8 @@
+# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
+title: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks
Review Comment:
Agree the entry is thin, though both terms are fairly standard IMO: "stampede" is well known from cache stampede and in distributed contexts, though sometimes "thundering herd" is also used, and "trip" is the normal word for a circuit breaker firing.
If there is an issue, it's in detail, not vocabulary I think. I will split it into two files though, roughly:
- `LoadAverageCircuitBreaker now caches its sampled value for a short TTL, so it stops re-polling the OS load average per request at high RPS.`
- `MemoryCircuitBreaker now measures post-GC live heap data, so it no longer trips when the heap is full of collectible garbage.`
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3454742751
##########
changelog/unreleased/SOLR-18284-load-and-memory-circuit-breakers.yml:
##########
@@ -0,0 +1,8 @@
+# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
+title: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks
Review Comment:
[0] "stampede"? "trip"? I understand what this is pointing to, but IMO there's not quite enough information here for a user reading our changelog to judge whether they're impacted or whether they need to read more.
IMO we'd be better off breaking this into two changelog entries (i.e. totally separate files) that give a tad more detail on the negative behavior users will no longer see.
e.g.
> LoadAverageCircuitBreaker now uses cached metric values to avoid consuming unnecessary CPU at high RPS.
and...
> MemoryCircuitBreaker now looks at post-GC heap utilization to avoid false-positives when the heap is filled with collectible garbage.
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/TtlSampledMetric.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.solr.util.circuitbreaker;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+
+/**
+ * Single-flight, time-bounded cache around an expensive metric sample.
+ *
+ * <ul>
Review Comment:
[Q] What is this list representing and does it belong in Javadocs? AFAICT this class doesn't expose the fresh/stale/cold distinction in any way, so it's largely an implementation detail?
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -17,75 +17,145 @@
package org.apache.solr.util.circuitbreaker;
-import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.lang.management.MemoryUsage;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
-import org.apache.solr.util.RefCounted;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
+import org.apache.solr.common.util.EnvUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Tracks the current JVM heap usage and triggers if a moving heap usage average over 30 seconds
- * exceeds the defined percentage of the maximum heap size allocated to the JVM. Once the average
- * memory usage goes below the threshold, it will start allowing queries again.
+ * Trips when post-collection live data in the JVM heap exceeds a configured percentage of the
+ * maximum heap size.
*
- * <p>The memory threshold is defined as a percentage of the maximum memory allocated -- see
- * memThreshold in <code>solrconfig.xml</code>.
+ * <p>The signal is read from {@link MemoryPoolMXBean#getCollectionUsage()} on the old/tenured heap
+ * pool, which reports memory usage immediately after the most recent collection that affected that
+ * pool. This is the only memory reading that distinguishes "live data" from "garbage waiting to be
+ * collected."
+ *
+ * <p>Earlier versions of this breaker sampled {@link MemoryMXBean#getHeapMemoryUsage()} on a
+ * 30-second moving average, which produced a high signal during normal operation: with a
+ * generational collector, {@code used} climbs toward {@code max} between collections — that's the
+ * steady-state shape, not a problem. The new signal updates only when an old-gen GC runs, which is
+ * the only point at which "how full is the heap really?" has a defined answer.
+ *
+ * <p>Pool selection by collector:
+ *
+ * <ul>
+ * <li><b>G1 / Parallel / Serial / generational ZGC:</b> uses the pool whose name matches the
+ * word-boundary pattern {@code \b(Old|Tenured)\b}.
+ * <li><b>Non-generational ZGC and Shenandoah:</b> single combined heap pool — the breaker sums
+ * {@code getCollectionUsage()} across every {@link MemoryType#HEAP} pool instead.
+ * </ul>
+ *
+ * <p>Pre-first-GC, {@link MemoryPoolMXBean#getCollectionUsage()} can return {@code null} on every
+ * pool; in that case the breaker reports {@code 0} live bytes and will not trip until the JVM has
+ * performed at least one collection on a heap pool.
+ *
+ * <p>The threshold semantics are unchanged: configure a percentage of the maximum heap size, and
Review Comment:
[0] "semantics are unchanged" - Similar to my comment on L44 above, references to how things _used to behave_ or past history are comprehensible in this PR but won't make much sense six months from now. Change-history is best left out of Javadocs IMO
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -17,75 +17,145 @@
package org.apache.solr.util.circuitbreaker;
-import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.lang.management.MemoryUsage;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
-import org.apache.solr.util.RefCounted;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
+import org.apache.solr.common.util.EnvUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Tracks the current JVM heap usage and triggers if a moving heap usage average over 30 seconds
- * exceeds the defined percentage of the maximum heap size allocated to the JVM. Once the average
- * memory usage goes below the threshold, it will start allowing queries again.
+ * Trips when post-collection live data in the JVM heap exceeds a configured percentage of the
+ * maximum heap size.
*
- * <p>The memory threshold is defined as a percentage of the maximum memory allocated -- see
- * memThreshold in <code>solrconfig.xml</code>.
+ * <p>The signal is read from {@link MemoryPoolMXBean#getCollectionUsage()} on the old/tenured heap
+ * pool, which reports memory usage immediately after the most recent collection that affected that
+ * pool. This is the only memory reading that distinguishes "live data" from "garbage waiting to be
+ * collected."
+ *
+ * <p>Earlier versions of this breaker sampled {@link MemoryMXBean#getHeapMemoryUsage()} on a
Review Comment:
[Q] Is there a reason that a javadocs reader would care what the behavior used to be? This feels like LLM output that could be probably be trimmed back
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/TtlSampledMetric.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.solr.util.circuitbreaker;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+
+/**
+ * Single-flight, time-bounded cache around an expensive metric sample.
+ *
+ * <ul>
+ * <li><b>Fresh:</b> within the TTL window every caller returns the cached value with one volatile
+ * read.
+ * <li><b>Stale:</b> exactly one thread runs the underlying sampler at a time (chosen by CAS); all
+ * other concurrent callers immediately return the most recent published value rather than
+ * queueing behind the refresh or piling onto the sampler.
+ * <li><b>Cold (no sample yet):</b> guarded by a one-time {@code synchronized (this)} block so
+ * only the first caller computes; the rest see the result. The monitor is held across the
+ * sampler invocation, so concurrent first-callers will block — but this path runs at most
+ * once per instance, and only callers that arrive before any value has been published are
+ * affected.
+ * </ul>
+ *
+ * <p>Used by {@link LoadAverageCircuitBreaker} and {@link MemoryCircuitBreaker} so that high-QPS
+ * admission control cannot stampede the OS load-average syscall or the post-GC heap-pool walk: even
+ * under thousands of concurrent {@code isTripped()} callers, the underlying sampler is invoked at
+ * most once per TTL window.
+ *
+ * <p><b>Exception behavior:</b> if the {@code source} supplier throws, the exception propagates to
+ * the calling thread and no new sample is published. Any previously-published value remains and
+ * other concurrent callers continue to see it; the next caller to find the entry stale will retry
+ * the supplier. The {@code refreshing} flag is always released, so a thrown sampler does not wedge
+ * the single-flight latch.
+ */
+final class TtlSampledMetric<T> {
Review Comment:
[Q] The implementation here is great and I applaud your concurrency code. But I'm a little surprised that we'd have to implement this functionality ourselves and that there's nothing "off the shelf" that we could use instead. Surely we're not the first folks out there to calculate primitives with a TTL.
Did you consider (and discard) the idea of using Caffeine or something similar here instead? Or might that be worth considering?
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -96,19 +166,51 @@ public MemoryCircuitBreaker setThreshold(double thresholdValueInPercentage) {
@Override
public boolean isTripped() {
-
- long localAllowedMemory = getCurrentMemoryThreshold();
+ long localAllowedMemory = heapMemoryThreshold;
long localSeenMemory = getAvgMemoryUsage();
allowedMemory.set(localAllowedMemory);
-
seenMemory.set(localSeenMemory);
- return (localSeenMemory >= localAllowedMemory);
+ return localSeenMemory >= localAllowedMemory;
}
+ /**
+ * Returns post-GC live bytes for use by {@link #isTripped()}, cached for {@link
+ * CircuitBreakerRegistry#SYSPROP_SAMPLE_TTL_MS} ms so high-QPS callers don't repeatedly walk the
+ * heap pool list.
+ *
+ * <p>The historical name is preserved for source-compatibility. Tests that need to inject a
+ * synthetic value typically override this method directly (which bypasses the cache); tests that
+ * want to feed a synthetic sample <em>through</em> the cache should override {@link
+ * #samplePostGcLiveBytes()} instead. The implementation no longer averages anything.
+ */
Review Comment:
[0] Can you expand a bit more on the rationale for retaining the now-incorrect name? I understand the bit about having tests that override this method, but seemingly those tests could be updated if we decided to change the name of this method?
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -17,75 +17,145 @@
package org.apache.solr.util.circuitbreaker;
-import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.lang.management.MemoryUsage;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
-import org.apache.solr.util.RefCounted;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
+import org.apache.solr.common.util.EnvUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Tracks the current JVM heap usage and triggers if a moving heap usage average over 30 seconds
- * exceeds the defined percentage of the maximum heap size allocated to the JVM. Once the average
- * memory usage goes below the threshold, it will start allowing queries again.
+ * Trips when post-collection live data in the JVM heap exceeds a configured percentage of the
+ * maximum heap size.
*
- * <p>The memory threshold is defined as a percentage of the maximum memory allocated -- see
- * memThreshold in <code>solrconfig.xml</code>.
+ * <p>The signal is read from {@link MemoryPoolMXBean#getCollectionUsage()} on the old/tenured heap
+ * pool, which reports memory usage immediately after the most recent collection that affected that
+ * pool. This is the only memory reading that distinguishes "live data" from "garbage waiting to be
+ * collected."
+ *
+ * <p>Earlier versions of this breaker sampled {@link MemoryMXBean#getHeapMemoryUsage()} on a
+ * 30-second moving average, which produced a high signal during normal operation: with a
+ * generational collector, {@code used} climbs toward {@code max} between collections — that's the
+ * steady-state shape, not a problem. The new signal updates only when an old-gen GC runs, which is
+ * the only point at which "how full is the heap really?" has a defined answer.
+ *
+ * <p>Pool selection by collector:
+ *
+ * <ul>
+ * <li><b>G1 / Parallel / Serial / generational ZGC:</b> uses the pool whose name matches the
Review Comment:
[Q] Can you provide a bit more context on this and the stuff at L75-L112 below? Specifically: what value does this differentiation serve? MemoryCircuitBreaker was agnostic of pool and GC-algorithm prior to this PR and there's nothing in SOLR-18284 that points to that being an issue.
Is this a fix for an additional bug that you found while working on this PR? If so, is that additional bug documented somewhere?
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -17,75 +17,145 @@
package org.apache.solr.util.circuitbreaker;
-import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.lang.management.MemoryUsage;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
-import org.apache.solr.util.RefCounted;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
+import org.apache.solr.common.util.EnvUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Tracks the current JVM heap usage and triggers if a moving heap usage average over 30 seconds
- * exceeds the defined percentage of the maximum heap size allocated to the JVM. Once the average
- * memory usage goes below the threshold, it will start allowing queries again.
+ * Trips when post-collection live data in the JVM heap exceeds a configured percentage of the
Review Comment:
[0] "collection" is a pretty overloaded term in Solr; consider rephrasing to indicate garbage-collection more explicitly
##########
solr/core/src/test/org/apache/solr/util/circuitbreaker/TestHeapPressureCircuitBreakers.java:
##########
@@ -0,0 +1,115 @@
+/*
+ * 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.solr.util.circuitbreaker;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.solr.SolrTestCase;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+/**
+ * Verifies that {@link MemoryCircuitBreaker} reads post-GC live data — not the raw, garbage-
+ * inflated heap usage — so it does not trip on transient pre-collection peaks that GC will reclaim.
+ */
+public class TestHeapPressureCircuitBreakers extends SolrTestCase {
+
+ @Before
+ public void setUpProps() {
+ System.setProperty(CircuitBreakerRegistry.SYSPROP_SAMPLE_TTL_MS, "0");
+ }
+
+ @After
+ public void tearDownProps() {
+ System.clearProperty(CircuitBreakerRegistry.SYSPROP_SAMPLE_TTL_MS);
+ }
+
+ @Test
+ public void memoryBreakerReadsRealHeapPools() {
+ // Sanity: the in-process JVM exposes at least one heap pool. samplePostGcLiveBytes() must
+ // return a non-negative value computed from real pool data, regardless of which collector
+ // is active. (Pre-first-GC, getCollectionUsage() can be zero on every pool — accept that.)
+ boolean hasHeapPool = false;
+ for (MemoryPoolMXBean pool : ManagementFactory.getMemoryPoolMXBeans()) {
+ if (pool.getType() == MemoryType.HEAP) {
+ hasHeapPool = true;
+ break;
+ }
+ }
+ assertTrue("JVM must expose at least one HEAP MemoryPoolMXBean", hasHeapPool);
+
+ long bytes = new MemoryCircuitBreaker().samplePostGcLiveBytes();
+ assertTrue("post-GC live bytes must be non-negative, got " + bytes, bytes >= 0);
+ }
+
+ @Test
+ public void memoryBreakerTripsWhenOverThreshold() {
+ MemoryCircuitBreaker breaker =
+ new MemoryCircuitBreaker() {
+ @Override
+ protected long getAvgMemoryUsage() {
+ return Long.MAX_VALUE; // simulate "heap is exhausted post-GC"
+ }
+ };
+ breaker.setThreshold(50.0);
+ assertTrue(breaker.isTripped());
+ }
+
+ @Test
+ public void memoryBreakerDoesNotTripWhenUnderThreshold() {
+ MemoryCircuitBreaker breaker =
+ new MemoryCircuitBreaker() {
+ @Override
+ protected long getAvgMemoryUsage() {
+ return 0L; // simulate "post-GC live data is empty"
+ }
+ };
+ breaker.setThreshold(50.0);
+ assertFalse(breaker.isTripped());
+ }
+
+ @Test
+ public void samplePostGcLiveBytesIsCachedThroughGetAvgMemoryUsage() {
+ // With a long TTL, getAvgMemoryUsage() should consult samplePostGcLiveBytes() at most once
+ // across many isTripped() invocations — i.e. the cache wraps the overridable hook, not just
+ // the static heap-pool walk.
+ System.setProperty(CircuitBreakerRegistry.SYSPROP_SAMPLE_TTL_MS, "60000");
+ try {
+ AtomicInteger calls = new AtomicInteger();
+ MemoryCircuitBreaker breaker =
+ new MemoryCircuitBreaker() {
+ @Override
+ protected long samplePostGcLiveBytes() {
+ calls.incrementAndGet();
+ return 0L;
+ }
+ };
+ breaker.setThreshold(50.0);
+ for (int i = 0; i < 50; i++) {
+ breaker.isTripped();
+ }
+ assertEquals(
+ "samplePostGcLiveBytes() must be invoked at most once per TTL window", 1, calls.get());
+ } finally {
+ System.setProperty(CircuitBreakerRegistry.SYSPROP_SAMPLE_TTL_MS, "0");
Review Comment:
[0] This can probably be dropped given the `@After` method above that clears this between test methods (and the test-rule in `SolrTestCase` that clears it between test-classes).
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470079920
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -17,75 +17,145 @@
package org.apache.solr.util.circuitbreaker;
-import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.lang.management.MemoryUsage;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
-import org.apache.solr.util.RefCounted;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
+import org.apache.solr.common.util.EnvUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Tracks the current JVM heap usage and triggers if a moving heap usage average over 30 seconds
- * exceeds the defined percentage of the maximum heap size allocated to the JVM. Once the average
- * memory usage goes below the threshold, it will start allowing queries again.
+ * Trips when post-collection live data in the JVM heap exceeds a configured percentage of the
+ * maximum heap size.
*
- * <p>The memory threshold is defined as a percentage of the maximum memory allocated -- see
- * memThreshold in <code>solrconfig.xml</code>.
+ * <p>The signal is read from {@link MemoryPoolMXBean#getCollectionUsage()} on the old/tenured heap
+ * pool, which reports memory usage immediately after the most recent collection that affected that
+ * pool. This is the only memory reading that distinguishes "live data" from "garbage waiting to be
+ * collected."
+ *
+ * <p>Earlier versions of this breaker sampled {@link MemoryMXBean#getHeapMemoryUsage()} on a
+ * 30-second moving average, which produced a high signal during normal operation: with a
+ * generational collector, {@code used} climbs toward {@code max} between collections — that's the
+ * steady-state shape, not a problem. The new signal updates only when an old-gen GC runs, which is
+ * the only point at which "how full is the heap really?" has a defined answer.
+ *
+ * <p>Pool selection by collector:
+ *
+ * <ul>
+ * <li><b>G1 / Parallel / Serial / generational ZGC:</b> uses the pool whose name matches the
Review Comment:
Yes, this is the fix, not a side change. I will say that plainly in the JIRA and the class javadoc.
MemoryCircuitBreaker was agnostic to the pool and GC algorithms, but that was because it could do the wrong thing at times. The old breaker read `MemoryMXBean.getHeapMemoryUsage().getUsed()’. That is whole-heap occupancy, including garbage that has not yet been collected. On a generational collector, that number climbs toward `max` between collections during normal operation. That is the steady-state shape of a healthy JVM, not heap pressure to trigger a circuit breaker. Using 99% of the heap is meaningless - that can be 1 second before a harmless GC drops it to 10%. The breaker tripped on pre-GC peaks that the next collection was about to reclaim.
`getCollectionUsage()` on the old-gen pool reports occupancy right after the last collection - live data, not garbage, not live data + garbage. That is the only reading that answers "Is the heap actually full?"
`getCollectionUsage()` is per-pool, so you have to pick a pool, and the one you want is the old/tenured generation where long-lived data is, and each collector names that differently:
- G1 → `G1 Old Gen`
- Parallel → `PS Old Gen`
- Serial → `Tenured Gen`
- generational ZGC → `ZGC Old Generation`
Those match the `\b(Old|Tenured)\b` name pattern, so the circuit breaker reads getCollectionUsage() on that pool. Non-generational ZGC and Shenandoah are different, and some yet to be introduced collectors may be too - they have one combined heap pool and no old-gen name to match. When nothing matches the pattern, the fallback sums `getCollectionUsage()` across every `HEAP`-typed pool instead. So the name match plus fallback is there because the pool layout changes depending on the collector.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470082135
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -17,75 +17,145 @@
package org.apache.solr.util.circuitbreaker;
-import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.lang.management.MemoryUsage;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
-import org.apache.solr.util.RefCounted;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
+import org.apache.solr.common.util.EnvUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Tracks the current JVM heap usage and triggers if a moving heap usage average over 30 seconds
- * exceeds the defined percentage of the maximum heap size allocated to the JVM. Once the average
- * memory usage goes below the threshold, it will start allowing queries again.
+ * Trips when post-collection live data in the JVM heap exceeds a configured percentage of the
+ * maximum heap size.
*
- * <p>The memory threshold is defined as a percentage of the maximum memory allocated -- see
- * memThreshold in <code>solrconfig.xml</code>.
+ * <p>The signal is read from {@link MemoryPoolMXBean#getCollectionUsage()} on the old/tenured heap
+ * pool, which reports memory usage immediately after the most recent collection that affected that
+ * pool. This is the only memory reading that distinguishes "live data" from "garbage waiting to be
+ * collected."
+ *
+ * <p>Earlier versions of this breaker sampled {@link MemoryMXBean#getHeapMemoryUsage()} on a
+ * 30-second moving average, which produced a high signal during normal operation: with a
+ * generational collector, {@code used} climbs toward {@code max} between collections — that's the
+ * steady-state shape, not a problem. The new signal updates only when an old-gen GC runs, which is
+ * the only point at which "how full is the heap really?" has a defined answer.
+ *
+ * <p>Pool selection by collector:
+ *
+ * <ul>
+ * <li><b>G1 / Parallel / Serial / generational ZGC:</b> uses the pool whose name matches the
+ * word-boundary pattern {@code \b(Old|Tenured)\b}.
+ * <li><b>Non-generational ZGC and Shenandoah:</b> single combined heap pool — the breaker sums
+ * {@code getCollectionUsage()} across every {@link MemoryType#HEAP} pool instead.
+ * </ul>
+ *
+ * <p>Pre-first-GC, {@link MemoryPoolMXBean#getCollectionUsage()} can return {@code null} on every
+ * pool; in that case the breaker reports {@code 0} live bytes and will not trip until the JVM has
+ * performed at least one collection on a heap pool.
+ *
+ * <p>The threshold semantics are unchanged: configure a percentage of the maximum heap size, and
Review Comment:
Same response as above.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470130832
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/TtlSampledMetric.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.solr.util.circuitbreaker;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+
+/**
+ * Single-flight, time-bounded cache around an expensive metric sample.
+ *
+ * <ul>
+ * <li><b>Fresh:</b> within the TTL window every caller returns the cached value with one volatile
+ * read.
+ * <li><b>Stale:</b> exactly one thread runs the underlying sampler at a time (chosen by CAS); all
+ * other concurrent callers immediately return the most recent published value rather than
+ * queueing behind the refresh or piling onto the sampler.
+ * <li><b>Cold (no sample yet):</b> guarded by a one-time {@code synchronized (this)} block so
+ * only the first caller computes; the rest see the result. The monitor is held across the
+ * sampler invocation, so concurrent first-callers will block — but this path runs at most
+ * once per instance, and only callers that arrive before any value has been published are
+ * affected.
+ * </ul>
+ *
+ * <p>Used by {@link LoadAverageCircuitBreaker} and {@link MemoryCircuitBreaker} so that high-QPS
+ * admission control cannot stampede the OS load-average syscall or the post-GC heap-pool walk: even
+ * under thousands of concurrent {@code isTripped()} callers, the underlying sampler is invoked at
+ * most once per TTL window.
+ *
+ * <p><b>Exception behavior:</b> if the {@code source} supplier throws, the exception propagates to
+ * the calling thread and no new sample is published. Any previously-published value remains and
+ * other concurrent callers continue to see it; the next caller to find the entry stale will retry
+ * the supplier. The {@code refreshing} flag is always released, so a thrown sampler does not wedge
+ * the single-flight latch.
+ */
+final class TtlSampledMetric<T> {
Review Comment:
Yes, Caffeine can do this: `refreshAfterWrite` gives single-flight refresh that serves the stale value to other callers while one thread refreshes, and that is the behavior I want.
But this is one value, not a keyed map. Caffeine's `refreshAfterWrite` also refreshes async - on `ForkJoinPool.commonPool()` unless you wire in your own executor - which shifts the timing from "at most one sample per window" to "at most one in-flight refresh that lands whenever the pool runs it," and a heap-pool walk or load-average syscall ends up on a shared pool. I didn't like that for similar reasons to why I didn't like the idea of a simple Timer refreshing async and writing to a shared volatile - this, instead, gives me the exact behavior I want on a critical hot path - and I also did not want to allocate cache entries or wrap a key for a single primitive.
So I considered it and stayed with the small class. It is about 50 lines that we have exact control over and no new dependency that we have to force our use case on on the hot path. No extra executors, no extra threads, no non deterministic timing.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on PR #4514:
URL: https://github.com/apache/solr/pull/4514#issuecomment-4827707926
FYI, you can also click the force-pushed link to see the changes between the previous branch and the new branch.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470111120
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -96,19 +166,51 @@ public MemoryCircuitBreaker setThreshold(double thresholdValueInPercentage) {
@Override
public boolean isTripped() {
-
- long localAllowedMemory = getCurrentMemoryThreshold();
+ long localAllowedMemory = heapMemoryThreshold;
long localSeenMemory = getAvgMemoryUsage();
allowedMemory.set(localAllowedMemory);
-
seenMemory.set(localSeenMemory);
- return (localSeenMemory >= localAllowedMemory);
+ return localSeenMemory >= localAllowedMemory;
}
+ /**
+ * Returns post-GC live bytes for use by {@link #isTripped()}, cached for {@link
+ * CircuitBreakerRegistry#SYSPROP_SAMPLE_TTL_MS} ms so high-QPS callers don't repeatedly walk the
+ * heap pool list.
+ *
+ * <p>The historical name is preserved for source-compatibility. Tests that need to inject a
+ * synthetic value typically override this method directly (which bypasses the cache); tests that
+ * want to feed a synthetic sample <em>through</em> the cache should override {@link
+ * #samplePostGcLiveBytes()} instead. The implementation no longer averages anything.
+ */
Review Comment:
The reason was the access level: it is `protected` on a `public` non-final class, so it is part of the subclassing surface for external breakers. Renaming is a source-compat break for a cosmetic gain. I don't care about changing it, but I was attempting to be as gentle as I could given this was made a plugin point (a choice I don't like at all, but once it's made, I like to err in the users favor when I couldn't care less myself.)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3474918353
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/TtlSampledMetric.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.solr.util.circuitbreaker;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+
+/**
+ * Single-flight, time-bounded cache around an expensive metric sample.
+ *
+ * <ul>
+ * <li><b>Fresh:</b> within the TTL window every caller returns the cached value with one volatile
+ * read.
+ * <li><b>Stale:</b> exactly one thread runs the underlying sampler at a time (chosen by CAS); all
+ * other concurrent callers immediately return the most recent published value rather than
+ * queueing behind the refresh or piling onto the sampler.
+ * <li><b>Cold (no sample yet):</b> guarded by a one-time {@code synchronized (this)} block so
+ * only the first caller computes; the rest see the result. The monitor is held across the
+ * sampler invocation, so concurrent first-callers will block — but this path runs at most
+ * once per instance, and only callers that arrive before any value has been published are
+ * affected.
+ * </ul>
+ *
+ * <p>Used by {@link LoadAverageCircuitBreaker} and {@link MemoryCircuitBreaker} so that high-QPS
+ * admission control cannot stampede the OS load-average syscall or the post-GC heap-pool walk: even
+ * under thousands of concurrent {@code isTripped()} callers, the underlying sampler is invoked at
+ * most once per TTL window.
+ *
+ * <p><b>Exception behavior:</b> if the {@code source} supplier throws, the exception propagates to
+ * the calling thread and no new sample is published. Any previously-published value remains and
+ * other concurrent callers continue to see it; the next caller to find the entry stale will retry
+ * the supplier. The {@code refreshing} flag is always released, so a thrown sampler does not wedge
+ * the single-flight latch.
+ */
+final class TtlSampledMetric<T> {
Review Comment:
Yeah, it's small in that it's 50 LOC and no-dependencies. But even concise concurrency code is complex.
If you've considered alternatives then I'm sure you've thought about it more than I have and I trust your judgement there. So, SGTM. Just wanted to make sure the tradeoff was called out 👍
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3474967333
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -17,75 +17,145 @@
package org.apache.solr.util.circuitbreaker;
-import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.lang.management.MemoryUsage;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
-import org.apache.solr.util.RefCounted;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
+import org.apache.solr.common.util.EnvUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Tracks the current JVM heap usage and triggers if a moving heap usage average over 30 seconds
- * exceeds the defined percentage of the maximum heap size allocated to the JVM. Once the average
- * memory usage goes below the threshold, it will start allowing queries again.
+ * Trips when post-collection live data in the JVM heap exceeds a configured percentage of the
+ * maximum heap size.
*
- * <p>The memory threshold is defined as a percentage of the maximum memory allocated -- see
- * memThreshold in <code>solrconfig.xml</code>.
+ * <p>The signal is read from {@link MemoryPoolMXBean#getCollectionUsage()} on the old/tenured heap
+ * pool, which reports memory usage immediately after the most recent collection that affected that
+ * pool. This is the only memory reading that distinguishes "live data" from "garbage waiting to be
+ * collected."
+ *
+ * <p>Earlier versions of this breaker sampled {@link MemoryMXBean#getHeapMemoryUsage()} on a
+ * 30-second moving average, which produced a high signal during normal operation: with a
+ * generational collector, {@code used} climbs toward {@code max} between collections — that's the
+ * steady-state shape, not a problem. The new signal updates only when an old-gen GC runs, which is
+ * the only point at which "how full is the heap really?" has a defined answer.
+ *
+ * <p>Pool selection by collector:
+ *
+ * <ul>
+ * <li><b>G1 / Parallel / Serial / generational ZGC:</b> uses the pool whose name matches the
Review Comment:
Gotcha - appreciate the clarification. So the "correct" metric is only exposed at the level of individual pools; makes more sense, thanks!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3469925581
##########
changelog/unreleased/SOLR-18284-load-and-memory-circuit-breakers.yml:
##########
@@ -0,0 +1,8 @@
+# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
+title: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks
Review Comment:
Agree the entry is thin, though both terms are fairly standard — "stampede" is well known from cache stampede and in distributed contexts, though sometimes "thundering herd" is also used, and "trip" is the normal word for a circuit breaker firing. The real gap is detail, not vocabulary I think. I will split it into two files, roughly:
- `LoadAverageCircuitBreaker now caches its sampled value for a short TTL, so it stops re-polling the OS load average per request at high RPS.`
- `MemoryCircuitBreaker now measures post-GC live heap data, so it no longer trips when the heap is full of collectible garbage.`
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470137905
##########
solr/core/src/test/org/apache/solr/util/circuitbreaker/TestHeapPressureCircuitBreakers.java:
##########
@@ -0,0 +1,115 @@
+/*
+ * 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.solr.util.circuitbreaker;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.solr.SolrTestCase;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+/**
+ * Verifies that {@link MemoryCircuitBreaker} reads post-GC live data — not the raw, garbage-
+ * inflated heap usage — so it does not trip on transient pre-collection peaks that GC will reclaim.
+ */
+public class TestHeapPressureCircuitBreakers extends SolrTestCase {
+
+ @Before
+ public void setUpProps() {
+ System.setProperty(CircuitBreakerRegistry.SYSPROP_SAMPLE_TTL_MS, "0");
+ }
+
+ @After
+ public void tearDownProps() {
+ System.clearProperty(CircuitBreakerRegistry.SYSPROP_SAMPLE_TTL_MS);
+ }
+
+ @Test
+ public void memoryBreakerReadsRealHeapPools() {
+ // Sanity: the in-process JVM exposes at least one heap pool. samplePostGcLiveBytes() must
+ // return a non-negative value computed from real pool data, regardless of which collector
+ // is active. (Pre-first-GC, getCollectionUsage() can be zero on every pool — accept that.)
+ boolean hasHeapPool = false;
+ for (MemoryPoolMXBean pool : ManagementFactory.getMemoryPoolMXBeans()) {
+ if (pool.getType() == MemoryType.HEAP) {
+ hasHeapPool = true;
+ break;
+ }
+ }
+ assertTrue("JVM must expose at least one HEAP MemoryPoolMXBean", hasHeapPool);
+
+ long bytes = new MemoryCircuitBreaker().samplePostGcLiveBytes();
+ assertTrue("post-GC live bytes must be non-negative, got " + bytes, bytes >= 0);
+ }
+
+ @Test
+ public void memoryBreakerTripsWhenOverThreshold() {
+ MemoryCircuitBreaker breaker =
+ new MemoryCircuitBreaker() {
+ @Override
+ protected long getAvgMemoryUsage() {
+ return Long.MAX_VALUE; // simulate "heap is exhausted post-GC"
+ }
+ };
+ breaker.setThreshold(50.0);
+ assertTrue(breaker.isTripped());
+ }
+
+ @Test
+ public void memoryBreakerDoesNotTripWhenUnderThreshold() {
+ MemoryCircuitBreaker breaker =
+ new MemoryCircuitBreaker() {
+ @Override
+ protected long getAvgMemoryUsage() {
+ return 0L; // simulate "post-GC live data is empty"
+ }
+ };
+ breaker.setThreshold(50.0);
+ assertFalse(breaker.isTripped());
+ }
+
+ @Test
+ public void samplePostGcLiveBytesIsCachedThroughGetAvgMemoryUsage() {
+ // With a long TTL, getAvgMemoryUsage() should consult samplePostGcLiveBytes() at most once
+ // across many isTripped() invocations — i.e. the cache wraps the overridable hook, not just
+ // the static heap-pool walk.
+ System.setProperty(CircuitBreakerRegistry.SYSPROP_SAMPLE_TTL_MS, "60000");
+ try {
+ AtomicInteger calls = new AtomicInteger();
+ MemoryCircuitBreaker breaker =
+ new MemoryCircuitBreaker() {
+ @Override
+ protected long samplePostGcLiveBytes() {
+ calls.incrementAndGet();
+ return 0L;
+ }
+ };
+ breaker.setThreshold(50.0);
+ for (int i = 0; i < 50; i++) {
+ breaker.isTripped();
+ }
+ assertEquals(
+ "samplePostGcLiveBytes() must be invoked at most once per TTL window", 1, calls.get());
+ } finally {
+ System.setProperty(CircuitBreakerRegistry.SYSPROP_SAMPLE_TTL_MS, "0");
Review Comment:
I'll drop it.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470130832
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/TtlSampledMetric.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.solr.util.circuitbreaker;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+
+/**
+ * Single-flight, time-bounded cache around an expensive metric sample.
+ *
+ * <ul>
+ * <li><b>Fresh:</b> within the TTL window every caller returns the cached value with one volatile
+ * read.
+ * <li><b>Stale:</b> exactly one thread runs the underlying sampler at a time (chosen by CAS); all
+ * other concurrent callers immediately return the most recent published value rather than
+ * queueing behind the refresh or piling onto the sampler.
+ * <li><b>Cold (no sample yet):</b> guarded by a one-time {@code synchronized (this)} block so
+ * only the first caller computes; the rest see the result. The monitor is held across the
+ * sampler invocation, so concurrent first-callers will block — but this path runs at most
+ * once per instance, and only callers that arrive before any value has been published are
+ * affected.
+ * </ul>
+ *
+ * <p>Used by {@link LoadAverageCircuitBreaker} and {@link MemoryCircuitBreaker} so that high-QPS
+ * admission control cannot stampede the OS load-average syscall or the post-GC heap-pool walk: even
+ * under thousands of concurrent {@code isTripped()} callers, the underlying sampler is invoked at
+ * most once per TTL window.
+ *
+ * <p><b>Exception behavior:</b> if the {@code source} supplier throws, the exception propagates to
+ * the calling thread and no new sample is published. Any previously-published value remains and
+ * other concurrent callers continue to see it; the next caller to find the entry stale will retry
+ * the supplier. The {@code refreshing} flag is always released, so a thrown sampler does not wedge
+ * the single-flight latch.
+ */
+final class TtlSampledMetric<T> {
Review Comment:
Caffeine can do this — `refreshAfterWrite` gives single-flight refresh that serves the stale value to other callers while one thread refreshes. That is the behavior I want.
But this is one value, not a keyed map. Caffeine's `refreshAfterWrite` also refreshes asynchronously — on `ForkJoinPool.commonPool()` unless you wire in your own executor — so the timing shifts from "at most one sample per window" to "at most one in-flight refresh that lands whenever the pool runs it," and a heap-pool walk or load-average syscall ends up on a shared pool. On this hot path I did not want to allocate cache entries or wrap a key for a single primitive.
So I considered it and stayed with the small class. It is about 50 lines that we have exact control over and no new dependency on the hot path.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470033924
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -17,75 +17,145 @@
package org.apache.solr.util.circuitbreaker;
-import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.lang.management.MemoryUsage;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
-import org.apache.solr.util.RefCounted;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
+import org.apache.solr.common.util.EnvUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Tracks the current JVM heap usage and triggers if a moving heap usage average over 30 seconds
- * exceeds the defined percentage of the maximum heap size allocated to the JVM. Once the average
- * memory usage goes below the threshold, it will start allowing queries again.
+ * Trips when post-collection live data in the JVM heap exceeds a configured percentage of the
+ * maximum heap size.
*
- * <p>The memory threshold is defined as a percentage of the maximum memory allocated -- see
- * memThreshold in <code>solrconfig.xml</code>.
+ * <p>The signal is read from {@link MemoryPoolMXBean#getCollectionUsage()} on the old/tenured heap
+ * pool, which reports memory usage immediately after the most recent collection that affected that
+ * pool. This is the only memory reading that distinguishes "live data" from "garbage waiting to be
+ * collected."
+ *
+ * <p>Earlier versions of this breaker sampled {@link MemoryMXBean#getHeapMemoryUsage()} on a
Review Comment:
Partly, but there is a reason to keep a migration note. This is a plugin point — public, configured in `solrconfig.xml`, subclassable, and it still carries the deprecated `MemoryCircuitBreaker(int, int)` constructor for subclasses of the old version (we are going to break them, but I sided with a warn logging message over a hard break just because neither option seemed great). Anyone who tuned a threshold against the 30-second average, or extended it, will see different trip behavior now. "What changed and why" helps them, so it is not pure change-history.
I'll update it to be more of a present-tense design point and beef up the "earlier versions sampled X on a 30-second average" migration note in the changelog and the deprecated constructor's javadoc. That drops the retrospective from the class description without losing the warning for people coming from the old version.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470130832
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/TtlSampledMetric.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.solr.util.circuitbreaker;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+
+/**
+ * Single-flight, time-bounded cache around an expensive metric sample.
+ *
+ * <ul>
+ * <li><b>Fresh:</b> within the TTL window every caller returns the cached value with one volatile
+ * read.
+ * <li><b>Stale:</b> exactly one thread runs the underlying sampler at a time (chosen by CAS); all
+ * other concurrent callers immediately return the most recent published value rather than
+ * queueing behind the refresh or piling onto the sampler.
+ * <li><b>Cold (no sample yet):</b> guarded by a one-time {@code synchronized (this)} block so
+ * only the first caller computes; the rest see the result. The monitor is held across the
+ * sampler invocation, so concurrent first-callers will block — but this path runs at most
+ * once per instance, and only callers that arrive before any value has been published are
+ * affected.
+ * </ul>
+ *
+ * <p>Used by {@link LoadAverageCircuitBreaker} and {@link MemoryCircuitBreaker} so that high-QPS
+ * admission control cannot stampede the OS load-average syscall or the post-GC heap-pool walk: even
+ * under thousands of concurrent {@code isTripped()} callers, the underlying sampler is invoked at
+ * most once per TTL window.
+ *
+ * <p><b>Exception behavior:</b> if the {@code source} supplier throws, the exception propagates to
+ * the calling thread and no new sample is published. Any previously-published value remains and
+ * other concurrent callers continue to see it; the next caller to find the entry stale will retry
+ * the supplier. The {@code refreshing} flag is always released, so a thrown sampler does not wedge
+ * the single-flight latch.
+ */
+final class TtlSampledMetric<T> {
Review Comment:
Yes, Caffeine can do this: `refreshAfterWrite` provides a single-flight refresh that serves the stale value to other callers while one thread refreshes, which is the behavior I want.
But this is one value, not a keyed map. Caffeine's `refreshAfterWrite` also refreshes async. It uses `ForkJoinPool.commonPool()` unless you provide your own executor. This changes the timing from "at most one sample per window" to "at most one in-flight refresh that completes whenever the pool runs it." A heap-pool walk or load-average syscall ends up on the shared pool, which I didn't like. The reasons are similar to my concerns about using a simple Timer, refreshing async, and writing to a shared volatile. This method instead gives me the exact behavior I want on a critical hot path. I also did not want to allocate cache entries or wrap a key just for a single primitive.
So I considered it and decided to stay with the small class. It is about 50 lines that we have exact control over, with no new dependencies that force our use case onto them on the hot path. No extra executors, no extra threads, no nondeterministic timing, no extra allocations.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470111120
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -96,19 +166,51 @@ public MemoryCircuitBreaker setThreshold(double thresholdValueInPercentage) {
@Override
public boolean isTripped() {
-
- long localAllowedMemory = getCurrentMemoryThreshold();
+ long localAllowedMemory = heapMemoryThreshold;
long localSeenMemory = getAvgMemoryUsage();
allowedMemory.set(localAllowedMemory);
-
seenMemory.set(localSeenMemory);
- return (localSeenMemory >= localAllowedMemory);
+ return localSeenMemory >= localAllowedMemory;
}
+ /**
+ * Returns post-GC live bytes for use by {@link #isTripped()}, cached for {@link
+ * CircuitBreakerRegistry#SYSPROP_SAMPLE_TTL_MS} ms so high-QPS callers don't repeatedly walk the
+ * heap pool list.
+ *
+ * <p>The historical name is preserved for source-compatibility. Tests that need to inject a
+ * synthetic value typically override this method directly (which bypasses the cache); tests that
+ * want to feed a synthetic sample <em>through</em> the cache should override {@link
+ * #samplePostGcLiveBytes()} instead. The implementation no longer averages anything.
+ */
Review Comment:
The reason was the access level: it is protected on a public non-final class, so it is part of the subclassing surface for external breakers. Renaming is a source-compat break for a cosmetic gain. I don't care about changing it, but I was attempting to be as gentle as I could, given this was made a plugin point (a choice I don't like at all, but once it's made, I like to err in the user's favor when I couldn't care less myself)
Basically, the choice was to hard break the plugin point or try and maintain some form of back company - neither option is great, but as you can see with the old constructor and warn logging, I chose one - and the only thing I wasn't going to do is go half and half.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470130832
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/TtlSampledMetric.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.solr.util.circuitbreaker;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+
+/**
+ * Single-flight, time-bounded cache around an expensive metric sample.
+ *
+ * <ul>
+ * <li><b>Fresh:</b> within the TTL window every caller returns the cached value with one volatile
+ * read.
+ * <li><b>Stale:</b> exactly one thread runs the underlying sampler at a time (chosen by CAS); all
+ * other concurrent callers immediately return the most recent published value rather than
+ * queueing behind the refresh or piling onto the sampler.
+ * <li><b>Cold (no sample yet):</b> guarded by a one-time {@code synchronized (this)} block so
+ * only the first caller computes; the rest see the result. The monitor is held across the
+ * sampler invocation, so concurrent first-callers will block — but this path runs at most
+ * once per instance, and only callers that arrive before any value has been published are
+ * affected.
+ * </ul>
+ *
+ * <p>Used by {@link LoadAverageCircuitBreaker} and {@link MemoryCircuitBreaker} so that high-QPS
+ * admission control cannot stampede the OS load-average syscall or the post-GC heap-pool walk: even
+ * under thousands of concurrent {@code isTripped()} callers, the underlying sampler is invoked at
+ * most once per TTL window.
+ *
+ * <p><b>Exception behavior:</b> if the {@code source} supplier throws, the exception propagates to
+ * the calling thread and no new sample is published. Any previously-published value remains and
+ * other concurrent callers continue to see it; the next caller to find the entry stale will retry
+ * the supplier. The {@code refreshing} flag is always released, so a thrown sampler does not wedge
+ * the single-flight latch.
+ */
+final class TtlSampledMetric<T> {
Review Comment:
Yes, Caffeine can do this: `refreshAfterWrite` provides a single-flight refresh that serves the stale value to other callers while one thread refreshes, which is largely the behavior I want.
But this is one value, not a keyed map. Caffeine's `refreshAfterWrite` also refreshes async. It uses `ForkJoinPool.commonPool()` unless you provide your own executor. This changes the timing from "at most one sample per window" to "at most one in-flight refresh that completes whenever the pool runs it." A heap-pool walk or load-average syscall ends up on the shared pool, which I didn't like. The reasons are similar to my concerns about using a simple Timer, refreshing async, and writing to a shared volatile. This method instead gives me the exact behavior I want on a critical hot path. I also did not want to allocate cache entries or wrap a key just for a single primitive.
So I considered it and decided to stay with the small class. It is about 50 lines that we have exact control over, with no new dependencies that force our use case onto them on the hot path. No extra executors, no extra threads, no nondeterministic timing, no extra allocations.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3469930748
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/TtlSampledMetric.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.solr.util.circuitbreaker;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+
+/**
+ * Single-flight, time-bounded cache around an expensive metric sample.
+ *
+ * <ul>
Review Comment:
You are right. `get()` is the whole surface, and it exposes none of that split. I will drop the taxonomy from the class javadoc and keep those notes as inline comments on each branch in `get()`, where most of them already are. The class javadoc will just state the contract: time-bounded single-flight cache, sampler runs at most once per TTL window even under a concurrent stampede.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470079920
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -17,75 +17,145 @@
package org.apache.solr.util.circuitbreaker;
-import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.lang.management.MemoryUsage;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
-import org.apache.solr.util.RefCounted;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
+import org.apache.solr.common.util.EnvUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Tracks the current JVM heap usage and triggers if a moving heap usage average over 30 seconds
- * exceeds the defined percentage of the maximum heap size allocated to the JVM. Once the average
- * memory usage goes below the threshold, it will start allowing queries again.
+ * Trips when post-collection live data in the JVM heap exceeds a configured percentage of the
+ * maximum heap size.
*
- * <p>The memory threshold is defined as a percentage of the maximum memory allocated -- see
- * memThreshold in <code>solrconfig.xml</code>.
+ * <p>The signal is read from {@link MemoryPoolMXBean#getCollectionUsage()} on the old/tenured heap
+ * pool, which reports memory usage immediately after the most recent collection that affected that
+ * pool. This is the only memory reading that distinguishes "live data" from "garbage waiting to be
+ * collected."
+ *
+ * <p>Earlier versions of this breaker sampled {@link MemoryMXBean#getHeapMemoryUsage()} on a
+ * 30-second moving average, which produced a high signal during normal operation: with a
+ * generational collector, {@code used} climbs toward {@code max} between collections — that's the
+ * steady-state shape, not a problem. The new signal updates only when an old-gen GC runs, which is
+ * the only point at which "how full is the heap really?" has a defined answer.
+ *
+ * <p>Pool selection by collector:
+ *
+ * <ul>
+ * <li><b>G1 / Parallel / Serial / generational ZGC:</b> uses the pool whose name matches the
Review Comment:
Yes, this is the fix, not a side change. I will say that plainly in the JIRA and the class javadoc.
MemoryCircuitBreaker was agnostic to the pool and GC algorithms, but that was because it could do the wrong thing at times. The old breaker read `MemoryMXBean.getHeapMemoryUsage().getUsed(). That is whole-heap occupancy, including garbage that has not yet been collected. On a generational collector, that number climbs toward `max` between collections during normal operation. That is the steady-state shape of a healthy JVM, not heap pressure to trigger a circuit breaker. Using 99% of the heap is meaningless - that can be 1 second before a harmless GC drops it to 10%. The breaker tripped on pre-GC peaks that the next collection was about to reclaim.
`getCollectionUsage()` on the old-gen pool reports occupancy right after the last collection - live data, not garbage, not live data + garbage. That is the only reading that answers "Is the heap actually full?"
`getCollectionUsage()` is per-pool, so you have to pick a pool, and the one you want is the old/tenured generation where long-lived data is, and each collector names that differently:
- G1 → `G1 Old Gen`
- Parallel → `PS Old Gen`
- Serial → `Tenured Gen`
- generational ZGC → `ZGC Old Generation`
Those match the `\b(Old|Tenured)\b` name pattern, so the circuit breaker reads `getCollectionUsage()` on that pool. Non-generational ZGC and Shenandoah are different, and some yet to be introduced collectors may be too - they have one combined heap pool and no old-gen name to match. When nothing matches the pattern, the fallback sums `getCollectionUsage()` across every `HEAP`-typed pool instead. So the name match plus fallback is there because the pool layout changes depending on the collector.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470111120
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -96,19 +166,51 @@ public MemoryCircuitBreaker setThreshold(double thresholdValueInPercentage) {
@Override
public boolean isTripped() {
-
- long localAllowedMemory = getCurrentMemoryThreshold();
+ long localAllowedMemory = heapMemoryThreshold;
long localSeenMemory = getAvgMemoryUsage();
allowedMemory.set(localAllowedMemory);
-
seenMemory.set(localSeenMemory);
- return (localSeenMemory >= localAllowedMemory);
+ return localSeenMemory >= localAllowedMemory;
}
+ /**
+ * Returns post-GC live bytes for use by {@link #isTripped()}, cached for {@link
+ * CircuitBreakerRegistry#SYSPROP_SAMPLE_TTL_MS} ms so high-QPS callers don't repeatedly walk the
+ * heap pool list.
+ *
+ * <p>The historical name is preserved for source-compatibility. Tests that need to inject a
+ * synthetic value typically override this method directly (which bypasses the cache); tests that
+ * want to feed a synthetic sample <em>through</em> the cache should override {@link
+ * #samplePostGcLiveBytes()} instead. The implementation no longer averages anything.
+ */
Review Comment:
The reason was the access level: it is protected on a public non-final class, so it is part of the subclassing surface for external breakers. Renaming is a source-compat break for a cosmetic gain. I don't care about changing it, but I was attempting to be as gentle as I could, given this was made a plugin point (a choice I don't like at all, but once it's made, I like to err in the user's favor when I couldn't care less myself)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470079920
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -17,75 +17,145 @@
package org.apache.solr.util.circuitbreaker;
-import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.lang.management.MemoryUsage;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
-import org.apache.solr.util.RefCounted;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
+import org.apache.solr.common.util.EnvUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Tracks the current JVM heap usage and triggers if a moving heap usage average over 30 seconds
- * exceeds the defined percentage of the maximum heap size allocated to the JVM. Once the average
- * memory usage goes below the threshold, it will start allowing queries again.
+ * Trips when post-collection live data in the JVM heap exceeds a configured percentage of the
+ * maximum heap size.
*
- * <p>The memory threshold is defined as a percentage of the maximum memory allocated -- see
- * memThreshold in <code>solrconfig.xml</code>.
+ * <p>The signal is read from {@link MemoryPoolMXBean#getCollectionUsage()} on the old/tenured heap
+ * pool, which reports memory usage immediately after the most recent collection that affected that
+ * pool. This is the only memory reading that distinguishes "live data" from "garbage waiting to be
+ * collected."
+ *
+ * <p>Earlier versions of this breaker sampled {@link MemoryMXBean#getHeapMemoryUsage()} on a
+ * 30-second moving average, which produced a high signal during normal operation: with a
+ * generational collector, {@code used} climbs toward {@code max} between collections — that's the
+ * steady-state shape, not a problem. The new signal updates only when an old-gen GC runs, which is
+ * the only point at which "how full is the heap really?" has a defined answer.
+ *
+ * <p>Pool selection by collector:
+ *
+ * <ul>
+ * <li><b>G1 / Parallel / Serial / generational ZGC:</b> uses the pool whose name matches the
Review Comment:
Yes, this is the fix, not a side change. I will say that plainly in the JIRA and the class javadoc.
MemoryCircuitBreaker was agnostic to the pool and GC algorithms, but that was because it could do the wrong thing at times. The old breaker read `MemoryMXBean.getHeapMemoryUsage().getUsed()` That is whole-heap occupancy, including garbage that has not yet been collected. On a generational collector, that number climbs toward `max` between collections during normal operation. That is the steady-state shape of a healthy JVM, not heap pressure to trigger a circuit breaker. Using 99% of the heap is meaningless - that can be 1 second before a harmless GC drops it to 10%. The breaker tripped on pre-GC peaks that the next collection was about to reclaim.
`getCollectionUsage()` on the old-gen pool reports occupancy right after the last collection - live data, not garbage, not live data + garbage. That is the only reading that answers "Is the heap actually full?"
`getCollectionUsage()` is per-pool, so you have to pick a pool, and the one you want is the old/tenured generation where long-lived data is, and each collector names that differently:
- G1 → `G1 Old Gen`
- Parallel → `PS Old Gen`
- Serial → `Tenured Gen`
- generational ZGC → `ZGC Old Generation`
Those match the `\b(Old|Tenured)\b` name pattern, so the circuit breaker reads getCollectionUsage() on that pool. Non-generational ZGC and Shenandoah are different, and some yet to be introduced collectors may be too - they have one combined heap pool and no old-gen name to match. When nothing matches the pattern, the fallback sums `getCollectionUsage()` across every `HEAP`-typed pool instead. So the name match plus fallback is there because the pool layout changes depending on the collector.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3474952122
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -17,75 +17,145 @@
package org.apache.solr.util.circuitbreaker;
-import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.lang.management.MemoryUsage;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
-import org.apache.solr.util.RefCounted;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
+import org.apache.solr.common.util.EnvUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Tracks the current JVM heap usage and triggers if a moving heap usage average over 30 seconds
- * exceeds the defined percentage of the maximum heap size allocated to the JVM. Once the average
- * memory usage goes below the threshold, it will start allowing queries again.
+ * Trips when post-collection live data in the JVM heap exceeds a configured percentage of the
+ * maximum heap size.
*
- * <p>The memory threshold is defined as a percentage of the maximum memory allocated -- see
- * memThreshold in <code>solrconfig.xml</code>.
+ * <p>The signal is read from {@link MemoryPoolMXBean#getCollectionUsage()} on the old/tenured heap
+ * pool, which reports memory usage immediately after the most recent collection that affected that
+ * pool. This is the only memory reading that distinguishes "live data" from "garbage waiting to be
+ * collected."
+ *
+ * <p>Earlier versions of this breaker sampled {@link MemoryMXBean#getHeapMemoryUsage()} on a
Review Comment:
Ah, that's great; appreciate the clarification. Agree with you on the user-relevance, and also agree it makes more sense as a changelog entry detail. It'll find more eyes there.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470079920
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -17,75 +17,145 @@
package org.apache.solr.util.circuitbreaker;
-import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.lang.management.MemoryUsage;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
-import org.apache.solr.util.RefCounted;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
+import org.apache.solr.common.util.EnvUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Tracks the current JVM heap usage and triggers if a moving heap usage average over 30 seconds
- * exceeds the defined percentage of the maximum heap size allocated to the JVM. Once the average
- * memory usage goes below the threshold, it will start allowing queries again.
+ * Trips when post-collection live data in the JVM heap exceeds a configured percentage of the
+ * maximum heap size.
*
- * <p>The memory threshold is defined as a percentage of the maximum memory allocated -- see
- * memThreshold in <code>solrconfig.xml</code>.
+ * <p>The signal is read from {@link MemoryPoolMXBean#getCollectionUsage()} on the old/tenured heap
+ * pool, which reports memory usage immediately after the most recent collection that affected that
+ * pool. This is the only memory reading that distinguishes "live data" from "garbage waiting to be
+ * collected."
+ *
+ * <p>Earlier versions of this breaker sampled {@link MemoryMXBean#getHeapMemoryUsage()} on a
+ * 30-second moving average, which produced a high signal during normal operation: with a
+ * generational collector, {@code used} climbs toward {@code max} between collections — that's the
+ * steady-state shape, not a problem. The new signal updates only when an old-gen GC runs, which is
+ * the only point at which "how full is the heap really?" has a defined answer.
+ *
+ * <p>Pool selection by collector:
+ *
+ * <ul>
+ * <li><b>G1 / Parallel / Serial / generational ZGC:</b> uses the pool whose name matches the
Review Comment:
Yes, this is the fix, not a side change. I will say that plainly in the JIRA and the class javadoc.
MemoryCircuitBreaker was agnostic of pool and GC-algorithm, but that's because it could be when it was not doing the right thing at all. The old breaker read `MemoryMXBean.getHeapMemoryUsage().getUsed()`. That is whole-heap occupancy including garbage that has not been collected yet. On a generational collector that number climbs toward `max` between collections during normal operation. That is the steady-state shape of a healthy JVM, not heap pressure. Using 99% of the heap is meaningless - that can be 1 second before a harmless GC drops it to 10%. The breaker tripped on pre-GC peaks that the next collection was about to reclaim.
`getCollectionUsage()` on the old-gen pool reports occupancy right after the last collection — live data. That is the only reading that answers "is the heap actually full?"
`getCollectionUsage()` is per-pool, so you have to pick a pool, and the one you want is the old/tenured generation where long-lived data settles. Each collector names that differently:
- G1 → `G1 Old Gen`
- Parallel → `PS Old Gen`
- Serial → `Tenured Gen`
- generational ZGC → `ZGC Old Generation`
Those match the `\b(Old|Tenured)\b` name pattern, so the breaker reads `getCollectionUsage()` on that pool. Non-generational ZGC and Shenandoah are different — they have one combined heap pool and no old-gen name to match. When nothing matches the pattern, the fallback sums `getCollectionUsage()` across every `HEAP`-typed pool instead. So the name match plus fallback is there because the pool layout changes per collector.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller merged PR #4514:
URL: https://github.com/apache/solr/pull/4514
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470033924
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -17,75 +17,145 @@
package org.apache.solr.util.circuitbreaker;
-import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.lang.management.MemoryUsage;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
-import org.apache.solr.util.RefCounted;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
+import org.apache.solr.common.util.EnvUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Tracks the current JVM heap usage and triggers if a moving heap usage average over 30 seconds
- * exceeds the defined percentage of the maximum heap size allocated to the JVM. Once the average
- * memory usage goes below the threshold, it will start allowing queries again.
+ * Trips when post-collection live data in the JVM heap exceeds a configured percentage of the
+ * maximum heap size.
*
- * <p>The memory threshold is defined as a percentage of the maximum memory allocated -- see
- * memThreshold in <code>solrconfig.xml</code>.
+ * <p>The signal is read from {@link MemoryPoolMXBean#getCollectionUsage()} on the old/tenured heap
+ * pool, which reports memory usage immediately after the most recent collection that affected that
+ * pool. This is the only memory reading that distinguishes "live data" from "garbage waiting to be
+ * collected."
+ *
+ * <p>Earlier versions of this breaker sampled {@link MemoryMXBean#getHeapMemoryUsage()} on a
Review Comment:
Partly, but there is a reason to keep a migration note. This is a plugin point - it's public, configured in `solrconfig.xml`, and subclassable. And it still carries the deprecated `MemoryCircuitBreaker(int, int)` constructor for subclasses of the old version (we are going to break them, but I sided with a warning log message over a hard break, since neither option seemed great). Anyone who tuned a threshold against the 30-second average, or extended it, will see different trip behavior now. "What changed and why" helps them, so it is not pure change history, or irrelevant implementation history details that no one would care about.
I'll update it to make it a present-tense design point. I'll also add details about "earlier versions sampled X on a 30-second average" to the migration note in the changelog and to the javadoc for the deprecated constructor. This removes the retrospective from the class description but still warns users coming from the old version.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470079920
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -17,75 +17,145 @@
package org.apache.solr.util.circuitbreaker;
-import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.lang.management.MemoryUsage;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
-import org.apache.solr.util.RefCounted;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
+import org.apache.solr.common.util.EnvUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Tracks the current JVM heap usage and triggers if a moving heap usage average over 30 seconds
- * exceeds the defined percentage of the maximum heap size allocated to the JVM. Once the average
- * memory usage goes below the threshold, it will start allowing queries again.
+ * Trips when post-collection live data in the JVM heap exceeds a configured percentage of the
+ * maximum heap size.
*
- * <p>The memory threshold is defined as a percentage of the maximum memory allocated -- see
- * memThreshold in <code>solrconfig.xml</code>.
+ * <p>The signal is read from {@link MemoryPoolMXBean#getCollectionUsage()} on the old/tenured heap
+ * pool, which reports memory usage immediately after the most recent collection that affected that
+ * pool. This is the only memory reading that distinguishes "live data" from "garbage waiting to be
+ * collected."
+ *
+ * <p>Earlier versions of this breaker sampled {@link MemoryMXBean#getHeapMemoryUsage()} on a
+ * 30-second moving average, which produced a high signal during normal operation: with a
+ * generational collector, {@code used} climbs toward {@code max} between collections — that's the
+ * steady-state shape, not a problem. The new signal updates only when an old-gen GC runs, which is
+ * the only point at which "how full is the heap really?" has a defined answer.
+ *
+ * <p>Pool selection by collector:
+ *
+ * <ul>
+ * <li><b>G1 / Parallel / Serial / generational ZGC:</b> uses the pool whose name matches the
Review Comment:
Yes, this is the fix, not a side change. I will say that plainly in the JIRA and the class javadoc.
MemoryCircuitBreaker was agnostic of pool and GC-algorithm, but that's because it could be when it was not doing the right thing at all. The old breaker read `MemoryMXBean.getHeapMemoryUsage().getUsed()`. That is whole-heap occupancy including garbage that has not been collected yet. On a generational collector that number climbs toward `max` between collections during normal operation. That is the steady-state shape of a healthy JVM, not heap pressure to circuit break on. Using 99% of the heap is meaningless - that can be 1 second before a harmless GC drops it to 10%. The breaker tripped on pre-GC peaks that the next collection was about to reclaim.
`getCollectionUsage()` on the old-gen pool reports occupancy right after the last collection - live data, not garbage, not live data + garbage. That is the only reading that answers "is the heap actually full?"
`getCollectionUsage()` is per-pool, so you have to pick a pool, and the one you want is the old/tenured generation where long-lived data is - and each collector names that differently:
- G1 → `G1 Old Gen`
- Parallel → `PS Old Gen`
- Serial → `Tenured Gen`
- generational ZGC → `ZGC Old Generation`
Those match the `\b(Old|Tenured)\b` name pattern, so the circuit breaker reads `getCollectionUsage()` on that pool. Non-generational ZGC and Shenandoah are different, and some yet to be introduced collector may be too - they have one combined heap pool and no old-gen name to match. When nothing matches the pattern, the fallback sums `getCollectionUsage()` across every `HEAP`-typed pool instead. So the name match plus fallback is there because the pool layout changes depending on the collector.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470079920
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -17,75 +17,145 @@
package org.apache.solr.util.circuitbreaker;
-import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.lang.management.MemoryUsage;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
-import org.apache.solr.util.RefCounted;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
+import org.apache.solr.common.util.EnvUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Tracks the current JVM heap usage and triggers if a moving heap usage average over 30 seconds
- * exceeds the defined percentage of the maximum heap size allocated to the JVM. Once the average
- * memory usage goes below the threshold, it will start allowing queries again.
+ * Trips when post-collection live data in the JVM heap exceeds a configured percentage of the
+ * maximum heap size.
*
- * <p>The memory threshold is defined as a percentage of the maximum memory allocated -- see
- * memThreshold in <code>solrconfig.xml</code>.
+ * <p>The signal is read from {@link MemoryPoolMXBean#getCollectionUsage()} on the old/tenured heap
+ * pool, which reports memory usage immediately after the most recent collection that affected that
+ * pool. This is the only memory reading that distinguishes "live data" from "garbage waiting to be
+ * collected."
+ *
+ * <p>Earlier versions of this breaker sampled {@link MemoryMXBean#getHeapMemoryUsage()} on a
+ * 30-second moving average, which produced a high signal during normal operation: with a
+ * generational collector, {@code used} climbs toward {@code max} between collections — that's the
+ * steady-state shape, not a problem. The new signal updates only when an old-gen GC runs, which is
+ * the only point at which "how full is the heap really?" has a defined answer.
+ *
+ * <p>Pool selection by collector:
+ *
+ * <ul>
+ * <li><b>G1 / Parallel / Serial / generational ZGC:</b> uses the pool whose name matches the
Review Comment:
Yes, this is the fix, not a side change. I will say that plainly in the JIRA and the class javadoc.
MemoryCircuitBreaker was agnostic of pool and GC-algorithm, but that's because it could be when it was not doing the right thing at all. The old breaker read `MemoryMXBean.getHeapMemoryUsage().getUsed()`. That is whole-heap occupancy including garbage that has not been collected yet. On a generational collector that number climbs toward `max` between collections during normal operation. That is the steady-state shape of a healthy JVM, not heap pressure. Using 99% of the heap is meaningless - that can be 1 second before a harmless GC drops it to 10%. The breaker tripped on pre-GC peaks that the next collection was about to reclaim.
`getCollectionUsage()` on the old-gen pool reports occupancy right after the last collection — live data. That is the only reading that answers "is the heap actually full?"
`getCollectionUsage()` is per-pool, so you have to pick a pool, and the one you want is the old/tenured generation where long-lived data settles. Each collector names that differently:
- G1 → `G1 Old Gen`
- Parallel → `PS Old Gen`
- Serial → `Tenured Gen`
- generational ZGC → `ZGC Old Generation`
Those match the `\b(Old|Tenured)\b` name pattern, so the breaker reads `getCollectionUsage()` on that pool. Non-generational ZGC and Shenandoah are different, and some yet to be introduced collector may be too - they have one combined heap pool and no old-gen name to match. When nothing matches the pattern, the fallback sums `getCollectionUsage()` across every `HEAP`-typed pool instead. So the name match plus fallback is there because the pool layout changes depending on the collector.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18284: Load-average and memory circuit breakers no longer stampede or trip on transient pre-GC heap peaks [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4514:
URL: https://github.com/apache/solr/pull/4514#discussion_r3470079920
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -17,75 +17,145 @@
package org.apache.solr.util.circuitbreaker;
-import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryPoolMXBean;
+import java.lang.management.MemoryType;
+import java.lang.management.MemoryUsage;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
-import org.apache.solr.util.RefCounted;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
+import org.apache.solr.common.util.EnvUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Tracks the current JVM heap usage and triggers if a moving heap usage average over 30 seconds
- * exceeds the defined percentage of the maximum heap size allocated to the JVM. Once the average
- * memory usage goes below the threshold, it will start allowing queries again.
+ * Trips when post-collection live data in the JVM heap exceeds a configured percentage of the
+ * maximum heap size.
*
- * <p>The memory threshold is defined as a percentage of the maximum memory allocated -- see
- * memThreshold in <code>solrconfig.xml</code>.
+ * <p>The signal is read from {@link MemoryPoolMXBean#getCollectionUsage()} on the old/tenured heap
+ * pool, which reports memory usage immediately after the most recent collection that affected that
+ * pool. This is the only memory reading that distinguishes "live data" from "garbage waiting to be
+ * collected."
+ *
+ * <p>Earlier versions of this breaker sampled {@link MemoryMXBean#getHeapMemoryUsage()} on a
+ * 30-second moving average, which produced a high signal during normal operation: with a
+ * generational collector, {@code used} climbs toward {@code max} between collections — that's the
+ * steady-state shape, not a problem. The new signal updates only when an old-gen GC runs, which is
+ * the only point at which "how full is the heap really?" has a defined answer.
+ *
+ * <p>Pool selection by collector:
+ *
+ * <ul>
+ * <li><b>G1 / Parallel / Serial / generational ZGC:</b> uses the pool whose name matches the
Review Comment:
Yes, this is the fix, not a side change. I will say that plainly in the JIRA and the class javadoc.
MemoryCircuitBreaker was agnostic to the pool and GC algorithms, but that was because it could do the wrong thing at times. The old breaker read `MemoryMXBean.getHeapMemoryUsage().getUsed()’. That is whole-heap occupancy, including garbage that has not yet been collected. On a generational collector, that number climbs toward `max` between collections during normal operation. That is the steady-state shape of a healthy JVM, not heap pressure to trigger a circuit breaker. Using 99% of the heap is meaningless - that can be 1 second before a harmless GC drops it to 10%. The breaker tripped on pre-GC peaks that the next collection was about to reclaim.
`getCollectionUsage()` on the old-gen pool reports occupancy right after the last collection - live data, not garbage, not live data + garbage. That is the only reading that answers "Is the heap actually full?"
`getCollectionUsage()` is per-pool, so you have to pick a pool, and the one you want is the old/tenured generation where long-lived data is, and each collector names that differently:
- G1 → `G1 Old Gen`
- Parallel → `PS Old Gen`
- Serial → `Tenured Gen`
- generational ZGC → `ZGC Old Generation`
Those match the `\b(Old|Tenured)\b` name pattern, so the circuit breaker reads `getCollectionUsage() ` on that pool. Non-generational ZGC and Shenandoah are different, and some yet to be introduced collectors may be too - they have one combined heap pool and no old-gen name to match. When nothing matches the pattern, the fallback sums `getCollectionUsage()` across every `HEAP`-typed pool instead. So the name match plus fallback is there because the pool layout changes depending on the collector.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org