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

9897: Logs errors encountered by block prefetch workers. #56

Merged

Conversation

colin-nolan
Copy link
Contributor

No description provided.

@tfmorris
Copy link

tfmorris commented Nov 16, 2016

Ugh!

         except Exception:
              pass

Now there's an anti-pattern if there ever was one.

@@ -516,7 +517,7 @@ def _block_prefetch_worker(self):
return
self._keep.get(b)
except Exception:
pass
_logger.error(traceback.format_exc())

Choose a reason for hiding this comment

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

I think it'd be useful to know where there exception is happening, so perhaps something like:

_logger.exception('Exception doing block prefetch')

This would also allow you to avoid having to import the traceback module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that knowing that a failure has occurred is a step in the right direction. However, that solution doesn't provide any insight into the nature and origin of the related fault. In addition, it obfuscates the location of that exception handler, making it difficult for someone not familiar with the codebase to know where to start.

A traceback provides much more useful information, including the location you suggest would be useful (albeit expressed without knowledge of the higher level context).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the purpose of this Pokémon exception handler is to suppress failures from unknown rare errors. Therefore, the message should scarcely be seen by users of the SDK. This being the case, it seems reasonable for it to be intended for developers.

With regards to the extra import: the import statement could be moved in the except block?

Copy link
Contributor

Choose a reason for hiding this comment

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

apparently _logger.exception() does include a stack trace in the output

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe include "Warning:" in the exception message as well so that people know this is not an expected exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe that this is the case:

>>> _logger = logging.getLogger("test")
>>> def test():
...     _logger.exception("Some message")
... 
>>> test()
ERROR:test:Some message
None

Copy link
Member

Choose a reason for hiding this comment

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

@colin-nolan "_logger.exception() does include a stack trace" is true when called from an exception handler (as in "This method should only be called from an exception handler." :)

import logging
logging.basicConfig()
_logger = logging.getLogger()
try:
    nan = 1/0
except:
    _logger.exception("boom")
ERROR:root:boom
Traceback (most recent call last):
  File "boom.py", line 5, in <module>
    nan = 1/0
ZeroDivisionError: integer division or modulo by zero

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies; I stand corrected. I wasn't aware that this method's behaviour was context sensitive.

@arvados-bot arvados-bot merged commit e41abf0 into arvados:master Nov 22, 2016
@tomclegg
Copy link
Member

@colin-nolan thanks!

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.

5 participants