ref(aci): Big delayed_workflow refactor #93569
Conversation
github-actions
bot
added
the
Scope: Backend
| "Failed to parse workflow event data", | ||
| extra={"key": key, "value": value, "error": str(e)}, | ||
| ) | ||
| raise ValueError(f"Failed to parse Redis data: {str(e)}") from e |
There was a problem hiding this comment.
even if one of the items in Redis is added incorrectly, should we still proceed with processing or error out on the whole batch?
maybe we should remove it from the batch if we're not going to process it?
There was a problem hiding this comment.
Thank you for flagging this; I wanted to revisit this before sending it for review, but then the weekend came and I forgot.
Yeah, I agree that log.exception and continue is probably our preferred approach here. May even make it a required parameter to make the callsites obvious about it.
| in event_data.trigger_group_to_dcg_model[ | ||
| DataConditionHandler.Group.WORKFLOW_TRIGGER | ||
| ] | ||
| ): | ||
| workflow_triggers.add(dcg) | ||
| elif ( | ||
| dcg.id | ||
| in trigger_group_to_dcg_model[DataConditionHandler.Group.ACTION_FILTER] | ||
| in event_data.trigger_group_to_dcg_model[ | ||
| DataConditionHandler.Group.ACTION_FILTER | ||
| ] |
There was a problem hiding this comment.
something i noticed about my own code is that we could probably just do set intersection to figure out which ones are workflow triggers and which ones are action filters
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #93569 +/- ##
===========================================
+ Coverage 37.97% 83.21% +45.24%
===========================================
Files 9774 10333 +559
Lines 552441 596806 +44365
Branches 23194 23194
===========================================
+ Hits 209769 496612 +286843
+ Misses 342213 99735 -242478
Partials 459 459 |
| workflow_id = event_data.dcg_to_workflow.get(dcg.id) | ||
| workflow_env = workflows_to_envs[workflow_id] if workflow_id else None |
|
|
||
| logger.info( | ||
| "delayed_workflow.workflows", | ||
| extra={ | ||
| "data": workflow_event_dcg_data, | ||
| "workflows": set(dcg_to_workflow.values()), | ||
| "data": redis_data, |
There was a problem hiding this comment.
i had to google if this would log nicely and it will! yay
The core aim here is to bundle up all of the redis-derived data (still needs another naming pass) and pass that around as an immutable fact rather than obscuring things by having various dicts being passed everywhere.
Still some weirdness here, but the aim is to bundle up all of the redis-derived data (TODO: better name for it) and pass that around as an immutable fact rather than obscuring things by having various dicts being passed everywhere.