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

[AIRFLOW-6040] ReadTimeoutError should not raise exception #7616

Merged
merged 1 commit into from
Mar 4, 2020

Conversation

dimberman
Copy link
Contributor

@dimberman dimberman commented Mar 3, 2020


Issue link: AIRFLOW-6040

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID*
  • Unit tests coverage for changes (not needed for documentation changes)
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

* For document-only changes commit message can start with [AIRFLOW-XXXX].


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.
Read the Pull Request Guidelines for more information.

@boring-cyborg boring-cyborg bot added area:Scheduler including HA (high availability) scheduler k8s labels Mar 3, 2020
@dimberman dimberman requested a review from ashb March 3, 2020 20:53
@dimberman
Copy link
Contributor Author

continuation of #6643

@dimberman dimberman changed the title [AIRFLOW_6040] ReadTimeoutError should not raise an exception [AIRFLOW_6040] ReadTimeoutError should not raise exception Mar 3, 2020
@dimberman dimberman changed the title [AIRFLOW_6040] ReadTimeoutError should not raise exception [AIRFLOW-6040] ReadTimeoutError should not raise exception Mar 3, 2020
@dimberman dimberman force-pushed the AIRFLOW_6040-handle-timeouts branch from a50887d to 5658156 Compare March 3, 2020 21:01
@dimberman dimberman force-pushed the AIRFLOW_6040-handle-timeouts branch from 5658156 to 6f61cdb Compare March 3, 2020 21:06
@dimberman
Copy link
Contributor Author

cc: @pvcnt @sbrandtb

@kaxil kaxil merged commit c0e5da5 into apache:master Mar 4, 2020
@sbrandtb
Copy link
Contributor

sbrandtb commented Mar 4, 2020

@dimberman I do not fully agree with making this a warning. Warnings often confuse people and this read timeout is totally expected. I would even demote it to debug because we would only look into this if something is fishy anyways.

@maxirus
Copy link

maxirus commented Mar 5, 2020

I don't think this is the right approach for fixing AIRFLOW-6040. The Watcher isn't being ran in its own thread so it must timeout so it's not blocking further execution. Therefor it's not an error or a warning as this is the expected behavior you're looking for.

galuszkak pushed a commit to FlyrInc/apache-airflow that referenced this pull request Mar 5, 2020
@sbrandtb
Copy link
Contributor

sbrandtb commented Mar 5, 2020

@maxirus While I agree with this being the wrong approach to fix the issue, I beg to differ: Yes, it is not run in a separate thread. But it is run in a separate process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Scheduler including HA (high availability) scheduler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants