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.

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

Upgrade github.com/Shopify/sarama v1.33.0 to github.com/IBM/sarama v1.40.0 #4294

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

axfor
Copy link

@axfor axfor commented Mar 11, 2023


Deprecation notice of sarama-cluster:
image


@axfor axfor requested a review from a team as a code owner March 11, 2023 16:12
@axfor axfor requested a review from albertteoh March 11, 2023 16:12
@axfor axfor force-pushed the upgrade-sarama-v1.38.1 branch 2 times, most recently from 52af9be to 52f7aac Compare March 11, 2023 16:25
@yurishkuro
Copy link
Member

This looks a lot more than a version bump, please describe the changes.

@axfor
Copy link
Author

axfor commented Mar 12, 2023

This looks a lot more than a version bump, please describe the changes.


Deprecation notice of sarama-cluster:
image


pkg/kafka/consumer/config.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer.go Show resolved Hide resolved
@yurishkuro
Copy link
Member

Note: merging from main invalidates previous CI runs, but unit tests there are failing due to this change. https://github.com/jaegertracing/jaeger/actions/runs/4492589597/jobs/7912198192

@axfor axfor force-pushed the upgrade-sarama-v1.38.1 branch 2 times, most recently from d428632 to 39cbfea Compare March 26, 2023 08:29
@axfor
Copy link
Author

axfor commented Mar 26, 2023

Note: merging from main invalidates previous CI runs, but unit tests there are failing due to this change. https://github.com/jaegertracing/jaeger/actions/runs/4492589597/jobs/7912198192

Based on the main branch, I used git merge --squash upgrade-sarama-v1.38.1 to merge all the commits of my branch


See axfor#8 :

@codecov
Copy link

codecov bot commented Mar 26, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 🎉

Comparison is base (2c1bf07) 97.04% compared to head (77acc07) 97.08%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4294      +/-   ##
==========================================
+ Coverage   97.04%   97.08%   +0.03%     
==========================================
  Files         301      301              
  Lines       17857    17944      +87     
==========================================
+ Hits        17330    17421      +91     
+ Misses        422      419       -3     
+ Partials      105      104       -1     
Flag Coverage Δ
unittests 97.08% <100.00%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
cmd/ingester/app/consumer/message.go 100.00% <ø> (ø)
plugin/storage/kafka/factory.go 100.00% <ø> (ø)
plugin/storage/kafka/options.go 89.38% <ø> (ø)
plugin/storage/kafka/writer.go 100.00% <ø> (ø)
cmd/ingester/app/consumer/consumer.go 98.31% <100.00%> (+1.34%) ⬆️
cmd/ingester/app/consumer/consumer_metrics.go 100.00% <100.00%> (ø)
cmd/ingester/app/consumer/deadlock_detector.go 100.00% <100.00%> (ø)
cmd/ingester/app/consumer/offset/manager.go 100.00% <100.00%> (ø)
cmd/ingester/app/consumer/processor_factory.go 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@axfor
Copy link
Author

axfor commented Apr 2, 2023

Codecov Report

Patch coverage: 2.70% and project coverage change: -0.82 ⚠️

Comparison is base (ea55beb) 97.10% compared to head (39cbfea) 96.29%.

❗ Current head 39cbfea differs from pull request most recent head 555aff3. Consider uploading reports for the commit 555aff3 to get more accurate results

Additional details and impacted files
☔ View full report in Codecov by Sentry. 📢 Do you have feedback about the report comment? Let us know in this issue.


I added more unit tests

@axfor axfor requested a review from yurishkuro April 3, 2023 06:23
@axfor axfor force-pushed the upgrade-sarama-v1.38.1 branch 3 times, most recently from 380ec42 to b76ebe0 Compare April 7, 2023 18:05
@axfor
Copy link
Author

axfor commented Apr 7, 2023

@yurishkuro @albertteoh
I added more unit tests and please re-review ! Thank you

cmd/ingester/app/consumer/consumer.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/processor_factory.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer_test.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer_test.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer_test.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer_test.go Outdated Show resolved Hide resolved
pkg/kafka/consumer/config.go Outdated Show resolved Hide resolved
@axfor axfor force-pushed the upgrade-sarama-v1.38.1 branch 3 times, most recently from 532f13a to 1bbe8d1 Compare April 15, 2023 16:50
@axfor axfor force-pushed the upgrade-sarama-v1.38.1 branch 2 times, most recently from cc4f731 to d26207e Compare April 16, 2023 02:01
@axfor
Copy link
Author

axfor commented Apr 16, 2023

Codecov Report

Patch coverage: 2.70% and project coverage change: -0.82 ⚠️

Comparison is base (ea55beb) 97.10% compared to head (39cbfea) 96.29%.

❗ Current head 39cbfea differs from pull request most recent head 555aff3. Consider uploading reports for the commit 555aff3 to get more accurate results

Additional details and impacted files
☔ View full report in Codecov by Sentry. 📢 Do you have feedback about the report comment? Let us know in this issue.

I added more unit tests

Patch code coverage up to 100%

refer to my repository:

https://app.codecov.io/github/jaegertracing/jaeger/commit/d26207e598c39806a6bff7689bf0e71347c3718a


image

@axfor
Copy link
Author

axfor commented Apr 18, 2023

Merge main to axfor:upgrade-sarama-v1.38.1 and resolve go.mod conflicts

cmd/ingester/app/consumer/consumer_test.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer_test.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer.go Outdated Show resolved Hide resolved
// NewTestConfig returns a config meant to be used by tests.
// Due to inconsistencies with the request versions the clients send using the default Kafka version
// and the response versions our mocks use, we default to the minimum Kafka version in most tests
func NewTestConfig() *sarama.Config {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you may have missed this and others in this file.

@axfor axfor force-pushed the upgrade-sarama-v1.38.1 branch 2 times, most recently from 47430ea to 131da26 Compare April 22, 2023 11:08
pkg/kafka/consumer/config.go Show resolved Hide resolved
cmd/ingester/app/consumer/processor_factory_test.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/processor_factory_test.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/processor_factory_test.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer_test.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer_test.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer_test.go Show resolved Hide resolved
cmd/ingester/app/consumer/consumer.go Outdated Show resolved Hide resolved
@axfor axfor force-pushed the upgrade-sarama-v1.38.1 branch 6 times, most recently from 7ff2980 to 1eba14a Compare April 30, 2023 08:59
cmd/ingester/app/consumer/offset/manager.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer_test.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer_test.go Show resolved Hide resolved
cmd/ingester/app/consumer/consumer_test.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer_test.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer_test.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer_test.go Outdated Show resolved Hide resolved
cmd/ingester/app/consumer/consumer_test.go Outdated Show resolved Hide resolved
@axfor axfor changed the title Bump github.com/Shopify/sarama from v1.33.0 to v1.38.1 Bump github.com/Shopify/sarama v1.33.0 to github.com/IBM/sarama v1.40.0 Jul 22, 2023
Signed-off-by: axfor <aixiaoxiang2009@hotmail.com>
Signed-off-by: axfor <aixiaoxiang2009@hotmail.com>
Signed-off-by: axfor <aixiaoxiang2009@hotmail.com>
@axfor axfor changed the title Bump github.com/Shopify/sarama v1.33.0 to github.com/IBM/sarama v1.40.0 Upgrade github.com/Shopify/sarama v1.33.0 to github.com/IBM/sarama v1.40.0 Jul 22, 2023
@yurishkuro
Copy link
Member

@axfor thanks for the work. Are you interested in completing it per this ticket's scope #4591 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants