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 2021/12/18 18:10:53 UTC
[GitHub] [airflow] dstandish opened a new issue #20398: SnowflakeHook should not store query_ids
dstandish opened a new issue #20398:
URL: https://github.com/apache/airflow/issues/20398
When you call `SnowflakeHook.run`, instance attribute `query_ids` is emptied and appended with new query ids. See [here](https://github.com/apache/airflow/blob/05e4cd1c6a93ba96f9adbaf7973e7729697ca934/airflow/providers/snowflake/hooks/snowflake.py#L284).
Suppose you want to run multiple snowflake commands, perhaps submitted in different ways at different times, perhaps asynchronously.
It's not obvious that the query_ids are reset when you call run again.
I don't think this is the best behavior.
We should just return the query ids and let the user decide what 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] potiuk commented on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-1000143644
Assigned you !
--
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] sanyud24 commented on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
sanyud24 commented on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-1002911257
Thanks a lot !! . It'll be very helpful for me.
--
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] dstandish commented on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-997466394
Sorry @potiuk I should have asked a better question. I want to do a breaking change (albeit a very tiny and not super significant one) by having the hook no longer store query ids on the instance.
But I know that we don't want to do major releases all the time. Maybe this isn't "significant enough" to even do? Does it have to wait until there are other bigger changes?
--
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 issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-997452572
Depends if it's done in backwards-compatible way.
--
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 issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-998901122
I concur with @dstandish. The Async/DefferableOperators change the "default" approach of our for Hooks - we had some discussion on it recently and all the hooks shoudl gravitate towards being stateless. It is not yet "formalized" as we need some examples to base it on, but making (and keeping) the Hooks stateless should be the new "theme" of Airflow very soon - the more we go async way, the more we will need it.
--
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] dstandish commented on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-998894798
My take is that generally speaking _hooks_ should not manage state like this; instead, that is the job of the thing using the hook (be it an operator or a function as in `@task`).
When it's managed by the caller then you don't have side effects and hidden behavior that you need to remember or document. When there's a good reason fine but when not I'd say it's better not to.
In that bigquery **hook** example there's not much benefit in setting that attribute.
Instead one can manage it in the operator:
```python
running_job_id = hook.run_query(...)
```
In the **operator** there can certainly be legitimate reasons for setting an instance attribute like this because it can be used in `on_kill`. XCom is totally different issue and certainly no complaints there. My concern is just with the hook.
And with hooks I wouldn't necessarily say it should _never_ be done. We did it [here](https://github.com/apache/airflow/blob/main/airflow/hooks/subprocess.py#L76) with subprocess hook. But there was at least a half-decent reason. We use that attribute in on_kill. We can't return it from `run_command` because the method waits sychronously for the command to complete. We considered exactly this question [here](https://github.com/apache/airflow/pull/13423#issuecomment-753511787), and thought about returning a runner object that managed the state of the command.
But in the snowflake case, as well as the bigquery case, the query ids are not known until after they've completed so there's really no reason to store them on the instance. It's just retrospective metadata at that point.
And looking ahead, if we were to add a `run_async` method to snowflake hook (which could be useful with deferrable operators) then we should return the query id, not store it. Because then you can reuse the hook repeatedly, for multiple commands, and the hook doesn't have to make any assumptions about what the behavior of the operator willl be.
--
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] dstandish commented on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-998319866
thanks @potiuk
OK @sanyud24 this may not actually the best item for a "good first issue". You are welcome to keep the issue and do the work and see what consensus emerges concerning this issue. But be warned that one possible outcome is that the change is rejected.
Let me know what you'd like to do. If you want to take this on, I'll assign to you. Otherwise I'll assign to myself and shepherd this through, and I'll keep you in mind as other, better good first issues pop up.
Thanks
--
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] dstandish commented on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-1003169948
thanks @sanyud24 i will close this one now
--
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] sanyud24 commented on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
sanyud24 commented on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-1000110065
> thanks @potiuk
>
> OK @sanyud24 this may not actually the best item for a "good first issue". You are welcome to keep the issue and do the work and see what consensus emerges concerning this issue. But be warned that one possible outcome is that the change is rejected.
>
> Let me know what you'd like to do. If you want to take this on, I'll assign to you. Otherwise I'll assign to myself and shepherd this through, and I'll keep you in mind as other, better good first issues pop up.
>
> Thanks
Thank you, I would like to take this issue up, the possible approach could be removing all occurrences where we are storing query_ids and returning query_ids with execution_info in the form of tupple.
--
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] mik-laj commented on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-998392759
Related: https://github.com/apache/airflow/pull/15533
CC: @mobuchowski
--
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 edited a comment on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-998901122
I concur with @dstandish. The Async/DefferableOperators change the "default" approach of our for Hooks - we had some discussion on it recently and all the hooks should gravitate towards being stateless. It is not yet "formalized" as we need some examples to base it on, but making (and keeping) the Hooks stateless should be the new "theme" of Airflow very soon - the more we go async way, the more we will need it.
--
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 issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-997803634
> Sorry @potiuk I should have asked a better question. I want to do a breaking change (albeit a very tiny and not super significant one) by having the hook no longer store query ids on the instance.
> But I know that we don't want to do major releases all the time. Maybe this isn't "significant enough" to even do? Does it have to wait until there are other bigger changes?
I think it's best to discuss it in PR. I am not able to give authoritaive answer as I am not "single point of decision" - but having PR and few eyes looking at that and reaching consensus is always best approach.
I personally think that the first question in such cases should be - Can we do it in a non-braking fashion (with optional parameter and deprecation warning) that will have low to medium maintenance and code overhead?
If the answer is yes - this is the preffered way. If in doubt we should always err on the side of backward compatibility. Our users are part of the community and "stability" for them and "least surprise" principle is much more important that slight improvement of maintainer's convenience.
I think If we have aa good reason that the changes cannot be done in backwards-compatible way, or they would involve significant maintenance overhead or the decisions or coming from external factors (for example as we had with google upgrading all it's libraries in backwards-incompatible way), then and only then we shoudl go for updating major version of the provider and backwards incompatibility.
Now - in this case, I am not 100% sure how the change will look like, and there is no reason why we should make the decisions before we see the code. When we see the PR and comment, there is always a chance to revert the decision - and either make it backwards-compatible if we see that the maintenance overhead is acceptable, or decide to break compatibility if we see that we will complicate the code too much or we think that users could benefit from breaking the compatibility.
It's your decision as PR creator to choose the proposed approach, and further disusssion and consensus among the rviewers to agree if it's a good decision.
--
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] mobuchowski commented on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
mobuchowski commented on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-999550302
> It's just retrospective metadata at that point
And that's what our use case is.
I agree with your statements about stateless hooks. It's just that we'd prefer to have this metadata in some way. If it were returned from the hook (as a tuple with current `execution_info`?) it would be great for us too.
--
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] dstandish commented on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-997403419
Awesome @sanyud24 I can assign it to you
@potiuk would this change have to be part of a major release?
--
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 edited a comment on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-997803634
> Sorry @potiuk I should have asked a better question. I want to do a breaking change (albeit a very tiny and not super significant one) by having the hook no longer store query ids on the instance.
> But I know that we don't want to do major releases all the time. Maybe this isn't "significant enough" to even do? Does it have to wait until there are other bigger changes?
I think it's best to discuss it in PR. I am not able to give authoritaive answer as I am not "single point of decision" - but having PR and few eyes looking at that and reaching consensus is always best approach.
I personally think that the first question in such cases should be - Can we do it in a non-breaking fashion (with optional parameter and deprecation warning) that will have low to medium maintenance and code overhead?
If the answer is yes - this is the preffered way. If in doubt we should always err on the side of backward compatibility. Our users are part of the community and "stability" for them and "least surprise" principle is much more important that slight improvement of maintainer's convenience.
I think If we have aa good reason that the changes cannot be done in backwards-compatible way, or they would involve significant maintenance overhead or the decisions or coming from external factors (for example as we had with google upgrading all it's libraries in backwards-incompatible way), then and only then we shoudl go for updating major version of the provider and backwards incompatibility.
Now - in this case, I am not 100% sure how the change will look like, and there is no reason why we should make the decisions before we see the code. When we see the PR and comment, there is always a chance to revert the decision - and either make it backwards-compatible if we see that the maintenance overhead is acceptable, or decide to break compatibility if we see that we will complicate the code too much or we think that users could benefit from breaking the compatibility.
It's your decision as PR creator to choose the proposed approach, and further disusssion and consensus among the rviewers to agree if it's a good decision.
--
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] mobuchowski commented on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
mobuchowski commented on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-998641063
Can we expose the query ids in some other way then?
For example, bigquery stores their id in xcoms: https://github.com/apache/airflow/blob/9876e19273cd56dc53d3a4e287db43acbfa65c4b/airflow/providers/google/cloud/operators/bigquery.py#L737
I know it's different layer, but the motivation is the same here.
--
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] dstandish edited a comment on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-998894798
My take is that generally speaking _hooks_ should not manage state like this; instead, that is the job of the thing using the hook (be it an operator or a function as in `@task`).
When it's managed by the caller then you don't have side effects and hidden behavior that you need to remember or document. When there's a good reason fine but when not I'd say it's better not to.
In that bigquery **hook** example there's not much benefit in setting that attribute.
Instead one can manage it in the operator:
```python
running_job_id = hook.run_query(...)
```
In the **operator** there can certainly be legitimate reasons for setting an instance attribute like this because it can be used in `on_kill`. XCom is totally different issue and certainly no complaints there. My concern is just with the hook.
And with hooks I wouldn't necessarily say it should _never_ be done. We did it [here](https://github.com/apache/airflow/blob/main/airflow/hooks/subprocess.py#L76) with subprocess hook. But there was at least a half-decent reason. We use that attribute in on_kill. We can't return it from `run_command` because the method waits sychronously for the command to complete. We considered exactly this question [here](https://github.com/apache/airflow/pull/13423#issuecomment-753511787), and thought about returning a runner object that managed the state of the command but figured it was not worth the complexity tradeoff. But maybe we should have done something like that.
But in the snowflake case, as well as the bigquery case, the query ids are not known until after they've completed so there's really no reason to store them on the instance. It's just retrospective metadata at that point.
And looking ahead, if we were to add a `run_async` method to snowflake hook (which could be useful with deferrable operators) then we should return the query id, not store it. Because then you can reuse the hook repeatedly, for multiple commands, and the hook doesn't have to make any assumptions about what the behavior of the operator willl be.
--
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] sanyud24 commented on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
sanyud24 commented on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-997393953
Hi , I would like to work on this issue .
--
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] sanyud24 edited a comment on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
sanyud24 edited a comment on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-1002911257
Thanks a lot @dstandish !! . It'll be very helpful for me.
--
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 edited a comment on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-997803634
> Sorry @potiuk I should have asked a better question. I want to do a breaking change (albeit a very tiny and not super significant one) by having the hook no longer store query ids on the instance.
> But I know that we don't want to do major releases all the time. Maybe this isn't "significant enough" to even do? Does it have to wait until there are other bigger changes?
I think it's best to discuss it in PR. I am not able to give authoritaive answer as I am not "single point of decision" - but having PR and few eyes looking at that and reaching consensus is always best approach.
I personally think that the first question in such cases should be - Can we do it in a non-breaking fashion (with optional parameter and deprecation warning) that will have low to medium maintenance and code overhead?
If the answer is yes - this is the preffered way. If in doubt we should always err on the side of backward compatibility. Our users are part of the community and "stability" for them and "least surprise" principle is much more important that slight improvement of maintainer's convenience.
I think If we have a good reason that the changes cannot be done in backwards-compatible way, or they would involve significant maintenance overhead or the decisions or coming from external factors (for example as we had with google upgrading all it's libraries in backwards-incompatible way), then and only then we shoudl go for updating major version of the provider and backwards incompatibility.
Now - in this case, I am not 100% sure how the change will look like, and there is no reason why we should make the decisions before we see the code. When we see the PR and comment, there is always a chance to revert the decision - and either make it backwards-compatible if we see that the maintenance overhead is acceptable, or decide to break compatibility if we see that we will complicate the code too much or we think that users could benefit from breaking the compatibility.
It's the decision of PR author to choose the proposed approach, and further disusssion and consensus among the reviewers to agree if it's a good decision.
--
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 edited a comment on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-997803634
--
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] mobuchowski edited a comment on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
mobuchowski edited a comment on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-998641063
Can we expose the query ids in some other way then?
Bigquery does the same: https://github.com/apache/airflow/blob/9876e19273cd56dc53d3a4e287db43acbfa65c4b/airflow/providers/google/cloud/hooks/bigquery.py#L2326
And then stores it in xcoms: https://github.com/apache/airflow/blob/9876e19273cd56dc53d3a4e287db43acbfa65c4b/airflow/providers/google/cloud/operators/bigquery.py#L737
I know it's different layer, but the motivation is the same here.
--
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] dstandish commented on issue #20398: SnowflakeHook should not store query_ids
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #20398:
URL: https://github.com/apache/airflow/issues/20398#issuecomment-1002878498
sorry @sanyud24 it seems that it is not be practical to make this change. sorry for launching you into an unfruitful effort. i'll try and keep you in mind next time a good issue comes up, and i'll be sure to make sure it's not so marginal.
--
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