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

Add ActiveBrokers() method to Kafka AsyncProducer #187

Closed
wants to merge 13 commits into from
Closed

Add ActiveBrokers() method to Kafka AsyncProducer #187

wants to merge 13 commits into from

Conversation

gm42
Copy link

@gm42 gm42 commented Mar 19, 2020

This PR adds an ActiveBrokers() method to Kafka AsyncProducer. It also expands the GoDoc comment for Close() method and fixes a typo in an error message.

Which problem is this PR solving?

Fixes #169

Signed-off-by: Sotirios Mantziaris <smantziaris@gmail.com>
Signed-off-by: Sotirios Mantziaris <smantziaris@gmail.com>
Signed-off-by: Sotirios Mantziaris <smantziaris@gmail.com>
Signed-off-by: Sotirios Mantziaris <smantziaris@gmail.com>
Signed-off-by: Sotirios Mantziaris <smantziaris@gmail.com>
Signed-off-by: Sotirios Mantziaris <smantziaris@gmail.com>
Signed-off-by: Sotirios Mantziaris <smantziaris@gmail.com>
@codecov
Copy link

codecov bot commented Mar 19, 2020

Codecov Report

Merging #187 into master will decrease coverage by 0.01%.
The diff coverage is 84.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
- Coverage   84.40%   84.38%   -0.02%     
==========================================
  Files          51       52       +1     
  Lines        3046     3068      +22     
==========================================
+ Hits         2571     2589      +18     
- Misses        397      399       +2     
- Partials       78       80       +2     
Impacted Files Coverage Δ
async/kafka/option.go 100.00% <ø> (ø)
trace/kafka/builder.go 94.66% <60.00%> (-2.52%) ⬇️
trace/kafka/kafka.go 83.33% <63.63%> (-0.80%) ⬇️
async/amqp/option.go 86.95% <100.00%> (ø)
async/kafka/group/group.go 83.13% <100.00%> (ø)
async/kafka/kafka.go 92.59% <100.00%> (ø)
async/kafka/simple/simple.go 90.47% <100.00%> (ø)
internal/validation/validation.go 100.00% <100.00%> (ø)
reliability/retry/retry.go 100.00% <100.00%> (ø)
sync/http/route.go 100.00% <100.00%> (ø)
... and 4 more

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 e4e95f7...1c209ac. Read the comment docs.

mantzas
mantzas previously approved these changes Mar 22, 2020
@mantzas
Copy link

mantzas commented Mar 24, 2020

@gm42 you have breaking changes due to a refactoring of packages. All client code has now been moved to the client package.

drakos74
drakos74 previously approved these changes Mar 24, 2020
func (ap *AsyncProducer) ActiveBrokers() []string {
brokers := ap.prodClient.Brokers()
activeBrokerAddresses := make([]string, len(brokers))
for i, b := range brokers {

Choose a reason for hiding this comment

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

Would you consider using „copy“ for a slice ? And let go do the memory re-arrangement under the hood ?

Copy link
Author

Choose a reason for hiding this comment

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

@drakos74 can you make an example for this specific loop? I don't think copy can be used here.

@gm42 gm42 dismissed stale reviews from drakos74 and mantzas via bf1c72b March 25, 2020 09:17
@gm42
Copy link
Author

gm42 commented Mar 25, 2020

Closed in favor of #192

@gm42 gm42 closed this Mar 25, 2020
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.

Kafka: add support for active brokers in Producers
3 participants