Improve datastore type hints

[clokep]

By being explicit about methods we should be able to get better type checking (although this isn't entirely proven in this PR). The idea is to mostly treat datastores the way we handle config classes, each as an individual unit that has to reach back to a "root" object to query other datastores.

This means that the main DataStore object would become a bundle of SQLBaseStore objects instead of a single entity which has super-classes of SQLBaseStore.

As a proof-of-concept I converted the RelationsStore (it is simple and I know it fairly well). Other stores could be done in follow-ups.

Related to #11165

Downsides of this approach is it might make #11733 worse as you now have a DataStore which includes a RelationsStore? It adds another layer to the puzzle.

[clokep]

(I should probably mention that this would need to be expanded to the other datastore classes, e.g. for workers, etc.)

[clokep]

This needs a bit more to get it working with all the various datastores we create...

[squahtx]

Looks okay so far.

I think we're missing a few things before we can apply this pattern to the rest of the stores.

• CacheInvalidationWorkerStore and SQLBaseStore assume there is only one instance of themselves. Is there a plan for how we're going to handle them?
• The bit of SQLBaseStore that involves external_cached_functions won't work for split out stores. I'm not sure what it does.
• process_replication_rows and process_replication_position won't get called on split out stores right now, so the massive chain of overrides and super() calls will break. We'd probably want to refactor things to avoid the massive chain anyway, and have stores register their interest in particular row types.

[squahtx]

Should we be ignoring store_names that refer to missing fields here?

[squahtx]

I'm going to put this back in the queue in case anyone has more thoughts.

[clokep]

• CacheInvalidationWorkerStore and SQLBaseStore assume there is only one instance of themselves. Is there a plan for how we're going to handle them?

I'm failing to see how we would end up with more than one (per process)? Or are you just suggesting it might be possible to do that somehow?

• The bit of SQLBaseStore that involves external_cached_functions won't work for split out stores. I'm not sure what it does.

Good-catch, I think we need to do something similar to _attempt_to_invalidate_cache. 👍

• process_replication_rows and process_replication_position won't get called on split out stores right now, so the massive chain of overrides and super() calls will break. We'd probably want to refactor things to avoid the massive chain anyway, and have stores register their interest in particular row types.

This sounds like a good change to make regardless, maybe I'll just do this as a separate PR.

[squahtx]

• CacheInvalidationWorkerStore and SQLBaseStore assume there is only one instance of themselves. Is there a plan for how we're going to handle them?

I'm failing to see how we would end up with more than one (per process)? Or are you just suggesting it might be possible to do that somehow?

It's more that every store inherits from one of the two. Even now with this PR, I think there are two SQLBaseStores? One of them is the big data store and the other is the RelationsStore.

(I'm not suggesting that there should be exactly one or we should support multiple CacheInvalidationWorkerStore/SQLBaseStores. It's just something to think about.)

[reivilibre]

Nothing to add to Sean's review — looks sensible overall and given this has stuck around for a while in the queue I don't think anyone else has any objections.