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

Root directory per worker process #1374

Merged
merged 15 commits into from
Dec 20, 2016
Merged

Conversation

tagomoris
Copy link
Member

@tagomoris tagomoris commented Dec 15, 2016

This change introduces system-wide "root directory" and root directory per plugins.
It's enabled when:

  • root_dir specified in <system> section
  • @id configured in each plugin configuration sections

The root directories per plugin instances must be unique in a filesystem (between many workers of fluentd). So it uses worker_id provided by supervisor process via environment variable.
The path of directories are: /system-wide-root/workerN/plugin-id (generated and provided via Fluent::Plugin::Base#plugin_root_dir). Plugins will creates files or directories under that.

This change also adds 2 implementations to use this new feature:

  • buf_file: configure buffer file path automatically
  • storage_local: configure storage file path automatically

I have an additional plan to fix in_tail to use storage plugin as replacement of pos_file, but it's will be added by another pull-request.

@tagomoris tagomoris added feature request *Deprecated Label* Use enhancement label in general v0.14 labels Dec 15, 2016
@tagomoris tagomoris self-assigned this Dec 15, 2016
@tagomoris tagomoris changed the title [WIP] Root directory per worker process Root directory per worker process Dec 15, 2016
@tagomoris tagomoris force-pushed the root-directory-per-worker-process branch from ad8fda7 to 176baaa Compare December 16, 2016 08:08
@tagomoris tagomoris force-pushed the root-directory-per-worker-process branch from 176baaa to d52686b Compare December 19, 2016 11:23
@tagomoris
Copy link
Member Author

@repeatedly could you review this change?

@@ -56,6 +51,14 @@ def initialize
def configure(conf)
super

unless @path
Copy link
Member

Choose a reason for hiding this comment

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

In the future, should we validate @path value for avoding conflict trouble?
Fixed path should not allowed with multiprocess configuration or
support %{worker_id}/%{plugin_id} placeholder?

Copy link
Member Author

Choose a reason for hiding this comment

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

Buffer chunks have unique_id, which are generated with time and random value.
So it SHOULD NOT conflict with other chunks in other processes. No need to do such thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahhh, there are some problem about resuming at startup...

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyway, plugin_id can't help that situation. plugin_id is (should be) unique in a worker process, not server global.


def plugin_root_dir
return @_plugin_root_dir if @_plugin_root_dir
return nil unless system_config.root_dir
Copy link
Member

Choose a reason for hiding this comment

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

Put log.warn is better?
Maybe, users hard to find the cause why buffer_path/storage is not configured automatically.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's not about users.
This code is called by plugins, and plugin should check whether plugin_root_dir is available or not by return value, and raise errors with messages which shows how to configure that plugin.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason of this implementation is, plugin MAY use any default values (paths or any others) if root directory is unavailable.

def plugin_root_dir
return @_plugin_root_dir if @_plugin_root_dir
return nil unless system_config.root_dir
return nil unless plugin_id_configured?
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@tagomoris
Copy link
Member Author

Can I merge this?

@repeatedly
Copy link
Member

LGTM

@tagomoris tagomoris merged commit 934ddf9 into master Dec 20, 2016
@tagomoris tagomoris deleted the root-directory-per-worker-process branch December 20, 2016 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request *Deprecated Label* Use enhancement label in general v0.14
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants