Provide alternative in-memory queue for publishing events #763
Conversation
…jryan/baseplate.py into replace_posix_queue_event_publishing
sydjryan
changed the title
|
This is maybe an ignorant question, but I feel like I'm missing something. With the in-memory implementation, how is the item that's on the queue making it from the main baseplate application to the sidecar process? |
There was a problem hiding this comment.
Looks good for a start. Jayme is right though that the baseplate service will need to talk to the sidecar, so we need another queue type RemoteMessageQueue that will call a server in the sidecar. Likewise the sidecar needs to expose such a server RemoteMessageQueueServer that uses the InMemoryMessageQueue for the method implementation.
|
🏃 Have to drop from this for now with the Snoodev Code Yellow (not focusing on baseplate releases), please feel free to re-tag once we're clear (we're aiming to exit SCY by end of quarter) |
There was a problem hiding this comment.
My biggest concern is how this will be used, an in memory queue only works if the Baseplate server and the process consuming the queue share memory, in most cases where this is being used, like event publishing, this is not the case. It was mentioned that maybe we need a server to front this, at that point we might want to consider using something existing and not writing our own. It would be great to see a one pager to get a general idea of the approach and goals for this change.
baseplate/sidecars/__init__.py
Outdated
| @@ -4,6 +4,10 @@ | |||
| from typing import NamedTuple | |||
| from typing import Optional | |||
|
|
|||
| import gevent | |||
|
|
|||
| gevent.monkey.patch_all() | |||
There was a problem hiding this comment.
@eaceaser lmk if there's somewhere better to put this
There was a problem hiding this comment.
Lets move this somewhere where we can wrap it in if cfg.queue_type == QueueType.IN_MEMORY.value -- that way we know we're not changing existing behavior at all
There was a problem hiding this comment.
we still need to remove this b/c otherwise it'll monkeypatch the sidecar when using the posix queue
| service RemoteMessageQueueService { | ||
| PutResponse put( | ||
| 1: binary message | ||
| 2: double timeout |
There was a problem hiding this comment.
do we have a use case where we want to vary this? Otherwise I'd recommend we remove the timeout arg and just set a timeout in the sidecar configuration.
There was a problem hiding this comment.
I defaulted to None (block forever) to match posix queue behavior. but doing a quick search it actually looks like most (potentially all?) calls set timeout=0 i.e. dont wait at all. seems like the None default might not be best, but I dont want to change behavior
baseplate/sidecars/__init__.py
Outdated
| @@ -4,6 +4,10 @@ | |||
| from typing import NamedTuple | |||
| from typing import Optional | |||
|
|
|||
| import gevent | |||
|
|
|||
| gevent.monkey.patch_all() | |||
There was a problem hiding this comment.
we still need to remove this b/c otherwise it'll monkeypatch the sidecar when using the posix queue
|
Removing 2.7 label for now due to conflicts. @sydjryan do we still want to get this merged in a future release? I know this has been sitting for a while now, just want to make sure it's still good to merge once we fix up the conflicts. |
💸 TL;DR
The event & trace publisher sidecars currently use a POSIX queue to have a buffer in case the services are unavailable. We may not always have access to the special sysctl settings required by to use a POSIX queue, so this PR adds an optional in-memory queue (using
queue.Queue). We still default to the POSIX queue, but can optionally pass a command line flag to use this new implementation instead.We need to gevent patch very early - will add this to the sidecar container entrypoint here: https://github.snooguts.net/reddit/docker-baseplate-sidecars/pull/36
📜 Details
Jira
One-pager
🧪 Testing Steps / Validation
Added both integration & unit tests for the new implementation.
✅ Checks