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

add discard_logs_on_reconnect_error in asyncsender #176

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

Conversation

enjoy-binbin
Copy link

issue: #175

When using asyncsender, there is a queue holds the logs to be send:

  • when except socket.gaierror, i clear the queue and print log
  • this avoids the back of the log blocked when sent
  • but the first one will inevitably have this problem
  • i'm thinking about other methods, maybe just simply print some logs

This is my solution, it doesn’t look particularly good. so feel free to close this if un-needed

@coveralls
Copy link

coveralls commented Apr 24, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling 6995dbb on enjoy-binbin:connect-hang into ace80f4 on fluent:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.07%) to 98.928% when pulling bb8efb8 on enjoy-binbin:connect-hang into ace80f4 on fluent:master.

@enjoy-binbin enjoy-binbin force-pushed the connect-hang branch 2 times, most recently from 5a23fd5 to c7b7a70 Compare April 24, 2021 08:20
@arcivanov
Copy link
Member

I'll review it at depth when I have time, but I'm inclined not to accept this change.

Unless I'm missing something the error should lead to a reconnect and resending of the logs. Clearing the queue will cause the loss of perfectly good logs due to a simple reconnect issue, which would not be acceptable.

@enjoy-binbin
Copy link
Author

Yep Not problem. I understand it is risky

Or maybe we should just print some logs to help the user for debug

@arcivanov
Copy link
Member

arcivanov commented Apr 24, 2021

We can consider a tunable setting which would "discard logs on reconnect" and defaults to False. Reconnects are a part of life and discarding logs isn't useful in most cases IMO.

I tend to make decisions against arbitrary loss of data.

@enjoy-binbin
Copy link
Author

Right. I can give a try. Thanks @arcivanov

@enjoy-binbin enjoy-binbin changed the title clear queue when socket.gaierror occurs add discard_logs_on_reconnect_error in asyncsender Apr 24, 2021
@enjoy-binbin
Copy link
Author

@arcivanov
I add a discard_logs_on_reconnect_error setting default false. Maybe not good enough

@arcivanov arcivanov reopened this May 7, 2021
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.

3 participants