feat: update gem version requirement of rdkafka #1442
Conversation
joker1007
requested review from
a team,
ahayworth,
arielvalentin,
dazuma,
ericmustin,
fbogsany,
kaylareopelle,
mwear,
plantfansam,
robbkidd,
robertlaurin,
simi and
xuan-cao-swi
as code owners
|
I think it's ok to leave the rdkafka seems still keep the each_batch but with raise error. If user use > v0.20.0, and still use |
joker1007
force-pushed
the
update-rdkafka-requirement
branch
from
78310e3 to
de95123
Compare
|
@xuan-cao-swi |
|
Deleting each_batch for earlier versions of the gem means the instrumentation will change. If we no longer want to maintain backward compatibility this change should also define a new minimum version supported where the each_batch method was removed. |
| compatible do | ||
| gem_version = Gem::Version.new(::Rdkafka::VERSION) | ||
| Gem::Requirement.new('>= 0.10.0', '< 0.20.0').satisfied_by?(gem_version) | ||
| Gem::Requirement.new('>= 0.10.0', '< 0.22.0').satisfied_by?(gem_version) |
There was a problem hiding this comment.
I think @arielvalentin means to change to Gem::Requirement.new('>= 0.20.0').satisfied_by?(gem_version) for new instrumentation-rdkafka version
|
Hi @joker1007, could you take a look at @xuan-cao-swi's comment? |
|
I've been so busy for a couple of weeks that I haven't had time to check GitHub properly. |
kaylareopelle
changed the title
kaylareopelle
left a comment
kaylareopelle
left a comment
There was a problem hiding this comment.
Thanks @joker1007! I'm sorry for our subsequent delay. I really appreciate your patience. I was getting ready to merge this and noticed a few other things.
I realized today that the supported version range of the rdkafka gem published on their repo is 0.18.0 - 0.22.0. We generally have instrumentation support for all the versions supported by the maintainers of the library. I think we need to increase the min version to 0.18.0.
Would you mind making the following updates?
- updating this PR to wrap a condition around
each_batchbased on the versions that support it,0.19.0and below? - update the
Appraisalsfile to run the tests on the newly supported version range.
This reverts commit de95123.
|
@kaylareopelle OK, I'm going to do the following tasks.
I can do more tasks if you discard versions that are not maintained from our supported versions.
For now, I will implement 1 and 2. |
|
It appears that approval is needed again to run the workflow. |
|
Hi @joker1007, thanks for your understanding. Please take care of 3 and 4 if you can as well! |
|
I have done the tasks. In summary, I have implemented the following
|
|
This looks great! Thank you for the changes, @joker1007. |
* chore: update gem version requirement of rdkafka * fix: consider `each_batch` removed in rdkafka-0.20.0 * chore: delete redundant `::` * fix: delete `each_batch` method from targets of instrumentation * fix: fix version constraint of rdkafka * Revert "fix: delete `each_batch` method from targets of instrumentation" This reverts commit de95123. * ci: update Appraisals of rdkafka to support recent versions * fix: loosen version constraints to support old versions * fix: Make the unmaintained versions of rdkafka-ruby unsupported --------- Co-authored-by: Kayla Reopelle <[email protected]>
4 participants
The current latest version of rdkafka-ruby is v0.21.0, which is not available in the current instrumentation.
Therefore, update the version of gem requriement.
The changelog of rdkafka does not seem to show any breaking changes that affect tracing.