Skip to content

Conversation

@oleksiidav
Copy link
Contributor

@oleksiidav oleksiidav commented Aug 17, 2023

Update Airflow to switch from default offset-based pagination (deprecated) to using a page token

@boring-cyborg
Copy link

boring-cyborg bot commented Aug 17, 2023

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@eladkal
Copy link
Contributor

eladkal commented Aug 17, 2023

What is JOBS-13077 ?

@oleksiidav oleksiidav changed the title [JOBS-13077] Switch ListJobs to token-based pagination Switch ListJobs to token-based pagination Aug 17, 2023
@oleksiidav
Copy link
Contributor Author

What is JOBS-13077 ?

Tracking ticket from Databricks side, nevermind

@oleksiidav oleksiidav force-pushed the listjobs-token-pagination branch from f60fa07 to 16d9232 Compare August 18, 2023 15:02
@oleksiidav oleksiidav requested review from alexott and eladkal August 18, 2023 15:02
Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

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

code looks good - can we add a unit test for token usage?

@oleksiidav oleksiidav requested a review from alexott August 21, 2023 13:12
@oleksiidav
Copy link
Contributor Author

Added test @alexott, PTAL!

Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

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

lgtm.

@eladkal WDYT?

@oleksiidav oleksiidav force-pushed the listjobs-token-pagination branch from 91d173e to 7ff7fc8 Compare August 23, 2023 08:17
@oleksiidav
Copy link
Contributor Author

Hi @eladkal, could you please take another look?

@potiuk
Copy link
Member

potiuk commented Aug 24, 2023

LGTM @eladkal ?

@oleksiidav oleksiidav force-pushed the listjobs-token-pagination branch from 2c6add3 to c8c1c0f Compare August 25, 2023 14:53
@oleksiidav oleksiidav force-pushed the listjobs-token-pagination branch from 276cdec to fb5fd82 Compare August 28, 2023 12:44
@oleksiidav
Copy link
Contributor Author

Heyy @eladkal , I have updated the tests/formatting, can we pls merge?

@eladkal eladkal changed the title Switch ListJobs to token-based pagination Update list_jobs function in DatabricksHook to token-based pagination Sep 8, 2023
@eladkal
Copy link
Contributor

eladkal commented Sep 11, 2023

@oleksiidav static checks are failing you are missing import of AirflowProviderDeprecationWarning

@potiuk
Copy link
Member

potiuk commented Sep 11, 2023

Static check to fix.

@oleksiidav
Copy link
Contributor Author

Thanks @eladkal @potiuk ! Fixed it now. Did not have access to test/linter workflow to run checks

@potiuk potiuk force-pushed the listjobs-token-pagination branch from 0dded80 to 85df103 Compare September 11, 2023 20:39
@potiuk potiuk merged commit dfec053 into apache:main Sep 12, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Sep 12, 2023

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

Sign up for free to 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.

4 participants