Add config option for ignore_after #87

guyboertje · 2015-12-21T16:50:51Z

NOTES FOR REVIEWERS:

This builds on changes in filewatch v0.7.0, now published
This builds on changes in the multiline codec v2.0.5, now published
Logic: filewatch knowing about close_older, calls timed_out on a file input listener which calls evict(path) on the identity_map_codec which calls auto_flush on the multiline codec and also evicts the identity from the map. Auto_flush should have already assembled and sent the event from any buffered lines if the auto_flush interval is less than the close_older interval.

suyograo · 2015-12-22T08:04:57Z

lib/logstash/inputs/file.rb

+    # set the mode, defaults to "tail"
+  # in read_to_eof mode filewatch will signal the EOF with a "\x04" control character
+  # in tail mode behaviour is as before
+  config :ignore_after, :validate => :number, :default => 24 * 60 * 60


Filebeat calls this ignore_older. We talked about being consistent with filebeat to aid any migration to and from these components.

Also, can we add a more descriptive message (copied from filebeat) -

"If this option is specified, file input ignores any files that were modified before the specified timespan in milliseconds. This timeout also triggers a release of resources obtained"

ref: https://www.elastic.co/guide/en/beats/filebeat/current/filebeat-configuration-details.html#_ignore_older

sure, the comment needs to change.
but I spoke to Tudor and they are having real issues with the name and the two functions controlled by one option thing - I asked him to comment on our issue, he did: #81 (comment)

guyboertje · 2015-12-22T17:14:32Z

Jenkins please test this

suyograo · 2015-12-22T18:21:58Z

logstash-input-file.gemspec

@@ -1,7 +1,7 @@
 Gem::Specification.new do |s|

  s.name            = 'logstash-input-file'
-  s.version         = '2.0.3'
+  s.version         = '2.0.4'


Lets make it 2.1.0. This is new functionality being added

ph · 2015-12-22T18:31:46Z

This PR doesn't have an update to the changelog.

ph · 2015-12-22T18:32:47Z

lib/logstash/inputs/file.rb

-    stop # if the pipeline restarts this input.
-    @tail = FileWatch::Tail.new(@tail_config)
+    # if the pipeline restarts this input.
+    stop


The stop here is to close unwanted open fd so we don't leak them?
Might be good to add comment why we actually call stop here.

Will expand comment

ph · 2015-12-22T19:11:33Z

jenkins test this please

ph · 2015-12-22T19:13:23Z

I did a review of the code, added a few comments.

Also the test are passing locally but not on jenkins. I have modified the jenkins job to clear the workspace before running any test. Without that option we can have cached/incompatible version of the gem available.

I also did a really minimal test of the file input and it look good!

ph · 2015-12-22T20:31:10Z

@guyboertje I was looking the the jenkins jobs that test all the plugins and I have found this problem.
http://build-eu-00.elasticsearch.org/view/LS%20Master/job/Logstash_Master_Default_Plugins/903/console

I think this PR also fix this issue?

  1) LogStash::Inputs::File when wildcard path and a multiline codec is specified collects separate multiple line events from each file
     Failure/Error: expect(events.size).to eq(event_count)

       expected: 2
            got: 0

       (compared using ==)
     # ./vendor/bundle/jruby/1.9/gems/logstash-input-file-2.0.3/spec/inputs/file_spec.rb:242:in `(root)'
     # ./vendor/bundle/jruby/1.9/gems/rspec-wait-0.0.8/lib/rspec/wait.rb:46:in `(root)'
     # ./rakelib/test.rake:72:in `(root)'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/task.rb:240:in `execute'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/task.rb:235:in `execute'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/task.rb:179:in `invoke_with_call_chain'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/task.rb:172:in `invoke_with_call_chain'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/task.rb:165:in `invoke'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/application.rb:150:in `invoke_task'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/application.rb:106:in `top_level'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/application.rb:106:in `top_level'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/application.rb:115:in `run_with_threads'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/application.rb:100:in `top_level'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/application.rb:78:in `run'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/application.rb:176:in `standard_exception_handling'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/application.rb:75:in `run'

guyboertje · 2015-12-23T18:17:13Z

Jenkins jobs should work now. All waits are dependent on state change occurring before continue and not guess-work sleeps.

ph · 2015-12-28T14:20:25Z

@jsvd I agree with your comment, but I suggest we create a specific issue for it. this would also allow us to remove the last pieces of insists.

ph · 2015-12-28T14:25:45Z

code LGTM but lets refactor the test in another run?

guyboertje · 2015-12-28T14:48:33Z

+1 on refactoring tests later. Thanks guys.

begin to add ignore_after config option depends on filewatch 0.6.8 add timed_out method to TailListener and specs revert Gemfile changes for PR split and rename ignore_after option, update gem deps to published ver. bump ver, make suggested review changes and improve spec reliability update changelog Fixes logstash-plugins#81, Fixes logstash-plugins#89, Fixes logstash-plugins#90

elasticsearch-bot · 2015-12-29T12:19:46Z

Guy Boertje merged this into the following branches!

Branch	Commits
master	`61142ab`

ruflin · 2016-01-14T08:17:44Z

lib/logstash/inputs/file.rb

+  # was last modified before the specified timespan in seconds, the file is
+  # ignored. After it's discovery, if an ignored file is modified it is no
+  # longer ignored and any new data is read. The default is 24 hours.
+  config :ignore_older, :validate => :number, :default => 24 * 60 * 60


@guyboertje I'm currently adding the close_older function also the filebeat. As we have now ignore_older and close_older, I suggest to set ignore_older to at least 72 hours or even "infinite" by default. The reasoning behind this is that we had some edge cases in the past. Any thoughts? https://github.com/elastic/filebeat/issues/181

suyograo reviewed Dec 22, 2015
View reviewed changes

guyboertje assigned guyboertje and ph and unassigned guyboertje Dec 22, 2015

suyograo reviewed Dec 22, 2015
View reviewed changes

ph reviewed Dec 22, 2015
View reviewed changes

guyboertje force-pushed the feature/ignore_after branch from c7f0cb8 to 9d83422 Compare December 29, 2015 12:15

guyboertje closed this in 61142ab Dec 29, 2015

guyboertje deleted the feature/ignore_after branch December 30, 2015 14:09

ruflin reviewed Jan 14, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config option for ignore_after #87

Add config option for ignore_after #87

guyboertje commented Dec 21, 2015

suyograo Dec 22, 2015

suyograo Dec 22, 2015

guyboertje Dec 22, 2015

guyboertje commented Dec 22, 2015

suyograo Dec 22, 2015

ph Dec 22, 2015

guyboertje Dec 23, 2015

ph commented Dec 22, 2015

ph Dec 22, 2015

guyboertje Dec 23, 2015

ph commented Dec 22, 2015

ph commented Dec 22, 2015

ph commented Dec 22, 2015

guyboertje commented Dec 23, 2015

ph commented Dec 28, 2015

ph commented Dec 28, 2015

guyboertje commented Dec 28, 2015

elasticsearch-bot commented Dec 29, 2015

ruflin Jan 14, 2016

Add config option for ignore_after #87

Add config option for ignore_after #87

Conversation

guyboertje commented Dec 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guyboertje commented Dec 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ph commented Dec 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ph commented Dec 22, 2015

ph commented Dec 22, 2015

ph commented Dec 22, 2015

guyboertje commented Dec 23, 2015

ph commented Dec 28, 2015

ph commented Dec 28, 2015

guyboertje commented Dec 28, 2015

elasticsearch-bot commented Dec 29, 2015

Choose a reason for hiding this comment