Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify upload handling #246

Merged
merged 7 commits into from
Apr 13, 2019
Merged

Simplify upload handling #246

merged 7 commits into from
Apr 13, 2019

Conversation

boydgreenfield
Copy link
Contributor

@boydgreenfield boydgreenfield commented Apr 11, 2019

Uses concurrent.futures to simplify threading greatly.

Todos:

  • Synchronized global status bar
  • Make sure KeyboardInterrupt works properly (once only!)
  • Ensure Python 2.7 compatibility
  • Test on Windows (TODO with collaborator)

@boydgreenfield boydgreenfield force-pushed the ng-simpler-uploads branch 2 times, most recently from a3ea193 to ef5d757 Compare April 11, 2019 02:14
@coveralls
Copy link

coveralls commented Apr 11, 2019

Coverage Status

Coverage decreased (-0.2%) to 84.032% when pulling 682993f on ng-simpler-uploads into f441cf3 on master.

@boydgreenfield
Copy link
Contributor Author

Ha... concurrent.futures makes things simpler... Oh boy.

@boydgreenfield boydgreenfield force-pushed the ng-simpler-uploads branch 11 times, most recently from 32a9f20 to a345b61 Compare April 13, 2019 16:14
@boydgreenfield boydgreenfield marked this pull request as ready for review April 13, 2019 16:23
This commit also reworks the passing of the click.progressbar
to the uploader and requires it's passed as an object vs. boolean
from the top invoking call. This better supports both library and CLI
usage.
Specifically we add support for "canceling" and "finalizign" statuses
in the click.progressbar element. Note that we could generalize this
further by subclassing the click internal class, but it's already a
sufficiently gross hack that we're probably better off *not* further
extending the click internals.
…s threads

We do this via the use of the `atexit` module, which will handle any
program exits. Note we have to use our own `atexit_unregister` function
because `atexit.unregister` is unavailable in Python 2.7.
Notably, remove the use of boolean return values and rely exclusively
on exception hierarchy and retry-able vs. non-retryable exceptions.
This includes minor flow control logic changes (exceptions vs. return
values), but no major changes to the upload code paths.
Effectively, we do the following here:
1. Always check for /status if available when uploading via a proxy
2. If /status has a `complete: false` field, poll up to 15 minutes
3. Raise a RetryableUploadException or fail based on /status code

(2) specifically fixes bugs we were encountering wherein the Python
client could send all of the data and either:
A. Send a file with an error but not be listening for the reply,
   experience a ConnectionError, and then retry automatically
   even if the file was invalid and the proxy returned a 400
B. Send data too fast for the proxy to compress (with data getting
   buffered in the ALB/nginx) and then abort *despite the proxy
   ultimately completing successfully*

(A) led to bad files not being caught by the proxy. (B) could lead to
duplicate uploads to to erroneous retry requests. Both of these fixes
bring our implementation into line with the frontend JS code on
app.onecodex.com.
No longer display "INFO" for logging.info messages when invoked via the
CLI. We can now further configure these for both Python 2 and Python 3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants