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

[Internal] Update Jobs GetRun API to support paginated responses for jobs and ForEach tasks with >100 runs #725

Closed
wants to merge 13 commits into from

Conversation

gkiko10
Copy link
Contributor

@gkiko10 gkiko10 commented Aug 13, 2024

Changes

Jobs GetRun 2.2 behind the scenes pagination

Tests

  • make test run locally
  • make fmt applied
  • relevant integration tests applied

Copy link
Contributor

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just a few small nits about style & test cases. The main question I have is whether we should add an extra documentation comment for this method.

databricks/sdk/mixins/jobs.py Outdated Show resolved Hide resolved
databricks/sdk/mixins/jobs.py Show resolved Hide resolved
requests_mock.get(make_path_pattern(1337, "tokenToThirdPage"), text=json.dumps(run3))
w = WorkspaceClient(config=config)

run = w.jobs.get_run(1337, page_token="initialToken")
Copy link
Contributor

Choose a reason for hiding this comment

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

Will users ever specify an initial token?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point users cannot get initial page token from any SDK function. So this parameter is redundant. The function that is overloaded is generated from openApi spec and it also comes with the page_token param. So I assumed that it's ok to repeat the same function contract in the mixin as well.
I see two paths:

  1. we hide the page_token param in the mixin and make our change opaque to the users
  2. we do not hide this parameter and stay consistent with the openApi spec for this spec and for the other functions that use page_token. Maybe in the future the initial token will be exposed to the users 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed offline. Let's leave the parameter there so the mixin matches the underlying API. We'll clean up token parameters separately and can remove it here when we do that.

requests_mock.get(make_path_pattern(1337, "tokenToThirdPage"), text=json.dumps(run3))
w = WorkspaceClient(config=config)

run = w.jobs.get_run(1337, page_token="initialToken")
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto about the initial token?

@gkiko10 gkiko10 requested a review from ricardoc-db August 16, 2024 13:56
@mgyucht mgyucht changed the title [Internal] Add customization for GetRun [Feature] Added support for ForEach tasks and jobs with >100 tasks in the GetRun API Aug 16, 2024
@mgyucht mgyucht changed the title [Feature] Added support for ForEach tasks and jobs with >100 tasks in the GetRun API [Internal] Update Jobs GetRun API to support paginated responses for jobs and ForEach tasks with >100 runs Aug 19, 2024
@mgyucht mgyucht enabled auto-merge August 19, 2024 07:27
@mgyucht mgyucht disabled auto-merge August 19, 2024 07:30
@gkiko10 gkiko10 temporarily deployed to test-trigger-is November 1, 2024 09:38 — with GitHub Actions Inactive
Copy link

github-actions bot commented Nov 7, 2024

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/sdk-py

Inputs:

  • PR number: 725
  • Commit SHA: e728169979e03b3640808fbe38826f62b5c64a95

Checks will be approved automatically on success.

@eng-dev-ecosystem-bot
Copy link
Collaborator

Test Details: go/deco-tests/11727266120

@gkiko10 gkiko10 changed the title [Internal] Update Jobs GetRun API to support paginated responses for jobs and ForEach tasks with >100 runs [Internal] Update Jobs GetRun API to support paginated responses for jobs and ForEach tasks Nov 8, 2024
@gkiko10 gkiko10 changed the title [Internal] Update Jobs GetRun API to support paginated responses for jobs and ForEach tasks [Internal] Update Jobs GetRun API to support paginated responses for jobs and ForEach tasks with >100 runs Nov 8, 2024
@gkiko10
Copy link
Contributor Author

gkiko10 commented Nov 8, 2024

closing in favour of #819

@gkiko10 gkiko10 closed this Nov 8, 2024
auto-merge was automatically disabled November 8, 2024 14:35

Pull request was closed

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.

6 participants