Skip to content

log errors when project download fails (refs #156)#184

Closed
alexbruy wants to merge 1 commit intomasterfrom
log-exceptions
Closed

log errors when project download fails (refs #156)#184
alexbruy wants to merge 1 commit intomasterfrom
log-exceptions

Conversation

@alexbruy
Copy link
Copy Markdown
Contributor

Save exception details to the log when download fails.

Refs #156.

Copy link
Copy Markdown
Contributor

@wonder-sk wonder-sk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you try to disable your networking while download job is running, will it store the error properly?


except ClientError:
except ClientError as e:
mp.log.error("Error while downloading project: " + str(e))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
mp.log.error("Error while downloading project: " + str(e))
mp.log.error("Error while querying project info: " + str(e))

"""
for future in job.futures:
if future.done() and future.exception() is not None:
job.mp.log.error("Error while downloading project: " + str(future.exception()))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to produce more details here - including traceback, because it could be arbitrary exception...

for future in job.futures:
if future.done() and future.exception() is not None:
job.mp.log.error("Error while downloading project: " + str(future.exception()))
job.mp.log.info("--- download aborted")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking that as the handling of failed futures is the same in _is_running() and in _finalize() methods, maybe it would be good to move it to a new function to avoid code duplication? So here the _is_running() body would become just this:

  1. _abort_download_on_failed_future()
  2. test for future.running()

@wonder-sk
Copy link
Copy Markdown
Contributor

could you please also add a check in test_download_failure() once #182 is merged, to check that job.failure_log_file contains the exception?

@alexbruy
Copy link
Copy Markdown
Contributor Author

Closed, as we decided to merge this with #182.

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.

2 participants