Skip to content

Conversation

@cabiad
Copy link

@cabiad cabiad commented Feb 2, 2015

Uses a threadlocal variable to store errors, which a user of this library can optionally access and act on after initializing the sender (initially connecting to the fluentd server) or emitting an event.

Also includes relevant tests.

setup.py Outdated
Copy link
Author

Choose a reason for hiding this comment

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

whoops, I'll resubmit in a sec without this change

@cabiad
Copy link
Author

cabiad commented Feb 24, 2015

@kiyoto - I just wanted to follow up on our earlier email discussion. Do you think this PR can be merged?

@cabiad
Copy link
Author

cabiad commented Mar 11, 2015

🔔 - Any update on this?

@kiyoto
Copy link
Contributor

kiyoto commented Mar 11, 2015

Ah sorry. @yyuu and @kzk do you think you can review and merge the changes?

@yyuu
Copy link
Member

yyuu commented Mar 12, 2015

@kiyoto The test code won't success if fluentd isn't listening on 24224/tcp. It should be rewritten with using mock to pass tests on travis-ci.org.

// I found travis-ci.org is not working on fluent/fluent-logger-python. See also #32

@cabiad
Copy link
Author

cabiad commented Mar 12, 2015

@yyuu and @kiyoto - The existing tests also fail in that case, but because error information was discarded, the failure was not detectable. I added comments to indicate this where appropriate to existing tests.

I believe that this is therefore an important fix. In my opinion, it makes sense to take this increment of value right away. We can update all tests to work properly without a running fluentd instance in another PR.

@ghost
Copy link

ghost commented Apr 15, 2016

Hiding the connect or send error is crazy, this library needs to be fixed. Look how the ruby library returns a success or failure by bubbling up a true or false on the post() method here:
https://github.com/fluent/fluent-logger-ruby/blob/master/bin/fluent-post

This lib should ideally do the same, beside storing the error, a new event should have a post() method and that should return a true/false on success or not. That makes usage far more clear IMO.

repeatedly added a commit that referenced this pull request Jul 25, 2016
This patch is based on cabiad's patch.
#29
@repeatedly repeatedly mentioned this pull request Jul 25, 2016
@repeatedly
Copy link
Member

I merged #59.
This patch is baesd on this PR with additional changes.
Thanks for the contribution!

@repeatedly repeatedly closed this Jul 26, 2016
@tophat-jenkins tophat-jenkins deleted the upstream branch June 29, 2022 20:13
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.

4 participants