You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@libcloud.apache.org by GitBox <gi...@apache.org> on 2020/01/04 04:34:02 UTC

[GitHub] [libcloud] c-w opened a new pull request #1400: Implemented chunked upload for Azure Blobs storage driver

c-w opened a new pull request #1400: Implemented chunked upload for Azure Blobs storage driver
URL: https://github.com/apache/libcloud/pull/1400
 
 
   ## Implemented chunked upload for Azure Blobs storage driver
   
   ### Description
   
   This pull request fixes https://github.com/apache/libcloud/issues/1399 by implementing chunked upload in the Azure Blobs storage driver via the [Put Block](https://docs.microsoft.com/en-us/rest/api/storageservices/put-block) and [Put Block List](https://docs.microsoft.com/en-us/rest/api/storageservices/put-block-list) APIs.
   
   The implementation mostly leverages the `AzureBlobsStorageDriver._upload_in_chunks` function that was first introduced in [24f34c9](https://github.com/apache/libcloud/blob/24f34c99c9440523a53e940a346bced551281953/libcloud/storage/drivers/azure_blobs.py#L732-L788) and was stopped being used in [6e0040d](https://github.com/apache/libcloud/commit/6e0040d8904cacb5dbe88309e9051be08cdc59f9) with the the following main updates:
   
   - Enable setting object metadata, content-type and content-md5 for chunked uploads.
   
   - Drop support for PageBlob. **This is a breaking API change.** However, given the non-trivial differences between the PageBlob and BlockBlob APIs, I'd hypothesize that the improved simplicity of the code will aid in the long run with bugfixes and maintenance. If keeping support for PageBlob is a hard requirement, I suspect a non-trivial amount of refactoring would have to be undertaken to cleanly support chunked upload for both BlockBlob and PageBlob object types. I'm open to do the work as part of this pull request, but I'd love to first discuss the pros/cons of both approaches.
   
   Additionally, the following companion changes were made:
   
   - Remove unused `upload_func` and `upload_func_kwargs` arguments in `StorageDriver._upload_object` ([7e55057](https://github.com/apache/libcloud/commit/7e550577758cb95ec42819e77b6773566a180e5c))
   - Rationalize determination of content type to a new helper function `StorageDriver._determine_content_type` ([0dc5cbb](https://github.com/apache/libcloud/commit/0dc5cbb0410fa3b1f9ca0d54b59d514cbc7e6c7a))
   - Switch `libcloud.utils.files.read_in_chunks` from type checking to duck-typing to ensure that more iterators (e.g. opened file objects) use the fast `read(int)` code-path ([0fded11](https://github.com/apache/libcloud/commit/0fded110e69f020388d76d8094d3ca25fa868e13))
   
   ### Status
   
   - work in progress
   
   ### Checklist
   
   - [x] Code linting ([CI passed](https://travis-ci.org/CatalystCode/libcloud/builds/632547395))
   - [ ] Documentation (*TODO: changelog entry*)
   - [ ] Tests ([E2E tests passed](https://clewolff.visualstudio.com/libcloud-tests/_build/results?buildId=661), *TODO: unit tests*)
   - [x] ICLA ([committer](http://people.apache.org/phonebook.html?uid=clewolff))

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [libcloud] c-w commented on a change in pull request #1400: Implemented chunked upload for Azure Blobs storage driver

Posted by GitBox <gi...@apache.org>.
c-w commented on a change in pull request #1400: Implemented chunked upload for Azure Blobs storage driver
URL: https://github.com/apache/libcloud/pull/1400#discussion_r364517127
 
 

 ##########
 File path: libcloud/storage/base.py
 ##########
 @@ -639,13 +624,30 @@ def _upload_object(self, object_name, content_type, request_path,
         if not response.success():
             response.parse_error()
 
-        if upload_func:
-            upload_func(**upload_func_kwargs)
-
         return {'response': response,
                 'bytes_transferred': stream_length,
                 'data_hash': stream_hash}
 
+    def _determine_content_type(self, content_type, object_name,
+                                file_path=None):
+        if not content_type:
 
 Review comment:
   Cleaned up the function in 498ac88.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [libcloud] codecov-io edited a comment on issue #1400: Implemented chunked upload for Azure Blobs storage driver

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #1400: Implemented chunked upload for Azure Blobs storage driver
URL: https://github.com/apache/libcloud/pull/1400#issuecomment-570756207
 
 
   # [Codecov](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=h1) Report
   > Merging [#1400](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=desc) into [trunk](https://codecov.io/gh/apache/libcloud/commit/6dca82e649456b42d23f439854d3dc807c806abf?src=pr&el=desc) will **increase** coverage by `0.07%`.
   > The diff coverage is `92.4%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/libcloud/pull/1400/graphs/tree.svg?width=650&token=PYoduksh69&height=150&src=pr)](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##            trunk    #1400      +/-   ##
   ==========================================
   + Coverage   86.58%   86.65%   +0.07%     
   ==========================================
     Files         364      364              
     Lines       76193    76081     -112     
     Branches     7439     7421      -18     
   ==========================================
   - Hits        65968    65928      -40     
   + Misses       7400     7329      -71     
   + Partials     2825     2824       -1
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [libcloud/test/storage/test\_base.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvdGVzdC9zdG9yYWdlL3Rlc3RfYmFzZS5weQ==) | `92.92% <ø> (-0.41%)` | :arrow_down: |
   | [libcloud/storage/drivers/s3.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvc3RvcmFnZS9kcml2ZXJzL3MzLnB5) | `89.3% <100%> (+0.12%)` | :arrow_up: |
   | [libcloud/test/storage/test\_azure\_blobs.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvdGVzdC9zdG9yYWdlL3Rlc3RfYXp1cmVfYmxvYnMucHk=) | `93.37% <100%> (+1.37%)` | :arrow_up: |
   | [libcloud/test/storage/test\_atmos.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvdGVzdC9zdG9yYWdlL3Rlc3RfYXRtb3MucHk=) | `95.11% <100%> (ø)` | :arrow_up: |
   | [libcloud/storage/drivers/atmos.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvc3RvcmFnZS9kcml2ZXJzL2F0bW9zLnB5) | `78.03% <100%> (+0.13%)` | :arrow_up: |
   | [libcloud/utils/files.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvdXRpbHMvZmlsZXMucHk=) | `100% <100%> (ø)` | :arrow_up: |
   | [libcloud/storage/base.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvc3RvcmFnZS9iYXNlLnB5) | `83.01% <100%> (ø)` | :arrow_up: |
   | [libcloud/storage/drivers/azure\_blobs.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvc3RvcmFnZS9kcml2ZXJzL2F6dXJlX2Jsb2JzLnB5) | `85.13% <88.46%> (+15.33%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=footer). Last update [6dca82e...cfdb2bd](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [libcloud] c-w merged pull request #1400: Implemented chunked upload for Azure Blobs storage driver

Posted by GitBox <gi...@apache.org>.
c-w merged pull request #1400: Implemented chunked upload for Azure Blobs storage driver
URL: https://github.com/apache/libcloud/pull/1400
 
 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [libcloud] Kami commented on a change in pull request #1400: Implemented chunked upload for Azure Blobs storage driver

Posted by GitBox <gi...@apache.org>.
Kami commented on a change in pull request #1400: Implemented chunked upload for Azure Blobs storage driver
URL: https://github.com/apache/libcloud/pull/1400#discussion_r364073143
 
 

 ##########
 File path: libcloud/storage/base.py
 ##########
 @@ -584,7 +584,6 @@ def _save_object(self, response, obj, destination_path,
     def _upload_object(self, object_name, content_type, request_path,
                        request_method='PUT',
                        headers=None, file_path=None, stream=None,
-                       upload_func=None, upload_func_kwargs=None,
 
 Review comment:
   Yep, I assume those arguments became unused when libcloud switched to the requests library (that change also seemed to have introduced a lot of unintentional regressions related to streaming uploads and downloading).
   
   Sadly it's not trivial to write good unit / integration tests for that. In the past we mostly had unit tests which mocked some of that functionality for tests so real life issues and regressions introduced as part of the requests migration were not caught.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [libcloud] codecov-io commented on issue #1400: Implemented chunked upload for Azure Blobs storage driver

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #1400: Implemented chunked upload for Azure Blobs storage driver
URL: https://github.com/apache/libcloud/pull/1400#issuecomment-570756207
 
 
   # [Codecov](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=h1) Report
   > Merging [#1400](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=desc) into [trunk](https://codecov.io/gh/apache/libcloud/commit/6dca82e649456b42d23f439854d3dc807c806abf?src=pr&el=desc) will **increase** coverage by `0.07%`.
   > The diff coverage is `92.3%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/libcloud/pull/1400/graphs/tree.svg?width=650&token=PYoduksh69&height=150&src=pr)](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##            trunk    #1400      +/-   ##
   ==========================================
   + Coverage   86.58%   86.65%   +0.07%     
   ==========================================
     Files         364      364              
     Lines       76193    76081     -112     
     Branches     7439     7421      -18     
   ==========================================
   - Hits        65968    65928      -40     
   + Misses       7400     7329      -71     
   + Partials     2825     2824       -1
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [libcloud/test/storage/test\_base.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvdGVzdC9zdG9yYWdlL3Rlc3RfYmFzZS5weQ==) | `92.92% <ø> (-0.41%)` | :arrow_down: |
   | [libcloud/storage/drivers/s3.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvc3RvcmFnZS9kcml2ZXJzL3MzLnB5) | `89.3% <100%> (+0.12%)` | :arrow_up: |
   | [libcloud/storage/base.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvc3RvcmFnZS9iYXNlLnB5) | `83.01% <100%> (ø)` | :arrow_up: |
   | [libcloud/test/storage/test\_azure\_blobs.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvdGVzdC9zdG9yYWdlL3Rlc3RfYXp1cmVfYmxvYnMucHk=) | `93.37% <100%> (+1.37%)` | :arrow_up: |
   | [libcloud/test/storage/test\_atmos.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvdGVzdC9zdG9yYWdlL3Rlc3RfYXRtb3MucHk=) | `95.11% <100%> (ø)` | :arrow_up: |
   | [libcloud/storage/drivers/atmos.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvc3RvcmFnZS9kcml2ZXJzL2F0bW9zLnB5) | `78.03% <100%> (+0.13%)` | :arrow_up: |
   | [libcloud/utils/files.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvdXRpbHMvZmlsZXMucHk=) | `100% <100%> (ø)` | :arrow_up: |
   | [libcloud/storage/drivers/azure\_blobs.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvc3RvcmFnZS9kcml2ZXJzL2F6dXJlX2Jsb2JzLnB5) | `85.13% <88.23%> (+15.33%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=footer). Last update [6dca82e...0fded11](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [libcloud] codecov-io edited a comment on issue #1400: Implemented chunked upload for Azure Blobs storage driver

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #1400: Implemented chunked upload for Azure Blobs storage driver
URL: https://github.com/apache/libcloud/pull/1400#issuecomment-570756207
 
 
   # [Codecov](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=h1) Report
   > Merging [#1400](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=desc) into [trunk](https://codecov.io/gh/apache/libcloud/commit/6dca82e649456b42d23f439854d3dc807c806abf?src=pr&el=desc) will **increase** coverage by `0.08%`.
   > The diff coverage is `93.18%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/libcloud/pull/1400/graphs/tree.svg?width=650&token=PYoduksh69&height=150&src=pr)](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##            trunk    #1400      +/-   ##
   ==========================================
   + Coverage   86.58%   86.66%   +0.08%     
   ==========================================
     Files         364      364              
     Lines       76193    76082     -111     
     Branches     7439     7422      -17     
   ==========================================
   - Hits        65968    65933      -35     
   + Misses       7400     7325      -75     
   + Partials     2825     2824       -1
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [libcloud/test/storage/test\_base.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvdGVzdC9zdG9yYWdlL3Rlc3RfYmFzZS5weQ==) | `92.92% <ø> (-0.41%)` | :arrow_down: |
   | [libcloud/storage/drivers/s3.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvc3RvcmFnZS9kcml2ZXJzL3MzLnB5) | `89.3% <100%> (+0.12%)` | :arrow_up: |
   | [libcloud/test/storage/test\_azure\_blobs.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvdGVzdC9zdG9yYWdlL3Rlc3RfYXp1cmVfYmxvYnMucHk=) | `94.27% <100%> (+2.27%)` | :arrow_up: |
   | [libcloud/test/storage/test\_atmos.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvdGVzdC9zdG9yYWdlL3Rlc3RfYXRtb3MucHk=) | `95.11% <100%> (ø)` | :arrow_up: |
   | [libcloud/storage/drivers/atmos.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvc3RvcmFnZS9kcml2ZXJzL2F0bW9zLnB5) | `78.03% <100%> (+0.13%)` | :arrow_up: |
   | [libcloud/utils/files.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvdXRpbHMvZmlsZXMucHk=) | `100% <100%> (ø)` | :arrow_up: |
   | [libcloud/storage/base.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvc3RvcmFnZS9iYXNlLnB5) | `83.01% <100%> (ø)` | :arrow_up: |
   | [libcloud/storage/drivers/azure\_blobs.py](https://codecov.io/gh/apache/libcloud/pull/1400/diff?src=pr&el=tree#diff-bGliY2xvdWQvc3RvcmFnZS9kcml2ZXJzL2F6dXJlX2Jsb2JzLnB5) | `85.13% <88.46%> (+15.33%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=footer). Last update [6dca82e...9174502](https://codecov.io/gh/apache/libcloud/pull/1400?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [libcloud] Kami commented on a change in pull request #1400: Implemented chunked upload for Azure Blobs storage driver

Posted by GitBox <gi...@apache.org>.
Kami commented on a change in pull request #1400: Implemented chunked upload for Azure Blobs storage driver
URL: https://github.com/apache/libcloud/pull/1400#discussion_r364073383
 
 

 ##########
 File path: libcloud/storage/base.py
 ##########
 @@ -639,13 +624,30 @@ def _upload_object(self, object_name, content_type, request_path,
         if not response.success():
             response.parse_error()
 
-        if upload_func:
-            upload_func(**upload_func_kwargs)
-
         return {'response': response,
                 'bytes_transferred': stream_length,
                 'data_hash': stream_hash}
 
+    def _determine_content_type(self, content_type, object_name,
+                                file_path=None):
+        if not content_type:
 
 Review comment:
   I would probably inverse that logic and do if content type and return early to avoid one level of nesting, but that's just a personal style preference.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services