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

Consider removing kinesis connection from 'start' #33

Closed
wryun opened this issue Oct 3, 2015 · 11 comments
Closed

Consider removing kinesis connection from 'start' #33

wryun opened this issue Oct 3, 2015 · 11 comments

Comments

@wryun
Copy link

wryun commented Oct 3, 2015

If kinesis is for some reason inaccessible when this plugin starts, the whole fluentd process crashes (exceptions thrown in start aren't handled, it seems).

In my opinion this is not ideal behaviour, both because there might be other output sources that should keep functioning in the absence of kinesis, AND if kinesis returns fluentd should have been buffering in the meanwhile.

Consider, for instance, the fluentd elasticsearch plugin's approach: https://github.com/uken/fluent-plugin-elasticsearch/blob/master/lib/fluent/plugin/out_elasticsearch.rb
(nothing is done in 'start')

What do you think?

@yuta-imai
Copy link
Contributor

@wryun

Thanks for your idea. I understand your situation. In my opinion, I think we should leave it as is, because if we cannot ensure connection with Kinesis Stream at initialization, we cannot guarantee even if Kinesis Stream exists or not. Please refer to S3 plugin

I know in some case, Kinesis does not respond to initial connection ensuring, however this is not often.

What do you think?

@wryun
Copy link
Author

wryun commented Oct 8, 2015

But say kinesis is really unreachable (this happened to us because of a bad network configuration). Don't we still want fluentd to buffer and send logs to other places?

(maybe this is more a problem with fluentd's handling of exception in 'start'...)

@wryun
Copy link
Author

wryun commented Oct 8, 2015

(it's also hard to detect failed starts in this situation in upstart, because it takes a long time to crash)

@yuta-imai
Copy link
Contributor

OK, so how about disabling it as configuration option?

@wryun
Copy link
Author

wryun commented Oct 8, 2015

That sounds like a great idea. Or even just document this as the failure behaviour (it surprised me).

@yuta-imai
Copy link
Contributor

@wryun

OK! I will do this in next week.

@yuta-imai
Copy link
Contributor

I will do this tonight.

@yuta-imai
Copy link
Contributor

I have left Amazon last week, so I need PR from this time.

@riywo
Copy link
Contributor

riywo commented Oct 30, 2015

Added a new option by #35. Thank you for the PR, @imaifactory !

@riywo riywo closed this as completed Oct 30, 2015
@wryun
Copy link
Author

wryun commented Oct 30, 2015

Thanks very much, @imaifactory and @riywo :)

@yuta-imai
Copy link
Contributor

@riywo thanks for merging :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants