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

Improve Filebeat organisiation and Cleanup #1894

Merged
merged 1 commit into from
Jun 22, 2016

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented Jun 21, 2016

The overall goal is to decouple all the modules to make them better reusable and extendable.
In addition the current organisation was not very intuitive.

  • Move registrar to its own package
  • Improve startup handling of filebeat
  • Rename state to states in registrar as this is more accurate
  • Add warn message for truncated files. See https://github.com/elastic/beats/pull/1882/files#r67523587
  • Move ignore older to ProspectorLog as only used there
  • Remove Prospector reference from ProspectorStdin
  • Remove empty filebeat test file
  • Cleanup New function naming
  • Implement defer statements to stop running services
  • Clean up crawler / prospector stopping

@urso
Copy link

urso commented Jun 21, 2016

Registrar is now stopped before publisher is closed. Could this have any side affects?

Yes. Potential side effect is, publisher still returning ACKed lines, the registrar can not handle anymore. Effect is lines need to be send again one restart. In general I like to treat pipelines being setup, configured + stopped in a stack like manner (makes it easier to reason about potential resource usage between stages).

Didn't check yet, but when stopping the registrar, is some channel that has been used by publisher to forward results to registrar being closed? In this case we're asking for a panic on shutdown.

done chan struct{}
crawler *crawler.Crawler
pub publish.LogPublisher
done chan struct{}
Copy link

Choose a reason for hiding this comment

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

uh, I like how this struct has become smaller. Can we rename FbConfig to Config at some point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it also hurts me every time I see the variables ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to config. We have beatConfig in the generator. But I now even prefer config.

@urso
Copy link

urso commented Jun 21, 2016

In general I really like the changes. But start/stop handling in (*Filebeat).Run() needs some cleanup within this PR. I'm pretty much in favour of using defer X.Stop() right after starting some component.

@ruflin ruflin force-pushed the improve-namespacing branch from c9c1a52 to 1ecd4a4 Compare June 22, 2016 10:53
// Start publisher
publisher.Start()
// Stopping publisher (might potentially drop items)
defer publisher.Stop()
Copy link

Choose a reason for hiding this comment

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

with spooler being stopped before publisher, we really might drop items? Hmm... maybe if publish_async: true in config 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.

We now have the flexibility to make optimizations here if needed. Currently the logic should be identical to what we had before.

The overall goal is to decouple all the modules to make them better reusable and extendable.
In addition the current organisation was not very intuitive.

* Move registrar to its own package
* Improve startup handling of filebeat
* Rename state to states in registrar as this is more accurate
* Add warn message for truncated files. See https://github.com/elastic/beats/pull/1882/files#r67523587
* Move ignore older to ProspectorLog as only used there
* Remove Prospector reference from ProspectorStdin
* Remove empty filebeat test file
* Cleanup New function naming
* Implement defer statements to stop running services
* Clean up crawler / prospector stopping
@ruflin ruflin force-pushed the improve-namespacing branch from 1ecd4a4 to 6079993 Compare June 22, 2016 12:09
@urso
Copy link

urso commented Jun 22, 2016

LGTM. Let's wait for travis/appveyor

@urso urso merged commit a15d0ef into elastic:master Jun 22, 2016
@ruflin ruflin deleted the improve-namespacing branch June 22, 2016 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants