Add support for registering custom circuit breaker

[seut]

This PR adds support for registering custom circuit breakers besides PARENT, REQUEST and FIELDDATA.
We needed to account memory consumption of different contexts than request or fielddata caches, but of course, this makes only sense by hooking up a new circuit breaker into the breaker hierarchy.
Maybe this is also interesting for someone else like plugin developers.

[clintongormley]

@dakrone what do you think about this PR?

[dakrone]

@clintongormley I am planning on looking at it

[dakrone]

@seut I like the idea of registering breakers, and that plugins could register their own breakers. I have some ideas about this though:

I think instead of having to register a name, and then registering the breaker, I think it would be better if we do it all in the register breaker method (since we have access to breakerSettings.getName()).

I think we can remove the built in breaker list, and then in the constructor for HierarchyCircuitBreakerService, we can call the registerBreaker method for the PARENT, FIELDDATA, and REQUEST breaker. I think it would be okay to overwrite a breaker (using the Name as the key).

If the registerBreaker method rebuilds the breakers map, it can then be used in ApplySettings as well, to re-register breakers that have different settings. I think this would reduce the amount of code a bit and move the logic to a single place.

[seut]

@dakrone Thanks for looking at it!

Yes I'd prefer to register the names inside the registerBreaker method, but the BreakerSettings class requires a Name instance... any suggestions to solve that? Also the Name instances are used to get a breaker and so are used all over the code..

And yes I've thought to refactor it so built-in breakers would be registered by the new registerBreaker method, but one drawback of this method is that on each call the immutable breaker map must be rebuild and re-applied (while synchronized), which is currently done only once for all built-in breakers. I don't think it will hurt so much because it's usually only done on startup and update settings calls (which normally won't occur so often), but was afraid you'd rejecting this ;) Another way would be to use a concurrent hash map, but maybe the overhead of it (specially on get calls at getBreaker) isn't worth comparing to a rarely occurring immutable map rebuilts.

[dakrone]

I think we can move to CircuitBreaker.Name being an interface, then FIELDDATA, REQUEST, etc can be implementations of the interface. We can then have CircuitBreaker.Name.FIELDDATA be a statically initialized instance of the Name. That would allow a plugin to create its own Name class for use when registering.

For the synchronized breaker map, I think it is fine to move to a ConcurrentHashMap here and remove the locks, especially since I don't expect the map to be contested at all.

[seut]

I thought about changing CircuitBreaker.Name to an interface also, but I'm not quite sure how to implement safe streaming of it. I will rethink it again ;-)
Using a ConcurrentHashMap sounds fine to me, will change that.

[dakrone]

I'm not quite sure how to implement safe streaming of it. I will rethink it again ;-)

If this is a 2.0 change only, you can change the streaming of it (since 2.0 requires a restart). If this goes to 1.x you can use the out.getVersion().onOrAfter(Version.FOO) to implement version checking on both the serialization and deserializing.

[seut]

@dakrone After thinking again about the CircuitBreaker.Name implementation, I cannot figure out how a Interface would eliminate the names registration. Because, for creating instances from a stream input, we need to instantiate a concrete class or use a registry like I did. The only other way would be do use a abstract (or normal like it is now) class which is registering its its concrete child classes in its constructor. This would move the registration from ``�Name.register()tonew Name()`.
What about eliminating the `CircuitBreaker.Name` class at all and using strings instead? This would result in a little network overhead comparing to streaming integers but afaik the names are only streamed when stats are requested...

[dakrone]

What about eliminating the CircuitBreaker.Name class at all and using strings instead? This would result in a little network overhead comparing to streaming integers but afaik the names are only streamed when stats are requested...

Sorry, I wasn't clear when I talked about changing the streaming, your solution is what I was suggesting. If you want to target 1.x you'll need to add backwards compatibility (using the .onOrAfter(...) method I mentioned previously). If you want to only add this to 2.0, you can just change the streaming to use strings instead.

[seut]

@dakrone just pushed a fixup commit including all discussed changes. will squash it if everything is fine now.

[seut]

@dakrone ah yeah and I will create another PR for a 1.x backport of this once this PR is accepted

[dakrone]

I think it's okay to make these lowercase now that they aren't enums

[dakrone]

@seut I added a couple of comments, but this is looking good!

I also think it would be extremely useful to add an integration test for this to test that breaker stats correctly include custom circuit breakers. You should be able to put it in CircuitBreakerServiceTests and use

Iterable<CircuitBreakerService> serviceIter = internalCluster().getInstances(CircuitBreakerService.class);
for (CircuitBreakerService s : serviceIter) {
  s.registerBreaker(...);
}
// Maybe increment the breaker here
NodesStatsResponse resp = client().admin().cluster().prepareNodesStats().clear().setBreaker(true).get();
// Check response

to get the circuit breaker service, then you can register a custom breaker and issue a nodes stats to ensure that the stats include the custom breaker (you can also increment/decrement the breaker there). Does that make sense? I'm happy to help if not.

[seut]

@dakrone pushed a fixup including all discussed changes + an integration test. because ConcurrentMap.replace() does not support null values, I had to implement it slightly different to harden it against race conditions of concurrent calls. please let me know if you see any other issues. thx!

[seut]

any review news? ;)

[dakrone]

@seut I'm looking at this and hoping to merge it in today, sorry for taking so long

[seut]

ah wanted to squash my review fixups first, but anyway, thanks for merging! :)

[dakrone]

Thanks for submitting the feature!