fluent / fluent-plugin-s3

Amazon S3 input and output plugin for Fluentd
https://docs.fluentd.org/output/s3
314 stars 215 forks source link

Add warning on multi workers #392

Closed daipom closed 2 years ago

daipom commented 2 years ago

This is a fix for #391.

daipom commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

daipom commented 2 years ago

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

fujimotos commented 2 years ago

Merged via f55d70f5115df863c776099dc2cad88817adbf84, f5fa697200c236f6278fa18e937e4e2e6cee9537, and bf8bb2c9b1bf49ddb34e77e76707e3c180bf86a5.

daipom commented 2 years ago

Thank you for reorganizing commits and merging!

fujimotos commented 2 years ago

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.