Skip to content
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

Add capacity to list jobs by ID + make default #307

Merged
merged 1 commit into from
Apr 21, 2024
Merged

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Apr 20, 2024

Here, in addition to the new job list sort orders added by #304, add one
more for sorting by job ID, and make it the default.

This is another breaking change in the job list API, and I wouldn't
normally make it at this point, but our next release will contain a
number of breaking changes, including to the job list API, so the time
is now.

My rationale for the change:

  • Listing and paginating by ID is an overwhelmingly common pattern in
    web and database APIs, and I think it being the default would be more
    intuitive for more people.

  • Ordering by ID is more ergonomic because no JobListParams.States
    invocation needs to made. It's shorter, and especially when a fairly
    normal use case will be to iterate across all job rows, this is a
    minor nicety.

  • Ordering by ID allows the entire job collection to be iterated
    regardless of job state. JobListOrderByTime requires a state, and
    some order is needed for cursors to work.

  • The behavior of JobListOrderByTime is a little odd in that it
    changes dynamically based on the requested list states, and there's no
    way to intuit what the order will be without knowing a lot about River
    internals and thinking very carefully about it. Furthermore, the time
    that'd be chosen wasn't documented anywhere, so the only way to know
    for sure what it would be was to read River's source code.

  • With the inclusion of Change JobList ergnomics - add filtering by states, return cursor from JobList function #304, JobListOrderByTime's behavior has gotten
    even a little more surprising because the state to be chosen to select
    a timestamp was the first value sent to JobListParams.States, with
    any additional values sent ignored, also creating a somewhat nonsensical
    result (e.g. States(running, available) would select attempted_at,
    but would be NULL for any jobs in the available state). This
    behavior was not documented.

I also found a bug that was a hold over from #304 as more than one sort
order became available. The function JobListCursorFromJob took a sort
order, but would produce the wrong result unless the user remembered to
set the exact same sort order on their job list parameters. For example,
this would do the wrong thing:

res, err = client.JobList(ctx, NewJobListParams().After(JobListCursorFromJob(job4, JobListOrderByScheduledAt)))

JobListCursorFromJob would extract scheduled_at from job4, but
then list using to the default job list order. Previously that was based
on time, so this result would've been wrong unless the job list
parameters filtered to state available, retryable, or scheduled so
that scheduled_at was also used when comparing to other jobs.

A caller could compensate by specifying sort order in both places, but
this is pretty ugly, and there was no check to make sure that the list
paramaters and cursor shared the same order:

res, err = client.JobList(ctx, NewJobListParams().OrderBy(JobListOrderByScheduledAt, SortOrderAsc).After(JobListCursorFromJob(job4, JobListOrderByScheduledAt)))

The corrected version of this doesn't use an order when initializing the
cursor, instead using the one from the job list params, meaning that the
same time field is always used between list query and what's extracted
from the cursor's job row.

res, err = client.JobList(ctx, NewJobListParams().OrderBy(JobListOrderByScheduledAt, SortOrderAsc).After(JobListCursorFromJob(job4)))

This also makes the invocation shorter and more ergonomic to call.

Along with the above, we also make a few tweaks to documentation.
JobListOrderByTime now documents which timestamps it uses, and
indicates that only the first value sent to JobListParams.States will
be respected.

@brandur brandur requested a review from bgentry April 20, 2024 01:12
@brandur
Copy link
Contributor Author

brandur commented Apr 20, 2024

@bgentry Okay, you're probably not going to love this one, but since we're changing around the API somewhat anyway, I wanted to pitch it. Mind taking a look?

