-
Notifications
You must be signed in to change notification settings - Fork 14k
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
Add timeout parameter to druid operator so it can be used in hook #19984
Add timeout parameter to druid operator so it can be used in hook #19984
Conversation
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)
|
I think you should add/modify unit tests for the hook. Happy to Approve & Run them if you add them @piotrkmita |
…heck every 5 seconds
Hey @potiuk Does it make sense or have you meant something else? |
I thought more about adding a test to |
…out to check every 5 seconds" This reverts commit 21b0076.
I reverted previous commit and added tests for timeout and max_ingestion_time. |
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease. |
@@ -23,6 23,7 @@ | |||
from airflow.utils.types import DagRunType | |||
|
|||
DEFAULT_DATE = timezone.datetime(2017, 1, 1) | |||
DEFAULT_TIMEOUT = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unnecessary? If you want a constant, you should probably put it in the provider code instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've replaced it with variable in the test. Is it fine now?
Awesome work, congrats on your first merged pull request! |
Hello!
We've started using that DruidOperator few weeks ago. We've realized that the DruidHook used there is calling druid overlord every 1 second to get status, because it's using default timeout and it's not possible to set it in the operator itself. In our case ingestion takes around 1 hour, so there is no need to check it that often.
Example log:
We would would like to add timeout parameter to DruidOperator, so we can set how often DruidHook will be calling overlord to get status.
Example log after change with timeout 180 seconds:
Please let me know if I've missed something.
Thank you!
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.