SOLR-15474 Make Circuit breakers pluggable (take 2) #1725

janhoy · 2023-06-27T13:21:04Z

https://issues.apache.org/jira/browse/SOLR-15474

This is a continuation of the effort from PR #193 which had grown out of date by almost 2 years and no activity.
Thus I updated that branch to main and squashed as one commit to kickstart this PR. I intend to bring it to mergable state.

To do before committable

Decide whether to move this to a cluster plugin instead of per core
Re-introduce support for <circuitBreaker class="solr.CircuitBreakerManager"> with all params for back compat
Update ref guide

cpoerschke

Great to see this effort continue!

solr/core/src/java/org/apache/solr/util/circuitbreaker/CPUCircuitBreaker.java

solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreaker.java

solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java

solr/core/src/test-files/solr/collection1/conf/solrconfig-memory-circuitbreaker.xml

solr/solr-ref-guide/modules/deployment-guide/pages/circuit-breakers.adoc

janhoy · 2023-08-23T14:27:58Z

In the last commits I re-introduced backward compatibility for configuring the legacy "CircuitBreakerManager" as a single plugin. I did this by renaming CircuitBreakerManager as CircuitBreakerRegistry, so the registry is now in charge of knowing about all CBs, while the old Manager is now a true plugin, which wraps the cpu and mem CBs.

I'll probably not pursue converting to cluster-plugin in this PR since it will be a 10.0 only thing. But I want to achieve separating between update and query breakers.

solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java

cpoerschke

16 / 18 files viewed -- only partly looked at the test changes so far.

And deprecating CircuitBreakerManager via morphing into a wrapper of two circuit breakers is a neat way to maintain backwards compatibility, very nice.

solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java

solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerRegistry.java

solr/solr-ref-guide/modules/deployment-guide/pages/circuit-breakers.adoc

janhoy · 2023-08-24T13:10:44Z

Thanks for reviewing @cpoerschke. Last bit of polish and then I hope to wrap up. Then this is a nice foundation to build /update circuit breaking on top of. I plan to let each CB plugin decide whether it should trip UPDATE, SEARCH or both request types. So you could e.g. have a CPU breaker on 80% for updates, and another one on 90% for search, letting you sacrifice freshness first and then user experience. But that is for another PR...

janhoy · 2023-08-24T13:27:52Z

Will likely need to squash and force-push in order for Crave.io to do its job... Anyone have a problem with that?

solr/solr-ref-guide/modules/deployment-guide/pages/circuit-breakers.adoc

Co-authored-by: Christine Poerschke <[email protected]> Signed-off-by: Jan Høydahl <[email protected]>

…BreakerManager.java Co-authored-by: Christine Poerschke <[email protected]>

Signed-off-by: Jan Høydahl <[email protected]>

janhoy · 2023-08-24T14:24:32Z

@dsmiley, @elyograg: I think this is ready, but appreciate if one of you would skim over this PR and see what you find.

elyograg · 2023-08-24T19:01:54Z

@janhoy this deals with code where I have no knowledge. I wouldn't know how to tell whether the change is good or not.

dsmiley · 2023-08-24T21:01:00Z

My colleagues have done some custom Circuit Breaking stuff in Solr... I'm hoping one of them might weigh-in here so please don't merge too quickly.

janhoy · 2023-08-25T08:31:12Z

My colleagues have done some custom Circuit Breaking stuff in Solr... I'm hoping one of them might weigh-in here so please don't merge too quickly.

This is not really touching the existing breaker logic itself, but the pluggability of them in solrconfig.xml.
There are other CB related issues as well you may want to weigh in to.

I'd also like to (in another issue) consider moving CB from core-level to node or cluster level and thus deprecate this solrconfig plugin altogether, but we need to stay back compat for now.

dsmiley · 2023-08-25T13:36:14Z

When you said "Then this is a nice foundation to build /update circuit breaking on top of.", I this caught my interest as this is what we're doing with Solr where I work. If this PR doesn't actually have to do with that, and is only about configuration, then merge away!

janhoy · 2023-08-25T14:16:48Z

When you said "Then this is a nice foundation to build /update circuit breaking on top of.", I this caught my interest as this is what we're doing with Solr where I work. If this PR doesn't actually have to do with that, and is only about configuration, then merge away!

Sure, this is just making CBs pluggable for real, so you could create a DayOfWeekCB if you want :)

janhoy · 2023-08-29T07:34:34Z

Looking for at least one "+1" before I proceed with the merge.

atris · 2023-08-29T07:43:49Z

I took a look and it looks good to me.

Signed-off-by: Jan Høydahl <[email protected]>

cpoerschke

LGTM

cpoerschke · 2023-08-29T10:25:01Z

solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java

+    if (memEnabled) {
+      sb.append(memCB.getDebugInfo()).append("\n");
    }


Suggested change

if (memEnabled) {

sb.append(memCB.getDebugInfo()).append("\n");

}

if (memEnabled) {

sb.append(memCB.getDebugInfo());

}

if (memEnabled && cpuEnabled) {

sb.append("\n");

}

cpoerschke · 2023-08-29T10:29:59Z

solr/core/src/test-files/solr/collection1/conf/solrconfig-pluggable-circuitbreaker.xml

+  </query>
+
+  <circuitBreaker class="solr.MemoryCircuitBreaker">
+    <int  name="threshold">75</int>


Suggested change

<int name="threshold">75</int>

<double name="threshold">75</double>

cpoerschke · 2023-08-29T10:41:50Z

solr/core/src/test/org/apache/solr/util/BaseTestCircuitBreaker.java

+    List<CircuitBreaker> registeredCircuitBreakers =
+        h.getCore().getCircuitBreakerRegistry().getRegisteredCircuitBreakers();
+
+    registeredCircuitBreakers.clear();


Could deregisterAll be used here instead (and then getRegisteredCircuitBreakers() could be removed)?

Suggested change

List<CircuitBreaker> registeredCircuitBreakers =

h.getCore().getCircuitBreakerRegistry().getRegisteredCircuitBreakers();

registeredCircuitBreakers.clear();

h.getCore().getCircuitBreakerRegistry().deregisterAll();

@cpoerschke Seems we had a race condition with your latest suggestions and my merge. Sorry for that, I could swear I saw some of those comments earlier, don't know how they got closed without merge.

I'll backport the initial commit. Would you mind filing a new PR with these latest changes? The commit message can reference same JIRA.

No problem! #1870 for the follow-up bits. Would you mind including it in the back port?

edit: never mind, back port already happened. I can back port after merge.

That was my thought, probably easier to correlate later..

* SOLR-15474 Make Circuit breakers pluggable Co-authored-by: Christine Poerschke <[email protected]> Co-authored-by: Atri Sharma <[email protected]> Signed-off-by: Jan Høydahl <[email protected]> (cherry picked from commit 4100049) Signed-off-by: Jan Høydahl <[email protected]>

janhoy requested review from cpoerschke and atris June 27, 2023 13:27

janhoy assigned madrob and unassigned madrob Jun 27, 2023

janhoy requested a review from madrob June 27, 2023 13:27

cpoerschke reviewed Jun 27, 2023

View reviewed changes

janhoy mentioned this pull request Jun 27, 2023

SOLR-15474: Support Pluggable Circuit Breakers #193

Closed

cpoerschke reviewed Aug 24, 2023

View reviewed changes

solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java Outdated Show resolved Hide resolved

cpoerschke reviewed Aug 24, 2023

View reviewed changes

solr/solr-ref-guide/modules/deployment-guide/pages/circuit-breakers.adoc Outdated Show resolved Hide resolved

SOLR-15474 Make Circuit breakers pluggable

c6ef934

Co-authored-by: Christine Poerschke <[email protected]> Signed-off-by: Jan Høydahl <[email protected]>

janhoy force-pushed the solr15474-pluggable-cb-take2 branch from 4050b9d to c6ef934 Compare August 24, 2023 13:54

janhoy requested review from dsmiley and elyograg August 24, 2023 14:07

janhoy and others added 3 commits August 24, 2023 16:15

Update solr/core/src/java/org/apache/solr/util/circuitbreaker/Circuit…

cb3632e

…BreakerManager.java Co-authored-by: Christine Poerschke <[email protected]>

Update solr/core/src/java/org/apache/solr/util/circuitbreaker/Circuit…

6c3d5bf

…BreakerManager.java Co-authored-by: Christine Poerschke <[email protected]>

Review feedback - revert to 75 threshold in legacy test

af519a2

Signed-off-by: Jan Høydahl <[email protected]>

atris approved these changes Aug 29, 2023

View reviewed changes

Tidy

896a8b7

Signed-off-by: Jan Høydahl <[email protected]>

janhoy merged commit 4100049 into apache:main Aug 29, 2023

janhoy deleted the solr15474-pluggable-cb-take2 branch August 29, 2023 10:47

cpoerschke approved these changes Aug 29, 2023

View reviewed changes

cpoerschke mentioned this pull request Aug 29, 2023

SOLR-15474: three more code review tweaks #1870

Merged

janhoy mentioned this pull request Aug 30, 2023

SOLR-15056: add circuit breaker for CPU, fix load circuit breaker #96

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-15474 Make Circuit breakers pluggable (take 2) #1725

SOLR-15474 Make Circuit breakers pluggable (take 2) #1725

janhoy commented Jun 27, 2023 •

edited

Loading

cpoerschke left a comment

janhoy commented Aug 23, 2023

cpoerschke left a comment

janhoy commented Aug 24, 2023 •

edited

Loading

janhoy commented Aug 24, 2023

janhoy commented Aug 24, 2023

elyograg commented Aug 24, 2023

dsmiley commented Aug 24, 2023

janhoy commented Aug 25, 2023

dsmiley commented Aug 25, 2023

janhoy commented Aug 25, 2023

janhoy commented Aug 29, 2023

atris commented Aug 29, 2023

cpoerschke left a comment

cpoerschke Aug 29, 2023

cpoerschke Aug 29, 2023

cpoerschke Aug 29, 2023

janhoy Aug 29, 2023

cpoerschke Aug 29, 2023 •

edited

Loading

janhoy Aug 29, 2023

	<int name="threshold">75</int>
	<double name="threshold">75</double>

SOLR-15474 Make Circuit breakers pluggable (take 2) #1725

SOLR-15474 Make Circuit breakers pluggable (take 2) #1725

Conversation

janhoy commented Jun 27, 2023 • edited Loading

cpoerschke left a comment

Choose a reason for hiding this comment

janhoy commented Aug 23, 2023

cpoerschke left a comment

Choose a reason for hiding this comment

janhoy commented Aug 24, 2023 • edited Loading

janhoy commented Aug 24, 2023

janhoy commented Aug 24, 2023

elyograg commented Aug 24, 2023

dsmiley commented Aug 24, 2023

janhoy commented Aug 25, 2023

dsmiley commented Aug 25, 2023

janhoy commented Aug 25, 2023

janhoy commented Aug 29, 2023

atris commented Aug 29, 2023

cpoerschke left a comment

Choose a reason for hiding this comment

cpoerschke Aug 29, 2023

Choose a reason for hiding this comment

cpoerschke Aug 29, 2023

Choose a reason for hiding this comment

cpoerschke Aug 29, 2023

Choose a reason for hiding this comment

janhoy Aug 29, 2023

Choose a reason for hiding this comment

cpoerschke Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

janhoy Aug 29, 2023

Choose a reason for hiding this comment

janhoy commented Jun 27, 2023 •

edited

Loading

janhoy commented Aug 24, 2023 •

edited

Loading

cpoerschke Aug 29, 2023 •

edited

Loading