Skip to content

Remove offset-based pagination from list_jobs function in DatabricksHook - #34926

Merged
potiuk merged 8 commits into
apache:mainfrom
oleksiidav:databricks-listjobs-deprecate-offset
Nov 3, 2023
Merged

Remove offset-based pagination from list_jobs function in DatabricksHook#34926
potiuk merged 8 commits into
apache:mainfrom
oleksiidav:databricks-listjobs-deprecate-offset

Conversation

@oleksiidav

@oleksiidavoleksiidav commented Oct 13, 2023

Copy link
Copy Markdown
Contributor

Databricks has deprecated the offset parameter from ListJobs API, and to soften and control the inevitable breaking change for Airflow users, as well as to conceal the implementation details we are removing the offset-based pagination from all connectors.

This is a follow-up to #33472. Airflow users passing offsets to list_jobs should be receiving deprecation warnings already starting from this release.

@oleksiidav

Copy link
Copy Markdown
ContributorAuthor

@alexott could you please take a look and assign a maintainer to review if you can?

@eladkal

eladkal commented Oct 13, 2023

Copy link
Copy Markdown
Contributor

This is a follow-up to #33472. Airflow users passing offsets to list_jobs have been receiving deprecation warnings for the last month.

#33472 have not been released yet so no have been warned

@oleksiidav

Copy link
Copy Markdown
ContributorAuthor

#33472 have not been released yet so no have been warned

I see @eladkal ! In any case, there is at least a month of time between that PR and this one. So this change can surely go into the following release after this one, assuming monthly release cadence

@Lee-W

Copy link
Copy Markdown
Member

Do we need to do something like #34738 (comment) in this PR as well? Looks like a breaking change.

@uranusjr

Copy link
Copy Markdown
Member

Yes we do. Or to not make this a breaking change, we can continue to accept the argument but just ignore it with a warning. I would prefer dropping it entirely though.

@oleksiidav

Copy link
Copy Markdown
ContributorAuthor

Thanks @uranusjr, I have added the breaking change note the changelog

@oleksiidav
oleksiidavforce-pushed the databricks-listjobs-deprecate-offset branch from 92f50c9 to 1df7863CompareOctober 24, 2023 14:31
@oleksiidav

Copy link
Copy Markdown
ContributorAuthor

Hi @eladkal, could you ptal? This is ready to be merged

Comment threadairflow/providers/databricks/CHANGELOG.rst Outdated
Comment threadairflow/providers/databricks/CHANGELOG.rst Outdated
@eladkal

Copy link
Copy Markdown
Contributor

Please also add 5.0.0 entry to versions in provider.yaml:

versions:
- 4.6.0
- 4.5.0
- 4.4.0

@oleksiidav

Copy link
Copy Markdown
ContributorAuthor

Thanks @eladkal ! Applied changes

@harishkesavaraoharishkesavarao left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, agree with removing the offset altogether.

@oleksiidav

Copy link
Copy Markdown
ContributorAuthor

I have spent some time but I'm really not sure where the bad revision error is coming from in provider docs GH workflow. I suspect it's the different commit introducing v4.7.0 in between before I added 5.0.0. @eladkal could I ask you to take over and merge? Or at least provide advice how to solve this?

@potiuk

potiuk commented Oct 31, 2023

Copy link
Copy Markdown
Member

You need to wait until 4.7.0 is released. It is being voted currently - but not yet released and the error you see is when documentation is generated to show the difference between released 4.7.0 and your 5.0.0 - but since 4.7.0 is not yet released (and tag not set) this is impossible to generate the difference from 4.7.0.

Just wait until 4.7.0 is released (will be announced on the devlist - likely tomorrow) and rebase. It should get fixed.

See https://lists.apache.org/thread/mpbkbdwftq7bmfx5vdnd9tz36xmq8rwc

@oleksiidav

Copy link
Copy Markdown
ContributorAuthor

Ah, thank you very much @potiuk! Will wait for a few days

@oleksiidav

Copy link
Copy Markdown
ContributorAuthor

Hi @potiuk@eladkal, let's merge!

@potiuk
potiuk merged commit 10bac85 into apache:mainNov 3, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@oleksiidav@eladkal@Lee-W@uranusjr@potiuk@harishkesavarao