-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[DS][49/n] Update static constructors to use Since operator #21975
base: 05-23-Allow_AllPartitionsSubsets_in_TimeWindowPartitionMapping
Are you sure you want to change the base?
Conversation
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @OwenKephart and the rest of your teammates on Graphite |
6432e5e
to
3d2806a
Compare
d4b4d48
to
6cf445c
Compare
3d2806a
to
3b9c102
Compare
6cf445c
to
09fbc35
Compare
3b9c102
to
8ddcdff
Compare
09fbc35
to
c4d4898
Compare
8ddcdff
to
4de29a2
Compare
c4d4898
to
0e4b4b8
Compare
4de29a2
to
c5335df
Compare
0e4b4b8
to
384a80c
Compare
c5335df
to
d8978d0
Compare
ff920de
to
1da4642
Compare
d8978d0
to
8f54ed6
Compare
303b5c7
to
d08e907
Compare
d08e907
to
4802b31
Compare
c1e746f
to
bdbaf33
Compare
03f2ed6
to
844091e
Compare
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.
This is all quite slick. I think we need to do a naming pass on all the downstack concepts, but the core structure here is compelling.
bdbaf33
to
068c772
Compare
844091e
to
e121cf7
Compare
068c772
to
c80942d
Compare
e121cf7
to
e5f3e6b
Compare
c80942d
to
6ea95c7
Compare
e5f3e6b
to
3383bad
Compare
6ea95c7
to
792aa21
Compare
3383bad
to
065bd0c
Compare
792aa21
to
5b01fff
Compare
065bd0c
to
ff4397d
Compare
| SchedulingCondition.will_be_requested() | ||
) | ||
all_deps_updated_since_cron = SchedulingCondition.all_deps_match( | ||
SchedulingCondition.newly_updated().since_last_cron_tick(cron_schedule, cron_timezone) |
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.
this is super cool
5b01fff
to
8af990d
Compare
ff4397d
to
5d95b55
Compare
Summary & Motivation
As title -- now we can take advantage of the "since" operator in a variety of different ways to simplify and improve our
on_cron
andeager
"composite conditions".On the
on_cron
side, note that the previous definition had a bad property, which is that if the requested materialization failed, a new one would be immediately re-requested (as nothing would have changed in the eyes of the system -- the parents would still be newer than the cron tick, the child would still be older than the cron tick, and there'd no long be an in-progress run).eager
had handling for this case (temporarily removed downstack), but it was ugly (basically just a fixed retry interval, which is conceptually confusing).This shows off the value of the new
since
operator -- you can just make sure that you've requested a materialization of an asset since the newest time a condition became true, which means that you can guarantee that the cron condition will only become true once per cron tick, because the CronTickPassed condition only becomes true once per cron tick. Similarly, the any_dep_matches(newly_updated) condition will only be true at the instant a parent is updated, so you can guarantee a maximum of one downstream run per parent update, regardless of if that downstream run fails etc.This basically reimplements the current AMP eager behavior, but in a much more principled way, as we assign actual operators and names to these bookkeeping operations, rather than relying on some rather arcane codepaths to magically do a similar thing.
How I Tested These Changes