client_test.go Outdated
@@ -1600,26 +1617,60 @@ func Test_Client_JobList(t *testing.T) {
job5 := testfactory.Job(ctx, t, bundle.exec, &testfactory.JobOpts{State: ptrutil.Ptr(rivertype.JobStateCompleted), ScheduledAt: ptrutil.Ptr(now.Add(-7 * time.Second)), FinalizedAt: ptrutil.Ptr(now.Add(-5 * time.Second))})
job6 := testfactory.Job(ctx, t, bundle.exec, &testfactory.JobOpts{State: ptrutil.Ptr(rivertype.JobStateCompleted), ScheduledAt: ptrutil.Ptr(now.Add(-7 * time.Second)), FinalizedAt: &now})

res, err := client.JobList(ctx, NewJobListParams().States(rivertype.JobStateAvailable).After(JobListCursorFromJob(job1, JobListOrderByTime)))
//
// JobListOrderByID
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it's too much nesting to separate these into separate cases? it's definitely a lot of stuff for a single case IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broke it up into three test cases and added a few more checks for JobListOrderByScheduled which was a little light on them.

Here, in addition to the new job list sort orders added by #304, add one
more for sorting by job ID, and make it the default.

This is another breaking change in the job list API, and I wouldn't
normally make it at this point, but our next release will contain a
number of breaking changes, including to the job list API, so the time
is now.

My rationale for the change:

* Listing and paginating by ID is an overwhelmingly common pattern in
  web and database APIs, and I think it being the default would be more
  intuitive for more people.

* Ordering by ID is more ergonomic because no `JobListParams.States`
  invocation needs to made. It's shorter, and especially when a fairly
  normal use case will be to iterate across all job rows, this is a
  minor nicety.

* Ordering by ID allows the entire job collection to be iterated
  regardless of job state. `JobListOrderByTime` requires a state, and
  some order is needed for cursors to work.

* The behavior of `JobListOrderByTime` is a little odd in that it
  changes dynamically based on the requested list states, and there's no
  way to intuit what the order will be without knowing a lot about River
  internals and thinking very carefully about it. Furthermore, the time
  that'd be chosen wasn't documented anywhere, so the only way to know
  for sure what it would be was to read River's source code.

* With the inclusion of #304, `JobListOrderByTime`'s behavior has gotten
  even a little more surprising because the state to be chosen to select
  a timestamp was the _first_ value sent to `JobListParams.States`, with
  any additional values sent ignored, also creating a somewhat nonsensical
  result (e.g. `States(running, available)` would select `attempted_at`,
  but would be `NULL` for any jobs in the `available` state). This
  behavior was not documented.

I also found a bug that was a hold over from #304 as more than one sort
order became available. The function `JobListCursorFromJob` took a sort
order, but would produce the wrong result unless the user remembered to
set the exact same sort order on their job list parameters. For example,
this would do the wrong thing:

    res, err = client.JobList(ctx, NewJobListParams().After(JobListCursorFromJob(job4, JobListOrderByScheduledAt)))

`JobListCursorFromJob` would extract `scheduled_at` from `job4`, but
then list using to the default job list order. Previously that was based
on time, so this result would've been wrong _unless_ the job list
parameters filtered to state `available`, `retryable`, or `scheduled` so
that `scheduled_at` was also used when comparing to other jobs.

A caller could compensate by specifying sort order in both places, but
this is pretty ugly, and there was no check to make sure that the list
paramaters and cursor shared the same order:

    res, err = client.JobList(ctx, NewJobListParams().OrderBy(JobListOrderByScheduledAt, SortOrderAsc).After(JobListCursorFromJob(job4, JobListOrderByScheduledAt)))

The corrected version of this doesn't use an order when initializing the
cursor, instead using the one from the job list params, meaning that the
same time field is always used between list query and what's extracted
from the cursor's job row.

    res, err = client.JobList(ctx, NewJobListParams().OrderBy(JobListOrderByScheduledAt, SortOrderAsc).After(JobListCursorFromJob(job4)))

This also makes the invocation shorter and more ergonomic to call.

Along with the above, we also make a few tweaks to documentation.
`JobListOrderByTime` now documents which timestamps it uses, and
indicates that only the first value sent to `JobListParams.States` will
be respected.
@brandur brandur force-pushed the brandur-id-order-2 branch from afd173f to 9874f19 Compare April 21, 2024 04:32
@brandur
Copy link
Contributor Author

brandur commented Apr 21, 2024

Thanks!

@brandur brandur merged commit e791aef into master Apr 21, 2024
10 checks passed
@brandur brandur deleted the brandur-id-order-2 branch April 21, 2024 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants