Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

filebeat: use zglob for wildcards #3795

Closed
wants to merge 2 commits into from
Closed

filebeat: use zglob for wildcards #3795

wants to merge 2 commits into from

Conversation

7AC
Copy link
Contributor

@7AC 7AC commented Mar 22, 2017

No description provided.

@7AC 7AC added the review label Mar 22, 2017
@@ -32,7 +32,6 @@ import:
version: ca8abbf65d0e61dedf061f98bd3850f250e27539
- package: github.com/samuel/go-thrift
version: 2187045faa54fce7f5028706ffeb2f2fc342aa7e
# This is the same as labix.org/v2/mgo. See https://github.com/Masterminds/glide/issues/221#issuecomment-179817757
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to have gotten lost. Is there a way to keep it besides manually editing the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like there isn't: Masterminds/glide#691
I'll re-add it

@tsg
Copy link
Contributor

tsg commented Mar 22, 2017

What worries me about the zglob library is that it uses fewer tests than the one from the standard library. Compare zglob with stdlib. Is there any indication that the functionality is a super-set of what we had before?

@7AC
Copy link
Contributor Author

7AC commented Mar 22, 2017

Ouch 29/54 of those patterns fail with zlog, I need to investigate more.

Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file scanning is a central part of filebeat. Only had a quick look at zglob and it seems to do some additional magic. Do we need all of this? Would we use a simplified implementation owned by us which does only what we need?

Before we do a switch I would like to see the following:

  • Pros / Cons of the switch because for example Golang did decide not to add it to the library
  • Performance Comparison: Is this change having a potential performance impact? Some benchmark comparisons of the two would be interesting.
  • Does zglob fall back to glob for "normal" cases?

@@ -11,6 +11,7 @@ import (
"github.com/elastic/beats/filebeat/input/file"
"github.com/elastic/beats/libbeat/logp"
"github.com/elastic/beats/libbeat/monitoring"
"github.com/mattn/go-zglob"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We normally put a new line between external and internal imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@@ -115,3 +115,4 @@ import:
version: 5a004441f897722c627870a981d02b29924215fa
- package: github.com/mitchellh/hashstructure
version: b098c52ef6beab8cd82bc4a32422cf54b890e8fa
- package: github.com/mattn/go-zglob
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case we are going to use an external package, the exact commit id or version should be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep not sure how that was missed. I'm going to revisit this anyways since zlog is clearly not a drop in replacement

- /server/internal
- /server/v2
- /protocol/v2
- lj
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this syntax change in glide?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess so

@7AC 7AC added in progress Pull request is currently in progress. and removed review labels Mar 24, 2017
@7AC
Copy link
Contributor Author

7AC commented Apr 4, 2017

We're coming up with a new solution, closing this

@7AC 7AC closed this Apr 4, 2017
@7AC 7AC deleted the fix/filebeat_zglob branch April 5, 2017 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress Pull request is currently in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants