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

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

Merged
merged 7 commits into from
Mar 23, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/fluent/log.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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.

end

private

def dump_stacktrace(backtrace, level)
Expand Down
38 changes: 19 additions & 19 deletions lib/fluent/plugin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
require 'fluent/config/error'

module Fluent
class Plugin
module Plugin
SEARCH_PATHS = []

# plugins for fluentd: fluent/plugin/type_NAME.rb
Expand All @@ -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)
klass = if klass_or_its_name.is_a? Class
klass_or_its_name
elsif klass_or_its_name.is_a? String
eval(klass_or_its_name) # const_get can't handle qualified klass name (ex: A::B)
else
raise ArgumentError, "invalid argument type #{klass_or_its_name.class}: #{klass_or_its_name}"
end
REGISTRIES.reduce(nil){|a, r| a || r.reverse_lookup(klass) }
end

def self.add_plugin_dir(dir)
REGISTRIES.each do |r|
r.paths.push(dir)
end
nil
end

def self.register_input(type, klass)
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.

register_impl('input', INPUT_REGISTRY, type, klass)
end
Expand Down Expand Up @@ -89,6 +71,24 @@ def self.register_formatter(type, klass_or_proc)
end
end

def self.lookup_type_from_class(klass_or_its_name)
klass = if klass_or_its_name.is_a? Class
klass_or_its_name
elsif klass_or_its_name.is_a? String
eval(klass_or_its_name) # const_get can't handle qualified klass name (ex: A::B)
else
raise ArgumentError, "invalid argument type #{klass_or_its_name.class}: #{klass_or_its_name}"
end
REGISTRIES.reduce(nil){|a, r| a || r.reverse_lookup(klass) }
end

def self.add_plugin_dir(dir)
REGISTRIES.each do |r|
r.paths.push(dir)
end
nil
end

def self.new_input(type)
new_impl('input', INPUT_REGISTRY, type)
end
Expand Down
100 changes: 100 additions & 0 deletions lib/fluent/plugin/base.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
#
# Fluentd
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

require 'fluent/plugin'
require 'fluent/configurable'
require 'fluent/plugin_id'
require 'fluent/log'
require 'fluent/plugin_helper'

module Fluent
module Plugin
class Base
include Configurable
include PluginId
include PluginLoggerMixin
include PluginHelper::Mixin

State = Struct.new(:configure, :start, :stop, :shutdown, :close, :terminate)

def initialize
super
@state = State.new(false, false, false, false, false, false)
end

def has_router?
false
end

def configure(conf)
super
@state.configure = true
self
end

def start
@log.reset
@state.start = true
self
end

def stop
@state.stop = true
self
end

def shutdown
@state.shutdown = true
self
end

def close
@state.close = true
self
end

def terminate
@state.terminate = true
@log.reset
self
end

def configured?
@state.configure
end

def started?
@state.start
end

def stopped?
@state.stop
end

def shutdown?
@state.shutdown
end

def closed?
@state.close
end

def terminated?
@state.terminate
end
end
end
end
36 changes: 36 additions & 0 deletions lib/fluent/plugin_helper.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#
# Fluentd
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

require 'fluent/plugin_helper/event_emitter'
require 'fluent/plugin_helper/thread'
require 'fluent/plugin_helper/event_loop'
require 'fluent/plugin_helper/timer'
require 'fluent/plugin_helper/child_process'

module Fluent
module PluginHelper
module Mixin
Copy link
Member

Choose a reason for hiding this comment

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

If only one method, how about merging into Base?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean to add self.helpers method to Fluent::Plugin::Base?
I think here is better, because it keeps Base class a bit simpler than adding all to Base.
We should require all helpers from anywhere either way, and require the file which has many require for all helpers from Fluent::Plugin::Base. Just requiring fluent/plugin_helper is much simpler than requiring many things from Base.

def self.included(mod)
mod.extend(Fluent::PluginHelper)
end
end

def helpers(*snake_case_symbols)
Copy link
Member

Choose a reason for hiding this comment

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

If we support 3rd party plugin helper, require "fluent/plugin/#{helper_module}" and include it 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.

I have no plan to make it possible to add plugin helpers for 3rd party modules. Plugin helpers will provide many core features (to listen sockets shared between workers, to create event loops, ...) and will be connected strongly with test drivers. It is very hard to maintain compatibilities for 3rd party helpers.
Is it different from your opinion?

Copy link
Member

Choose a reason for hiding this comment

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

Currently we have XXXMixin to help plugin implementaion.
Maybe, existing PluginHelper:::XXX and XXXMixin is very confusing.
Migrating into PluginHelper is better for developers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Plugin helper doesn't provide methods to override plugin methods (like format or others), and also doesn't overwrite any behavior of plugins by just included.
On the other hand, mixins can do everything. In my opinion, these should be separated from each other.

(Along this opinion, EventEmitter looks not good as a plugin helper, but good as a mixin. Hmm.)

Copy link
Member

Choose a reason for hiding this comment

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

Plugin helper doesn't provide methods to override plugin methods (like format or others), and also doesn't overwrite any behavior of plugins by just included.

Does this depend on PluginHelper implementation or do you have any validation mechanizm?
Plugins use both with include XXX code.
Seperating responsibility is good but it introduces complicated mechanizm.
And Mixin is too ruby-ish name so users may assume PluginHelper is good for providing development support like TypeConverter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Migrating some mixins in fluentd core to Plugin Helpers are not bad idea if it can be recreated with limited APIs and no implicit overriding of methods. I agree about your opinion that Mixin is too ruby-ish and to separate responsibilities to each helpers (with changing APIs from mixin to helpers).

Validating mechanism doesn't exist, because Plugin Helpers are only in Fluentd core.

I have no plan to prohibit to create any mixins (for 3rd party authors), but moving toward the direction without any mixins looks good especially in Fluentd core.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's better to migrate mixins to plugin helpers, to change to use explicit call to wrap plugin behaviors:

  helpers :recordfilter
  def start
    super
    recordfilter_wrap(:filter_stream, :set_time_key)
  end

helper_modules = snake_case_symbols.map{|name| Fluent::PluginHelper.const_get(name.to_s.split('_').map(&:capitalize).join) }
include *helper_modules
end
end
end
Loading