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

Update file with sync operation #941

Merged
merged 2 commits into from
Feb 10, 2020

Conversation

Aristat
Copy link
Contributor

@Aristat Aristat commented Feb 6, 2020

Need to add a cleanup file variable, after we renamed, but I don’t know how to do it better, just set it to nil or create a file?

Problem reproduced only without asyn logic(because we use one instance)
Without this fix, we will continue to write logs to the already renamed file

thanks!

@waltjones
Copy link
Contributor

You also need to close the previous file handle.

I should have asked before, but is there a reason you can't use logrotate (or equivalent)?

@Aristat
Copy link
Contributor Author

Aristat commented Feb 6, 2020

yes, need close, I will fix, thx.
if you mean https://github.com/logrotate/logrotate, we already use rollbar on many projects, but because of ddos - redis / sidekiq increases the load, and we want to write an agent that would read data from files so as not to remove rollbar from the projects(without additional utils)

@waltjones
Copy link
Contributor

Logrotate specifically manages rotation of log files. It can be used to do all of the opening, closing, and renaming of log files, including the log files from Rollbar.

@Aristat
Copy link
Contributor Author

Aristat commented Feb 6, 2020

Isn't collision possible with file flush there?(lost data or something else)?

@waltjones
Copy link
Contributor

With logrotate? There shouldn't be. That's exactly the use it was designed for, and has been used for years in large ops environments.

@Aristat
Copy link
Contributor Author

Aristat commented Feb 6, 2020

sounds good, we will try it too, thanks 👍
but now we have not very large loads, the current gem solution will be enough

@waltjones waltjones merged commit 65653bf into rollbar:master Feb 10, 2020
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