Fix cancel_on_kill after execution timeout for DataprocSubmitJobOperator by tauzen · Pull Request #22955 · apache/airflow

tauzen · 2022-04-12T17:12:28Z

Synchronous tasks killed by execution timeout weren't canceled
due to wrong assignment of job_id property.

boring-cyborg · 2022-04-12T17:12:32Z

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 (flake8, 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.
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

github-actions · 2022-04-12T19:59:21Z

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.

potiuk

nice!

potiuk · 2022-04-13T07:02:36Z

Static checks need fixing

tauzen · 2022-04-13T08:01:09Z

Thanks @potiuk! That's odd that mypy is complaining about this now, and wasn't before 🤔. Will try to fix this.

potiuk · 2022-04-13T10:00:33Z

Thanks @potiuk! That's odd that mypy is complaining about this now, and wasn't before thinking. Will try to fix this.

Mypy does seem to work a bit "unstable'y" - I think I know why and might be we will soon fix it.

tauzen · 2022-04-13T11:00:43Z

It's a bit sad that we need to have this additional job_id variable to keep mypy happy, it might be confusing for future contributors. Do you want me to add some additional comment about this in the code? On the bright side - fewer line changes in this PR.

potiuk · 2022-04-13T11:15:17Z

It's a bit sad that we need to have this additional job_id variable to keep mypy happy, it might be confusing for future contributors. Do you want me to add some additional comment about this in the code? On the bright side - fewer line changes in this PR.

I don't find it wrong. It's not to keep mypy happy, it's to make sure you don't accidentally use None. There is nothing wrong with the way you've done and it needs no explanation

I think in this case mypy gave you hint on how to make your code more redable and less error prone.

Contraray to popular belief peeople should optimise their code for easines of reading and correctness not to "avoid" creating variables. Compileres and interpreters will optimise it away and creating extra, properly named variable is often much better solution than .. adding a comment about it.

I often do that. In this case maybe even name it "new_job_id" and even expilcitly mark it as : str -> that will be the best "explanation" of what it is for and why it's there. And there will be no need for comment.

Comments are overrated on the other hand.

tauzen · 2022-04-13T11:30:59Z

Comments are overrated on the other hand.

Definitely 👍

Could you please clarify if you are ok with the current PR, or do you prefer to s/job_id/new_job_id and add :str to the definition of new_job_id. Either way is ok with me.

potiuk · 2022-04-13T11:40:13Z

Add it - you will see that it's going to be cleaner. And then you can even move self.job_id = new_job_id closer to the end of the function - and separate variabe will suddenly make a lot more sense

Synchronous tasks killed by execution timeout weren't canceled due to wrong assignment of job_id property.

tauzen · 2022-04-14T12:28:10Z

Thanks for the guidance @potiuk! 🙇 Ok, so this PR is approved and labeled with okay to merge, tests are green - can I assume it will be merged at your convenience?

boring-cyborg · 2022-04-14T14:37:56Z

Awesome work, congrats on your first merged pull request!

potiuk · 2022-04-14T14:38:03Z

Done :)

tauzen · 2022-04-14T14:47:14Z

Awesome, thank you very much @potiuk 😄

tauzen · 2022-04-12T17:12:28Z

Synchronous tasks killed by execution timeout weren't canceled
due to wrong assignment of job_id property.

