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

Better handling for exceptions in destinations - fixes #45 #156

Merged
merged 6 commits into from
Apr 28, 2015

Conversation

itamarst
Copy link
Owner

@itamarst itamarst commented Apr 1, 2015

Fixes #45.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 99.32% when pulling e43ec03 on sending-exceptions-45 into e6b4ae7 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 99.32% when pulling e43ec03 on sending-exceptions-45 into e6b4ae7 on master.

@itamarst itamarst added the review label Apr 1, 2015
message = {"hello": 123}
logger.write({"hello": 123})
self.assertEqual(
messages,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, the penny drops! So exceptions raised in failing destinations are logged to the working destinations.
Perhaps it'd be nice to demonstrate that the order of the destinations is unimportant ie failures in destinations further down the list will be logged to working destinations earlier in the destination list.

Copy link
Owner Author

Choose a reason for hiding this comment

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

It's not "working destinations", it's "all destinations".

Copy link
Owner Author

Choose a reason for hiding this comment

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

Re-read tests, I think coverage implicitly does what you want in later test.

@wallrj
Copy link
Contributor

wallrj commented Apr 28, 2015

Thanks @itamarst

Notes:

  • Tests all pass locally with a single warning /home/richard/projects/HybridLogic/eliot/eliot/tests/test_action.py:526: DeprecationWarning: Action should be initialized with a TaskLevel action = Action(logger, "uuid", "/1/2/", "sys:me") which may or may not be expected.
  • Merged forward and re-ran tests - they all passed.
  • Ran Flocker test suite using this branch of Eliot (merged forward). There were errors, but none related to Eliot.

Points:

  1. travis-ci shows a few other warnings which may warrant followup issues: https://travis-ci.org/ClusterHQ/eliot/builds/56744628

WARNING: 'default' html theme has been renamed to 'classic'. Please change your html_theme setting either to the new 'alabaster' default theme, or to 'classic' to keep using the old default.

/home/travis/build/ClusterHQ/eliot/.tox/pypy/site-packages/setuptools/dist.py:292: UserWarning: The version specified ('0.6.0-22-g0fcbad3') is an invalid version, this may not work as expected with newer versions of setuptools, pip, and PyPI. Please see PEP 440 for more details.
(versioneer upgrade maybe?)

WARNING: html_static_path entry u'/home/travis/build/ClusterHQ/eliot/docs/source/_static' does not exist
(I mentioned that one before)

  1. Might be nice to document the handling of exceptions during serialisation and when writing to destinations.

Please address or answer these and the inline comments above and merge when you're satisfied with the build results.

@wallrj wallrj added approved and removed review labels Apr 28, 2015
@itamarst
Copy link
Owner Author

I will fix two of the issues mentioned above (sphinx theme, versioneer) and then do release when Travis is happy.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.34% when pulling b618d6a on sending-exceptions-45 into 6646d6a on master.

itamarst added a commit that referenced this pull request Apr 28, 2015
Better handling for exceptions in destinations.

Fixes #45.
@itamarst itamarst merged commit 0bfe8b4 into master Apr 28, 2015
@itamarst itamarst deleted the sending-exceptions-45 branch October 27, 2017 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Destinations should not catch exceptions silently
3 participants