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

Create new out file plugins only for secondary #1154

Merged
merged 26 commits into from
Sep 1, 2016
Merged

Create new out file plugins only for secondary #1154

merged 26 commits into from
Sep 1, 2016

Conversation

ganmacs
Copy link
Member

@ganmacs ganmacs commented Aug 10, 2016

Consider points

  • Plugin's name
    This plugins's name is out_secondary_file now.
    It may be too verbose.
  • Supporting params
    I think supporting compress and append params.

Not for this PR tasks

  • Create a utility command that read dumped data

Close #873

@tagomoris tagomoris added feature request *Deprecated Label* Use enhancement label in general v0.14 labels Aug 10, 2016
@ganmacs ganmacs force-pushed the create-new-out_file-plugins-only-for-secondary branch 3 times, most recently from 769ae75 to b169908 Compare August 12, 2016 13:50
@ganmacs ganmacs force-pushed the create-new-out_file-plugins-only-for-secondary branch 9 times, most recently from 7dd7218 to 486c0d8 Compare August 15, 2016 11:37
@ganmacs ganmacs changed the title [WIP] Create new out file plugins only for secondary Create new out file plugins only for secondary Aug 15, 2016
@ganmacs ganmacs force-pushed the create-new-out_file-plugins-only-for-secondary branch from 486c0d8 to a6a1d11 Compare August 17, 2016 01:14
tagomoris
tagomoris reviewed Aug 18, 2016
View reviewed changes
lib/fluent/plugin/buffer.rb
@@ -55,7 +55,11 @@ class BufferChunkOverflowError < BufferError; end # A record size is larger than
# if chunk size (or records) is 95% or more after #write, then that chunk will be enqueued
config_param :chunk_full_threshold, :float, default: DEFAULT_CHUNK_FULL_THRESHOLD

Metadata = Struct.new(:timekey, :tag, :variables)
Metadata = Struct.new(:timekey, :tag, :variables) do
def nil?
Copy link
Member

Choose a reason for hiding this comment

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

Ruby's document says that "Only the object nil responds true to nil?".
This method breaks it. Change this method name to something else, like empty?.

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 changed this method name empty?
https://github.com/fluent/fluentd/pull/1154/files#diff-7d794f77abc9222413b9120533cff327R59

tagomoris
tagomoris reviewed Aug 26, 2016
View reviewed changes
test/plugin/test_out_secondary_file.rb
assert_equal 'out_file_test', d.instance.basename
assert_equal TMP_DIR, d.instance.directory
assert_equal :gzip, d.instance.compress
end
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

I added review comments.

@ganmacs ganmacs force-pushed the create-new-out_file-plugins-only-for-secondary branch from 8c7d4c7 to dcb294a Compare August 26, 2016 07:13
@ganmacs
Update out_secondary_file
df0b7a9 
* Add `directory` and `basename` config_pa rams(Remove `path` config_params)
* Do not support a path that includes "*"
* Add placeholder regex test
* Do not use chunk_id as path name(pass default name)
* Remove `gz` of compress type
* Fix some typos
@ganmacs ganmacs force-pushed the create-new-out_file-plugins-only-for-secondary branch from dcb294a to a1f5a10 Compare August 26, 2016 07:45
@ganmacs ganmacs force-pushed the create-new-out_file-plugins-only-for-secondary branch from a1f5a10 to cbc1f1f Compare August 29, 2016 01:09
@ganmacs
Copy link
Member Author

ganmacs commented Aug 29, 2016

@tagomoris
I updated.

tagomoris
tagomoris reviewed Aug 29, 2016
View reviewed changes
lib/fluent/plugin/out_secondary_file.rb
".gz"
end

test_path = generate_path(File.join(@directory, Time.now.strftime("%Y%m%d")))
Copy link
Member

Choose a reason for hiding this comment

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

test_path loses @basename.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. But test_path is used for a test that directory is writable or not.
I think it is reasonable that test_path loses @basename.

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 positive reason not to use @basename for this test?
@basename might contain /... (users do everything possible).

Copy link
Member Author

Choose a reason for hiding this comment

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

@basename might contain /... (users do everything possible).

I didn't care about it.
I added a test for it.
And I use basename as test_path

eec9803

@tagomoris
Copy link
Member

I found that Fluent::FileUtil.writable_p returns false if the directory of specified path doesn't exist.
It means configuration like directory /path/to/${tag}/%Y%m%d always raises configuration errors.
Hmm.

@ganmacs
Copy link
Member Author

ganmacs commented Aug 29, 2016

@tagomoris

I wrote a test that directory has placeholders ( https://github.com/fluent/fluentd/pull/1154/files#diff-39910dc690f56dd659cdccdca103e55aR417 ) and this test passed.

If configuration is like directory /path/to/${tag}/%Y%m%d, Fluent::FileUtil.writable_p check direcotry /path/to/ is writable or not (https://github.com/fluent/fluentd/blob/master/lib/fluent/compat/file_util.rb#L39).

I think it is good enough to check.

@tagomoris
Copy link
Member

@ganmacs Ah, I misread the code... you're right.
LGTM.

@tagomoris tagomoris merged commit 0dd02c8 into fluent:master Sep 1, 2016
@tagomoris
Copy link
Member

Merged! It'll be released as v0.14.5. Thank you!

@ganmacs ganmacs deleted the create-new-out_file-plugins-only-for-secondary branch November 28, 2019 00:06
daipom added a commit to daipom/fluentd-docs-gitbook that referenced this pull request Mar 6, 2023
@daipom
out_secondary_file: add missing option explanation
8d9cfbd 
This explanation is missing since the beginning.

* https://github.com/fluent/fluentd/blob/master/CHANGELOG.md#release-v0145---20160906
* fluent/fluentd#1154

Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>
@daipom daipom mentioned this pull request Mar 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
feature request *Deprecated Label* Use enhancement label in general v0.14
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ganmacs @tagomoris @repeatedly