Basic thread safety for ValuesSourceRegistry

[not-napoleon]

Had a test fail in another branch due to concurrent access of the registry internal data structures, so figured it was a good time to fix that.

[elasticmachine]

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

[polyfractal]

Do we need to allow threaded access to registration? E.g. based on the comment, it looks like this operates similar to agg registration today: only expected to happen at startup and then effectively immutable.

Today, core aggs register sequentially and then plugins register in a loop. The NamedXContent and NamedWriteable entries are later converted into Registries with immutable maps.

Not saying it has to end up the same, but seems like it'd be simpler to not allow concurrent access and eventually build an immutable object which can be shared? Or some kind of "finalization" that converts to an immutable object and disallows new registrations?

It's very possible I'm missing something though :)

[not-napoleon]

Sorry, I was unclear in the comment. Multiple aggs running at once could be trying to read from the registry at the same time. That needs to be thread safe. But since we only ever write during registration, I didn't want to use something like ConcurrentHashMap for the internal representations since that has stronger controls (and thus more overhead) than we need. The "new" immutable collections provide the safety we need for reads and are space and time efficient (unlike the unmodifiable wrappers, which add an extra layer of abstraction).

If we only register serially, then we probably don't need to synchronize register, but if we only access it serially and at startup, synchronizing it is cheap insurance since the implementation I have there will fail disastrously if it's accessed by two threads at once.

PS: I know what the test failures are about, will push a fix soon, just need to do some thinking ahead about it.

[polyfractal]

LGTM, thanks for the explanation! I was indeed a bit confused.

As discussed offline we can move this to some sort of finalization if the COW (:cow:) is too expensive, but should only be invoked ~50 times so kicking that can down the road until we know more :)

[not-napoleon]

Yeah, I think when we're wiring up the registration we should keep an eye out for a good spot to migrate to a finalizer pattern for this, but doing it now seems premature.

[not-napoleon]

@elasticmachine run elasticsearch-ci/1

[polyfractal]

Left a few minor comments/questions, otherwise 👍

[polyfractal]

Name seems a bit funky to me. Perhaps something like `getAggregatorRegistrar()?

[polyfractal]

Do we expect this to be temporary? I think based on our conversations it is (until we sort out why the tests are re-registering repeatedly)? Just asking because it'd be nice if we could keep all the aggs from having to implement this flag (easy to forget).

If we expect it to be permanent, we should probably use a SetOnce or something similar to ensure we don't change it.

[not-napoleon]

I hope this is temporary, but I'm going to go ahead and slap a SetOnce on it anyway. Honestly, I'm shocked this worked as is without being set to volatile, so that seems like cheap insurance overall. I'll add a line item to the meta ticket to revisit this so we don't forget (and probably a TODO in the code as well)