There's no reviewboard/broker/base/__init__.py file (exacerbated by us using namespaces = false in pyproject.toml's tool.setuptools.packages.find)

BaseBrokerBackend creates a self.logger but everything in here uses a module-level logger.

This logs the message but never resets the value. We'd then hit the assertion below in dev, or just crash in other ways in prod.

We should probably create with a more locked-down permissions mask.

I don't know if you've had a chance to look at the spec I put together on tasks, but I don't think we should use a result backend.
Instead, what I had planned out was a task model that could store (smaller) results directly, and larger things (like doc conversion or review bot) end up mapping to domain objects like reviews or file attachments.
If we end up actually using a result backend, this would need to be fixed to use "file://localhost{queue_path}" (no extra slash). Celery strips off 16 characters for "file://localhost". For absolute queue paths that currently ends up with a double leading slash, and if somehow the queue path is a relative path, it would end up adding an extra leading slash turning it into an (incorrect) absolute path.

This doesn't match the implementation/tests.
For send_task, the command name is in the headers (message_headers['task']) and the body is [args, kwargs, embed] as per Celery protocol 2.
For broadcast, the body is a {'method': ..., 'arguments': ...} dict.

This should probably protect with threading.Lock.
It would be nice to also listen to the siteconfig sync (or put in a TODO comment about that).

There's a lot of duplication in here.
It might be nice to have a _get_broker_settings(broker_id) helper which can validate and return the settings. That would be independently testable, and reusable by BaseBrokerSettingsForm.__init__, which currently does the same lookup but with no validation.

This is using a module-level logger instead of self.logger. Same for the other one just below.

Celery's ctor doesn't define a name arg, but it does take in **kwargs and silently ignore anything it doesn't know about.
I think this was intended to be main='reviewboard'

These should match the same order as they appear in the signature.

This is passing instance as a positional arg, but the first argument for KeyValueForm.__init__() is data. We need to explicitly pass instance=instance here.

Can we format as:

from djblets.registries.registry import (
    ALREADY_REGISTERED,
    ...
    UNREGISTER,
)

This is defining the entry point but the class doesn't inherit from EntryPointRegistry.
If we fix that we also need a yield from super().get_defaults() inside the get_defaults() implementation.

Our other registries use %(item)s. As-is this will render as:



"<class 'LocalBrokerBackend'>" is already a registered broker backend.

(with both quotes and repr). Alternatively just remove the quotes.

Same comment about %(item)s vs %(item)r

Should be reviewboard.broker.base.backends.BaseBrokerBackend

It looks like this is using an outdated name for get_celery_config. Same for other tests below.

Any time we open a file in text mode we should pass encoding=. When that's present I prefer to also be explicit with mode='r'.