Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

Expose Kafka Idempotency Config #45

Merged
merged 4 commits into from
Nov 23, 2021
Merged

Expose Kafka Idempotency Config #45

merged 4 commits into from
Nov 23, 2021

Conversation

fenriskiba
Copy link
Contributor

@fenriskiba fenriskiba commented Oct 20, 2021

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

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

Checklist:

  • 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.
  • All new and existing tests passed.

* Upgraded Sarama Version; Added Kafka Idempotency (only once) config
* Bump alpine-glibc version to resolve gcc vulnerability found in our security scan
* Bump Node Version to current Active LTS version
* Fixed issue with acks not being set properly (prerequisite for Idemptoncy)
* Added config for Kafka MaxOpenRequests (prerequisite for Idemptoncy)
@fenriskiba
Copy link
Contributor Author

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

@fenriskiba
Copy link
Contributor Author

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
Copy link
Member

@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

Copy link
Member

@marceloboeira marceloboeira left a comment

Choose a reason for hiding this comment

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

Left a few comments of things to do/change but otherwise looks good.

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
pkg/config/env.go Show resolved Hide resolved
pkg/config/env.go Show resolved Hide resolved
@zhouzhuojie
Copy link
Contributor

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

@@ -5,7 +5,7 @@ go 1.16
require (
cloud.google.com/go v0.37.4
github.com/DataDog/datadog-go v0.0.0-20180330214955-e67964b4021a
github.com/Shopify/sarama v1.19.0
github.com/Shopify/sarama v1.29.1
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a big version bump, any noticeable breaking changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only update I'm aware of that could be breaking is added SASLVersion in config #1410, though I'll admit, I'm not familiar enough with the SASL Handshake versions work to know how significant that is.

If you think that's a problem/risk, I could change it to v1.20.1. I went with latest (at the time of our internal testing) since I figured staying up to date would be preferable, but the Idempotency configs are back in v1.20.

@codecov-commenter
Copy link

Codecov Report

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

Impacted file tree graph

@@            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%) ⬆️

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
Copy link
Contributor

pending on @marceloboeira

@mergify mergify bot merged commit a696ab6 into openflagr:main Nov 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants