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: [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]
