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

Override default variables with SystemConfig #854

Merged

Conversation

cosmo0920
Copy link
Contributor

I've implemented to override default variables with system config mechanism.

This PR is related to this developer task: make process global constants obsoleve in fluent/env

ref: https://github.com/fluent/fluentd/wiki/Developer-notes-for-fixes-on-v0.14#plugin-miscellaneous

Remaining Task(s)

  • Write test code to confirm override feature
    • test code for overriding port
    • test code for overriding file permission
    • test code for overriding dir permission
      • out_file
      • buf_file
    • Omit permission related tests when running on Windows
    • remove DEFAULT_* from env.rb & add default values of ports or permissions to each plugins
    • remove xxx_override methods and use perm = @perm_param_of_plugin || system_config.file_permissions || PLUGIN_DEFAULT_PERM in plugin
    • squash puzzling commits
    • Tidying up tests
    • Remove port number overriding mechanism. Because it is useless.

Question(s)

  • Where is suitable place to put into obsoleted variables? Or, leave them as-is is enough? Solved.

@cosmo0920 cosmo0920 force-pushed the override-system-env-vars-system-config branch 2 times, most recently from a13cb72 to e8a31e3 Compare March 18, 2016 09:10
@cosmo0920 cosmo0920 changed the title [WIP] Override default variables with SystemConfig Override default variables with SystemConfig Mar 23, 2016
@cosmo0920
Copy link
Contributor Author

Oh, NTFS does not support UNIX like permissions!
It should do more work for Windows.... :<

@cosmo0920 cosmo0920 force-pushed the override-system-env-vars-system-config branch from 5dff253 to e7d5fa0 Compare March 23, 2016 04:46
@@ -31,6 +31,33 @@ def system_config_override(opts={})
@_system_config.send(:"#{key.to_s}=", value)
end
end

def listen_port_override(port=DEFAULT_LISTEN_PORT)
Copy link
Member

Choose a reason for hiding this comment

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

system_config_override is to override instance variables for each plugin instances. Once it was overridden, following system_config method refers instance variable, not global system config instance.
(In other words, plugin just refer system_config even in tests after system_config_override is called in setup or somewhere else.)

So, there's no need to add xxx_override methods.

@tagomoris
Copy link
Member

Default values of port, permission and others are for each plugins, not for Fluentd global.
The best way to fix it is:

  • remove DEFAULT_* from env.rb
  • add default values of ports or permissions to each plugins

Especially for file/directory permissions, it's better to provide system config parameters because there's not so variations about it (in many cases, permission control policy is system-wide).

  perm = @perm_param_of_plugin || system_config.file_permissions || PLUGIN_DEFAULT_PERM

@cosmo0920 cosmo0920 force-pushed the override-system-env-vars-system-config branch from e7d5fa0 to 524c44f Compare March 24, 2016 03:45
@tagomoris tagomoris added the pending To be done in the future label Mar 24, 2016
@cosmo0920 cosmo0920 force-pushed the override-system-env-vars-system-config branch from 524c44f to 654e4b3 Compare March 24, 2016 05:02
@tagomoris tagomoris removed the pending To be done in the future label Mar 24, 2016
@@ -46,6 +46,13 @@ class SystemConfig
config_param :rpc_endpoint, :string, default: nil
config_param :enable_get_dump, :bool, default: nil
config_param :process_name, default: nil
config_param :listen_port, :integer, default: nil
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove this line?
There's no need to configure system-wide listen_port because this configuration cannot be shared by anything.
Removing this line also makes port_override methods useless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I'll do it.

@cosmo0920 cosmo0920 force-pushed the override-system-env-vars-system-config branch from 2f72613 to 3766e13 Compare March 28, 2016 07:53
@repeatedly
Copy link
Member

@tagomoris How about this? Can we merge this?

@tagomoris
Copy link
Member

LGTM. I'll merge this right now.

@tagomoris tagomoris merged commit 3be4232 into fluent:master Mar 29, 2016
@cosmo0920
Copy link
Contributor Author

Thanks!

@cosmo0920 cosmo0920 deleted the override-system-env-vars-system-config branch March 29, 2016 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants