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/09/16 07:04:40 UTC
[GitHub] [airflow] ignaski opened a new pull request #18287: Fixed wasb hook attempting to create container when getting a blob client
ignaski opened a new pull request #18287:
URL: https://github.com/apache/airflow/pull/18287
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #18237
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ 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] ephraimbuddy commented on pull request #18287: Fixed wasb hook attempting to create container when getting a blob client
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #18287:
URL: https://github.com/apache/airflow/pull/18287#issuecomment-920870524
Closing and reopening to trigger the CI
--
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 #18287: Fixed wasb hook attempting to create container when getting a blob client
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18287:
URL: https://github.com/apache/airflow/pull/18287#issuecomment-920796541
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] boring-cyborg[bot] commented on pull request #18287: Fixed wasb hook attempting to create container when getting a blob client
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #18287:
URL: https://github.com/apache/airflow/pull/18287#issuecomment-921072940
Awesome work, congrats on your first merged pull request!
--
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] ephraimbuddy commented on pull request #18287: Fixed wasb hook attempting to create container when getting a blob client
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #18287:
URL: https://github.com/apache/airflow/pull/18287#issuecomment-921073647
Thanks you @ignaski 🚀
--
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] ephraimbuddy closed pull request #18287: Fixed wasb hook attempting to create container when getting a blob client
Posted by GitBox <gi...@apache.org>.
ephraimbuddy closed pull request #18287:
URL: https://github.com/apache/airflow/pull/18287
--
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] ignaski commented on a change in pull request #18287: Fixed wasb hook attempting to create container when getting a blob client
Posted by GitBox <gi...@apache.org>.
ignaski commented on a change in pull request #18287:
URL: https://github.com/apache/airflow/pull/18287#discussion_r709923042
##########
File path: airflow/providers/microsoft/azure/hooks/wasb.py
##########
@@ -172,7 +172,7 @@ def _get_blob_client(self, container_name: str, blob_name: str) -> BlobClient:
:param blob_name: The name of the blob. This needs not be existing
:type blob_name: str
"""
- container_client = self.create_container(container_name)
+ container_client = self._get_container_client(container_name)
Review comment:
One of the options, yes.
But in general, do you think it is correct to do a `write` operation during an action that is `read-only`? An example would be a blob sensor.
Strangely, the prefix sensor is not using `_get_blob_client` thus it works correctly without `rw` permissions.
--
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] ephraimbuddy commented on pull request #18287: Fixed wasb hook attempting to create container when getting a blob client
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #18287:
URL: https://github.com/apache/airflow/pull/18287#issuecomment-920951394
You can rebase @ignaski seems needed
--
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] ephraimbuddy merged pull request #18287: Fixed wasb hook attempting to create container when getting a blob client
Posted by GitBox <gi...@apache.org>.
ephraimbuddy merged pull request #18287:
URL: https://github.com/apache/airflow/pull/18287
--
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] ignaski commented on a change in pull request #18287: Fixed wasb hook attempting to create container when getting a blob client
Posted by GitBox <gi...@apache.org>.
ignaski commented on a change in pull request #18287:
URL: https://github.com/apache/airflow/pull/18287#discussion_r709987699
##########
File path: airflow/providers/microsoft/azure/hooks/wasb.py
##########
@@ -172,7 +172,7 @@ def _get_blob_client(self, container_name: str, blob_name: str) -> BlobClient:
:param blob_name: The name of the blob. This needs not be existing
:type blob_name: str
"""
- container_client = self.create_container(container_name)
+ container_client = self._get_container_client(container_name)
Review comment:
`_get_blob_client` is used in 3 methods: `check_for_blob`, `download` and `upload`. Only the `upload` is `write` type of operation. In this case, would it make sense to call `create_container` in the `upload` before asking for blob client? This way the method `_get_blob_client` could only be responsible for returning the client.
--
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] ephraimbuddy commented on pull request #18287: Fixed wasb hook attempting to create container when getting a blob client
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #18287:
URL: https://github.com/apache/airflow/pull/18287#issuecomment-920804658
Rebase so we can run the CI. There may be needs for tests
--
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] ignaski commented on pull request #18287: Fixed wasb hook attempting to create container when getting a blob client
Posted by GitBox <gi...@apache.org>.
ignaski commented on pull request #18287:
URL: https://github.com/apache/airflow/pull/18287#issuecomment-920882971
Do you think it would solve the tests failing without a rebase? The ci build fails because of dependency conflict
--
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] ignaski commented on a change in pull request #18287: Fixed wasb hook attempting to create container when getting a blob client
Posted by GitBox <gi...@apache.org>.
ignaski commented on a change in pull request #18287:
URL: https://github.com/apache/airflow/pull/18287#discussion_r709911759
##########
File path: airflow/providers/microsoft/azure/hooks/wasb.py
##########
@@ -172,7 +172,7 @@ def _get_blob_client(self, container_name: str, blob_name: str) -> BlobClient:
:param blob_name: The name of the blob. This needs not be existing
:type blob_name: str
"""
- container_client = self.create_container(container_name)
+ container_client = self._get_container_client(container_name)
Review comment:
> The current behaviour is intentional. We create the container if it doesn't already exist. If it already exists, we return the client of the already existing container. However, we can improve it by having `_get_blob_client` take an optional argument like `create_container_if_not_existing=True`. If True we call `create_container` else we call `_get_container_client`
The behaviour is quite unexpected. E.g. if I want to check for a blob I do not want a container to be created, because I am expecting a blob in an existing container. Furthermore, if connecting with read only permission, the current code throws an exception of permission denied.
--
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] ephraimbuddy commented on a change in pull request #18287: Fixed wasb hook attempting to create container when getting a blob client
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #18287:
URL: https://github.com/apache/airflow/pull/18287#discussion_r709900615
##########
File path: airflow/providers/microsoft/azure/hooks/wasb.py
##########
@@ -172,7 +172,7 @@ def _get_blob_client(self, container_name: str, blob_name: str) -> BlobClient:
:param blob_name: The name of the blob. This needs not be existing
:type blob_name: str
"""
- container_client = self.create_container(container_name)
+ container_client = self._get_container_client(container_name)
Review comment:
The current behaviour is intentional. We create the container if it doesn't already exist. If it already exists, we return the client of the already existing container. However, we can improve it by having `_get_blob_client` take an optional argument like `create_container_if_not_existing=True`. If True we call `create_container` else we call `_get_container_client`
--
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] ephraimbuddy commented on a change in pull request #18287: Fixed wasb hook attempting to create container when getting a blob client
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #18287:
URL: https://github.com/apache/airflow/pull/18287#discussion_r709919241
##########
File path: airflow/providers/microsoft/azure/hooks/wasb.py
##########
@@ -172,7 +172,7 @@ def _get_blob_client(self, container_name: str, blob_name: str) -> BlobClient:
:param blob_name: The name of the blob. This needs not be existing
:type blob_name: str
"""
- container_client = self.create_container(container_name)
+ container_client = self._get_container_client(container_name)
Review comment:
For backward compatibility, what do you think about using try/except here? e.g:
```python
try:
container_client = self.create_container(container_name)
except PermissionError:
container_client = self._get_container_client(container_name)
```
We can find the actual error to catch.
--
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] ephraimbuddy commented on a change in pull request #18287: Fixed wasb hook attempting to create container when getting a blob client
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #18287:
URL: https://github.com/apache/airflow/pull/18287#discussion_r709937544
##########
File path: airflow/providers/microsoft/azure/hooks/wasb.py
##########
@@ -172,7 +172,7 @@ def _get_blob_client(self, container_name: str, blob_name: str) -> BlobClient:
:param blob_name: The name of the blob. This needs not be existing
:type blob_name: str
"""
- container_client = self.create_container(container_name)
+ container_client = self._get_container_client(container_name)
Review comment:
We can actually make this a breaking change but have an argument on the method instead of try/except. Not Ok to do a write operation when you only have a` read-only` perms. So I will go for:
```python
def _get_blob_client(self, container_name: str, blob_name: str, existing_container=True):
if existing_container:
container_client = self._get_container_client(container_name)
else:
container_client = self.create_container(container_name)
...
```
We have many methods that depend on this, so changing it would mean a breaking change but @potiuk can advise the best option 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] ephraimbuddy closed pull request #18287: Fixed wasb hook attempting to create container when getting a blob client
Posted by GitBox <gi...@apache.org>.
ephraimbuddy closed pull request #18287:
URL: https://github.com/apache/airflow/pull/18287
--
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] ephraimbuddy commented on a change in pull request #18287: Fixed wasb hook attempting to create container when getting a blob client
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #18287:
URL: https://github.com/apache/airflow/pull/18287#discussion_r709941293
##########
File path: airflow/providers/microsoft/azure/hooks/wasb.py
##########
@@ -172,7 +172,7 @@ def _get_blob_client(self, container_name: str, blob_name: str) -> BlobClient:
:param blob_name: The name of the blob. This needs not be existing
:type blob_name: str
"""
- container_client = self.create_container(container_name)
+ container_client = self._get_container_client(container_name)
Review comment:
Instead of the above, we can edit the `create_container` method to return the container client if the container exists before creating a container. That way it won't be a breaking change. Can you try that @ignaski
--
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