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

Set system config variables explicitly #2674

Merged
merged 15 commits into from
Oct 30, 2019

Conversation

ganmacs
Copy link
Member

@ganmacs ganmacs commented Oct 29, 2019

Which issue(s) this PR fixes:
Ref #2624

What this PR does / why we need it:

Use @system_config explicitly so that supervisor doesn't need to expose all context to system_config. the current implementation is hard to understand since there are too many states.

Docs Changes:

no need

Release Note:

no need

Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Use @system_config via access the ivar not to expose ivar to supervisor

Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
supervise is used in lib/command/fluentd.rb

Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
…deprecated

Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
@ganmacs ganmacs force-pushed the set-system-config-variables-explicitly branch from de4e220 to 75a37eb Compare October 29, 2019 07:38
@ganmacs ganmacs self-assigned this Oct 29, 2019
s = Fluent::Supervisor.new(opts.merge(config_path: filepath))
s.configure
ensure
FileUtils.rm_r(CONFIG_DIR) rescue StandardError
Copy link
Member

Choose a reason for hiding this comment

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

what does 'rescue StandardError' mean?

Copy link
Member Author

@ganmacs ganmacs Oct 30, 2019

Choose a reason for hiding this comment

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

In order to resue Errno::ENOENT which happens when directory doesn't exist.

Copy link
Member

Choose a reason for hiding this comment

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

Your code means rescue returns StandardError, not catch StandardError.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh… I misunderstood something………… 29fd4d7

end

def change_privilege
ServerEngine::Privilege.change(@chuser, @chgroup)
end

def init_engine(supervisor: false)
def init_engine
Fluent::Engine.init(@system_config)
Copy link
Member

Choose a reason for hiding this comment

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

init_engine is now only one line. Can we replace init_engine with Fluent::Engine.init?

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 left it a method because others such as Supervisor#change_privilege, Supervisor#run_configure, Supervisor#run_engine are the same situation.
Do you think it's better to change these methods at this time? (I don't have a strong opinion about it. so if you have any opinion, I follow yours)

Copy link
Member

Choose a reason for hiding this comment

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

I see. So it should be in other patch for refactor methods.

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'll do it 👍

Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Copy link
Member

@repeatedly repeatedly left a comment

Choose a reason for hiding this comment

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

Please merge after test passed.

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.

2 participants