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.

Sign up for GitHub

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

Jump to bottom

Prospector level processors #3853

Merged
merged 3 commits into from
Apr 5, 2017
Merged

Prospector level processors #3853

merged 3 commits into from
Apr 5, 2017

Conversation

vjsamuel
Copy link
Contributor

Adding support to have prospector level filters. Kindly review the base code. If all is well, I can go ahead and add the system test cases and change logs.

@elasticmachine
Copy link
Collaborator

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.

1 similar comment
@elasticmachine
Copy link
Collaborator

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.

@ruflin ruflin added in progress Pull request is currently in progress. and removed review labels Mar 30, 2017
@ruflin
Copy link
Member

ruflin commented Mar 30, 2017

LGTM. Ok for me to add the system tests. @urso ok?

@vjsamuel
Copy link
Contributor Author

vjsamuel commented Apr 4, 2017

@ruflin I have added the system tests. I'll let the CI to complete. Local tests indicate that thing seem fine.

[success] 0.21% test_prospector.Test.test_prospector_filter_includefields: 0.2457s
[success] 0.21% test_prospector.Test.test_prospector_filter_dropfields: 0.2479s

Ran 116 tests in 118.419s

OK (SKIP=4)

@urso
Copy link

urso commented Apr 4, 2017

I'm a little suspicious about processor dropping complete events? e.g. What happens to the registry if all events are dropped? I think we need some more tests here.

@ruflin
Copy link
Member

ruflin commented Apr 4, 2017

Agree with steffen, we should be exta careful here not to break things. Lets have a system test for each one. We can inspect the registry files for that in the tests.

@vjsamuel vjsamuel force-pushed the prospector_level_processors branch from 789ae38 to 8ba51e8 Compare April 5, 2017 04:16
@vjsamuel vjsamuel force-pushed the prospector_level_processors branch from 8ba51e8 to 1bd5729 Compare April 5, 2017 04:46
ruflin
ruflin approved these changes Apr 5, 2017
View reviewed changes
filebeat/tests/system/test_registrar.py
# Windows checks
# TODO: Check for IdxHi, IdxLo, Vol in FileStateOS on Windows.
self.assertEqual(len(file_state_os), 3)
elif platform.system() == "SunOS":
Copy link
Member

Choose a reason for hiding this comment

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

Did you run the tests on such a platform? I would assume this could also break some other tests ...

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 copy pasted this from another test case as i assumed that it was there for a reason.

@ruflin ruflin merged commit e73bb6b into elastic:master Apr 5, 2017
@vjsamuel vjsamuel deleted the prospector_level_processors branch April 5, 2017 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@urso urso urso approved these changes

@ruflin ruflin ruflin approved these changes

Assignees
No one assigned
Labels
Filebeat Filebeat in progress Pull request is currently in progress. v6.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vjsamuel @elasticmachine @ruflin @urso @monicasarbu