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 2021/11/10 15:52:57 UTC

[GitHub] [libcloud] Germandrummer92 opened a new pull request #1592: Make retry handle http status codes

Germandrummer92 opened a new pull request #1592:
URL: https://github.com/apache/libcloud/pull/1592


   ## Retry Raw Http Requests
   
   ### Description
   
   We are currently facing some storage driver exceptions when uploading many small files. Stacktrace (parts removed):
   
   ```self._driver.upload_object_via_stream( File "/usr/local/lib/python3.8/site-packages/libcloud/storage/drivers/s3.py", line 754, in upload_object_via_stream return self._put_object(container=container, object_name=object_name, File "/usr/local/lib/python3.8/site-packages/libcloud/storage/drivers/s3.py", line 900, in _put_object result_dict = self._upload_object( File "/usr/local/lib/python3.8/site-packages/libcloud/storage/base.py", line 837, in _upload_object response.parse_error() File "/usr/local/lib/python3.8/site-packages/libcloud/storage/drivers/s3.py", line 147, in parse_error raise LibcloudError('Unknown error. Status code: %d' % (self.status), libcloud.common.types.LibcloudError: <LibcloudError in <class 'libcloud.storage.drivers.s3.S3StorageDriver'> 'Unknown error. Status code: 429'>```
   
   This change also enables retrying the raw requests. Together with #1588 (for which we have a workaround), this would solve our issue completely and we could keep using libcloud.
   
   I only added a call to retrying the raw requests.
   
   ### Status
   
   done, ready for review
   
   ### Checklist (tick everything that applies)
   
   - [X] [Code linting](http://libcloud.readthedocs.org/en/latest/development.html#code-style-guide) (required, can be done after the PR checks)
   - [X] Documentation
   - [X] [Tests](http://libcloud.readthedocs.org/en/latest/testing.html)
   - [ ] [ICLA](http://libcloud.readthedocs.org/en/latest/development.html#contributing-bigger-changes) (required for 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: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Germandrummer92 commented on pull request #1592: Make retry handle http status codes

Posted by GitBox <gi...@apache.org>.
Germandrummer92 commented on pull request #1592:
URL: https://github.com/apache/libcloud/pull/1592#issuecomment-862195138


   I am not super with the current solution if someone has a better idea let me know. Otherwise this would at least give the ability to handle these status code exceptions. I also think the parsing of the 429 into the RateLimitedError is currently not done correctly/not working, but with #1588 we could work around it. 
   
   Maybe we need a predicate for the exceptions instead of only the types configurable though @RunOrVeith. 


-- 
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

[GitHub] [libcloud] Kami commented on pull request #1592: Make retry handle http status codes

Posted by GitBox <gi...@apache.org>.
Kami commented on pull request #1592:
URL: https://github.com/apache/libcloud/pull/1592#issuecomment-965565489


   @Germandrummer92 Thanks for clarifying. And I already have a fix locally and will merge it shortly :)


-- 
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: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Germandrummer92 commented on pull request #1592: Make retry handle http status codes

Posted by GitBox <gi...@apache.org>.
Germandrummer92 commented on pull request #1592:
URL: https://github.com/apache/libcloud/pull/1592#issuecomment-965483417


   @RunOrVeith @Kami 
   
   I think like this it's a much smaller and cleaner change and shouldn't break anything while still retrying raw requests and correctly mapping the exceptions before they are checked in the retry class.
   
   re: the change in test_ovh: this test also fails for me on the current trunk code? maybe ovh changed their response code recentlyhere? (obviously parsing the exception message is maybe not the best idea :D)


-- 
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: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Germandrummer92 commented on a change in pull request #1592: Make retry handle http status codes

Posted by GitBox <gi...@apache.org>.
Germandrummer92 commented on a change in pull request #1592:
URL: https://github.com/apache/libcloud/pull/1592#discussion_r746811674



##########
File path: libcloud/common/ovh.py
##########
@@ -226,7 +226,7 @@ def handle_and_rethrow_user_friendly_invalid_region_error(host, e):
     """
     msg = str(e).lower()
 
-    if 'name or service not known' in msg or 'getaddrinfo failed' in msg:
+    if 'nodename nor servname provided, or not known' in msg or 'getaddrinfo failed' in msg:

Review comment:
       hmm interesting, maybe related to a requests version in my local environment or even os-specific (running on mac). changed 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: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Germandrummer92 edited a comment on pull request #1592: Make retry handle http status codes

Posted by GitBox <gi...@apache.org>.
Germandrummer92 edited a comment on pull request #1592:
URL: https://github.com/apache/libcloud/pull/1592#issuecomment-862195138


   I am not super happy with the current solution if someone has a better idea let me know. Otherwise this would at least give the ability to handle these status code exceptions. I also think the parsing of the 429 into the RateLimitedError is currently not done correctly/not working, but with #1588 we could work around it. 
   
   Maybe we need a predicate for the exceptions instead of only the types configurable though @RunOrVeith. 


-- 
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

[GitHub] [libcloud] Kami closed pull request #1592: Make retry handle http status codes

Posted by GitBox <gi...@apache.org>.
Kami closed pull request #1592:
URL: https://github.com/apache/libcloud/pull/1592


   


-- 
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: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Kami commented on pull request #1592: Make retry handle http status codes

Posted by GitBox <gi...@apache.org>.
Kami commented on pull request #1592:
URL: https://github.com/apache/libcloud/pull/1592#issuecomment-953270654


   Thanks for the contribution.
   
   Since it's a larger one, can you please look into filling an ICLA - https://www.apache.org/licenses/icla.pdf.
   
   I plan to review it once #1588 is merged.


-- 
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: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Germandrummer92 closed pull request #1592: Make retry handle http status codes

Posted by GitBox <gi...@apache.org>.
Germandrummer92 closed pull request #1592:
URL: https://github.com/apache/libcloud/pull/1592


   


-- 
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: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Kami commented on a change in pull request #1592: Make retry handle http status codes

Posted by GitBox <gi...@apache.org>.
Kami commented on a change in pull request #1592:
URL: https://github.com/apache/libcloud/pull/1592#discussion_r746800479



##########
File path: libcloud/common/ovh.py
##########
@@ -226,7 +226,7 @@ def handle_and_rethrow_user_friendly_invalid_region_error(host, e):
     """
     msg = str(e).lower()
 
-    if 'name or service not known' in msg or 'getaddrinfo failed' in msg:
+    if 'nodename nor servname provided, or not known' in msg or 'getaddrinfo failed' in msg:

Review comment:
       I needed to make a small change to get tests to pass (aka also check for ``'name or service not known' ``). I do agree that checking the error message is not the most robust approach though.




-- 
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: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Kami commented on pull request #1592: Make retry handle http status codes

Posted by GitBox <gi...@apache.org>.
Kami commented on pull request #1592:
URL: https://github.com/apache/libcloud/pull/1592#issuecomment-953272719


   @Germandrummer92 
   
   > Maybe we need a predicate for the exceptions instead of only the types configurable though @RunOrVeith.
   
   Yeah, having another more generic approach with a callable to which response / exception is passed or similar, would probably be more flexible than just having a fixed list of exception classes (and class with a callable which checks the exception type would just be a subclass of this parent 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: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Kami commented on pull request #1592: Make retry handle http status codes

Posted by GitBox <gi...@apache.org>.
Kami commented on pull request #1592:
URL: https://github.com/apache/libcloud/pull/1592#issuecomment-965573726


   I made a couple of small changes (07062df83f8867e11dc73c54a1e5ed3a44115294, 2c3e38087e18de9f452e856d10fdf8ccc5c8b926) and merged it into trunk. 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: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Kami commented on pull request #1592: Make retry handle http status codes

Posted by GitBox <gi...@apache.org>.
Kami commented on pull request #1592:
URL: https://github.com/apache/libcloud/pull/1592#issuecomment-962509755


   @Germandrummer92 Do you have any idea when you will have a chance to work on this PR?
   
   Ideally we would include those changes in the next release, but if it will take a while I will just do new release this week and we can include those changes in the next 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: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Kami commented on pull request #1592: Make retry handle http status codes

Posted by GitBox <gi...@apache.org>.
Kami commented on pull request #1592:
URL: https://github.com/apache/libcloud/pull/1592#issuecomment-961137293


   #1588 has been merged so now you can hopefully sync up your changes with latest trunk without too many issues.


-- 
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: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Germandrummer92 edited a comment on pull request #1592: Make retry handle http status codes

Posted by GitBox <gi...@apache.org>.
Germandrummer92 edited a comment on pull request #1592:
URL: https://github.com/apache/libcloud/pull/1592#issuecomment-965483417






-- 
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: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Germandrummer92 commented on pull request #1592: Make retry handle http status codes

Posted by GitBox <gi...@apache.org>.
Germandrummer92 commented on pull request #1592:
URL: https://github.com/apache/libcloud/pull/1592#issuecomment-965555443


   Yes, but there is another important change:
   
   Before the retry was only wrapping the raw requests request which actually doesn't raise any exception, since we don't explicitly call ```raise_for_status```, instead now it also wraps the creation of the responseCls (line  677 in common/base.py), in which the actual exception is raised (line 166 in common/base.py).
   
   So I actually think the retry wasn't very helpful before since the http exceptions weren't thrown.
   
   I will take a look at the failing tests now, they worked for me locally


-- 
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: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Germandrummer92 commented on pull request #1592: Make retry handle http status codes

Posted by GitBox <gi...@apache.org>.
Germandrummer92 commented on pull request #1592:
URL: https://github.com/apache/libcloud/pull/1592#issuecomment-954664362


   I signed the icla. 
   
   Overall i think it should work now as before, but the part in base.py with ignoring the HTTPException isn't super nice. Instead the better solution would probably be to have the retry wrapper wrapped around more code so that the actual exception by the rawResponseCls is then thrown and can be handled by the retry handler. I would only tackle that after #1588 is merged tough.


-- 
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: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Germandrummer92 commented on pull request #1592: Make retry handle http status codes

Posted by GitBox <gi...@apache.org>.
Germandrummer92 commented on pull request #1592:
URL: https://github.com/apache/libcloud/pull/1592#issuecomment-962620659


   @Kami Will try and get it done in the next few days. But also fine with putting it in the next 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: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org