rust-rdkafka icon indicating copy to clipboard operation
rust-rdkafka copied to clipboard

Upgrade librdkafka to v1.9.0.

Open gyfis opened this issue 2 years ago • 13 comments

Resolves #485

gyfis avatar Jun 27 '22 07:06 gyfis

Thanks! Did you run update-bindings.sh?

benesch avatar Jun 28 '22 04:06 benesch

@benesch Apologies, somehow completely missed the Contributing doc. I updated the bindings now, and also checked that no new errors were added in this version (by comparing pub enum rd_kafka_resp_err_t to pub fn rd_kafka_resp_err_t_to_rdkafka_error).

gyfis avatar Jun 28 '22 04:06 gyfis

No problem! Thanks for the PR. This looks good to me. I’m liable to forget to check back after CI completes, so feel free to ping me if/when this goes green.

benesch avatar Jun 28 '22 04:06 benesch

The ci / check is failing on librdkafka adding a HTTP client support via curl, which I didn't update here yet. I'll update the PR when I have the time - I'll try to add it under a feature flag, statically linked by default, with the option to dynamically link against system curl similar to how zstd is handled.

gyfis avatar Jun 28 '22 04:06 gyfis

I checked the existing features and decided to go with the libz path for the curl feature - optional, enabled by default, links dynamic library by default, and can be toggled to include curl that is statically linked.

gyfis avatar Jun 28 '22 17:06 gyfis

Hi @duarten, thanks for triggering the CI job! I missed one more place to put the curl default feature - now the check phase is passing (at least locally on my macOS).

I also added a missing build steps that correctly enable / disable CURL. I tested this locally by adding and removing the curl and curl-static from features when running cargo build, and all variants (no curl, curl, curl-static) pass for me as well.

One thing I'm wondering - with the changes I made, libcurl is a new dependency that is required by default (since the curl feature is turned on by default). @benesch do you think that's okay, or should I rewrite to disable curl by default, similar to zstd and everything else? Thanks!

gyfis avatar Jun 29 '22 06:06 gyfis

Hey all, I'm still struggling a little bit with making the Windows tests pass - I was able to add some missing library / include links for the compiler (so the top-level cargo build --all-targets --verbose --features "cmake-build,libz-static,curl-static" step is now passing), but it's still failing on the cargo-sys cargo test --features "cmake-build,libz-static,curl-static" test run. Below is the error message, and here's a link to the failed run.

Any help appreciated!

rdkafka.lib(rdhttp.obj) : error LNK2019: unresolved external symbol __imp_curl_global_init referenced in function rd_http_global_init
rdkafka.lib(rdhttp.obj) : error LNK2019: unresolved external symbol __imp_curl_easy_init referenced in function rd_http_req_init
rdkafka.lib(rdhttp.obj) : error LNK2019: unresolved external symbol __imp_curl_easy_setopt referenced in function rd_http_post_expect_json
rdkafka.lib(rdhttp.obj) : error LNK2019: unresolved external symbol __imp_curl_easy_perform referenced in function rd_http_req_perform_sync
rdkafka.lib(rdhttp.obj) : error LNK2019: unresolved external symbol __imp_curl_easy_cleanup referenced in function rd_http_get
rdkafka.lib(rdhttp.obj) : error LNK2019: unresolved external symbol __imp_curl_easy_getinfo referenced in function rd_http_get_json
D:\a\rust-rdkafka\rust-rdkafka\target\debug\deps\rdkafka_sys-6a9be4af9b107aaf.exe : fatal error LNK1120: 6 unresolved externals

gyfis avatar Jun 29 '22 21:06 gyfis

I think I got it; I needed to include the -DCURL_STATICLIB in the CFLAGS, as described in the curl docs. Here's a link to the passing windows check run: https://github.com/gyfis/rust-rdkafka/runs/7191217214?check_suite_focus=true

gyfis avatar Jul 05 '22 06:07 gyfis

Hi @duarten, @thijsc ! Thanks for the previous actions trigger. I’m done with the PR, ready for review - care to take a look? (cc @benesch) 🙌

gyfis avatar Jul 09 '22 10:07 gyfis

:+1: would be useful to get the updated librdkafka. I've been seeing some issues with mixed librdkafka version consumers in the same consumer group

djkazic avatar Jul 11 '22 12:07 djkazic

Hi again @duarten, @thijsc ! Sorry for the repeated notifications, and thanks for the previous actions trigger. I’m done with the PR, ready for review - care to take a look? (cc @benesch) 🙌

gyfis avatar Jul 26 '22 10:07 gyfis

Hi @benesch ! Apologies for pinging so much, just wondering what you think about the changes and if we can get the tests running to progress with the PR :) It's ready to be reviewed/merged. Let me know if there's anything you'd like to change here!

gyfis avatar Aug 12 '22 15:08 gyfis

@gyfis maybe is worth updating to latest patch version 1.9.2 ?

ElfoLiNk avatar Aug 25 '22 07:08 ElfoLiNk

Has anyone been testing this successfully? We've been hit with this bug from librdkafka https://github.com/edenhill/librdkafka/pull/3774/commits/7435bf4b9388f2d50fd3ac00636be71b13ea6a98 in production a few times, but can't really update.

mindreader avatar Oct 28 '22 14:10 mindreader

Sorry for the delay here! Bumped the version to v1.9.2 and merged. I also disabled the curl feature by default, since we keep all the other optionals disabled by default.

benesch avatar Oct 29 '22 16:10 benesch

Thanks very much for all the work here, @gyfis! This was an awesome PR.

benesch avatar Oct 29 '22 16:10 benesch