-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix log messages and its conditions #1227
Conversation
Tests are not fixed with modified log messages. |
Yes because I want feedback for more better message. |
@@ -56,7 +56,7 @@ def configure(conf) | |||
elsif @path | |||
# ok | |||
else # @_plugin_id_configured is true | |||
raise NotImplementedError, "implement this feature later with system_config" | |||
raise NotImplementedError, "implement this feature later with system_config. Add <storage> section for file store or remove @id for memory store temporary" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block is for "Make root path for buffers and storages" (root path is based on @id
) described in wiki. So it's wrong to tell users to remove @id
.
https://github.com/fluent/fluentd/wiki/Changes-proposed-for-v0.14.x#multi-processes--cpu-cores
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I know. The problem is users now hard to use without file because storage plugin doesn't have store type option.
Currently, existing users hit this error because they use following configuration for v0.12.
<source>
@type dummy
@id input_dummy
# ...
</source>
I think this configuration should work with v0.14 without error. My change is temporary. Better way is fallback to on memory store, not raising NotImplementedError
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 2 ways for in_dummy
.
- Add store_type like option to specify file or on-memory and log message in this line
- Fallback to on-memory storage, not raising
NotImplementedError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, falling back to on-memory storage is better than adding configuration parameters for temporal issue.
"worse performance" is bad for user impression. Several users misread "Why this plugin makes fluentd performance slower?" Perfomance is same as before without optimization, so log message should be changed.
0515d0a
to
efb879f
Compare
Update test for EventRouter. |
Test passed excluding unstable test. Re-run test. |
Test are passed. |
If no concern, I will merge it. |
No description provided.