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/28 19:05:11 UTC
[GitHub] [airflow] josh-fell opened a new pull request #20540: Remove `host` as an instance attr in `DatabricksHook`
josh-fell opened a new pull request #20540:
URL: https://github.com/apache/airflow/pull/20540
As part of #20526, parsing logic of `host` was moved out of the `__init__()` method in `DatabricksHook`. With this change, `host` no longer needs to be an instance attribute of the hook.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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 pull request #20540: Remove `host` as an instance attr in `DatabricksHook`
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #20540:
URL: https://github.com/apache/airflow/pull/20540#issuecomment-1002263115
I think "behavrioural" changes like that are not "breaking". Whether `self.host` is an instance of variable does not change the "use" of it - at most it optimizes some behaviours - in this case the "self.host" was created in both - DagParser and Task, when it was removed, it was only created in Task (during execute).
As we discussed here: https://github.com/apache/airflow/pull/19572#discussion_r776052141 - Databricks is not really a "generic" operator that you might want to extend or dertive from to use internal features. Unlke KPO for example which is generic and it's easier to imagine that people would like to change their behaviour by extending it. So I think we can consider it's internals are really "implementation details".
Unfortunately we do not have clear-cut what is breaking and what not :(. Theoretically even adding a default parameter might be breaking because somone could overload that method in a derived class, or you could imagine someone checks signature of the method.
This is really almost always an exercise of "how likely this change is breaking" rather than "clear-cut" 0/1 breaking/non-breaking. And except some obvious cases, almost always it will have to be judged individually.
--
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] github-actions[bot] commented on pull request #20540: Remove `host` as an instance attr in `DatabricksHook`
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20540:
URL: https://github.com/apache/airflow/pull/20540#issuecomment-1002246296
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.
--
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 pull request #20540: Remove `host` as an instance attr in `DatabricksHook`
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #20540:
URL: https://github.com/apache/airflow/pull/20540#issuecomment-1002268700
thanks @potiuk
--
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 pull request #20540: Remove `host` as an instance attr in `DatabricksHook`
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #20540:
URL: https://github.com/apache/airflow/pull/20540#issuecomment-1002260448
> > I like it... BUT... must we consider this a "breaking" change?
>
> Why ?
Because `host` will no longer be set after `__init__`
in last release it seems instance attr `host` was set in `__init__`
--
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] josh-fell commented on pull request #20540: Remove `host` as an instance attr in `DatabricksHook`
Posted by GitBox <gi...@apache.org>.
josh-fell commented on pull request #20540:
URL: https://github.com/apache/airflow/pull/20540#issuecomment-1002242954
CC @dstandish
--
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 #20540: Remove `host` as an instance attr in `DatabricksHook`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20540:
URL: https://github.com/apache/airflow/pull/20540#issuecomment-1002263115
I think "behavrioural" changes like that are not "breaking". Whether `self.host` is an instance of variable does not change the "use" of it - at most it optimizes some behaviours - in this case the "self.host" was created in both - DagParser and Task, when it was removed, it was only created in Task (during execute).
As we discussed here: https://github.com/apache/airflow/pull/19572#discussion_r776052141 - Databricks is not really a "generic" operator that you might want to extend or dertive from to use internal features. Unlke KPO for example which is generic and it's easier to imagine that people would like to change their behaviour by extending it. So I think we can consider it's internals are really "implementation details".
Unfortunately we do not have clear-cut what is breaking and what not :(. Theoretically even adding a default parameter might be breaking because somone could overload that method in a derived class, or you could imagine someone checks signature of the method.
This is really almost always an exercise of "how likely this change is breaking" rather than "clear-cut" 0/1 breaking/non-breaking. And except some obvious cases, almost always it will have to be judged individually.
--
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 pull request #20540: Remove `host` as an instance attr in `DatabricksHook`
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #20540:
URL: https://github.com/apache/airflow/pull/20540#issuecomment-1002260448
> > I like it... BUT... must we consider this a "breaking" change?
>
> Why ?
Because `host` will no longer be set after `__init__`
in last release it seems instance attr `host` was set in `__init__`
this among the most marginal of changes we should consider "breaking" but if we want to be strict it probably is right to consider it so. in this case the solution should probably be to make it a cached property. but @potiuk if you think we can just chop the attr that would be best from code perspective. based on @josh-fell's analyis it seems it was only an instance attr for one release (since v2.1.0)
--
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 pull request #20540: Remove `host` as an instance attr in `DatabricksHook`
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #20540:
URL: https://github.com/apache/airflow/pull/20540#issuecomment-1002248910
I like it... BUT... must we consider this a "breaking" change?
--
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 #20540: Remove `host` as an instance attr in `DatabricksHook`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20540:
URL: https://github.com/apache/airflow/pull/20540#issuecomment-1002259278
> I like it... BUT... must we consider this a "breaking" change?
Why ?
--
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 merged pull request #20540: Remove `host` as an instance attr in `DatabricksHook`
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #20540:
URL: https://github.com/apache/airflow/pull/20540
--
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 #20540: Remove `host` as an instance attr in `DatabricksHook`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20540:
URL: https://github.com/apache/airflow/pull/20540#issuecomment-1002246196
We can include it in the next provider's released (planned to do it tomorrow.)
--
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] josh-fell commented on pull request #20540: Remove `host` as an instance attr in `DatabricksHook`
Posted by GitBox <gi...@apache.org>.
josh-fell commented on pull request #20540:
URL: https://github.com/apache/airflow/pull/20540#issuecomment-1002259262
> I like it... BUT... must we consider this a "breaking" change?
For some historical context on this and moving the `get_connection()` call in and out of `__init__()` if it helps assist in a decision:
- v2.0.1
- `get_connection()` call exists in `__init__()`
- `host` is not an instance attr
- v2.0.2
- `get_connection()` removed `__init__()`
- `host` is not an instance attr
- v2.1.0
- `get_connection()` was put back into `__init__()` via #19835 which then added `host` as an instance attr
- `get_connection()` was then removed again `__init__()` via #20180 which kept `host` as an instance attr
- Ultimately, end state was `get_connection()` removed from `__init__()` but `host` remained an instance attr
- Since the last provider release:
- `get_connection()` moved back into `__init__()` to address Mypy reenablement errors in #20265
- `get_connection()` called as part of a cached prop in #20526
- `host` remained untouched until this PR
Quite the adventure.
--
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