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

Clear all metadata when we have the latest topic info #1033

Merged
merged 1 commit into from
Mar 15, 2018

Conversation

Mongey
Copy link
Contributor

@Mongey Mongey commented Feb 4, 2018

Currently, If you list topics, then delete a topic, and poll to find out when the topic is actually deleted, it will never appear as deleted.
This is because updateMetadata doesn't remove topics that no longer exist from the metadata.

An example can be seen over at terraform-provider-kafka

Here's a very naive solution that deletes the previous metadata, if you've got all the topic information from the broker.

@eapache
Copy link
Contributor

eapache commented Feb 6, 2018

Hmm, I would have thought this would already work? If you refresh metadata for a topic that has been deleted, you should presumably get back ErrUnknownTopicOrPartition which will result in the delete(client.metadata, topic.Name) being the only line run for that topic.

Oh, I guess this fix is only needed for the refresh-all case? It seems like polling the specific topic would be more efficient for your client anyway? But I suppose this PR is still more correct.

@Mongey
Copy link
Contributor Author

Mongey commented Feb 11, 2018

Yeah, this is only in the refresh all case, and I should actually just poll for the specific topic 😅, but this seems like undesired behaviour.

@@ -685,7 +686,9 @@ func (client *client) updateMetadata(data *MetadataResponse) (retry bool, err er
for _, broker := range data.Brokers {
client.registerBroker(broker)
}

if allKnownMetaData {
client.metadata = make(map[string]map[int32]*PartitionMetadata)
Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably clear client.cachedPartitionResults here too then?

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.

2 participants