feat(aci): Separate Buffer for Workflows #97549
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #97549 +/- ##
==========================================
+ Coverage 79.98% 80.64% +0.65%
==========================================
Files 8588 8587 -1
Lines 378443 378374 -69
Branches 24642 24632 -10
==========================================
+ Hits 302709 305128 +2419
+ Misses 75362 72874 -2488
Partials 372 372 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| FLUSH = "flush" | ||
|
|
||
|
|
||
| class BufferHookRegistry: |
There was a problem hiding this comment.
can we remove this yet? i'm mostly concerned about the legacy delayed_processor code.
There was a problem hiding this comment.
I think this should be okay, though it is hard to reason about.
The task now called process_buffer directly, and process buffer uses the registry to pick the right backend for the right delayed processor.
Though, I didn't think too hard about what the registry was trying to accomplish, so I may be missing something. 😬
| buffer_keys, | ||
| min=0, | ||
| max=fetch_time, | ||
| ) | ||
|
|
||
|
|
||
| if not redis_buffer_registry.has(BufferHookEvent.FLUSH): |
There was a problem hiding this comment.
🎉 - i never liked this code. just curious, if we want to add another handler when the buffer is flushed, would we have to manually add it to tasks/process_buffer.py::process_pending_batch? should we have a registry hook there or is this uncommon enough that we can be explicit about it?
There was a problem hiding this comment.
I think we've made it so there isn't just One buffer anymore, and the dispatching to the appropriate buffer is now handled by the config in delayed_processing_registry.
For the delayed processing-style batch scheduling, this seem sufficiently extensible (presumably, in some month it'll only be dispatching to workflows, and we may want to split "Buffer" as our method set is fairly independent and can pretty much work with a redis client), so I'm not too concerned with generalizing the pattern.
|
Seer failed to run. |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Buffer reliability is critical and has been proven to be a potential stability risk as we scale up. To allow Workflows infrastructure to fail without impacting the legacy system and to enable the use of newer backends without migrating live load, it's helpful to give Workflows it's own Buffer.
Buffer reliability is critical and has been proven to be a potential stability risk as we scale up. To allow Workflows infrastructure to fail without impacting the legacy system and to enable the use of newer backends without migrating live load, it's helpful to give Workflows it's own Buffer.
Buffer reliability is critical and has been proven to be a potential stability risk as we scale up.
To allow Workflows infrastructure to fail without impacting the legacy system and to enable the use
of newer backends without migrating live load, it's helpful to give Workflows it's own Buffer.