Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Kafka module: query each broker all the partitions it is a leader for #16556
Kafka module: query each broker all the partitions it is a leader for #16556
Changes from 5 commits
32fdbab
c4557fe
ee5275b
67f0cc6
d9128f8
73bf9f3
bf55b0d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leader id is available in
partition.Leader
. We could use this id for the grouping of partitions per broker.If we continue using
b.client.Leader()
we have to remember toClose()
the returned broker. Maybe we can useb.client.Brokers()
to look for the broker per id.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the method
b.client.Leader(topic, partition)
will always return the most actual leader (there might be a metadata update in background, right?).The method
b.client.Brokers()
returns brokers without opening connections to them. To establish a connection, I would need a configuration structure:broker.Open(conf *Config)
.Leader()
handles it on its own.Regarding
Close()
, I think you're right.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There can still be a problem with this approach, here we are calling
Leader
for each topic and partition, this may open too many connections to brokers, and we may be leaking connections because we only keep track of one connection per broker inleaderBrokers
.This is right, but between this moment and the moment we make the offsets request there can still be some metadata change, if we want to solve this for good (not sure if it worths it) we would need to handle leadership errors (second option in #13380).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answered below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit. This initialization is not needed,
append
will initialize it if needed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to assume that there cannot be two partitions for the same topic in the same broker? (Probably, but I am not sure about that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be a case in which the number of Kafka brokers is lower than number of Kafka partitions of the same topic, so would rather keep this map as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline, we may need to list multiple partitions for the same topic in the same broker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit and probably unneeded optimizatiod 😄: we could parallelize requests per leader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.