boring-cyborg · 2022-04-12T17:12:32Z

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 (flake8, 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.
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

github-actions · 2022-04-12T19:59:21Z

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.

potiuk

nice!

potiuk · 2022-04-13T07:02:36Z

Static checks need fixing

tauzen · 2022-04-13T08:01:09Z

Thanks @potiuk! That's odd that mypy is complaining about this now, and wasn't before 🤔. Will try to fix this.

potiuk · 2022-04-13T10:00:33Z

Thanks @potiuk! That's odd that mypy is complaining about this now, and wasn't before thinking. Will try to fix this.

Mypy does seem to work a bit "unstable'y" - I think I know why and might be we will soon fix it.

tauzen · 2022-04-13T11:00:43Z

It's a bit sad that we need to have this additional job_id variable to keep mypy happy, it might be confusing for future contributors. Do you want me to add some additional comment about this in the code? On the bright side - fewer line changes in this PR.

potiuk · 2022-04-13T11:15:17Z

It's a bit sad that we need to have this additional job_id variable to keep mypy happy, it might be confusing for future contributors. Do you want me to add some additional comment about this in the code? On the bright side - fewer line changes in this PR.

I don't find it wrong. It's not to keep mypy happy, it's to make sure you don't accidentally use None. There is nothing wrong with the way you've done and it needs no explanation

I think in this case mypy gave you hint on how to make your code more redable and less error prone.

Contraray to popular belief peeople should optimise their code for easines of reading and correctness not to "avoid" creating variables. Compileres and interpreters will optimise it away and creating extra, properly named variable is often much better solution than .. adding a comment about it.

I often do that. In this case maybe even name it "new_job_id" and even expilcitly mark it as : str -> that will be the best "explanation" of what it is for and why it's there. And there will be no need for comment.

Comments are overrated on the other hand.

tauzen · 2022-04-13T11:30:59Z

Comments are overrated on the other hand.

Definitely 👍

Could you please clarify if you are ok with the current PR, or do you prefer to s/job_id/new_job_id and add :str to the definition of new_job_id. Either way is ok with me.

potiuk · 2022-04-13T11:40:13Z

Add it - you will see that it's going to be cleaner. And then you can even move self.job_id = new_job_id closer to the end of the function - and separate variabe will suddenly make a lot more sense

Synchronous tasks killed by execution timeout weren't canceled due to wrong assignment of job_id property.

tauzen · 2022-04-14T12:28:10Z

Thanks for the guidance @potiuk! 🙇 Ok, so this PR is approved and labeled with okay to merge, tests are green - can I assume it will be merged at your convenience?

boring-cyborg · 2022-04-14T14:37:56Z

Awesome work, congrats on your first merged pull request!

potiuk · 2022-04-14T14:38:03Z

Done :)

tauzen · 2022-04-14T14:47:14Z

Awesome, thank you very much @potiuk 😄

tauzen · 2022-04-12T17:12:28Z

Synchronous tasks killed by execution timeout weren't canceled
due to wrong assignment of job_id property.

boring-cyborg · 2022-04-12T17:12:32Z

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 (flake8, 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.
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

github-actions · 2022-04-12T19:59:21Z

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.

potiuk

nice!

potiuk · 2022-04-13T07:02:36Z

Static checks need fixing

tauzen · 2022-04-13T08:01:09Z

Thanks @potiuk! That's odd that mypy is complaining about this now, and wasn't before 🤔. Will try to fix this.

potiuk · 2022-04-13T10:00:33Z

Thanks @potiuk! That's odd that mypy is complaining about this now, and wasn't before thinking. Will try to fix this.

Mypy does seem to work a bit "unstable'y" - I think I know why and might be we will soon fix it.

tauzen · 2022-04-13T11:00:43Z

It's a bit sad that we need to have this additional job_id variable to keep mypy happy, it might be confusing for future contributors. Do you want me to add some additional comment about this in the code? On the bright side - fewer line changes in this PR.

potiuk · 2022-04-13T11:15:17Z

It's a bit sad that we need to have this additional job_id variable to keep mypy happy, it might be confusing for future contributors. Do you want me to add some additional comment about this in the code? On the bright side - fewer line changes in this PR.

I don't find it wrong. It's not to keep mypy happy, it's to make sure you don't accidentally use None. There is nothing wrong with the way you've done and it needs no explanation

I think in this case mypy gave you hint on how to make your code more redable and less error prone.

Contraray to popular belief peeople should optimise their code for easines of reading and correctness not to "avoid" creating variables. Compileres and interpreters will optimise it away and creating extra, properly named variable is often much better solution than .. adding a comment about it.

I often do that. In this case maybe even name it "new_job_id" and even expilcitly mark it as : str -> that will be the best "explanation" of what it is for and why it's there. And there will be no need for comment.

Comments are overrated on the other hand.

tauzen · 2022-04-13T11:30:59Z

Comments are overrated on the other hand.

Definitely 👍

Could you please clarify if you are ok with the current PR, or do you prefer to s/job_id/new_job_id and add :str to the definition of new_job_id. Either way is ok with me.

potiuk · 2022-04-13T11:40:13Z

Add it - you will see that it's going to be cleaner. And then you can even move self.job_id = new_job_id closer to the end of the function - and separate variabe will suddenly make a lot more sense

Synchronous tasks killed by execution timeout weren't canceled due to wrong assignment of job_id property.

tauzen · 2022-04-14T12:28:10Z

Thanks for the guidance @potiuk! 🙇 Ok, so this PR is approved and labeled with okay to merge, tests are green - can I assume it will be merged at your convenience?

boring-cyborg · 2022-04-14T14:37:56Z

Awesome work, congrats on your first merged pull request!

potiuk · 2022-04-14T14:38:03Z

Done :)

tauzen · 2022-04-14T14:47:14Z

Awesome, thank you very much @potiuk 😄

tauzen · 2022-04-12T17:12:28Z

Synchronous tasks killed by execution timeout weren't canceled
due to wrong assignment of job_id property.

boring-cyborg · 2022-04-12T17:12:32Z

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 (flake8, 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.
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

github-actions · 2022-04-12T19:59:21Z

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.

potiuk

nice!

potiuk · 2022-04-13T07:02:36Z

Static checks need fixing

tauzen · 2022-04-13T08:01:09Z

Thanks @potiuk! That's odd that mypy is complaining about this now, and wasn't before 🤔. Will try to fix this.

potiuk · 2022-04-13T10:00:33Z

Thanks @potiuk! That's odd that mypy is complaining about this now, and wasn't before thinking. Will try to fix this.

Mypy does seem to work a bit "unstable'y" - I think I know why and might be we will soon fix it.

tauzen · 2022-04-13T11:00:43Z

It's a bit sad that we need to have this additional job_id variable to keep mypy happy, it might be confusing for future contributors. Do you want me to add some additional comment about this in the code? On the bright side - fewer line changes in this PR.

potiuk · 2022-04-13T11:15:17Z

It's a bit sad that we need to have this additional job_id variable to keep mypy happy, it might be confusing for future contributors. Do you want me to add some additional comment about this in the code? On the bright side - fewer line changes in this PR.

I don't find it wrong. It's not to keep mypy happy, it's to make sure you don't accidentally use None. There is nothing wrong with the way you've done and it needs no explanation

I think in this case mypy gave you hint on how to make your code more redable and less error prone.

Contraray to popular belief peeople should optimise their code for easines of reading and correctness not to "avoid" creating variables. Compileres and interpreters will optimise it away and creating extra, properly named variable is often much better solution than .. adding a comment about it.

I often do that. In this case maybe even name it "new_job_id" and even expilcitly mark it as : str -> that will be the best "explanation" of what it is for and why it's there. And there will be no need for comment.

Comments are overrated on the other hand.

tauzen · 2022-04-13T11:30:59Z

Comments are overrated on the other hand.

Definitely 👍

Could you please clarify if you are ok with the current PR, or do you prefer to s/job_id/new_job_id and add :str to the definition of new_job_id. Either way is ok with me.

potiuk · 2022-04-13T11:40:13Z

Add it - you will see that it's going to be cleaner. And then you can even move self.job_id = new_job_id closer to the end of the function - and separate variabe will suddenly make a lot more sense

Synchronous tasks killed by execution timeout weren't canceled due to wrong assignment of job_id property.

tauzen · 2022-04-14T12:28:10Z

Thanks for the guidance @potiuk! 🙇 Ok, so this PR is approved and labeled with okay to merge, tests are green - can I assume it will be merged at your convenience?

boring-cyborg · 2022-04-14T14:37:56Z

Awesome work, congrats on your first merged pull request!

potiuk · 2022-04-14T14:38:03Z

Done :)

tauzen · 2022-04-14T14:47:14Z

Awesome, thank you very much @potiuk 😄

tauzen · 2022-04-12T17:12:28Z

Synchronous tasks killed by execution timeout weren't canceled
due to wrong assignment of job_id property.

