You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "jamescnowell (via GitHub)" <gi...@apache.org> on 2023/06/05 16:29:26 UTC
[GitHub] [airflow] jamescnowell opened a new pull request, #31716: Revert "enhance spark_k8s_operator (#29977)"
jamescnowell opened a new pull request, #31716:
URL: https://github.com/apache/airflow/pull/31716
This reverts commit 9a4f6748521c9c3b66d96598036be08fd94ccf89.
Based on the discussion found
[here](https://github.com/apache/airflow/issues/31183), previous changes
to the Spark K8s Operator broke existing functionality and did not update the documentation for the newly enabled functionality.
The Spark Sensor no longer works, XCOM no longer works on the Operator itself, and the Operator does not fail when the Spark job fails.
Rather than attempt to fix or resolve the current implementation, I am reverting to the existing, documented implementation.
I would propose creating a _new_ Operator with alternative functionality (one which does not need a Sensor, copies logs, etc.) if that is desired.
closes: [#31183](https://github.com/apache/airflow/issues/31183)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jamescnowell commented on pull request #31716: Revert "enhance spark_k8s_operator (#29977)"
Posted by "jamescnowell (via GitHub)" <gi...@apache.org>.
jamescnowell commented on PR #31716:
URL: https://github.com/apache/airflow/pull/31716#issuecomment-1577367627
>additional functionality
As it is currently, it is a non-backwards compatible change in the functionality of the Operator, which previously relied entirely on the Sensor for monitoring the job status (as far as I know). Currently, the Operator maintains a Running status for the duration of the Spark job, which changes the possible DAG configurations.
I have an immediate need to have a working DAG, meaning I need to revert to an older version of the Operator (as this version does not fail when a job fails). On top of that, if the change moves forwards as-is, I will not only need to update my DAGs in the future, but will also lose features I may be relying on (the ability to have a fork in a DAG after launching a Spark job, but before it's finished).
I'm definitely _for_ an Operator that behaves in this way, but either by adding flags to the existing operator, or creating an Operator with a different name, and a deprecation schedule for the existing Operator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal closed pull request #31716: Revert "enhance spark_k8s_operator (#29977)"
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal closed pull request #31716: Revert "enhance spark_k8s_operator (#29977)"
URL: https://github.com/apache/airflow/pull/31716
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #31716: Revert "enhance spark_k8s_operator (#29977)"
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #31716:
URL: https://github.com/apache/airflow/pull/31716#issuecomment-1577506789
I agree with Jarek.
Your immidiate issue can be solved by downgrading the version of the provider till the underlying issue is resolved.
The fact that you agree and support the code modifications made by previous PR makes even stronger case not to revert.
Can you please modify the PR to adress the concerns you raised (docs etc..)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] blcksrx commented on pull request #31716: Revert "enhance spark_k8s_operator (#29977)"
Posted by "blcksrx (via GitHub)" <gi...@apache.org>.
blcksrx commented on PR #31716:
URL: https://github.com/apache/airflow/pull/31716#issuecomment-1580954451
@potiuk
I'm not completely agree this version is broken. It's could be fixed in no time!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #31716: Revert "enhance spark_k8s_operator (#29977)"
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #31716:
URL: https://github.com/apache/airflow/pull/31716#issuecomment-1577279715
I don't think revert is required?
This sounds more like a case where additional functionality + docs are needed and once we have it and release new version we can (maybe) yank the old one.
The PR in question was released in April. There may be users out there who relay on it and reverting it would cause their code to break.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jamescnowell commented on pull request #31716: Revert "enhance spark_k8s_operator (#29977)"
Posted by "jamescnowell (via GitHub)" <gi...@apache.org>.
jamescnowell commented on PR #31716:
URL: https://github.com/apache/airflow/pull/31716#issuecomment-1578859449
I'm not able to spend that much time on changes like that right now. You can close this PR if it's not something you plan on accepting.
I find it frustrating that non-backwards compatible changes are allowed into to "stable" API versions, as will many others.
For the "Some people may be relying on it" statement... while that is possible, the current version is fundamentally broken, so those people will have jobs that fail with no notification. I believe _most_ of those people will have ended up finding https://github.com/apache/airflow/issues/31183 as using the code provided in the examples errors out very quickly.
I understand this is already a sticky situation and there are no good solutions, this is simply the solution that I had time for, that appeared to be in line with having stable API version numbers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #31716: Revert "enhance spark_k8s_operator (#29977)"
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #31716:
URL: https://github.com/apache/airflow/pull/31716#issuecomment-1583007661
Closing in favor of https://github.com/apache/airflow/pull/31798
@jamescnowell I welcome you to assist with the review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #31716: Revert "enhance spark_k8s_operator (#29977)"
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #31716:
URL: https://github.com/apache/airflow/pull/31716#issuecomment-1581011865
> I find it frustrating that non-backwards compatible changes are allowed into to "stable" API versions, as will many others
This was not intentional.
We have several check points to minimize such issues but its not bullet proof. I welcome you to participate in testing releases to minimize such future cases.
The issue here is that segnificant time has passed since the release and reverting / yanking the version might cause more harm than good as there may be users that already used the modified version.
You issue is solved by simply not updating the provider till a fix is merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #31716: Revert "enhance spark_k8s_operator (#29977)"
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #31716:
URL: https://github.com/apache/airflow/pull/31716#issuecomment-1577113350
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]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) 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](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) 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](https://www.apache.org/foundation/policies/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]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #31716: Revert "enhance spark_k8s_operator (#29977)"
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #31716:
URL: https://github.com/apache/airflow/pull/31716#issuecomment-1577498828
>> I'm definitely for an Operator that behaves in this way, but either by adding flags to the existing operator, or creating an Operator with a different name, and a deprecation schedule for the existing Operator.
Can you do it this way rather than reverting the change. While I understand your case is impacted, it can be easily fixed by downgrading to earlier version of provider, so "properly" fixing it seems to be a better way than reverting. Reverting a change that has already been released only adds confusion, because if somone relies on what has been already released then you break the other's workflows while you are fixing yours. Can you add a flag to control the behaviour instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #31716: Revert "enhance spark_k8s_operator (#29977)"
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #31716:
URL: https://github.com/apache/airflow/pull/31716#issuecomment-1578882436
then maybe @blcksrx will have time to fix it since that was the #31183 change that broke it. Do you agree @blcksrx that this is "fundamentally broken" ?
Can you please fix it @blcksrx ? I think if not then indeed we will have to rever it, unless someone provides a fix.
> I find it frustrating that non-backwards compatible changes are allowed into to "stable" API versions, as will many others.
Surely @jamescnowell . It is frustrating for everyone. But maybe you do not understand the perspective and OSS spirit - it might happen. Maybe you could better do it with 80 providers, 5000 or so operators and 30 commits merged a day and multiple thousands of tests to prevent such errors from happening. Airflow has 2500 contributors and you get it for absolutely free. There are many ways you and others can give back - spending time (as the 2500 contributors) is one of the ways - and I understand you do not have more of the time (but then maybe github Sponsorship or finding someone in your company - that uses the software for free) would be a good way to give back for the countless hours people spend on contributing to Airflow so that your company can use it for free.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #31716: Revert "enhance spark_k8s_operator (#29977)"
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #31716:
URL: https://github.com/apache/airflow/pull/31716#issuecomment-1581094752
> @potiuk I'm not completely agree this version is broken. It's could be fixed in no time!
Can you make a fix then, please ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org