scan older files to be harvested based on mod time #4374

codeperfector · 2017-05-22T22:05:09Z

config option to scan older files first.

Have a look at issue: https://discuss.elastic.co/t/filebeat-harvester-picks-up-files-in-random-order-when-scanning/86542/4

elasticmachine · 2017-05-22T22:05:11Z

Can one of the admins verify this patch?

elasticmachine · 2017-05-22T22:07:02Z

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

karmi · 2017-05-22T22:15:16Z

Hi @waynemz, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

ruflin · 2017-05-23T13:33:31Z

@waynemz Thanks for sharing your implementation. This is definitively something we should discuss. Some thoughts:

In case of a large amount of files, this could have quite a performance impact as all files need to be touched to fetch the ModTime().
This feature should be off by default
If we introduce this feature, we should allow desc and ascending order as there were also dicussions where people were interested in the most recent files first.
In general this mainly becomes relevant if harvester_limit is used. Assuming harvester_limit is 10, it will still not guarantee which of the 10 oldest files is harvested at which speed.
Some ideas for the config naming if we allow both sort orders?

codeperfector · 2017-05-24T01:15:10Z

@ruflin I agree the feature should be off by default given the additional performance impact. Instead of a boolean the config option could be a string with values off/ascending/descending.
I believe some documentation might be needed to explain the performance impact and the fact this would only be most useful in combination with the harvester_limit option, especially when harvester_limit=1
As for the config name I would suggest something like prospector_scan_order but I'm open to any suggestions you have.

@karmi I added my other email to my github account.

ruflin · 2017-05-26T07:01:51Z

I like the suggestion of scan_order but would leave out the prospector part. It goes in line with scan_frequency. It's not really the scan that happens in this order but the starting of harvester, but I think that is more a technical thing. scan_order should be pretty simple to understand.

I have in the back of my head that if we add this file, pretty soon people will ask for it to be sorted by filename instead of date. Could be perhaps take this already into account now when we design the config. Or is it overkill?

codeperfector · 2017-05-28T20:09:20Z

@ruflin Thats a great suggestion. I'll incorporate the filename as well. That will bring the possible values for scan_order to five: off(default), oldest_file_first, newest_file_first, ascending_by_name, descending_by_name
Can you suggest better value strings?

codeperfector · 2017-05-29T13:26:54Z

I'll correct the go imports style issue but I could not figure out what went wrong with the test:
[fail] 1.09% test_base.Test.test_invalid_config_cli_param: 0.0296s
I didn't introduce a new cli param here

BTW I'm using Gogland Early Access as my editor

ruflin · 2017-05-29T13:38:18Z

For the config options I was more thinking of

scan.order: "desc", "asc"
scan.sort: "modtime", "filename", "none"

This kind of implies we should switch to scan.frequency one day ;-)

ruflin · 2017-05-29T13:39:03Z

For the failing check: Run make fmt in the filebeat directory. This should do the cleanup for you.

ruflin

Thanks for pushing this forward. I left a few comments.

Could you also please add a changelog entry? We also will have to add docs for this feature. Could you also add a very short doc entry to it? The config docs are in https://github.com/elastic/beats/blob/master/filebeat/docs/reference/configuration/filebeat-options.asciidoc

ruflin · 2017-06-01T00:14:34Z

filebeat/prospector/log/prospector.go

