-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create initial release #1
Comments
AutodiscoveryI've decided to forgo autodiscovery for now. I think this could work well inside a predictable project structure – such as a Django project – but a general purpose solution would need to exhaustively import a lot of modules in order to discover I think this is probably doable, but for now it seems best to omit this particularly magical feature. I suspect more knowledge of Python's import system will be required to do this well. Initial naive implementation was: # Initial naive (and abandoned) implementation of bus.py autodiscovery
def autodiscover(directory='.'):
"""Try to discover & import bus.py files"""
lightbus_directory = Path(__file__).parent.resolve()
# Find all bus.py files
matches = [
Path(match).resolve()
for match
in glob(str(Path(directory) / '**' / 'bus.py'), recursive=True)
]
# Filter out any that are within the lightbus package
matches = [m for m in matches if not str(m).startswith(str(lightbus_directory) + '/')]
# Import each match
for match in matches:
# WARNING: This use of spec_from_file_location() is incorrect. The first parameter needs to
# be the entire module name, but here we just use 'bus'.
spec = importlib.util.spec_from_file_location('bus', str(match))
bus_module = importlib.util.module_from_spec(spec)
spec.loader.exec_module(bus_module) |
After some thought, I think some basic autodiscovery will still be useful. Essentially using the first bus.py file yielded by a breadth-first search. Then determine the module name from sys.path, and perform the import. However, only the top level bus.py file will be imported, anything else will have to be imported from there. Will attempt implementation |
Work has been continuing over the last few months. Items above are getting checked off pretty quickly now, so I'm hoping this proof of concept / MVP should be ready for the light of day in the next few months. During this time we should also see the release of Redis 5.0, which will include streams (ref). |
Latest thoughts on how to specify parameters for events. Unlike RPCs, event definitions are provided by class properties rather than methods. With RPCs we can extract the RPC signature from the method definition using Python's built-in With events we do not use method definitions as this would imply that there should also be an implementation. The implementation should live in the API's clients not the API itself. We therefore use class properties, but as a result we lose the ability to extract a parameter signature. I've explored various ideas below regarding how we can specify parameters for events: # How can we specify parameters for events?
class TestApi(Api):
# The current implementation
user_registered1 = Event(parameters=['username', 'email', 'is_admin'])
# Simple, no default values, no **kwargs
user_registered2 = Event(parameters={'username': str, 'email': str, 'is_admin': bool})
# Verbose, but has default values and **kwargs
user_registered3 = Event(parameters=[
inspect.Parameter('username', kind=inspect.Parameter.KEYWORD_ONLY, annotation=str),
inspect.Parameter('email', kind=inspect.Parameter.KEYWORD_ONLY, annotation=str),
inspect.Parameter('is_admin', kind=inspect.Parameter.KEYWORD_ONLY, annotation=str),
inspect.Parameter('extra_fields', kind=inspect.Parameter.VAR_KEYWORD, annotation=str, default=False),
])
# As above, but customised for more concise definitions
user_registered4 = Event(parameters=[
Parameter('username', str),
Parameter('email', str),
Parameter('is_admin', str),
WildcardParameter('extra_fields', str, default=False),
])
# Does not support **kwargs. Parameters can no longer be inline
user_registered5 = Event(parameters=RegistrationEventParameters) Where class RegistrationEventParameters(NamedTuple):
username: str
email: str
is_admin: str = False Ultimately providing both options 1 & 4 seems preferable, in lieu of any better option. |
What should the config system look like?
What different kinds of config are there:
Future development:
Scratch pad: plugins:
internal_state:
ping_interval: 60
internal_metrics:
enable: false
bus:
schema:
transport:
redis: {}
apis:
default:
event_transport:
class: lightbus.RedisTransport
redis_url: redis://username:password@redis
auth:
validate: false
event_transport:
class: lightbus.RedisTransport
redis_url: redis://username:password@another_redis_host
|
A quick review of the various registries we are using:
Registries under consideration:
|
Recording some notes on lightbus prometheus exporter metrics:
|
Note to self: I discuss versions and migrations on pages 199-200 of notebook 1. |
Resolving CannotBlockHere errorsThere has been an ongoing issue with the Lightbus API which looks something like this:
The solution to date has been that one must write async handlers (e.g The solutionThe general solution to this is to run these hooks in an asyncio thread executor. Event loops are per-thread, so the hook its free to create its own loop as it now has its own thread. There is a problem with this solution however: Lightbus has not been designed with treading in mind, and so is not threads-safe. Lightbus has been designed using asyncio, which puts lightbus in control of context switches (i.e. at any Could Lightbus become thread safe? Most likely yes. However, I prefer to avoid this at this stage because 1) threading bugs are hard to track down, 2) I'm not particularly knowledgeable in this area, and 3) I would like Lightbus to stabilise more before embarking on this. There is an alternative however. We are not using threads because we need threads, we are using threads to provide a clean environment for user code (i.e. an environment in which an event loop can be started). We therefore came to the following solution:
|
The above solution has now been implemented and merged. It appears to be working well. |
All development tasks are now complete. All that's left to go at the 'Pre launch tasks' which relate mostly to some additional documentation sections. |
Initial release is out! This epic issue can finally be closed, with all tasks complete 🎉 |
Goals
MVP
Message protocol (standard headers)Handled by individual transportsApi
classEvent
classBusClient
,Message
, andXxxTransport
classes.Required prior to use in production
lightbus shell
)SchemaTransport
BusClient
Api-level custom log levels– Non-trivial implementation. Will leave for now.enabled
flagconfig:
commentsstream_use
config option, which will allow for serving all events on the API from a single streamRedisPluggable configuration source(Will do this if needed, we now supporthttp[s]://
urls anyway)@bus.on_start()
and@bus.on_stop()
. Consider refactoring plugins into this format too (the plugin system would have to operate around modules rather than classes, which may be a faff).@bus.configure()
to allow for per-service bus configuration. Is this a good idea?(Leave for now, will revisit if there is a need)lightbus_set_database()
BusNode
to something else. The 'node' term refers to the data structure it forms, but it can appear that a 'node' refers to the service.Transactional Transport: The duplicates table needs to track uniqueness on both message ID and consumer name. Migration steps:(Removed, see Transactional Transport #4)ALTER TABLE lightbus_processed_events DROP CONSTRAINT lightbus_processed_events_pkey;
ALTER TABLE lightbus_processed_events ADD COLUMN listener_name VARCHAR(200);
UPDATE lightbus_processed_events SET listener_name = 'default';
ALTER TABLE lightbus_processed_events ADD PRIMARY KEY (message_id, listener_name);
Django: I suspect the transactional middleware creates a new postgres connection for every request.(Removed, see Transactional Transport #4)consumer_group_prefix
->service_name
consume(consumer_group=...)
->consume(listener_name=...)
xxx.listen(fn, options["consumer_group"])
->xxx.listen(fn, listener_name="...")
Warn if duplicate consumer name used(Less important now thatlistener_name
is mandatory when setting up a new listener)feature/config-inheritance
. I'm not convinced this is a good idea)Docs- Will leave this for nowxpending
should happen in a loop until there are no more messagesIdeas for future work / notes:
Meta
toOptions
as this is more descriptive and doesn't clash with the 'metaclass' concept. Downside is that this diverges from Django.#:
style. Parse parameter descriptions out of docstring?)Reading
Pre launch tasks
@uses_django_db()
- Either include in docs, or integrate it Lightbus core (Docs for now, see Create Django Middleware #6 for future improvements)The text was updated successfully, but these errors were encountered: