fix(out_rdkafka2): fix compatibility with rdkafka 0.12.0

[raytung]

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]

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]

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]

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]]

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]]

This PR was automatically closed because of stale in 30 days

[ashie]

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]

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]

Thanks for your effort!