search

fluent / fluent-plugin-kafka

Kafka input and output plugin for Fluentd
Other
304 stars 178 forks source link

fix(out_rdkafka2): fix compatibility with rdkafka 0.12.0 #468

Closed raytung closed 1 year ago

raytung commented 2 years ago

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.

ashie commented 2 years ago

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

raytung commented 2 years ago

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

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.

github-actions[bot] commented 1 year ago

This PR has been automatically marked as stale because it has been open 90 days with no activity. Remove stale label or comment or this PR will be closed in 30 days

github-actions[bot] commented 1 year ago

This PR was automatically closed because of stale in 30 days

ashie commented 1 year ago

I'm sorry for not responding to this. I haven't forgotten this issue, but I'm quite busy and hard to check the details yet.

ashie commented 1 year ago

Sorry for my late response. Now I'm going to release v0.19 with this patch (with some nitpick fixes).

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.

I'm not sure, so that the current patch seems reasonable for safety.

ashie commented 1 year ago

Thanks for your effort!