fix 76 - change the way that target is created; rework response to quit #78

guyboertje · 2016-03-03T17:24:35Z

and timing of sincedb write.

guyboertje · 2016-03-03T17:54:14Z

spec/observe_tail_spec.rb

@@ -21,6 +21,16 @@
    FileUtils.rm_rf(sincedb_path)
  end

+  context "when sincedb path is given but ENV[\"HOME\"] or ENV[\"SINCEDB_PATH\"] is not given" do


This test fails on master

in this test we only test that the absense of HOME is tolerated, maybe we should adapt the name of this context?

jordansissel · 2016-03-03T18:18:04Z

lib/filewatch/tail.rb

    end

-    def initialize(opts = {})
-      @target = YieldingTail.new(opts)
+    def initialize(opts = {}, target = YieldingTail.new(opts))


Feels weird to have a scalar value after an options/keyword-style argument here. My feeling is that def initialize(target=..., opts={}) is ... better somehow, though I cannot quite articulate why I feel this way.

It does - I agree sort of, more so if one uses the hash shortcut:

FileWatch::Tail.new(:sincedb_path =>"", YieldingTail.new( :sincedb_path => sincedb_path, :start_new_files_at => :beginning, :stat_interval => 0 ) )

but the switching of the arguments would break:

the use of opts set in the previous argument when the default value for target is evaluated because target is not supplied,

the constructor expectation set already,

In practice no one should know about this dependency injection argument, but if someone want to write a different kind of tailing class - they have an injection mechanism.

jordansissel · 2016-03-03T18:29:15Z

The change looks OK to me. I haven't run the tests yet.

jordansissel · 2016-03-03T18:41:46Z

3 specs fail for me under MRI 2.2.1p85

rspec ./spec/watch_spec.rb:63 # FileWatch::Watch max open files when using ENV opens only 1 file
rspec ./spec/watch_spec.rb:75 # FileWatch::Watch max open files when using #max_open_files= opens only 1 file
rspec ./spec/watch_spec.rb:87 # FileWatch::Watch max open files when close_older is set opens both files

guyboertje · 2016-03-03T18:54:55Z

will update gemspec when I get a "Lets Go To Mars"

please show details of spec failures - they don't fail for me.

jordansissel · 2016-03-03T19:04:10Z

Failures:

  1) FileWatch::Watch max open files when using ENV opens only 1 file
     Failure/Error: expect(subject.max_active).to eq(max)

       expected: 1
            got: 4095

       (compared using ==)
     # ./spec/watch_spec.rb:66:in `block (4 levels) in <top (required)>'

  2) FileWatch::Watch max open files when using #max_open_files= opens only 1 file
     Failure/Error: expect(results).to eq([[:create_initial, file_path], [:modify, file_path]])

       expected: [[:create_initial, "/tmp/studtmp-f72fb5c94ff8a6c5d7d401bff1173ad36f20083f3d0418cef89d7f92319e/1.log"], [:modify, "/tmp/studtmp-f72fb5c94ff8a6c5d7d401bff1173ad36f20083f3d0418cef89d7f92319e/1.log"]]
            got: [[:create_initial, "/tmp/studtmp-f72fb5c94ff8a6c5d7d401bff1173ad36f20083f3d0418cef89d7f92319e/2.log"], [:modify, "/tmp/studtmp-f72fb5c94ff8a6c5d7d401bff1173ad36f20083f3d0418cef89d7f92319e/2.log"]]

       (compared using ==)

       Diff:

       @@ -1,5 +1,5 @@
        [[:create_initial,
       -  "/tmp/studtmp-f72fb5c94ff8a6c5d7d401bff1173ad36f20083f3d0418cef89d7f92319e/1.log"],
       +  "/tmp/studtmp-f72fb5c94ff8a6c5d7d401bff1173ad36f20083f3d0418cef89d7f92319e/2.log"],
         [:modify,
       -  "/tmp/studtmp-f72fb5c94ff8a6c5d7d401bff1173ad36f20083f3d0418cef89d7f92319e/1.log"]]
       +  "/tmp/studtmp-f72fb5c94ff8a6c5d7d401bff1173ad36f20083f3d0418cef89d7f92319e/2.log"]]
     # ./spec/watch_spec.rb:80:in `block (4 levels) in <top (required)>'

  3) FileWatch::Watch max open files when close_older is set opens both files
     Failure/Error:
       expect(results).to eq([
           [:create_initial, file_path], [:modify, file_path], [:timeout, file_path],
           [:create_initial, file_path2], [:modify, file_path2], [:timeout, file_path2]
         ])

       expected: [[:create_initial, "/tmp/studtmp-8886e850081c87b4c9c9cd2a11ef1c63d71e893616a5f64d40ddeeb18191/1.log"], [:modify, "/tmp/studtmp-8886e850081c87b4c9c9cd2a11ef1c63d71e893616a5f64d40ddeeb18191/1.log"], [:timeout, "/tmp/studtmp-8886e850081c87b4c9c9cd2a11ef1c63d71e893616a5f64d40ddeeb18191/1.log"], [:create_initial, "/tmp/studtmp-8886e850081c87b4c9c9cd2a11ef1c63d71e893616a5f64d40ddeeb18191/2.log"], [:modify, "/tmp/studtmp-8886e850081c87b4c9c9cd2a11ef1c63d71e893616a5f64d40ddeeb18191/2.log"], [:timeout, "/tmp/studtmp-8886e850081c87b4c9c9cd2a11ef1c63d71e893616a5f64d40ddeeb18191/2.log"]]
            got: [[:create_initial, "/tmp/studtmp-8886e850081c87b4c9c9cd2a11ef1c63d71e893616a5f64d40ddeeb18191/2.log"], [:modify, "/tmp/studtmp-8886e850081c87b4c9c9cd2a11ef1c63d71e893616a5f64d40ddeeb18191/2.log"]]

       (compared using ==)

       Diff:

       @@ -1,13 +1,5 @@
        [[:create_initial,
       -  "/tmp/studtmp-8886e850081c87b4c9c9cd2a11ef1c63d71e893616a5f64d40ddeeb18191/1.log"],
       - [:modify,
       -  "/tmp/studtmp-8886e850081c87b4c9c9cd2a11ef1c63d71e893616a5f64d40ddeeb18191/1.log"],
       - [:timeout,
       -  "/tmp/studtmp-8886e850081c87b4c9c9cd2a11ef1c63d71e893616a5f64d40ddeeb18191/1.log"],
       - [:create_initial,
          "/tmp/studtmp-8886e850081c87b4c9c9cd2a11ef1c63d71e893616a5f64d40ddeeb18191/2.log"],
         [:modify,
       -  "/tmp/studtmp-8886e850081c87b4c9c9cd2a11ef1c63d71e893616a5f64d40ddeeb18191/2.log"],
       - [:timeout,
          "/tmp/studtmp-8886e850081c87b4c9c9cd2a11ef1c63d71e893616a5f64d40ddeeb18191/2.log"]]
     # ./spec/watch_spec.rb:92:in `block (4 levels) in <top (required)>'

Finished in 1 minute 0.09 seconds (files took 0.10792 seconds to load)
47 examples, 3 failures

Failed examples:

rspec ./spec/watch_spec.rb:63 # FileWatch::Watch max open files when using ENV opens only 1 file
rspec ./spec/watch_spec.rb:75 # FileWatch::Watch max open files when using #max_open_files= opens only 1 file
rspec ./spec/watch_spec.rb:87 # FileWatch::Watch max open files when close_older is set opens both files

guyboertje · 2016-03-04T09:22:34Z

@jordansissel - what operating system and file system are you using for the tests?

It looks like, for failures 2 and 3, the OS is returning the globbed files as 2.log then 1.log - i.e. last written == first retrieved.

For the first failure, it looks like the changes to ENV are not being seen. What ruby version are you using?

guyboertje · 2016-03-04T09:49:52Z

I changed the create order of the files in the spec - 2.log is created before 1.log - maybe this will help with making failures 2 and 3 pass.

jordansissel · 2016-03-04T15:47:35Z

MRI 2.2.1, Fedora 23 64bit in a Hyper-V VM. Filesystem is XFS

guyboertje · 2016-03-04T16:56:47Z

ahhhh

jsvd · 2016-03-08T10:58:51Z

