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

feat: expose rackID option in ingester samara client #3395

Merged

Conversation

shyimo
Copy link
Contributor

@shyimo shyimo commented Nov 16, 2021

Signed-off-by: shaimoria shai.moria@zooz.com

Which problem is this PR solving?

It is common to have a Kafka cluster spanning multiple datacenters. For example, a common deployment is within an AWS region in which each availability zone is treated as a datacenter. In order to reduce cross region/rack network cost, we can tell the consumer which rack he's located on. this way the consumer will fetch only from partitions that are located on he's region/rack

Short description of the changes

  • expose kafka.consumer.rack-id cli argument in order to ingester consumers to fetch from closest replica

@shyimo shyimo requested a review from a team as a code owner November 16, 2021 07:56
@shyimo shyimo requested a review from vprithvi November 16, 2021 07:56
Support for KIP-392 in Sarama consumer client
This will allow ingester consumers to fetch from the closest replica.

On branch add-kafka-consumer-client-rack
Changes to be committed:
	modified:   cmd/ingester/app/flags.go
	modified:   cmd/ingester/app/flags_test.go
	modified:   pkg/kafka/consumer/config.go

Signed-off-by: Shai Moria <shai.moria@zooz.com>
Signed-off-by: shaimoria <shai.moria@zooz.com>
@shyimo shyimo force-pushed the add-kafka-consumer-client-rack branch from c4ad44d to 3bc0a3e Compare November 16, 2021 07:58
@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #3395 (c8caaf6) into master (c497f6d) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3395      +/-   ##
==========================================
- Coverage   96.48%   96.47%   -0.02%     
==========================================
  Files         260      260              
  Lines       15215    15220       +5     
==========================================
+ Hits        14680    14683       +3     
- Misses        452      453       +1     
- Partials       83       84       +1     
Impacted Files Coverage Δ
cmd/ingester/app/flags.go 100.00% <100.00%> (ø)
cmd/collector/app/server/zipkin.go 68.29% <0.00%> (-2.44%) ⬇️
cmd/query/app/static_handler.go 94.01% <0.00%> (-1.80%) ⬇️
pkg/config/tlscfg/cert_watcher.go 94.73% <0.00%> (+2.10%) ⬆️

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 c497f6d...c8caaf6. Read the comment docs.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Please explain the use case in the PR description

flagSet.String(
KafkaConsumerConfigPrefix+SuffixRackID,
"",
"Configure the 'rack' in which the consumer resides to enable - must be supported by kafka server")
Copy link
Member

Choose a reason for hiding this comment

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

Incomplete sentence

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @yurishkuro.
i update the description

@shyimo
Copy link
Contributor Author

shyimo commented Nov 18, 2021

@yurishkuro add explanation for the use case

set better description for 'rack-id' cli flag

Changes to be committed:
	modified:   cmd/ingester/app/flags.go

Signed-off-by: shaimoria <shai.moria@zooz.com>
@shyimo shyimo force-pushed the add-kafka-consumer-client-rack branch from 2fc220d to 824ca28 Compare November 18, 2021 12:16
@yurishkuro yurishkuro enabled auto-merge (squash) November 18, 2021 12:21
@yurishkuro yurishkuro merged commit 5ff8cce into jaegertracing:master Nov 19, 2021
rbroggi pushed a commit to rbroggi/jaeger that referenced this pull request Nov 22, 2021
@jpkrohling jpkrohling added this to the v1.29.0 milestone Dec 1, 2021
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

4 participants