search

logstash-plugins / logstash-input-file

Apache License 2.0
68 stars 101 forks source link

Fix #248, sinceDB now can handle pathnames with spaces #249

Closed joe-insitro closed 4 years ago

joe-insitro commented 5 years ago

Fixes #248

This also fixes a couple of tests that previously didn't test anything due to the input IO iterator not being rewound before being passed to the code under test.

Also, this updates the README to specify a missing build step.

colinsurprenant commented 4 years ago

@joe-insitro Thanks for your contribution. The fix parts.join(" ") LGTM - left a comment on the specs changes. Let me know if you plan on addressing it!

andsel commented 4 years ago

Globally LGTM.

I've given a look at PR tested locally and it's ok. The problem: if a file has space in file name then the deserializer of sincedb record miss to read the full name but /path/file name is seen as /path/file and this make the file to be considered as completely new. The solution: when the serializes comes to read the 6th column instead of just read the value in the array at the current position, it joins with the next items (if there are) Do @colinsurprenant see anything bad in doing this? If we add a new 7th column, probably we have to revisit the parser and in that case quote the filename. For now it works, is not so proof for expansions

colinsurprenant commented 4 years ago

@andsel As per my previous comment, the fix LGTM. I would prefer we revert the spec changes, except for the io.rewind. I do not see why we need to move inode_struct and sincedb_valueand the expectations outside the block scope.

elasticsearch-bot commented 4 years ago

Andrea Selva merged this into the following branches!

Branch Commits
master 2c167da02fe48cfc70c3fa87d0c6f8ec2855f806
colinsurprenant commented 4 years ago

Fix now available in v4.1.13. Thanks @joe-insitro @andsel !