elasticsearch/logstash-forwarder — pull request #151 by driskell (2014-02-01 10:11:16+00:00)
Title: Driskell

Hi Jordan / People,

I needed to do some changes to get the forwarder to work safely for me as the registrar persistence was not working properly at all. On each iteration, it only saves persistence for log files mentioned in the current batch of events due to a scoping problem on the state. But even fixing the scope means during startup, until all log files have been touched there is risk of state loss if another restart happens.

- Fixed the above by adding a new channel during startup, which allows resume_tracking to flag that it resumed a file. The previous persistence is loaded in main() now, and then all entries that resume_tracking starts harvesters on are passed to registrar, so that the initial re-write of the persistence ALWAYS contains the FULL list of files that still exist and where it was continued from.

Also, if a read ACK occurred (seems very often caused when the remote is logstash with redis output where redis has hit congestion_threshold) it would reconnect immediately and spam reconnect due to missing sleep. When it does sleep on other forms of errors, it reconnect in 1s too, which for me was too fast.

- I added a reconnect configuration option to the network settings in the JSON config file. This defaults to 1s. I've replaced the sleeps with this, and also added a sleep for when an ACK error occurs.

Lastly, I needed old files to be reopened even if the scanning stopped after 24 hours. Currently, the harvester stops after 24 hours on a file. The prospector has no knowledge of this so keeps the file in fileinfo[path] and never starts it again even if the file is modified again.

- I've done substantial changes to prospector. The dead time of 24h is now also (re?)implemented. I saw the bits in config.go and finished them so that each file can have a different dead time configured. When a harvester finishes it passes it's last known offset into a channel. If the prospector detects this it will then check modification time of the file and if it's no longer within the dead time it will respawn the harvester with its last known offset.

I also found a possibly logic problem during rotation where it might reopen the rotated file and reimport it all if it detected the new file BEFORE the rotated one. When detecting new file it would overwrite the fileinfo[path] for the previous, meaning the is_renamed_file fails when it does get to the rotated file. I think nobody sees this as Glob() maybe returning in alphabetical order, and most log rotate is in such a way it maintains correct order. Minor - but I adjusted to avoid this edge case as it would be bound to confuse the hell out of the first person that messes with the log rotate settings.

- This just needed the old file to be remember, so I added a missing_files[] to compliment the fileinfo[] since there was no way to keep two files same path in fileinfo[], and prefixing it or something just seemed dirty. If I'm honest I'm not fully happy with this, but it works.

I have posted in the following pull regarding the changes I have done:
https://github.com/elasticsearch/logstash-forwarder/pull/123

I've also seen this just appear by jhanson for the dead time config which conflicts with mine unfortunately. His approach was different and added it as a command line option.
https://github.com/elasticsearch/logstash-forwarder/pull/150

I've signed the CLA for a Kibana change I did earlier so that should be sorted. I really do apologise for not discussing this all in mailing list. I did look through the mailing list but seems activity for forwarder is low. It was something I just needed to get working and I did get a little carried away.

I welcome all feedback and comments, not all of my changes will be perfect or desired. I've cleaned up my commits today in a new branch and I'm willing to create other branches with cherry picked if desired.

Also, apologies for the long post. I do ramble on sometimes!

Regards,

Jason