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.

Sign up for GitHub

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

Jump to bottom

Enable log rotate for log #1235

Merged
merged 9 commits into from
Sep 26, 2016
Merged

Enable log rotate for log #1235

merged 9 commits into from
Sep 26, 2016

Conversation

ganmacs
Copy link
Member

@ganmacs ganmacs commented Sep 15, 2016

This PR closes #1231.

@ganmacs ganmacs force-pushed the enable-log-rotate-for-log branch 3 times, most recently from 1542544 to 20978f2 Compare September 16, 2016 02:07
@ganmacs ganmacs changed the title [WIP]Enable log rotate for log Enable log rotate for log Sep 16, 2016
@ganmacs ganmacs force-pushed the enable-log-rotate-for-log branch 5 times, most recently from 4ac2847 to 723e0a5 Compare September 20, 2016 01:22
tagomoris
tagomoris reviewed Sep 20, 2016
View reviewed changes
lib/fluent/supervisor.rb
$log = Fluent::Log.new(logger, @opts)
$log.enable_color(false) if @path
$log.enable_debug if @level <= Fluent::Log::LEVEL_DEBUG
end

def stdout?
@io == STDOUT
$log.out == STDOUT
Copy link
Member

Choose a reason for hiding this comment

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

$log might be replaced with other values by code outside.

Copy link
Member

Choose a reason for hiding this comment

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

Using @io should be kept to use here and #reopen!.

Copy link
Member

Choose a reason for hiding this comment

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

Aren't there fixes for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry. I forgot it.
I updated @io to @logdev. Because the name of @io instance variable is changed here (https://github.com/fluent/fluentd/pull/1235/files#diff-dbe0e1ec4079138e48ca6a4d7c7248f9R316).

lib/fluent/supervisor.rb
end

def reopen!
if @path && @path != "-"
@io.reopen(@path, "a")
$log.out.reopen(@path, "a")
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
tagomoris reviewed Sep 20, 2016
View reviewed changes
Copy link
Member

@tagomoris tagomoris left a comment

Choose a reason for hiding this comment

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

Added some review comments.
And, please add a test to confirm that logger actually generates numbers of files specified by options (and older files will be deleted).

lib/fluent/supervisor.rb
@@ -295,42 +299,48 @@ def self.load_config(path, params = {})
end

class LoggerInitializer
def initialize(path, level, chuser, chgroup, opts)
def initialize(path, level, chuser, chgroup, log_rotate_age, log_rotate_size, opts)
Copy link
Member

Choose a reason for hiding this comment

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

It's better to use keyword arguments to add options..

lib/fluent/command/fluentd.rb
opts[:log_rotate_age] = age
else
begin
opts[:log_rotate_age] = Integer(age)
Copy link
Member

Choose a reason for hiding this comment

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

Are there any reason to use Integer() instead of to_i?
IMO to_i is widely used and normal for most ruby programmer.

Copy link
Member Author

@ganmacs ganmacs Sep 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. I have a reason to use Integer() here.
I want to raise an error when it was passed a value which can't be changed to an integer.

p 'a'.to_i                      # => 0
p Integer('a')                  # raise TypeError

lib/fluent/command/fluentd.rb
@@ -81,6 +81,23 @@
opts[:log_path] = s
}

op.on('--log-rotate-age AGE', 'generations to keep rotated log files') {|age|
rotate_ages = %w(daily weekly monthly)
Copy link
Member

Choose a reason for hiding this comment

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

Use constant.

lib/fluent/command/fluentd.rb
else
begin
opts[:log_rotate_age] = Integer(age)
rescue
Copy link
Member

Choose a reason for hiding this comment

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

Use rescue with error class name (TypeError?).

test/test_log.rb
rotate_size: [1, 100, 0, '0'],
)
def test_log_with_logdevio(expected)
old = ENV['TZ']
Copy link
Member

Choose a reason for hiding this comment

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

Use with_timezone test helper method defined in lib/fluent/test/helpers.rb instead of doing by itself.

ganmacs
ganmacs commented Sep 23, 2016
View reviewed changes
Copy link
Member Author

@ganmacs ganmacs left a comment

Choose a reason for hiding this comment

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

I updated a code and reply a comment.
Plz re-review when you have time.

lib/fluent/command/fluentd.rb
opts[:log_rotate_age] = age
else
begin
opts[:log_rotate_age] = Integer(age)
Copy link
Member Author

@ganmacs ganmacs Sep 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. I have a reason to use Integer() here.
I want to raise an error when it was passed a value which can't be changed to an integer.

p 'a'.to_i                      # => 0
p Integer('a')                  # raise TypeError

tagomoris
tagomoris reviewed Sep 23, 2016
View reviewed changes
lib/fluent/command/fluentd.rb
end
}

op.on('--log-rotate-size SIZE', 'sets the size to rotate log files') {|s|
Copy link
Member

Choose a reason for hiding this comment

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

BYTES is better to show the value is an integer, not "size" (in fluentd's configuration file, e.g., 256k, 8m).

lib/fluent/supervisor.rb
$log = Fluent::Log.new(logger, @opts)
$log.enable_color(false) if @path
$log.enable_debug if @level <= Fluent::Log::LEVEL_DEBUG
end

def stdout?
@io == STDOUT
$log.out == STDOUT
Copy link
Member

Choose a reason for hiding this comment

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

Aren't there fixes for this?

test/test_log.rb
end
end

def test_log_rotates_specifed_size_with_logdevio
Copy link
Member

Choose a reason for hiding this comment

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

This test should check '.1' file is NOT created, as a test for rotate_age.

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 added. 7bf4ab4

@ganmacs ganmacs force-pushed the enable-log-rotate-for-log branch from 282d19d to 48f3cc0 Compare September 23, 2016 05:30
@ganmacs ganmacs force-pushed the enable-log-rotate-for-log branch 2 times, most recently from 84572a0 to 33ca208 Compare September 26, 2016 02:25
@ganmacs ganmacs force-pushed the enable-log-rotate-for-log branch from 33ca208 to b5513d1 Compare September 26, 2016 03:01
@tagomoris
Copy link
Member

Okay, LGTM. Thank you!

@tagomoris tagomoris merged commit 08c74bc into fluent:master Sep 26, 2016
@ganmacs ganmacs deleted the enable-log-rotate-for-log branch May 30, 2019 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tagomoris tagomoris tagomoris left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable log rotation for Fluent::Log
2 participants
@ganmacs @tagomoris