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

add new Plugin::Base class and PluginHelper modules #843

Merged
merged 7 commits into from
Mar 23, 2016
Merged

add new Plugin::Base class and PluginHelper modules #843

merged 7 commits into from
Mar 23, 2016

Conversation

tagomoris
Copy link
Member

PluginHelper provides utilities to write plugins:

  • high level methods to hide underlying features and options (e.g., threads, cool.io, ...)
  • pre-configured suite nice to use in plugin code (e.g., threads marked as abort_on_exception in default)
  • well organized test driver integration (in future): test code runs after threads start to run correctly (no more sleeps in test code!)

And change Fluent::Plugin from class to module (because it can't be instantiated) and add Fluent::Plugin::Base class as a base class of all plugins, to define methods for plugin life cycles which are used from plugin helpers.

@tagomoris tagomoris added enhancement Feature request or improve operations v0.14 labels Mar 10, 2016
@tagomoris
Copy link
Member Author

First bunch is for:

  • EventEmitter: handles @router (will be loaded by input plugins in default)
    • to separate output plugin from input plugin
    • to show which output plugins may emit records explicitly
  • Thread: create/manage threads
  • EventLoop: create/manage event loops of cool.io (event loop per plugin instance)
  • Timer: no more threads/sleeps for timers in plugin code
  • ChildProcess: handle process execution and terminating

@tagomoris tagomoris force-pushed the introduce-plugin-helpers branch 3 times, most recently from b4c74e7 to 54fe3aa Compare March 10, 2016 23:46
@tagomoris tagomoris force-pushed the introduce-plugin-helpers branch 15 times, most recently from f3661ba to d894f78 Compare March 13, 2016 05:47
@tagomoris tagomoris changed the title [WIP] adding PluginHelper modules add new Plugin::Base class and PluginHelper modules Mar 13, 2016
tagomoris
tagomoris reviewed Mar 13, 2016
View reviewed changes
lib/fluent/plugin.rb
@@ -35,24 +35,6 @@ class Plugin

REGISTRIES = [INPUT_REGISTRY, OUTPUT_REGISTRY, FILTER_REGISTRY, BUFFER_REGISTRY, PARSER_REGISTRY, FORMATTER_REGISTRY]

def self.lookup_type_from_class(klass_or_its_name)
Copy link
Member Author

Choose a reason for hiding this comment

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

Methods for internal use should be under public use.

@tagomoris
Copy link
Member Author

@repeatedly please take a glance before merge.

repeatedly
repeatedly reviewed Mar 14, 2016
View reviewed changes
lib/fluent/log.rb
@@ -245,6 +245,10 @@ def flush
@out.flush
end

def reset
@out.reset if @out.respond_to?(:reset)
Copy link
Member

Choose a reason for hiding this comment

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

IO object doesn't have reset method.
What the object do you assume?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, Is this for test purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. This is to initialize lines in logger between tests.

repeatedly
repeatedly reviewed Mar 14, 2016
View reviewed changes
lib/fluent/plugin_helper/thread.rb
yield *t_args
thread_exit = true
rescue => e
log.warn "thread exited by unexpected error", plugin: self.class, title: title, error_class: e.class, error: e
Copy link
Member

Choose a reason for hiding this comment

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

should set true to thread_exit?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@tagomoris tagomoris force-pushed the introduce-plugin-helpers branch 2 times, most recently from c8023cd to 89274c9 Compare March 14, 2016 23:34
@tagomoris
Copy link
Member Author

I pushed fixed for comments.

repeatedly
repeatedly reviewed Mar 15, 2016
View reviewed changes
lib/fluent/plugin_helper/event_loop.rb
@_event_loop = nil
@_event_loop_running = false
@_event_loop_mutex = nil
@_event_loop_run_timeout = EVENT_LOOP_RUN_DEFAULT_TIMEOUT
Copy link
Member

Choose a reason for hiding this comment

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

Resetting this value is needed?
If needed, nil is better?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. nil is better.
In basic design of new plugin API, plugins should be re-initialized after terminate. It's good for many things including tests.

@tagomoris tagomoris force-pushed the introduce-plugin-helpers branch from 89274c9 to 6271788 Compare March 15, 2016 22:07
@tagomoris
Copy link
Member Author

I pushed more fixes again.

@tagomoris tagomoris force-pushed the introduce-plugin-helpers branch from 9cad06f to 942b04b Compare March 23, 2016 09:29
@tagomoris
Copy link
Member Author

Now it's green on CIs.

@tagomoris tagomoris merged commit b19678e into master Mar 23, 2016
@tagomoris
Copy link
Member Author

Merged!

@tagomoris tagomoris deleted the introduce-plugin-helpers branch May 17, 2016 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement Feature request or improve operations v0.14
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tagomoris @repeatedly