fluent-plugin-kafka icon indicating copy to clipboard operation
fluent-plugin-kafka copied to clipboard

fix(out_rdkafka2): fix compatibility with rdkafka 0.12.0

Open raytung opened this issue 1 year ago • 3 comments

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.

raytung avatar Aug 17 '22 12:08 raytung

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

ashie avatar Aug 18 '22 02:08 ashie

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.

raytung avatar Aug 18 '22 02:08 raytung

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.

raytung avatar Aug 18 '22 09:08 raytung