[Internal] Update Jobs GetRun API to support paginated responses for jobs and ForEach tasks with >100 runs #324
Conversation
.codegen/_openapi_sha
Outdated
| 37e2bbe0cbcbbbe78a06a018d4fab06314a26a40 No newline at end of file | ||
| universe:/Users/ricardo.costa/universe |
There was a problem hiding this comment.
Still looking to get an official SHA here.
ricardoc-db
changed the title
ricardoc-db
requested review from
mgyucht,
gkiko10,
dvkuznietsov and
hectorcast-db
| if (paginatingIterations) { | ||
| run.getIterations().addAll(currRun.getIterations()); | ||
| } else { | ||
| run.getTasks().addAll(currRun.getTasks()); | ||
| } |
There was a problem hiding this comment.
we need this condition because in case of For each run, the singular task keeps showing up in the tasks array?
There was a problem hiding this comment.
My understanding is that we're only ever paginating one of the two fields, and never both at the same time.
This way we avoid having duplicate entries in the array that is not being paginated.
| } | ||
|
|
||
| // now that we've added all pages to the Run, the token is useless | ||
| run.setNextPageToken(null); |
There was a problem hiding this comment.
previous page token too I suppose
|
|
||
| @Override | ||
| public Run getRun(GetRunRequest request) { | ||
| Run run = super.getRun(request); |
There was a problem hiding this comment.
Is this calling API 2.2 or 2.1? I can't find where the version is defined
There was a problem hiding this comment.
Currently 2.1. The implementation is supposed to work with both (as long as the types are there).
There was a problem hiding this comment.
If I understand correctly you will override the String path in JobsImpl.java variable later?
There was a problem hiding this comment.
JobsImpl.java is generated. It follows that the String path there is also generated. My understanding is that when the version is updated on the proto, the next generation will automatically update those paths.
ricardoc-db
force-pushed
the
jobs-get-run-2.2-behind-the-scenes-pagination
branch
from
08727a7 to
6981221
Compare
| for (long runId : taskRunIds) { | ||
| tasks.add(new RunTask().setRunId(runId)); | ||
| } | ||
| run.setIterations(tasks); |
There was a problem hiding this comment.
You mean run.setTasks I assume?
There was a problem hiding this comment.
Indeed, thanks for spotting that. The test was passing previously because the expected object was also being constructed in the same way.
Fixed: 9967447
| } | ||
|
|
||
| @Override | ||
| public Run getRun(GetRunRequest request) { |
There was a problem hiding this comment.
Ditto here, we may want to have a separate doccomment.
There was a problem hiding this comment.
Ditto here
I may have missed another comment of yours.
I added a docstring to this method in this commit: e9eec92 Is this what you were looking for?
There was a problem hiding this comment.
Ah, you were referring to this? databricks/databricks-sdk-py#725 (comment)
There was a problem hiding this comment.
Improved further to explicitly mention the 100 task/iteration limit: 057f140
Did not attempt. |
|
@mgyucht tested manually against a job with 100+ tasks as well as a ForEach run with 100+ iterations, hardcoding both v 2.1 and 2.2. All combinations worked as expected. |
mgyucht
changed the title
ricardoc-db
changed the title
|
Removing myself from the reviewer list as it seems my review is not explicitly needed/requested. Please add me back if that is incorrect. |
|
Closed in favour of a new pr linked above |
Changes
Jobs GetRun 2.2 behind the scenes pagination
Tests
Unit tests