chore(deps): bump rdkafka to v0.34.0 and librdkafka to v2.2.0

[wangrunji0408]

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

As title. Please also review the change in madsim-rs/madsim#173, and see what's changed from the original rdkafka in this commit.

Checklist

• I have written necessary rustdoc comments
• I have added necessary unit tests and integration tests
• I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
• My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
• All checks passed in ./risedev check (or alias, ./risedev c)
• My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

• My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

• My PR contains user-facing changes.

Click here for Documentation

Types of user-facing changes

Please keep the types that apply to your changes, and remove the others.

• Installation and deployment
• Connector (sources & sinks)
• SQL commands, functions, and operators
• RisingWave cluster configuration changes
• Other (please specify in the release note below)

Release note

[xxchan]

Note that the upstream rust-rdkafka is still using librdkafka v1.9.2. But we require a newer version since we depend on the DNS resolution callback feature from Materialize, which has been included in the upstream librdkafka since v2.0.0. So we forked rdkafka-sys and updated its librdkafka to the latest v2.2.0, then switched rdkafka-sys from Mz's fork to our fork.

The story is a little long. Basically it means we switched to latest versions, and applied the same patch as the MZ fork?

Another question is what's the motivation to upgrade (besides keeping it up to date)? Since it's a critical dependency, and the upgrade is across multiple versions (v0.29.0->v0.33.2, v1.9.2->v2.2.0), has anyone went through the CHANGELOG, and/or tested it?

[wangrunji0408]

Basically it means we switched to latest versions, and applied the same patch as the MZ fork?

Yes. The Mz's patch has been included in the latest version.

Another question is what's the motivation to upgrade (besides keeping it up to date)? Since it's a critical dependency, and the upgrade is across multiple versions (v0.29.0->v0.33.2, v1.9.2->v2.2.0), has anyone went through the CHANGELOG, and/or tested it?

The original motivation was to fix potential memory leak in rdkafka according to @shanicky. But it looks like we have found the reason from another source (see #10888). So this upgrade is not critical at this time. We can pause it until every change has been reviewed and test passed.

The failure of e2e source/sink tests shows there might be problems somewhere :(

[shanicky]

The failure of e2e source/sink tests shows there might be problems somewhere :(

Initially, we suspected that rdkafka might have potential memory leak risks, and we were relying on a version from eight months ago, so we asked Runji if we could upgrade. Later, we found out that the issue likely stemmed from our configuration setup, so the upgrade didn’t seem as urgent. If there is an issue, it feels like we can delay it. 🥵

[xxchan]

we were relying on a version from eight months ago, so we asked Runji if we could upgrade

IMO 8 months isn’t that old but is also quite old. 🤣

Btw, I would recommend update regularly and one version at a time. Then we can review the changes much more easily. Also we can avoid cases like we upgrade it to fix a problem, but didn’t noticed the changes and introduced another problems..

the upgrade didn’t seem as urgent. If there is an issue, it feels like we can delay it

According to the reasons above, I think it’s a good time to begin reviewing the changes now. 😁(Although not urgent)

[xxchan]

Well, but we have a PR depending on the upgrade now. 🤣 #11203

[fuyufjh]

Well, but we have a PR depending on the upgrade now. 🤣 #11203

Don't mind. I can remove the new-version-specific option from that PR if you need to delay this.

[fuyufjh]

I tried to merge this into my branch and it doesn't seem to work:

dev=> create table s25 (v1 int, v2 varchar) with (
  connector = 'kafka',
  ...
) FORMAT PLAIN ENCODE JSON;
ERROR:  QueryError: internal error: error trying to connect: tcp connect error: Connection refused (os error 111)
dev=>

By the way, risedev also act wierdly. On my local Mac, it hangs forever

[fuyufjh]

Reminder: please help to update

https://github.com/risingwavelabs/risingwave/pull/11203/files#diff-851782ccb1f1b4dfb7b42b85cbc605f6f74fe1e5f957b5293ef0e345f4ee7fc1R65-R68

and

https://github.com/risingwavelabs/risingwave/pull/11203/files#diff-851782ccb1f1b4dfb7b42b85cbc605f6f74fe1e5f957b5293ef0e345f4ee7fc1R160-R162

after this PR gets merged

[wangrunji0408]

Seems there are some problems in this madsim-rdkafka update. An unit test is blocking madsim-rs/madsim@6932616. Needs more investigation.

[github-actions]

This PR has been open for 60 days with no activity. Could you please update the status? Feel free to ping a reviewer if you are waiting for review.

[wangrunji0408]

I found the reason why previous update was blocking. The resolve_cb function cherry-picked from Materialize didn't consider null pointer input, which lead to invalid memory access. I've added a check to fix this problem.

https://github.com/madsim-rs/madsim/blob/95d4a63a2e4d3572964e7c72df38cc010e81f24c/madsim-rdkafka/src/std/client.rs#L554-L557

[codecov]

Codecov Report

Merging #11232 (0a9ddb0) into main (80f1d58) will decrease coverage by 0.01%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #11232      +/-   ##
==========================================
- Coverage   69.22%   69.22%   -0.01%     
==========================================
  Files        1483     1483              
  Lines      243680   243680              
==========================================
- Hits       168693   168676      -17     
- Misses      74987    75004      +17     

Flag	Coverage Δ
rust	69.22% <ø> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[xxchan]

Since 1.3 already branches off and will soon be published, and seemingly no one is going to really review the CHANGELOG in depth, I'm OK to merge this into main now, and just let it go through tests of time.. 🤡