Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Apply PR#468 patch only for 0.12.x. #513

Merged
merged 1 commit into from
Jul 30, 2024
Merged

Apply PR#468 patch only for 0.12.x. #513

merged 1 commit into from
Jul 30, 2024

Conversation

kenhys
Copy link
Contributor

@kenhys kenhys commented Jul 29, 2024

With #468,
rdkafka 0.12.0 specific patch was introduced, but rdkafka's internal change
was reverted later in 0.13.x.

So this monkey patch should be effective only between 0.12.0 and 0.12.1 (only 2 versions were shipped in 0.12.x).
For 0.13.x or later, it seems harmful.

@kenhys
Copy link
Contributor Author

kenhys commented Jul 30, 2024

When #514 was merged, rebase this PR.

@kenhys kenhys marked this pull request as ready for review July 30, 2024 03:14
@kenhys
Copy link
Contributor Author

kenhys commented Jul 30, 2024

CI has passed.

@kenhys kenhys requested review from daipom and ashie and removed request for daipom July 30, 2024 03:15
@ashie
Copy link
Member

ashie commented Jul 30, 2024

You mean this commit?
karafka/rdkafka-ruby@26bb67a

@ashie
Copy link
Member

ashie commented Jul 30, 2024

Hmm, v0.13.0 isn't found in https://github.com/karafka/rdkafka-ruby/tags, but found in https://rubygems.org/gems/rdkafka/versions.

@kenhys
Copy link
Contributor Author

kenhys commented Jul 30, 2024

You mean this commit? karafka/rdkafka-ruby@26bb67a

I mean: #468 commit b223101

@ashie
Copy link
Member

ashie commented Jul 30, 2024

Ah sorry, I'm talking about rdkafka-ruby's change, I want to know exact revision of it:

but rdkafka's internal change was reverted later in 0.13.x.

@kenhys
Copy link
Contributor Author

kenhys commented Jul 30, 2024 •
edited
Loading

I guess change was introduced via https://github.com/karafka/rdkafka-ruby/pull/172/commits, then removed via
https://github.com/karafka/rdkafka-ruby/pull/211/commits.

Maybe this commit keeps compatibility with 0.12.x, but not for 0.13.0 or later.
karafka/rdkafka-ruby@aa2a1d1

@kenhys
Apply PR#468 patch only for 0.12.x.
c7c35d0 
With fluent#468,
 rdkafka 0.12.0 specific patch (Rdkafka::Producer::Client) was
introduced [1], but rdkafka's internal change was reverted [2]
(removed Rdkafka::Producer::Client) later in 0.13.x.

[1] karafka/rdkafka-ruby#172
[2] karafka/rdkafka-ruby#211

So this monkey patch should be effective only between 0.12.0 and
0.12.1 (only 2 versions were shipped in 0.12.x).
For 0.13.x or later, it seems harmful.

Signed-off-by: Kentaro Hayashi <[email protected]>
@kenhys
Copy link
Contributor Author

kenhys commented Jul 30, 2024

Update commit message to refer rdkafka-ruby PR.

@kenhys kenhys merged commit aa5bbf6 into fluent:master Jul 30, 2024
9 checks passed
@kenhys kenhys deleted the fix-close branch July 30, 2024 05:46
@kenhys
Copy link
Contributor Author

kenhys commented Jul 30, 2024

Thanks!

@kenhys kenhys mentioned this pull request Jul 30, 2024
Merged
@ashie
Copy link
Member

ashie commented Jul 30, 2024

Sorry, I overlooked #505 which tackles on same issue more carefully.
Probably we should revert this then improve #505.

@ashie ashie mentioned this pull request Jul 30, 2024
Merged
@kenhys kenhys mentioned this pull request Jul 31, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashie ashie ashie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kenhys @ashie