-
Notifications
You must be signed in to change notification settings - Fork 162
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
Do not update metadata cache on metadata call #409
Conversation
The call to `metadata` can include a list of topics. If it does, the metadata we receive will include metadata only for the specified topics. We don't want to replace our cache of all-topic metadata with a topic-specific response. We should leave the cache as-is. A slightly more optimal way to do this would be to update the metadata cache only if the metadata request did not specify any topics, but that seems a bit more effort than it's worth.
Hello, @richardlarocque! This is your first Pull Request that will be reviewed by SourceLevel, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information. |
I tried to run the tests on this PR locally, as described in CONTRIBUTING.md. They didn't quite work, but they didn't work for me on The idea behind this PR is as follows. When we try to produce a message, the client will end up having to update its metadata if it doesn't currently have metadata for the given topic ( kafka_ex/lib/kafka_ex/server.ex Lines 437 to 459 in 9255704
metadata state variable are an all-topic metadata response.
However, an explicit request for topic-specific metadata could, prior to the change presented here, replace the all-topic metadata maintained by the periodic timer with a topic-specific metadata response. That would be bad because the next produce request would then be more likely to have to go re-fetch all-topic metadata. That would be inefficient. Prior to #395, this bug would result in sending a lot of traffic at the "lead" broker, potentially overwhelming it with all-topic metadata requests. I suspect that PR partitally mitigates the effect since the metadata requests would be more evenly spread out and you could horizontally-scale your way past the problem, but still, the inefficiency would remain. |
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.
This seems reasonable to me. Thanks for the PR!
@joshuawscott what's our policy on approvals ? it seems that 2 is enough for such a simple change. |
oh yeah, 2 is plenty. |
do we need to update the other server implementation with this change, e.g. here? |
The call to
metadata
can include a list of topics. If it does, themetadata we receive will include metadata only for the specified topics.
We don't want to replace our cache of all-topic metadata with a
topic-specific response. We should leave the cache as-is.
A slightly more optimal way to do this would be to update the metadata
cache only if the metadata request did not specify any topics, but that
seems a bit more effort than it's worth.