fluent-plugin-kafka
fluent-plugin-kafka copied to clipboard
fix(out_rdkafka2): fix compatibility with rdkafka 0.12.0
Fixes https://github.com/fluent/fluent-plugin-kafka/issues/463
I think this is the simplest implementation that I could find to support rdkafka 0.12.0, but unfortunately because of how we're monkey patching the methods, it means we'd have to drop support for < 0.12.0 thus making this a breaking change.
Not sure if this is the right place to propose whether we should consider splitting out rdkafka into its own plugin (say fluent-plugin-rdkafka
) so we're able to put the rdkafka gem as a dependency in the gemspec and give it a proper version range.
The "test" for this change is that I have created a separate PR that only bumps rdkafka gem to 0.12.0 and nothing else. https://github.com/fluent/fluent-plugin-kafka/pull/469, then check its unit test logs https://github.com/fluent/fluent-plugin-kafka/runs/7878417388?check_suite_focus=true#step:5:1030
Comparing to the test logs for this PR, https://github.com/fluent/fluent-plugin-kafka/runs/7878310221?check_suite_focus=true, we don't see any more NoMethodError:undefined method 'join' for nil:NilClass
errors.
Not sure if this is the right place to propose whether we should consider splitting out rdkafka into its own plugin (say fluent-plugin-rdkafka) so we're able to put the rdkafka gem as a dependency in the gemspec and give it a proper version range.
I'm also considering this option. Currently rdkafka isn't included in our dependency so we can't even block rdkafka 0.12.0 or later. We should consider how should we realize it since some codes are shared between out_kafka2 and out_rdkafka. Instead of creating a new repository, adding a new gemspec file might be enough for this.
I think this is the simplest implementation that I could find to support rdkafka 0.12.0, but unfortunately because of how we're monkey patching the methods, it means we'd have to drop support for < 0.12.0 thus making this a breaking change.
Another option is that including both code and switching them by checking rdkafka's version.
Probably it can be checked by Rdkafka::VERSION
:
https://github.com/appsignal/rdkafka-ruby/blob/c02f217a189b71e3e253fe4ad59286b5fc9d4034/lib/rdkafka/version.rb#L4
Instead of creating a new repository, adding a new gemspec file might be enough for this.
Ah yeah, that will probably be the simpler option!
Another option is that including both code and switching them by checking rdkafka's version.
This could work. Let me play around with this idea.
Hey @ashie, as far as I can tell, the only reason for monkey patching the close
method is so that we can have a hard timeout when closing rdkafka's producer client.
https://github.com/fluent/fluent-plugin-kafka/pull/468/files#diff-357aa79f6db32ff5e634032b9c03c05fa1c626bc165938f3a0f74385e45fe8ccR369-R373
Would it be problematic if we just wait for the thread to shutdown by itself? If there's no foreseeable problems with that, this patch could be a lot simpler.