[Internal] Update Jobs GetRun API to support paginated responses for jobs and ForEach tasks with >100 runs #1009
Conversation
There was a problem hiding this comment.
First round of comments. Mostly looks good, my main feedback is about the tests/comments but the behavior seems OK. Were you able to manually test this?
@dvkuznietsov @gkiko10: I think we should think carefully about whether we want to include the non-paginated repeated fields that have a fixed value in every response (i.e. that the same JobClusters are returned in each GetRun request for different pages). This means that if you want to add pagination for any other fields in the response (like JobClusters), that would be a behavior change of your API, and we wouldn't be able to release the client code in the SDKs until that behavior change is released everywhere. Otherwise, those fields would contain a copy of the nested resources for each requested page (e.g. if there was 1 job cluster but five pages of tasks, the SDK would return a run with 5 job clusters). If we get this behavior right from the start, we can avoid this.
| return nil, err | ||
| } | ||
|
|
||
| isPaginatingIterations := run.Iterations != nil && len(run.Iterations) > 0 |
There was a problem hiding this comment.
For posterity, it would be good to document this behavior, as it isn't immediately obvious how this works from the spec itself. Something like:
When querying a Job run, a page token is returned when there are more than 100 tasks. No iterations are defined for a Job run. Therefore, the next page in the response only includes the next page of tasks.
When querying a ForEach task run, a page token is returned when there are more than 100 iterations. Only a single task is returned, corresponding to the ForEach task itself. Therefore, the client only reads the iterations from the next page and not the tasks.
For any other task type, iterations is empty and tasks only contains the one task, so no pagination is needed.
service/jobs/ext_api_test.go
Outdated
| assert.Empty(t, run.NextPageToken) | ||
| assert.Empty(t, run.PrevPageToken) |
There was a problem hiding this comment.
Not now, but longer-term, we may want to hide these fields from the SDK in general. cc @hectorcast-db @renaudhartert-db
service/jobs/ext_api_test.go
Outdated
| }, | ||
| }, | ||
| }, | ||
| //API 2.1 stubs |
There was a problem hiding this comment.
Are these used in any test?
There was a problem hiding this comment.
The current generated code of jobs/impl.go references 2.1 endpoints. Once the new docs are public they will reference 2.2 endpoint. The tests will not fail because they mock both 2.1 and 2.2
mgyucht
changed the title
gkiko10
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
renaudhartert-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
Test Details: go/deco-tests/11730422764 |
|
Closing in favour of #1089 |
Pull request was closed
Changes
Jobs GetRun 2.2 behind the scenes pagination
Tests
make testpassingmake fmtapplied