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

Add warning on multi workers #392

Closed
wants to merge 4 commits into from

Conversation

daipom
Copy link
Contributor

@daipom daipom commented Mar 9, 2022

This is a fix for #391.

@daipom
Copy link
Contributor Author

daipom commented Mar 9, 2022

This just fixes a warning log.
I don't have the environment, so I can't check the behavior.
However, I wrote test codes and checked all success of configure subtest.

@fujimotos
Copy link
Member

fujimotos commented Mar 9, 2022

I think the general direction of this patch is good. However:

So we need to add the same warning for the case of multi-workers.

Won't this implementation produce false-positives? For example, if we use multiple workers but put out_s3 in <worker 0>, that should be a perfectly fine way to use this plugin.

I'm thinking something like this:

<system>
  worker 3
</system>
<worker 0>
  <match **>
     @type s3
     ...
   </match>
</worker>

Nonetheless, this patch is likely to produce warnings on such cases, which I think can be confusing to users.

@daipom
Copy link
Contributor Author

daipom commented Mar 10, 2022

I think the general direction of this patch is good. However:

So we need to add the same warning for the case of multi-workers.

Won't this implementation produce false-positives? For example, if we use multiple workers but put out_s3 in <worker 0>, that should be a perfectly fine way to use this plugin.

I'm thinking something like this:

<system>
  worker 3
</system>
<worker 0>
  <match **>
     @type s3
     ...
   </match>
</worker>

Nonetheless, this patch is likely to produce warnings on such cases, which I think can be confusing to users.

In this case, system_config.workers is 1, so the warning is not produced.
(https://github.com/fluent/fluentd/blob/master/lib/fluent/plugin/base.rb#L61)

We should also have a test code for this case,
but I don't know how to specify the worker directive on the test code, so I can't do that right away.

@ashie
Copy link
Member

ashie commented Mar 10, 2022

BTW I've fixed CI failure which isn't concerned with this PR in #393

daipom added 4 commits March 10, 2022 10:42
We can use `$ bundle exec rake test TESTOPTS="-t'S3OutputTest::configure'"`
to execute this subtest.

Signed-off-by: daipom <[email protected]>
This test must fail at this commit.
Next comming fix will make it all succeed.

`$ bundle exec rake test TESTOPTS="-t'S3OutputTest::configure'"`

* Failure: test_configure_warning_on_multi_threads[non_objectkey-multi_worker](S3OutputTest::configure)
* Failure: test_configure_warning_on_multi_threads[default_objectkey-multi_worker](S3OutputTest::configure)

Signed-off-by: daipom <[email protected]>
This makes `configure` subtest all succeed.

`$ bundle exec rake test TESTOPTS="-t'S3OutputTest::configure'"`

Signed-off-by: daipom <[email protected]>
Workers consist of processes, so fix some `multi thread` words too.

Signed-off-by: daipom <[email protected]>
@daipom daipom force-pushed the add-warning-on-multi-workers branch from 0cdb512 to a6a891a Compare March 10, 2022 01:42
@daipom
Copy link
Contributor Author

daipom commented Mar 10, 2022

@ashie Thank you! I have rebased this branch and all checks have passed.

@fujimotos
Copy link
Member

Merged via f55d70f, f5fa697, and bf8bb2c.

@fujimotos fujimotos closed this Mar 11, 2022
@daipom
Copy link
Contributor Author

daipom commented Mar 11, 2022

Thank you for reorganizing commits and merging!

@fujimotos
Copy link
Member

fujimotos commented Mar 11, 2022

Thank you for reorganizing commits and merging!

Background note on why I edited these commits:

One of truly awesome feature of Git is git bisect. It allows you to
perform biinary search on Git history to find a bad commit fast.

git-bisect - Use binary search to find the commit that introduced a bug

https://git-scm.com/docs/git-bisect

Now, to make git bisect actually usable, we need to keep master history
bisectable -- each commit should be non-breaking.

So while I understand that "test-first" camp people often intentionally
introduce a breaking commit (which I think is fine as a development
methodology), I'd rather keep the public tree bisectable. So I edited em.

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