boring-cyborg · 2022-04-12T17:12:32Z

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 (flake8, 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.
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

github-actions · 2022-04-12T19:59:21Z

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.

potiuk

nice!

potiuk · 2022-04-13T07:02:36Z

Static checks need fixing

tauzen · 2022-04-13T08:01:09Z

Thanks @potiuk! That's odd that mypy is complaining about this now, and wasn't before 🤔. Will try to fix this.

potiuk · 2022-04-13T10:00:33Z

Thanks @potiuk! That's odd that mypy is complaining about this now, and wasn't before thinking. Will try to fix this.

Mypy does seem to work a bit "unstable'y" - I think I know why and might be we will soon fix it.

tauzen · 2022-04-13T11:00:43Z

It's a bit sad that we need to have this additional job_id variable to keep mypy happy, it might be confusing for future contributors. Do you want me to add some additional comment about this in the code? On the bright side - fewer line changes in this PR.

potiuk · 2022-04-13T11:15:17Z

It's a bit sad that we need to have this additional job_id variable to keep mypy happy, it might be confusing for future contributors. Do you want me to add some additional comment about this in the code? On the bright side - fewer line changes in this PR.

I don't find it wrong. It's not to keep mypy happy, it's to make sure you don't accidentally use None. There is nothing wrong with the way you've done and it needs no explanation

I think in this case mypy gave you hint on how to make your code more redable and less error prone.

Contraray to popular belief peeople should optimise their code for easines of reading and correctness not to "avoid" creating variables. Compileres and interpreters will optimise it away and creating extra, properly named variable is often much better solution than .. adding a comment about it.

I often do that. In this case maybe even name it "new_job_id" and even expilcitly mark it as : str -> that will be the best "explanation" of what it is for and why it's there. And there will be no need for comment.

Comments are overrated on the other hand.

tauzen · 2022-04-13T11:30:59Z

Comments are overrated on the other hand.

Definitely 👍

Could you please clarify if you are ok with the current PR, or do you prefer to s/job_id/new_job_id and add :str to the definition of new_job_id. Either way is ok with me.

potiuk · 2022-04-13T11:40:13Z

Add it - you will see that it's going to be cleaner. And then you can even move self.job_id = new_job_id closer to the end of the function - and separate variabe will suddenly make a lot more sense

Synchronous tasks killed by execution timeout weren't canceled due to wrong assignment of job_id property.

tauzen · 2022-04-14T12:28:10Z

Thanks for the guidance @potiuk! 🙇 Ok, so this PR is approved and labeled with okay to merge, tests are green - can I assume it will be merged at your convenience?

boring-cyborg · 2022-04-14T14:37:56Z

Awesome work, congrats on your first merged pull request!

potiuk · 2022-04-14T14:38:03Z

Done :)

tauzen · 2022-04-14T14:47:14Z

Awesome, thank you very much @potiuk 😄

tauzen · 2022-04-12T17:12:28Z

Synchronous tasks killed by execution timeout weren't canceled
due to wrong assignment of job_id property.

boring-cyborg · 2022-04-12T17:12:32Z

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 (flake8, 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.
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

github-actions · 2022-04-12T19:59:21Z

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.

potiuk

nice!

potiuk · 2022-04-13T07:02:36Z

Static checks need fixing

tauzen · 2022-04-13T08:01:09Z

Thanks @potiuk! That's odd that mypy is complaining about this now, and wasn't before 🤔. Will try to fix this.

potiuk · 2022-04-13T10:00:33Z

Thanks @potiuk! That's odd that mypy is complaining about this now, and wasn't before thinking. Will try to fix this.

Mypy does seem to work a bit "unstable'y" - I think I know why and might be we will soon fix it.

tauzen · 2022-04-13T11:00:43Z

It's a bit sad that we need to have this additional job_id variable to keep mypy happy, it might be confusing for future contributors. Do you want me to add some additional comment about this in the code? On the bright side - fewer line changes in this PR.

potiuk · 2022-04-13T11:15:17Z

It's a bit sad that we need to have this additional job_id variable to keep mypy happy, it might be confusing for future contributors. Do you want me to add some additional comment about this in the code? On the bright side - fewer line changes in this PR.

I don't find it wrong. It's not to keep mypy happy, it's to make sure you don't accidentally use None. There is nothing wrong with the way you've done and it needs no explanation

I think in this case mypy gave you hint on how to make your code more redable and less error prone.

Contraray to popular belief peeople should optimise their code for easines of reading and correctness not to "avoid" creating variables. Compileres and interpreters will optimise it away and creating extra, properly named variable is often much better solution than .. adding a comment about it.

I often do that. In this case maybe even name it "new_job_id" and even expilcitly mark it as : str -> that will be the best "explanation" of what it is for and why it's there. And there will be no need for comment.

Comments are overrated on the other hand.

tauzen · 2022-04-13T11:30:59Z

Comments are overrated on the other hand.

Definitely 👍

Could you please clarify if you are ok with the current PR, or do you prefer to s/job_id/new_job_id and add :str to the definition of new_job_id. Either way is ok with me.

potiuk · 2022-04-13T11:40:13Z

Add it - you will see that it's going to be cleaner. And then you can even move self.job_id = new_job_id closer to the end of the function - and separate variabe will suddenly make a lot more sense

Synchronous tasks killed by execution timeout weren't canceled due to wrong assignment of job_id property.

tauzen · 2022-04-14T12:28:10Z

Thanks for the guidance @potiuk! 🙇 Ok, so this PR is approved and labeled with okay to merge, tests are green - can I assume it will be merged at your convenience?

boring-cyborg · 2022-04-14T14:37:56Z

Awesome work, congrats on your first merged pull request!

potiuk · 2022-04-14T14:38:03Z

Done :)

tauzen · 2022-04-14T14:47:14Z

Awesome, thank you very much @potiuk 😄

tauzen · 2022-04-12T17:12:28Z

Synchronous tasks killed by execution timeout weren't canceled
due to wrong assignment of job_id property.

boring-cyborg · 2022-04-12T17:12:32Z

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 (flake8, 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.
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

github-actions · 2022-04-12T19:59:21Z

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.

potiuk

nice!

potiuk · 2022-04-13T07:02:36Z

Static checks need fixing

tauzen · 2022-04-13T08:01:09Z

Thanks @potiuk! That's odd that mypy is complaining about this now, and wasn't before 🤔. Will try to fix this.

potiuk · 2022-04-13T10:00:33Z

Thanks @potiuk! That's odd that mypy is complaining about this now, and wasn't before thinking. Will try to fix this.

Mypy does seem to work a bit "unstable'y" - I think I know why and might be we will soon fix it.

tauzen · 2022-04-13T11:00:43Z

It's a bit sad that we need to have this additional job_id variable to keep mypy happy, it might be confusing for future contributors. Do you want me to add some additional comment about this in the code? On the bright side - fewer line changes in this PR.

potiuk · 2022-04-13T11:15:17Z

It's a bit sad that we need to have this additional job_id variable to keep mypy happy, it might be confusing for future contributors. Do you want me to add some additional comment about this in the code? On the bright side - fewer line changes in this PR.

I don't find it wrong. It's not to keep mypy happy, it's to make sure you don't accidentally use None. There is nothing wrong with the way you've done and it needs no explanation

I think in this case mypy gave you hint on how to make your code more redable and less error prone.

Contraray to popular belief peeople should optimise their code for easines of reading and correctness not to "avoid" creating variables. Compileres and interpreters will optimise it away and creating extra, properly named variable is often much better solution than .. adding a comment about it.

I often do that. In this case maybe even name it "new_job_id" and even expilcitly mark it as : str -> that will be the best "explanation" of what it is for and why it's there. And there will be no need for comment.

Comments are overrated on the other hand.

tauzen · 2022-04-13T11:30:59Z

Comments are overrated on the other hand.

Definitely 👍

Could you please clarify if you are ok with the current PR, or do you prefer to s/job_id/new_job_id and add :str to the definition of new_job_id. Either way is ok with me.

potiuk · 2022-04-13T11:40:13Z

Add it - you will see that it's going to be cleaner. And then you can even move self.job_id = new_job_id closer to the end of the function - and separate variabe will suddenly make a lot more sense

Synchronous tasks killed by execution timeout weren't canceled due to wrong assignment of job_id property.

tauzen · 2022-04-14T12:28:10Z

Thanks for the guidance @potiuk! 🙇 Ok, so this PR is approved and labeled with okay to merge, tests are green - can I assume it will be merged at your convenience?

boring-cyborg · 2022-04-14T14:37:56Z

Awesome work, congrats on your first merged pull request!

potiuk · 2022-04-14T14:38:03Z

Done :)

tauzen · 2022-04-14T14:47:14Z

Awesome, thank you very much @potiuk 😄

tauzen · 2022-04-12T17:12:28Z

Synchronous tasks killed by execution timeout weren't canceled
due to wrong assignment of job_id property.

boring-cyborg · 2022-04-12T17:12:32Z

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 (flake8, 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.
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

github-actions · 2022-04-12T19:59:21Z

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.

potiuk

nice!

potiuk · 2022-04-13T07:02:36Z

Static checks need fixing

tauzen · 2022-04-13T08:01:09Z

Thanks @potiuk! That's odd that mypy is complaining about this now, and wasn't before 🤔. Will try to fix this.

potiuk · 2022-04-13T10:00:33Z

Thanks @potiuk! That's odd that mypy is complaining about this now, and wasn't before thinking. Will try to fix this.

Mypy does seem to work a bit "unstable'y" - I think I know why and might be we will soon fix it.

tauzen · 2022-04-13T11:00:43Z

It's a bit sad that we need to have this additional job_id variable to keep mypy happy, it might be confusing for future contributors. Do you want me to add some additional comment about this in the code? On the bright side - fewer line changes in this PR.

potiuk · 2022-04-13T11:15:17Z

It's a bit sad that we need to have this additional job_id variable to keep mypy happy, it might be confusing for future contributors. Do you want me to add some additional comment about this in the code? On the bright side - fewer line changes in this PR.

I don't find it wrong. It's not to keep mypy happy, it's to make sure you don't accidentally use None. There is nothing wrong with the way you've done and it needs no explanation

I think in this case mypy gave you hint on how to make your code more redable and less error prone.

Contraray to popular belief peeople should optimise their code for easines of reading and correctness not to "avoid" creating variables. Compileres and interpreters will optimise it away and creating extra, properly named variable is often much better solution than .. adding a comment about it.

I often do that. In this case maybe even name it "new_job_id" and even expilcitly mark it as : str -> that will be the best "explanation" of what it is for and why it's there. And there will be no need for comment.

Comments are overrated on the other hand.

tauzen · 2022-04-13T11:30:59Z

Comments are overrated on the other hand.

Definitely 👍

Could you please clarify if you are ok with the current PR, or do you prefer to s/job_id/new_job_id and add :str to the definition of new_job_id. Either way is ok with me.

potiuk · 2022-04-13T11:40:13Z

Add it - you will see that it's going to be cleaner. And then you can even move self.job_id = new_job_id closer to the end of the function - and separate variabe will suddenly make a lot more sense

Synchronous tasks killed by execution timeout weren't canceled due to wrong assignment of job_id property.

tauzen · 2022-04-14T12:28:10Z

Thanks for the guidance @potiuk! 🙇 Ok, so this PR is approved and labeled with okay to merge, tests are green - can I assume it will be merged at your convenience?

boring-cyborg · 2022-04-14T14:37:56Z

Awesome work, congrats on your first merged pull request!

potiuk · 2022-04-14T14:38:03Z

Done :)

tauzen · 2022-04-14T14:47:14Z

Awesome, thank you very much @potiuk 😄

tauzen requested a review from turbaszek as a code owner April 12, 2022 17:12

boring-cyborgBot added area:providers provider:google Google (including GCP) related issues labels Apr 12, 2022

uranusjr approved these changes Apr 12, 2022
View reviewed changes

github-actionsBot added the okay to merge It's ok to merge this PR as it does not require more tests label Apr 12, 2022

potiuk reviewed Apr 12, 2022
View reviewed changes

tauzen force-pushed the fix-dataproc-submit-cancel-on-timeout branch from 3962a73 to 1818ff1Compare April 13, 2022 10:49

Fix cancel_on_kill after execution timeout for DataprocSubmitJobOperator
79ac6f8
Synchronous tasks killed by execution timeout weren't canceled due to wrong assignment of job_id property.

tauzen force-pushed the fix-dataproc-submit-cancel-on-timeout branch from 1818ff1 to 79ac6f8Compare April 13, 2022 12:33

potiuk merged commit ea1ae19 into apache:mainApr 14, 2022

potiuk mentioned this pull request May 11, 2022
Status of testing Providers that were prepared on May 12, 2022 #23659
Closed
59 tasks

lidalei mentioned this pull request Jul 27, 2022
Fix BigQueryInsertJobOperator cancel_on_kill #25342
Merged

tauzen requested a review from turbaszek as a code owner April 12, 2022 17:12

boring-cyborgBot added area:providers provider:google Google (including GCP) related issues labels Apr 12, 2022

uranusjr approved these changes Apr 12, 2022
View reviewed changes

github-actionsBot added the okay to merge It's ok to merge this PR as it does not require more tests label Apr 12, 2022

potiuk reviewed Apr 12, 2022
View reviewed changes

tauzen force-pushed the fix-dataproc-submit-cancel-on-timeout branch from 3962a73 to 1818ff1Compare April 13, 2022 10:49

Fix cancel_on_kill after execution timeout for DataprocSubmitJobOperator
79ac6f8
Synchronous tasks killed by execution timeout weren't canceled due to wrong assignment of job_id property.

tauzen force-pushed the fix-dataproc-submit-cancel-on-timeout branch from 1818ff1 to 79ac6f8Compare April 13, 2022 12:33

potiuk merged commit ea1ae19 into apache:mainApr 14, 2022

potiuk mentioned this pull request May 11, 2022
Status of testing Providers that were prepared on May 12, 2022 #23659
Closed
59 tasks

lidalei mentioned this pull request Jul 27, 2022
Fix BigQueryInsertJobOperator cancel_on_kill #25342
Merged

tauzen requested a review from turbaszek as a code owner April 12, 2022 17:12

boring-cyborgBot added area:providers provider:google Google (including GCP) related issues labels Apr 12, 2022

uranusjr approved these changes Apr 12, 2022
View reviewed changes

github-actionsBot added the okay to merge It's ok to merge this PR as it does not require more tests label Apr 12, 2022

potiuk reviewed Apr 12, 2022
View reviewed changes

tauzen force-pushed the fix-dataproc-submit-cancel-on-timeout branch from 3962a73 to 1818ff1Compare April 13, 2022 10:49

Fix cancel_on_kill after execution timeout for DataprocSubmitJobOperator
79ac6f8
Synchronous tasks killed by execution timeout weren't canceled due to wrong assignment of job_id property.

tauzen force-pushed the fix-dataproc-submit-cancel-on-timeout branch from 1818ff1 to 79ac6f8Compare April 13, 2022 12:33

potiuk merged commit ea1ae19 into apache:mainApr 14, 2022

potiuk mentioned this pull request May 11, 2022
Status of testing Providers that were prepared on May 12, 2022 #23659
Closed
59 tasks

lidalei mentioned this pull request Jul 27, 2022
Fix BigQueryInsertJobOperator cancel_on_kill #25342
Merged

tauzen requested a review from turbaszek as a code owner April 12, 2022 17:12

boring-cyborgBot added area:providers provider:google Google (including GCP) related issues labels Apr 12, 2022

uranusjr approved these changes Apr 12, 2022
View reviewed changes

github-actionsBot added the okay to merge It's ok to merge this PR as it does not require more tests label Apr 12, 2022

potiuk reviewed Apr 12, 2022
View reviewed changes

tauzen force-pushed the fix-dataproc-submit-cancel-on-timeout branch from 3962a73 to 1818ff1Compare April 13, 2022 10:49

Fix cancel_on_kill after execution timeout for DataprocSubmitJobOperator
79ac6f8
Synchronous tasks killed by execution timeout weren't canceled due to wrong assignment of job_id property.

tauzen force-pushed the fix-dataproc-submit-cancel-on-timeout branch from 1818ff1 to 79ac6f8Compare April 13, 2022 12:33

potiuk merged commit ea1ae19 into apache:mainApr 14, 2022

potiuk mentioned this pull request May 11, 2022
Status of testing Providers that were prepared on May 12, 2022 #23659
Closed
59 tasks

lidalei mentioned this pull request Jul 27, 2022
Fix BigQueryInsertJobOperator cancel_on_kill #25342
Merged

tauzen requested a review from turbaszek as a code owner April 12, 2022 17:12

boring-cyborgBot added area:providers provider:google Google (including GCP) related issues labels Apr 12, 2022

uranusjr approved these changes Apr 12, 2022
View reviewed changes

github-actionsBot added the okay to merge It's ok to merge this PR as it does not require more tests label Apr 12, 2022

potiuk reviewed Apr 12, 2022
View reviewed changes

tauzen force-pushed the fix-dataproc-submit-cancel-on-timeout branch from 3962a73 to 1818ff1Compare April 13, 2022 10:49

Fix cancel_on_kill after execution timeout for DataprocSubmitJobOperator
79ac6f8
Synchronous tasks killed by execution timeout weren't canceled due to wrong assignment of job_id property.

tauzen force-pushed the fix-dataproc-submit-cancel-on-timeout branch from 1818ff1 to 79ac6f8Compare April 13, 2022 12:33

potiuk merged commit ea1ae19 into apache:mainApr 14, 2022

potiuk mentioned this pull request May 11, 2022
Status of testing Providers that were prepared on May 12, 2022 #23659
Closed
59 tasks

lidalei mentioned this pull request Jul 27, 2022
Fix BigQueryInsertJobOperator cancel_on_kill #25342
Merged

tauzen requested a review from turbaszek as a code owner April 12, 2022 17:12

boring-cyborgBot added area:providers provider:google Google (including GCP) related issues labels Apr 12, 2022

uranusjr approved these changes Apr 12, 2022
View reviewed changes

github-actionsBot added the okay to merge It's ok to merge this PR as it does not require more tests label Apr 12, 2022

potiuk reviewed Apr 12, 2022
View reviewed changes

tauzen force-pushed the fix-dataproc-submit-cancel-on-timeout branch from 3962a73 to 1818ff1Compare April 13, 2022 10:49

Fix cancel_on_kill after execution timeout for DataprocSubmitJobOperator
79ac6f8
Synchronous tasks killed by execution timeout weren't canceled due to wrong assignment of job_id property.

tauzen force-pushed the fix-dataproc-submit-cancel-on-timeout branch from 1818ff1 to 79ac6f8Compare April 13, 2022 12:33

potiuk merged commit ea1ae19 into apache:mainApr 14, 2022

potiuk mentioned this pull request May 11, 2022
Status of testing Providers that were prepared on May 12, 2022 #23659
Closed
59 tasks

lidalei mentioned this pull request Jul 27, 2022
Fix BigQueryInsertJobOperator cancel_on_kill #25342
Merged

tauzen requested a review from turbaszek as a code owner April 12, 2022 17:12

boring-cyborgBot added area:providers provider:google Google (including GCP) related issues labels Apr 12, 2022

uranusjr approved these changes Apr 12, 2022
View reviewed changes

github-actionsBot added the okay to merge It's ok to merge this PR as it does not require more tests label Apr 12, 2022

potiuk reviewed Apr 12, 2022
View reviewed changes

tauzen force-pushed the fix-dataproc-submit-cancel-on-timeout branch from 3962a73 to 1818ff1Compare April 13, 2022 10:49

Fix cancel_on_kill after execution timeout for DataprocSubmitJobOperator
79ac6f8
Synchronous tasks killed by execution timeout weren't canceled due to wrong assignment of job_id property.

tauzen force-pushed the fix-dataproc-submit-cancel-on-timeout branch from 1818ff1 to 79ac6f8Compare April 13, 2022 12:33

potiuk merged commit ea1ae19 into apache:mainApr 14, 2022

potiuk mentioned this pull request May 11, 2022
Status of testing Providers that were prepared on May 12, 2022 #23659
Closed
59 tasks

lidalei mentioned this pull request Jul 27, 2022
Fix BigQueryInsertJobOperator cancel_on_kill #25342
Merged

tauzen requested a review from turbaszek as a code owner April 12, 2022 17:12

boring-cyborgBot added area:providers provider:google Google (including GCP) related issues labels Apr 12, 2022

uranusjr approved these changes Apr 12, 2022
View reviewed changes

github-actionsBot added the okay to merge It's ok to merge this PR as it does not require more tests label Apr 12, 2022

potiuk reviewed Apr 12, 2022
View reviewed changes

tauzen force-pushed the fix-dataproc-submit-cancel-on-timeout branch from 3962a73 to 1818ff1Compare April 13, 2022 10:49

Fix cancel_on_kill after execution timeout for DataprocSubmitJobOperator
79ac6f8
Synchronous tasks killed by execution timeout weren't canceled due to wrong assignment of job_id property.

tauzen force-pushed the fix-dataproc-submit-cancel-on-timeout branch from 1818ff1 to 79ac6f8Compare April 13, 2022 12:33

potiuk merged commit ea1ae19 into apache:mainApr 14, 2022

potiuk mentioned this pull request May 11, 2022
Status of testing Providers that were prepared on May 12, 2022 #23659
Closed
59 tasks

lidalei mentioned this pull request Jul 27, 2022
Fix BigQueryInsertJobOperator cancel_on_kill #25342
Merged

Uh oh!

Conversation

tauzen commented Apr 12, 2022

Uh oh!

boring-cyborgBot commented Apr 12, 2022

Uh oh!

github-actionsBot commented Apr 12, 2022

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

potiuk commented Apr 13, 2022

Uh oh!

tauzen commented Apr 13, 2022

Uh oh!

potiuk commented Apr 13, 2022

Uh oh!

tauzen commented Apr 13, 2022

Uh oh!

potiuk commented Apr 13, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tauzen commented Apr 13, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

potiuk commented Apr 13, 2022

Uh oh!

tauzen commented Apr 14, 2022

Uh oh!

boring-cyborgBot commented Apr 14, 2022

Uh oh!

potiuk commented Apr 14, 2022

Uh oh!

tauzen commented Apr 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

tauzen commented Apr 12, 2022

Uh oh!

boring-cyborgBot commented Apr 12, 2022

Uh oh!

github-actionsBot commented Apr 12, 2022

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

potiuk commented Apr 13, 2022

Uh oh!

tauzen commented Apr 13, 2022

Uh oh!

potiuk commented Apr 13, 2022

Uh oh!

tauzen commented Apr 13, 2022

Uh oh!

potiuk commented Apr 13, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tauzen commented Apr 13, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

potiuk commented Apr 13, 2022

Uh oh!

tauzen commented Apr 14, 2022

Uh oh!

boring-cyborgBot commented Apr 14, 2022

Uh oh!

potiuk commented Apr 14, 2022

Uh oh!

tauzen commented Apr 14, 2022

Uh oh!

Reviewers

Assignees

potiuk commented Apr 13, 2022•
edited
Loading

tauzen commented Apr 13, 2022•
edited
Loading

potiuk commented Apr 13, 2022•
edited
Loading

tauzen commented Apr 13, 2022•
edited
Loading

potiuk commented Apr 13, 2022•
edited
Loading

tauzen commented Apr 13, 2022•
edited
Loading