openflagr / flagr-archived

[Archived] - Merged and move to https://github.com/openflagr/flagr

https://openflagr.github.io/flagr

Apache License 2.0

33 stars 7 forks source link

Expose Kafka Idempotency Config #45

Closed fenriskiba closed 2 years ago

fenriskiba commented 2 years ago

Resolving Issue #21

Description

Upgraded Sarama Version
Exposed Sarama's new Idempotency config
Fixed issue with acks not being set properly (prerequisite for Idemptoncy)
Added config for Kafka MaxOpenRequests (prerequisite for Idemptoncy)

Motivation and Context

See Issue #21

How Has This Been Tested?

Manually regression and stress tested in our OpenShift environment, both with default Kafka configs and Idempotency enabled.

Types of changes

[x] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[x] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[ ] I have added tests to cover my changes.
[x] All new and existing tests passed.

fenriskiba commented 2 years ago

@zhouzhuojie or @marceloboeira would one of you be able to review this?

fenriskiba commented 2 years ago

Is there a way to rerun the ci / unit_test suite? It seems like it timed out, but I can't see any reason why it would and it runs fine in my local. I'm wondering if it just got caught on a network issue or something.

marceloboeira commented 2 years ago

@fenriskiba I haven't had the time yet to review this, I'll take a look later this week. In any case, I just re-triggered the jobs to see if it works now

zhouzhuojie commented 2 years ago

yes, let's separate the docker image bump from Kafka config changes, easy to revert if needed. thank you!

codecov-commenter commented 2 years ago

Codecov Report

Merging #45 (5f19115) into main (f6a8dd2) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #45      +/-   ##
==========================================
+ Coverage   81.55%   81.57%   +0.01%     
==========================================
  Files          27       27              
  Lines        2174     2176       +2     
==========================================
+ Hits         1773     1775       +2     
  Misses        314      314              
  Partials       87       87

Impacted Files	Coverage Δ
pkg/handler/data_recorder_kafka.go	`73.14% <100.00%> (+0.50%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f6a8dd2...5f19115. Read the comment docs.

zhouzhuojie commented 2 years ago

pending on @marceloboeira