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

prohibit fluent logger to chocke on its own log messages in case of failure #59

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danischroeter
Copy link

if the fluent logger is used as a logger for slf4j and it fails to send messages it will choke on its own failure messages (cannot send message) once the connection to fluentd is broken
->this change breaks this circle by only logging the failure to send messages max once per minute

…nd messages it will choke on its own failure messages (cannot send message)

->this change breaks this circle by only logging the failure to send messages once per minute
@komamitsu
Copy link
Member

@danischroeter afc9e84 mitigates the issue?

@danischroeter
Copy link
Author

@komamitsu sorry was off...
nope the referenced commit only reduces the problem to the case when reconnect is enabled. For reconnect the problem stays the same.
If you agree I resolve the merge conflict so you could merge.

@@ -179,6 +179,14 @@ private synchronized boolean send(byte[] bytes) {
return true;
}

private void logCannotSendMaxOncePerMinute() {
// to not choke on our own log message
if(System.currentTimeMillis() + 60 * 1000 > lastCannotSendLogTimestamp) {
Copy link
Member

Choose a reason for hiding this comment

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

The suppression time 60 seconds seems too long to me. Could you make this value configurable (default: 10 seconds)?

@komamitsu
Copy link
Member

I see. Can I ask you to resolve the merge conflict and address this comment https://github.com/fluent/fluent-logger-java/pull/59/files#r80361139 ? 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.

2 participants