AIP-83 amendment -- update backfill#46248
Conversation
prabhusneha
force-pushed
the
update_backfill_logic
branch
from
88f3c48 to
42a19c9
Compare
jedcunningham
added
the
AIP-83
| log.info("Doing session rollback.") | ||
| session.rollback() | ||
|
|
||
| should_create_backfill_dag_run( |
There was a problem hiding this comment.
do you know why we are calling should_create_backfill_dag_run here?
this seems might be a mistake?
There was a problem hiding this comment.
Here should_create_backfill_dag_run is called to create backfill dag run record if the insert fails.
Created a separate function for this to reuse in try and error handling
There was a problem hiding this comment.
But if insert fails, wouldn’t a new insert still fail?
There was a problem hiding this comment.
The try block creates a backfill by inserting into dag_run table and if that fails, the error handling here is inserting a row in backfill_dag_run table with non_create_reason.
This was suggested here - #46248 (comment)
There was a problem hiding this comment.
If there are any concerns I can address them in another PR along with adding a test
There was a problem hiding this comment.
So the logic is (simplified)
- Check if there’s an existing run
- If so, return
- Otherwise, try to create a new run
- If that succeeds, good.
- Otherwise, race condition? Go to
should_create_backfill_dag_runagain; in this case,_get_latest_dag_run_row_queryshould return that conflicting run instead, and we only insert a new BackfillDagRun without a new DagRun.
It might be better if it’s clearer that the DagRun fetched in the second should_create_backfill_dag_run call should be a different one, but I’m not exactly sure how. Maybe call _get_latest_dag_run_row_query in _create_backfill_dag_run instead? Not sure.
There was a problem hiding this comment.
yeah the problem here is in part naming.
a function called "should_create_x" reads like it will just give you information. so you would not expect it to be creating records, or to depend on it to create a record.
otherwise it's confusing. you could either get rid of this method, or limit it to only give info (e.g. maybe just return BackfillDagRunExceptionReason | None and do the insert conditionally at call site)
Adding a test in a separate PR |
@prabhusneha I think Rahul has added test here: 46605. Please check. |
8 participants
Closes: #46202
Depends on: #46187
Once uniqueness on logical date is restored, the change in backfill logic handles the removal of locking of dagrun that was added to avoid the creation of multiple concurrent dag runs for a given logical date.