@@ -7,6 +7,9 @@ import (
 	"path/filepath"
 	"time"



can you remove newline?

ruflin · 2017-06-01T00:18:01Z

filebeat/prospector/log/prospector.go

+}
+
+func getSortedFiles(scanOrder string, scanSort string, sortInfos []FileSortInfo) []FileSortInfo {
+	switch scanOrder + "_" + scanSort {


Could you perhaps to that in two switch / case? So first check the scanSort and then the order inside each? It feels a bit strange to create a string for comparison. Perhaps there is an even "shorter" option.

You could move the sort.Slice part to after switch and only define the function which will be used inside the switch cases. This will make the cod cleaner I think.

ruflin · 2017-06-01T00:21:05Z

filebeat/prospector/log/prospector.go

 // Scan starts a scanGlob for each provided path/glob
 func (p *Prospector) scan() {

-	for path, info := range p.getFiles() {
+	paths := p.getFiles()
+	files := getSortInfos(paths)


This should not happen in every call, only if sorting is enabled as this could have a performance impact. I would expect something like:

if config.sort.Enabled() { paths := paths.Sort()

This is heavily simplified but you get what I'm trying to explain? Do we even need to expose the fileinfo outside of the sort?

codeperfector · 2017-06-01T22:56:53Z

@ruflin I made the suggested changes. Can you resolve the conflict with changelog?

codeperfector · 2017-06-06T02:20:02Z

I fixed the goimports style but there is some build failure with the full yml file. Not sure how to resolve that.

tsg · 2017-06-06T12:34:15Z

@waynemz You can fix that by running make update. Now there is also a conflict in the CHANGELOG (sorry, we've merged a large changelog modification) so you need to sync with master.

codeperfector · 2017-06-10T20:32:14Z

@ruflin @tsg Fixed all conflicts. Please approve.

tsg · 2017-06-12T10:06:52Z

jenkins, test it

tsg · 2017-06-12T10:09:44Z

filebeat/prospector/log/prospector.go

+			sortFunc = func(i, j int) bool {
+				return sortInfos[i].info.ModTime().After(sortInfos[j].info.ModTime())
+			}
+		default:


Would it be possible to return an explicit error on these default branches?

@tsg what do you want to happen if the default case is reached? we can abort with an error message (panic), or log an error and use the default unsorted behavior.

tsg · 2017-06-12T10:15:15Z

filebeat/prospector/log/prospector.go

+	paths := p.getFiles()
+	if strings.ToLower(p.config.ScanSort) != "none" {
+		sortInfos = getSortedFiles(strings.ToLower(p.config.ScanOrder),
+			strings.ToLower(p.config.ScanSort),


I'm not a fan of calling ToLower here, as this creates some leniency in the config parsing which we don't have in other places.

@waynemz Did you see this comment?

tsg · 2017-06-12T10:16:28Z

filebeat/prospector/log/prospector.go

+		var path string
+		var info os.FileInfo
+
+		if strings.ToLower(p.config.ScanSort) != "none" {


Removing ToLower here should also mean less work on the default case.

ruflin

LGTM. Left 2 minor comments. Also check comments from @tsg

ruflin · 2017-06-13T06:07:51Z

filebeat/docs/reference/configuration/filebeat-options.asciidoc

+
+Specifies if files should be harvested in order and how to determine the order. Possible values are modtime, filename and none. To sort by file modification time use modtime otherwise use filename.
+
+If you specify a value other than none for this setting you can determine whether to use ascending or descending order using `scan.order`


Could you add . dots at the end of the sentences? Also check below.

ruflin · 2017-06-13T06:10:59Z

CHANGELOG.asciidoc

@@ -132,6 +132,8 @@ https://github.com/elastic/beats/compare/v6.0.0-alpha1...v6.0.0-alpha2[View comm
 - Add `logging.files` `permissions` option. {pull}4295[4295]

 *Filebeat*
+- Added ability to sort harvested files. {pull}4374[4374]
+- Add experimental Redis slow log prospector type. {pull}4180[4180]


This line probably sneaked in during rebasing?

ruflin · 2017-06-13T06:11:47Z

@waynemz I really appreciate all the work you put into this. Thanks a lot for adding this feature.

codeperfector · 2017-06-14T01:05:33Z

Thanks @ruflin I appreciate the approval.
@tsg please take a look

codeperfector · 2017-06-18T21:09:39Z

Someone please take a look, there are no conflicts as of now.

ruflin

Sorry for the late review. I left a few additional comments. It's mainly about the error management we should improve.

ruflin · 2017-06-20T10:44:35Z

filebeat/prospector/log/prospector.go

+	return sortInfos
+}
+
+func getSortedFiles(scanOrder string, scanSort string, sortInfos []FileSortInfo) ([]FileSortInfo, string) {


The return argument should be error and not string. This is quite standard in Golang. Like this you can check

if err != nil { ... }

ruflin · 2017-06-20T10:44:43Z

filebeat/prospector/log/prospector.go

+}
+
+func getSortedFiles(scanOrder string, scanSort string, sortInfos []FileSortInfo) ([]FileSortInfo, string) {
+	var sortFunc func(i, j int) bool = nil


no need to set it to nil here

ruflin · 2017-06-20T10:45:15Z

filebeat/prospector/log/prospector.go

+				return sortInfos[i].info.ModTime().After(sortInfos[j].info.ModTime())
+			}
+		default:
+			return nil, "Unexpected value for scan.order: " + scanOrder


you can use fmt.Errorf("Unexpected value for scan.order: %v", scanOrder) here

ruflin · 2017-06-20T10:45:22Z

filebeat/prospector/log/prospector.go

+				return strings.Compare(sortInfos[i].info.Name(), sortInfos[j].info.Name()) > 0
+			}
+		default:
+			return nil, "Unexpected value for scan.order: " + scanOrder


see comment above

ruflin · 2017-06-20T10:45:28Z

filebeat/prospector/log/prospector.go

+			return nil, "Unexpected value for scan.order: " + scanOrder
+		}
+	default:
+		return nil, "Unexpected value for scan.sort: " + scanSort


see comment above

ruflin · 2017-06-20T10:45:39Z

filebeat/prospector/log/prospector.go

+		sort.Slice(sortInfos, sortFunc)
+	}
+
+	return sortInfos, ""


return sortInfos, nil

ruflin · 2017-06-20T10:46:25Z

filebeat/prospector/log/prospector.go

+	var absolutePath string
+	absolutePath, err = filepath.Abs(path)
+	if err != nil {
+		logp.Err("could not fetch abs path for file %s: %s", absolutePath, err)


Should we return here on error? Means this also can return an error?

ruflin · 2017-06-20T10:46:39Z

filebeat/prospector/log/prospector.go

+
+	paths := p.getFiles()
+
+	var err string = ""


var err error

ruflin · 2017-06-20T10:47:23Z

filebeat/prospector/log/prospector.go

+	if p.config.ScanSort != "none" {
+		sortInfos, err = getSortedFiles(strings.ToLower(p.config.ScanOrder),
+			strings.ToLower(p.config.ScanSort),
+			getSortInfos(paths))


We should handle the error directly inside the if clause. In case of error should we return? what are we doing?

In this case we are going to print an error saying why sorting failed and continue the regular unsorted route.

ruflin · 2017-06-20T10:48:03Z

filebeat/prospector/log/prospector.go

 // Scan starts a scanGlob for each provided path/glob
 func (p *Prospector) scan() {

-	for path, info := range p.getFiles() {
+	var sortInfos []FileSortInfo = nil


No need to set it to nil, also follow up below

codeperfector · 2017-06-21T02:15:35Z

@ruflin @tsg added error handling

codeperfector · 2017-06-22T21:25:37Z

@ruflin @tsg fixed it.

ruflin · 2017-06-26T10:44:52Z

@waynemz Thanks for your work on this. LGTM.

Two things that came to my mind which can be done a follow up PR:

We should use Validate on the config to validate only valid scan options are set
We should mark the feature in this Validate part experimental at first so we get some feedback early on and can still make heavy changes on it if needed.

@waynemz Do you want to add this to this PR or better as a follow up?

ruflin · 2017-06-27T06:20:35Z

@waynemz Merged. Thanks a lot for the work on this one. I will open a follow up PR with the above suggestions.

ruflin · 2017-06-27T06:36:33Z

@waynemz Here is the follow up PR. I made a few minor changes. It would be great to also add some system tests for this feature to confirm it works as expected: #4564

codeperfector · 2017-06-28T00:49:51Z

Thanks @ruflin

Follow up PR from elastic#4374.

Follow up PR from #4374.

scan older files to be harvested based on mod time

0664d1b

ruflin added Filebeat Filebeat review labels May 23, 2017

scan_order to allow sorting of files by mod time or file name

be36fdb

code review comments

ee7cfea

ruflin reviewed Jun 1, 2017

View reviewed changes

ruflin added the in progress Pull request is currently in progress. label Jun 1, 2017

added new properties for scan.sort, scan.order; code review suggestions

46e4ac2

codeperfector added 2 commits June 2, 2017 13:11

Merge branch 'master' into feature/harvester-scan-older

8bc7932

correct goimports style

e7e615b

codeperfector added 2 commits June 10, 2017 16:24

Merge branch 'master' into feature/harvester-scan-older

e6609dd

fix filebeat.full.yml

4c23c36

tsg reviewed Jun 12, 2017

View reviewed changes

ruflin approved these changes Jun 13, 2017

View reviewed changes

code review fixes

92ff5a0

ruflin reviewed Jun 20, 2017

View reviewed changes

error handling

fe3eda0

code review fix

23e16a1

ruflin merged commit b0edcbe into elastic:master Jun 27, 2017

ruflin mentioned this pull request Jun 27, 2017

Cleanup scan config options. Mark it experimental. #4564

Merged

ruflin added a commit to ruflin/beats that referenced this pull request Jun 28, 2017

Cleanup scan config options. Mark it experimental.

a8f1de4

Follow up PR from elastic#4374.

andrewkroh pushed a commit that referenced this pull request Jul 12, 2017

Cleanup scan config options. Mark it experimental. (#4564)

8a122a4

Follow up PR from #4374.

codeperfector added a commit to codeperfector/beats that referenced this pull request Sep 6, 2017

Scan older files to be harvested based on mod time (elastic#4374)

c56d3b4


		Specifies if files should be harvested in order and how to determine the order. Possible values are modtime, filename and none. To sort by file modification time use modtime otherwise use filename.

		If you specify a value other than none for this setting you can determine whether to use ascending or descending order using `scan.order`

scan older files to be harvested based on mod time #4374

scan older files to be harvested based on mod time #4374

Conversation

codeperfector commented May 22, 2017

elasticmachine commented May 22, 2017

elasticmachine commented May 22, 2017

karmi commented May 22, 2017

ruflin commented May 23, 2017

codeperfector commented May 24, 2017

ruflin commented May 26, 2017

codeperfector commented May 28, 2017

codeperfector commented May 29, 2017 • edited Loading

ruflin commented May 29, 2017

ruflin commented May 29, 2017

ruflin left a 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

codeperfector commented Jun 1, 2017

codeperfector commented Jun 6, 2017

tsg commented Jun 6, 2017

codeperfector commented Jun 10, 2017

tsg commented Jun 12, 2017

Choose a reason for hiding this comment

codeperfector Jun 14, 2017 • edited Loading

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

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Jun 13, 2017

codeperfector commented Jun 14, 2017

codeperfector commented Jun 18, 2017

ruflin left a 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

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

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

codeperfector commented Jun 21, 2017

codeperfector commented Jun 22, 2017

ruflin commented Jun 26, 2017

ruflin commented Jun 27, 2017

ruflin commented Jun 27, 2017

codeperfector commented Jun 28, 2017

codeperfector commented May 29, 2017 •

edited

Loading

codeperfector Jun 14, 2017 •

edited

Loading