lib/filewatch/watch.rb

        file_deleteable = []
        # creates this array just once
        watched_files = @files.values

+        quit_proc = lambda do


I think this name is a bit misleading, this is more a cleanup task that only runs if quitting is true, and also due to tap's return behaviour, also returns quitting.

I don't have a good alternative other than to make this cleanup task an actual method and be more verbose by doing the 2 steps: cleanup if quitting; quitting, or moving "cleanup" to the ensure block of this method? assuming it will always run when leaving this method?

Anyway, it's up to you to change anything. I'm finding the code a bit hard to read, but it's correct.

I thought about using a method, but my logic is as follows:

its this method's exit finaliser (i looked v briefly at throw/catch but the syntax looked to be even more confusing)

the external method would have to have the file_deletable array passed to it so its not really a utility method

I never though about an ensure block - good idea 👍

One thing, do you think the ensure block executes in the synchronised block scope? I am not sure, more than likely not.

yeah, this would require a move of the synchronised call to outside of the each method..so instead of

def each(&block) synchronised do # ... end end

instead do

# ... synchronised do each(&block) end

up to you, I don't have a strong feeling on this, just a tiny itch

jsvd · 2016-03-09T13:55:45Z

LGTM

and timing of sincedb write. add failing spec doh! change create order of files in spec closes 76 fix smell of quit_proc and add tests to verify unreadable file handling split up HOME and SINCEDB_PATH change the logging in each when a general error occurs bump version closes jordansissel#76

fix 76 - change the way that target is created; rework response to quit

guyboertje force-pushed the fix/76 branch from ea86692 to 550d48c Compare March 3, 2016 17:51

guyboertje reviewed Mar 3, 2016
View reviewed changes

guyboertje force-pushed the fix/76 branch from 550d48c to e4033e1 Compare March 3, 2016 18:09

jordansissel reviewed Mar 3, 2016
View reviewed changes

guyboertje force-pushed the fix/76 branch from e4033e1 to 2eef32a Compare March 4, 2016 09:47

jsvd reviewed Mar 8, 2016
View reviewed changes

guyboertje force-pushed the fix/76 branch from b1ea7fb to 5ad3848 Compare March 9, 2016 15:32

guyboertje pushed a commit that referenced this pull request Mar 9, 2016

Merge pull request #78 from guyboertje/fix/76

703862a

fix 76 - change the way that target is created; rework response to quit

guyboertje merged commit 703862a into jordansissel:master Mar 9, 2016

guyboertje deleted the fix/76 branch March 10, 2016 10:03

guyboertje mentioned this pull request Mar 16, 2016

Filewatch library complains if HOME or SINCEDB_PATH variables are unset logstash-plugins/logstash-input-file#101

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix 76 - change the way that target is created; rework response to quit #78

fix 76 - change the way that target is created; rework response to quit #78

guyboertje commented Mar 3, 2016

guyboertje Mar 3, 2016

jsvd Mar 8, 2016

guyboertje Mar 8, 2016

jordansissel Mar 3, 2016

guyboertje Mar 3, 2016

jordansissel commented Mar 3, 2016

jordansissel commented Mar 3, 2016

guyboertje commented Mar 3, 2016

jordansissel commented Mar 3, 2016

guyboertje commented Mar 4, 2016

guyboertje commented Mar 4, 2016

jordansissel commented Mar 4, 2016

guyboertje commented Mar 4, 2016

jsvd Mar 8, 2016

guyboertje Mar 8, 2016

guyboertje Mar 8, 2016

jsvd Mar 8, 2016

jsvd Mar 8, 2016

jsvd commented Mar 9, 2016

fix 76 - change the way that target is created; rework response to quit #78

fix 76 - change the way that target is created; rework response to quit #78

Conversation

guyboertje commented Mar 3, 2016

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 Mar 3, 2016

jordansissel commented Mar 3, 2016

guyboertje commented Mar 3, 2016

jordansissel commented Mar 3, 2016

guyboertje commented Mar 4, 2016

guyboertje commented Mar 4, 2016

jordansissel commented Mar 4, 2016

guyboertje commented Mar 4, 2016

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

jsvd commented Mar 9, 2016