-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(aci): Separate Buffer for Workflows #97549
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #97549 +/- ##
=======================================
Coverage 80.64% 80.65%
=======================================
Files 8572 8573 +1
Lines 377728 377732 +4
Branches 24579 24579
=======================================
+ Hits 304625 304661 +36
+ Misses 72737 72705 -32
Partials 366 366 |
|
||
buffer.backend.delete_keys( | ||
buffer.delete_keys( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Delayed Workflow Handler Registration Failure
An incorrect import of LazyServiceWrapper
in sentry.workflow_engine.tasks.delayed_workflows.py
prevents the module from loading. This stops the DelayedWorkflow
handler from being registered in delayed_processing_registry
, causing the workflow buffer to accumulate and never be flushed.
Additional Locations (1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're wrong. The import isn't incorrect, it's just inconsistent with other imports of the same class that are pulling it from where it used to exist and where it is still re-exported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm if we're okay to remove the buffer hook registry in the legacy code.
FLUSH = "flush" | ||
|
||
|
||
class BufferHookRegistry: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove this yet? i'm mostly concerned about the legacy delayed_processor
code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 - 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.