Page MenuHomeElementl

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

Authored by yuhan on May 11 2021, 10:55 PM.

Details

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

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 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
yuhan requested review of this revision.May 12 2021, 4:35 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

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

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

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)

I think this looks good!

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

don't need this anymore