Page MenuHomeElementl
Differential D7878

monitor sensor 1/ rename cursor to before_cursor, add arg after_cursor and ascending to RunStorage.get_runs
AbandonedPublic
Actions

Authored by • yuhan on May 11 2021, 10:55 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 21, 3:16 AM
Unknown Object (File)
Mon, Mar 13, 3:17 AM
Unknown Object (File)
Fri, Mar 3, 7:30 AM
Unknown Object (File)
Feb 21 2023, 8:06 PM
Unknown Object (File)
Feb 21 2023, 8:05 PM
Unknown Object (File)
Feb 21 2023, 8:04 PM
Unknown Object (File)
Feb 14 2023, 6:28 AM
Unknown Object (File)
Jan 31 2023, 10:56 PM
View All 69 Files
Subscribers
None

Details

Reviewers
• prha
• dgibson
alangenfeld
Summary
  • cursor becomes before_cursor
  • add after_cursor for querying runs after a cursor
  • add ascending. defaults to False.
runs = [1,2,3,4,5,6]
get_runs(before_cursor=3) = [2,1] # existing behavior
get_runs(before_cursor=6, limit=1) = [5] # existing behavior
get_runs(before_cursor=6, limit=1, ascending=True) = [1]
get_runs(before_cursor=6, after_cursor=3) = [5,4]
get_runs(after_cursor=1, limit=2) = [6,5]
get_runs(after_cursor=1, ascending=True, limit=2) = [2,3] # this will be used most of the time for after cursor)
Test Plan

bk

Diff Detail

Repository
R1 dagster
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

• yuhan updated this revision to Diff 37422.May 11 2021, 10:55 PM
• yuhan created this revision.

up

• yuhan updated this revision to Diff 37426.May 11 2021, 11:27 PM

add test

• yuhan updated this revision to Diff 37427.May 11 2021, 11:30 PM

up

Harbormaster failed remote builds in B30443: Diff 37421!May 12 2021, 12:01 AM
Harbormaster failed remote builds in B30444: Diff 37422!
Harbormaster failed remote builds in B30447: Diff 37426!May 12 2021, 12:37 AM
Harbormaster returned this revision to the author for changes because remote builds failed.May 12 2021, 12:42 AM
Harbormaster failed remote builds in B30448: Diff 37427!
• yuhan updated this revision to Diff 37435.May 12 2021, 4:11 AM

rebase

• yuhan retitled this revision from add arg after_cursor to RunStorage.get_runs to monitor sensor 1/ add arg after_cursor to RunStorage.get_runs.May 12 2021, 4:25 AM
Harbormaster completed remote builds in B30455: Diff 37435.May 12 2021, 4:35 AM
• yuhan requested review of this revision.May 12 2021, 4:35 AM
• yuhan added a child revision: D7476: monitor sensor prototype.May 12 2021, 5:45 AM
• dgibson requested changes to this revision.May 12 2021, 3:51 PM

Goal here seems very reasonable. Question for you and other reviewers - Would adding an ascending=True/False flag be another way to deal with this? (i.e. the same cursor could be either a before_cursor or after_cursor depending on the order that you want the results)?

The benefit of that that I can see is that after this diff there's still no way to get the first N elements after a cursor (you can only get the *last* N elements after a cursor, or between two cursors). But if you're paginating through a list going forwards with an after_cursor, you probably want the first N not the last N right? And an ascending flag would accomplish that.

(And most of the time you want to set a limit when paginating with a cursor I think, or you can get an unbounded number of results)

This revision now requires changes to proceed.May 12 2021, 3:51 PM
• yuhan added a comment.May 12 2021, 7:18 PM

to make sure im understanding it correctly, say we have runs = [1,2,3,4,5,6,7,8,9,10] and we will get:
get_runs(cursor=5, ascending=True, limit=2) = [6,7] where cursor is an after cursor
get_runs(cursor=5, ascending=False, limit=2) = [2,1] where cursor is a before cursor

my reservation is the ascending + cursor combined logic could be confusing. how about adding both of them and deprecate cursor to rename to before_cursor? so we'll have get_runs(filters=None, before_cursor=None, limit=None, after_cursor=None, ascending=False, cursor=None) which is similar to what we've done in D6234

• dgibson added a comment.May 12 2021, 11:52 PM

That would work too - the most important thing to me is that you’re able to
paginate in a forwards direction with a limit

• yuhan updated this revision to Diff 37498.May 13 2021, 6:22 AM

cursor -> before_cursor
+ ascending=False

• yuhan retitled this revision from monitor sensor 1/ add arg after_cursor to RunStorage.get_runs to monitor sensor 1/ rename cursor to before_cursor, add arg after_cursor and ascending to RunStorage.get_runs.May 13 2021, 6:28 AM
• yuhan edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B30506: Diff 37498.May 13 2021, 6:45 AM
• prha accepted this revision.May 14 2021, 8:25 PM

I think this looks good!

• dgibson accepted this revision.May 17 2021, 2:05 PM

oops did not realize I was still blocking this, sorry!

This revision is now accepted and ready to land.May 17 2021, 2:05 PM
• yuhan abandoned this revision.Jun 3 2021, 11:28 PM

don't need this anymore

Revision Contents
Changeset List

PathSize
python_modules/
dagster/
dagster/
core/
instance/
12 lines
storage/
runs/
26 lines
74 lines
72 lines
dagster_tests/
core_tests/
storage_tests/
utils/
33 lines

Diff 37498

View Options

python_modules/dagster/dagster/core/instance/__init__.py

Loading...
View Options

python_modules/dagster/dagster/core/storage/runs/base.py

Loading...
View Options

python_modules/dagster/dagster/core/storage/runs/in_memory.py

Loading...
View Options

python_modules/dagster/dagster/core/storage/runs/sql_run_storage.py

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/storage_tests/utils/run_storage.py

Loading...