-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Multi worker processing #1386
Multi worker processing #1386
Conversation
I'm getting the workers works well:
```
$ bundle exec bin/fluentd -c example/in_forward_workers.conf
2016-12-22 20:02:15 +0900 [info]: reading config file path="example/in_forward_workers.conf"
2016-12-22 20:02:15 +0900 [info]: starting fluentd-0.14.10 pid=21028
2016-12-22 20:02:15 +0900 [info]: spawn command to main: cmdline=["/Users/tagomoris/.rbenv/versions/2.3.1/bin/ruby", "-Eascii-8bit:ascii-8bit", "-rbundler/setup", "bin/fluentd", "-c", "example/in_forward_workers.conf", "--under-supervisor"]
2016-12-22 20:02:16 +0900 [info]: reading config file path="example/in_forward_workers.conf"
2016-12-22 20:02:16 +0900 [info]: starting fluentd-0.14.10 without supervision pid=21059
2016-12-22 20:02:16 +0900 [info]: gem 'fluentd' version '0.14.10'
2016-12-22 20:02:16 +0900 [info]: adding match pattern="test" type="stdout"
2016-12-22 20:02:16 +0900 [info]: reading config file path="example/in_forward_workers.conf"
2016-12-22 20:02:16 +0900 [info]: starting fluentd-0.14.10 without supervision pid=21058
2016-12-22 20:02:16 +0900 [info]: gem 'fluentd' version '0.14.10'
2016-12-22 20:02:16 +0900 [info]: adding match pattern="test" type="stdout"
2016-12-22 20:02:16 +0900 [info]: adding source type="forward"
2016-12-22 20:02:16 +0900 [info]: using configuration file:
workers 3
|
de1001a
to
2b37783
Compare
Current status:
|
Before, we considered following syntax:
Is this dropped? |
@repeatedly Yes, in my current opinion. It's not "symmetric". |
I just found an idea to enable a configuration section only in specified worker process:
It's much easy to implement, and also easy to understand which worker runs the configuration specified by |
It is acceptable configuration for me. |
OK, i'll create an another issue for that feature request. It's too large to implement in this feature branch, and acceptable to implement in future versions, I think. |
e765bfe
to
08db55d
Compare
@repeatedly could you review this feature? |
08db55d
to
5314b7a
Compare
@repeatedly ping |
worker_id_part = if type == :default && (@process_type == :worker0 || @process_type == :workers) | ||
@worker_id_part | ||
else | ||
"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.freeze
end | ||
|
||
if plugin_id_configured? | ||
@log.optional_header = "[#{@id}] " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need #{self.class.name}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Including plugin class name looks too verbose for me.
It's not useful for end users, and it's clear when -v
specified via filename.
@@ -67,33 +73,42 @@ def configure(conf) | |||
|
|||
@@buffer_paths[@path] = type_of_owner | |||
|
|||
if File.exist?(@path) | |||
if File.directory?(@path) | |||
if File.exist?(@path) && File.directory?(@path) || !File.exist?(@path) && !@path.include?('.*') # directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need both File.exist?(@path)
and !File.exist?(@path)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because this clause must NOT match the condition of File.exist?(@path) && !File.directory?(@path)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, please use ()
to separate conditions.
&& || && combination in one line is hard to read and maintain.
end | ||
|
||
def multi_workers_ready? | ||
### TODO: add hack to synchronize for multi workers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need more tasks to improve multi workers support in out_file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it's just mistake not to remove this comment.
show_plugin_config if @show_plugin_config | ||
read_config | ||
set_system_config | ||
|
||
if @workers < 1 | ||
raise Fluent::ConfigError, "invalid number of workers:#{@workers}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalid number of workers. Must be "> 0"
is more clear for users.
else | ||
"" | ||
end | ||
log_msg = "#{time.strftime(@time_format)}[#{LEVEL_TEXT[level]}]: #{worker_id_part}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about inserting worker_id_part
before :
?
It seems clear that this is system header, not message body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing there may breaks user-side integrations to match log level parts.
OT: We should write |
I've added some commits for review comments. |
Is it manual operation, right? |
Exactly. |
9d0367e
to
2cf7086
Compare
Updated/added some commits based on current master HEAD. |
I'm pushing some other pull-requests for loggers and |
LGTM. |
…hown or not. In multi workers status, the same log messages will be shown 2 or more times without any controls. With this patch, loggger will show logs about process-wide control just once.
…lugins on all workers
6e09a17
to
ec6a29a
Compare
This feature request is to implement "symmetric multi worker processing", which runs specified number of Fluentd worker processes, to use 2 or more CPU cores by just one configuration file.
All plugins used in this configuration MUST support multi worker feature, and MUST show it by
#multi_worker_ready?
method (returns true or false after#configure
).In default, all input and output plugins return
false
, and all other plugins returntrue
. 3rd party plugins SHOULD claim whether it supports multi worker processing or not.Points:
mv worker3/* worker2
)