You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/04/25 11:37:11 UTC
[GitHub] [airflow] malthe opened a new pull request, #23215: Fix bad merge
malthe opened a new pull request, #23215:
URL: https://github.com/apache/airflow/pull/23215
This condition seems to stem from a time where multiple variants of the provider was implemented using a single class.
--
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] malthe commented on pull request #23215: Fix bad merge
Posted by GitBox <gi...@apache.org>.
malthe commented on PR #23215:
URL: https://github.com/apache/airflow/pull/23215#issuecomment-1108508648
@potiuk I guess what we can do is add a logging message in `DbApiHook` (nothing to do) and then at some point, remove the condition from the provider(s).
I just think it should be noted that in fact the task did nothing.
--
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 #23215: Fix bad merge
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23215:
URL: https://github.com/apache/airflow/pull/23215#issuecomment-1108503081
> But that message (or an exception) could be printed in the base class instead so that all database providers share this logic.
And we have to remember about compatibility with providers. We cannot rely on DBApi being present in the same version DBApi is part of the core - so the right way of introducing the change is to add it in the provider (and possibly add it in DBApi for the future but it cannot be reiled upon until min-compatibility is reached.
DBApiHook is quite special because of that and even has this note:
```
#########################################################################################
# #
# Note! Be extra careful when changing this file. This hook is used as a base for #
# a number of DBApi-related hooks and providers depend on the methods implemented #
# here. Whatever you add here, has to backwards compatible unless #
# `>=<Airflow version>` is added to providers' requirements using the new feature #
# #
#########################################################################################
```
--
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] malthe commented on pull request #23215: Fix bad merge
Posted by GitBox <gi...@apache.org>.
malthe commented on PR #23215:
URL: https://github.com/apache/airflow/pull/23215#issuecomment-1108470184
@eladkal perhaps there should be a logging message instead, e.g. "Nothing to do" – !
--
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 #23215: Fix bad merge
Posted by GitBox <gi...@apache.org>.
eladkal commented on PR #23215:
URL: https://github.com/apache/airflow/pull/23215#issuecomment-1108513911
> I just think it should be noted that in fact the task did nothing.
I have no objection (and even welcome it) but I don't think we do that anywhere in any operator...
taking it a step forward... If we know a task is do to nothing maybe we shouldn't even execute the operator.. it's just a waste (Similar to how we are not really executing `EmptyOperator`)
--
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 #23215: Fix bad merge
Posted by GitBox <gi...@apache.org>.
eladkal commented on PR #23215:
URL: https://github.com/apache/airflow/pull/23215#issuecomment-1108469680
I agree with @uranusjr
Also I don't understand what is the problem with the current code. Can you elaborate on that?
--
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 #23215: Fix bad merge
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23215:
URL: https://github.com/apache/airflow/pull/23215#issuecomment-1108510874
> @potiuk I guess what we can do is add a logging message in `DbApiHook` (nothing to do) and then at some point, remove the condition from the provider(s).
>
> I just think it should be noted that in fact the task did nothing.
Yep. Fully agree with both statements.
--
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] uranusjr commented on pull request #23215: Fix bad merge
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #23215:
URL: https://github.com/apache/airflow/pull/23215#issuecomment-1108463798
From what I can tell this condition is needed? `run()` doesn’t make sense if `sql` is empty. Although you can also argue it shouldn’t be empty in the first place, and it’s reasonable to fail the task if it is.
--
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] uranusjr commented on pull request #23215: Fix bad merge
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #23215:
URL: https://github.com/apache/airflow/pull/23215#issuecomment-1108493215
But the logic isn’t present in DbApiHook. Should we not add it in this PR?
--
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] malthe commented on pull request #23215: Fix bad merge
Posted by GitBox <gi...@apache.org>.
malthe commented on PR #23215:
URL: https://github.com/apache/airflow/pull/23215#issuecomment-1108512219
Closing this one.
--
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] malthe closed pull request #23215: Fix bad merge
Posted by GitBox <gi...@apache.org>.
malthe closed pull request #23215: Fix bad merge
URL: https://github.com/apache/airflow/pull/23215
--
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