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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
