Merge all other contributions #32

manusfreedom · 2014-06-05T10:26:34Z

New features
Fix Windows bugs

…hile preserving the sincedb_write_interval

…hing'

Conflicts: filewatch.gemspec

jordansissel · 2014-08-01T06:11:25Z

lib/filewatch/tail.rb

+      @sincedb_write_pending = false
+      @sincedb_writing = false
+
+      System.gc()


Why do this GC call?

Also,I did not add this below so I assume you are talking to the group

Sent from my iPad

On Aug 1, 2014, at 2:11 AM, Jordan Sissel notifications@github.com wrote:

In lib/filewatch/tail.rb:

@@ -242,6 +291,12 @@ def _sincedb_write
rescue => e
@logger.warn("_sincedb_write rename/sync failed: #{tmp} -> #{path}: #{e}")
end
+

@sincedb_last_write = now

@sincedb_write_pending = false

@sincedb_writing = false

System.gc()
Why do this GC call?

—
Reply to this email directly or view it on GitHub.

I added it to be sure we regulary clean the memory.
I use this gem on logstash and we can have memory problem during mass write in logs.

Adding some context here, I tried this updated tail.rb code in Logstash and it fails with the following error:

In order for it to run in Logstash, I have to add the following 2 lines to the beginning of the file so it knows what System is.

require "java" java_import "java.lang.System"

Certainly, curious if the GC call is required since the existing Logstash filewatch tail.rb doesn't have it.

I've never found it necessary to invoke GC manually in Logstash, so without knowing more details, I'll remove this before merging.

jordansissel · 2014-08-01T06:13:11Z

I haven't fully reviewed the code, but I am nervous about the lack of any tests for the fixes as proposed.

I recognize that there aren't much in the way of tests in the current codebase, but this code is pretty old and I am a bit more aware of my need to test these days. Thoughts on adding tests?

bdanziger · 2014-08-01T08:34:00Z

I am not sure if this question is directed to me or others. I found some issues with _sincedb code and restarting which were a big problem or me. It was clear that the code wasn't flusuhing the file properly. I was going to setup my test cases to run as was expected but I realized I might not have the latest code and I became confused. I found different version in ruby gems that was a little I fferent then what was in the git. So, I emailed out a bunch of questions about this and never heard back from anyone. So, I didn't know the right thing to do, made myself my own version and moved on. My version with the changes i proposed has been running without a single problem since march or April on a daily basis and I am running about 5 or 6 instances of this. I had my own test case but it was manual.

Sent from my iPad

On Aug 1, 2014, at 2:13 AM, Jordan Sissel notifications@github.com wrote:

I haven't fully reviewed the code, but I am nervous about the lack of any tests for the fixes as proposed.

I recognize that there aren't much in the way of tests, but this code is pretty old and I am a bit more aware of my need to test these days. Thoughts on adding tests?

—
Reply to this email directly or view it on GitHub.

bdanziger · 2014-08-01T08:38:10Z

I hit send too soon,,,,

Github , it looks like it is at 0.5.0
Ruby gems shows 0.5.1

I am not sure how ruby gems could be ahead of the github so I had no idea if I had the latest version of the code.

If you can clue me in on how I make sure I have the latest one, I can go back to this ....make sure the changes are where they should be and don't conflict with any other releases and I can add in a test case.

But, I would need a little direction. Maybe it is me but the different versions make no sense to me,

Thanks

Sent from my iPad

On Aug 1, 2014, at 2:13 AM, Jordan Sissel notifications@github.com wrote:

I haven't fully reviewed the code, but I am nervous about the lack of any tests for the fixes as proposed.

I recognize that there aren't much in the way of tests, but this code is pretty old and I am a bit more aware of my need to test these days. Thoughts on adding tests?

—
Reply to this email directly or view it on GitHub.

manusfreedom · 2014-08-01T09:29:19Z

@jordansissel, I don't know how can I add tests, I can only give my experience using my version.
We use this version with my patched logstash (https://github.com/manusfreedom/logstash).
It's deployed on 120 servers (40 productions), who generate 1 to 2 millions of logs per day each.
And ruby-filewatch manages from 10 to 75 log files per server.
All servers are under Windows Server 2008 R2 or 2012 R2.

Here, a screenshot of Kibana for 9 productions servers: http://demo.ovh.eu/View/44e45f23270933c2384f332f9d96ee76/0
111 millions of logstash events during last week (and some are multilines logs).

jordansissel · 2014-08-12T06:16:55Z

lib/filewatch/watch.rb

+          # since there is no update, we should pass control back in case the caller needs to do any work
+          # otherwise, they can ONLY do other work when a file is created or modified
+          @logger.debug("#{path}: nothing to update")
+          yield(:noupdate, path)


a no-op event doesn't feel like it fits well. filewatch isn't intended to be a ticking event system, seems like sincedb syncing should be done in a separate thread instead of using this method.

I am not agree, because if both thread are not perfectly sync, you can lost or duplicate some data after a stop/start.

If there are any questions with my update, I can answer then very specifically when I get back to the hotel tonight.

Unfortunately, I was not certain that the git had the latest code because I found differences with the same gem code. I questioned this months ago but never heard back. I will help in any way you would like . Just let me know

Sent from Yahoo Mail on Android

Not sure I follow that. Which threads see not on sync...tail and watch? All this is doing is letting one give up control so it doesn't lock out the caller....otherwise sincedb can become stale. I have tested this for months without an issue...where prior start and stop constantly were having issues because sincedb was not flushed out. I don't have the code in front of me because I am away on vacation but in the the release, I also fixed a start stop timing issue that is documented in the update . Maybe this is what you are referring to...but hard to tell from the statement

Sent from Yahoo Mail on Android

jordansissel · 2014-08-12T06:42:32Z

I am in favor of improved windows support, and have mostly completed my review of this patch.

My main concern is the backwards incompatibility in the sincedb file format. We cannot change this format. I suspect the change was to support files with spaces in them? If so, we can work around that without changing the file format.

The GC call should also be removed especially because it is not valid under MRI or Rubinius.

If there is no response on this within a day or so, I will fix the code prior to merging because there are a number of users concerned with windows support problems.

manusfreedom · 2014-08-12T20:43:12Z

@jordansissel I perfectly understand your concern about backwards incompatibility, maybe you change the space by another char only on windows?
For GC, I will keep it on my version, I understand the problem about MRI or Rubinius.

michio-nikaido · 2014-08-12T20:58:36Z

lib/filewatch/tail.rb

+        # updated files overwrite original ones, resulting in inode changes.  In order to 
+        # avoid having the sincedb.member check from failing in this scenario, we'll 
+        # construct the inode key using the path which will be 'stable'
+        inode = [path, stat.dev_major, stat.dev_minor]


I updated my PR with this change to better handle some uncommon (but legal) path characters in linux:

michio-nikaido@0e3e6c2

I don't see it here, so I wanted to point that out because an embedded space would otherwise throw off the line parsing for sincedb

Conflicts: lib/filewatch/tail.rb lib/filewatch/watch.rb

Fix @sincedb[sincedb_record_uid] null when addin a file Fix missing end keyword Fix typo

manusfreedom · 2015-06-04T16:14:24Z

My last commits are in test with logstash 1.5.0 on 50 servers and will be with 120 servers next monday.

manusfreedom · 2015-06-10T06:47:23Z

Works well on 120 servers and million lines.
Can be merged and/or tested.

bmeier-pros and others added 8 commits January 14, 2014 15:41

merge in change from @johnarnold to bump up the block size

7581443

enhancing sincedb to properly flush when files have been read from, w…

e297063

…hile preserving the sincedb_write_interval

Add follow_only_path flag to dumb down filewatching to just paths

a376432

Merge remote-tracking branch 'remotes/bdanziger/sincedb_enhanced_flus…

96b0300

…hing'

Merge remote-tracking branch 'remotes/bwmeier/edwinf-patch'

1f8c14c

Merge remote-tracking branch 'remotes/bwmeier/filewatch-patches'

e955a55

Conflicts: filewatch.gemspec

Change sincedb file inner separtor to work on path with space

bcbaebd

Fix sincedb write

1a3b577

jordansissel reviewed Aug 1, 2014
View reviewed changes

jordansissel reviewed Aug 12, 2014
View reviewed changes

michio-nikaido reviewed Aug 12, 2014
View reviewed changes

suyograo mentioned this pull request Nov 26, 2014

File Input - .sincedb file is broken on Windows logstash-plugins/logstash-input-file#2

Closed

This was referenced Feb 19, 2015

Clean up obsolete PR and issues #49

Open

on EOF, write to sincedb #24

Closed

manusfreedom added 4 commits June 1, 2015 17:45

Merge remote-tracking branch 'remotes/jordansissel/master'

92e660f

Conflicts: lib/filewatch/tail.rb lib/filewatch/watch.rb

Fix too much end

2d92102

Fix no sincedb write at start (sincedb_write_pending init)

a9f884a

Fix @sincedb[sincedb_record_uid] null when addin a file Fix missing end keyword Fix typo

Fix sincedb writed before ending current work.

7ad7421

manusfreedom mentioned this pull request Jun 10, 2015

Add follow_only_path option, linked to https://github.com/jordansissel/ruby-filewatch/pull/32 logstash-plugins/logstash-input-file#50

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge all other contributions #32

Merge all other contributions #32

manusfreedom commented Jun 5, 2014

jordansissel Aug 1, 2014

bdanziger Aug 1, 2014

manusfreedom Aug 1, 2014

ppf2 Aug 1, 2014

jordansissel Aug 1, 2014

jordansissel commented Aug 1, 2014

bdanziger commented Aug 1, 2014

bdanziger commented Aug 1, 2014

manusfreedom commented Aug 1, 2014

jordansissel Aug 12, 2014

manusfreedom Aug 12, 2014

bdanziger Aug 12, 2014

bdanziger Aug 12, 2014

jordansissel commented Aug 12, 2014

manusfreedom commented Aug 12, 2014

michio-nikaido Aug 12, 2014

manusfreedom commented Jun 4, 2015

manusfreedom commented Jun 10, 2015

Merge all other contributions #32

Are you sure you want to change the base?

Merge all other contributions #32

Conversation

manusfreedom commented Jun 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordansissel commented Aug 1, 2014

bdanziger commented Aug 1, 2014

bdanziger commented Aug 1, 2014

manusfreedom commented Aug 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordansissel commented Aug 12, 2014

manusfreedom commented Aug 12, 2014

Choose a reason for hiding this comment

manusfreedom commented Jun 4, 2015

manusfreedom commented Jun 10, 2015