fix producer topic metadata on-demand fetch when topic error happens

[andyxning]

This PR can help fix producer topic metadata on-demand fetch when topic error happens in metadata response.

There is an option Full in Config to config whether producer should fetch all topic metadata in a cluster or do it on-demand.
This feature is added in #937.

But there is an situation where we may make on-demand topic metadata fetching fall back to empty once an topic error happens in metadata update.

For example, if ErrUnknownTopicOrPartition or ErrInvalidTopic or ErrTopicAuthorizationFailed or any other errors that will make the switch case continue happens, the topic name will be deleted from client.metadata. This will result in an empty topic slice when refresh topic metadata next time in https://github.com/Shopify/sarama/blob/master/client.go#L648-L666 from client.Topics() which actually read topics from client.metadata. Thus we lost the topic name which we need to periodically refresh metadata.

We should not update client.metadata or delete any topic key from it.

[andyxning]

/cc @eapache @chandradeepak @wvanbergen

[andyxning]

/cc @burke

[eapache]

If ErrLeaderNotAvailable happens, the topic will be deleted from the map but will immediately be re-added at line 756. I don't understand how this is a bug.

[andyxning]

If ErrLeaderNotAvailable happens, the topic will be deleted from the map but will immediately be re-added at line 756. I don't understand how this is a bug.

@eapache Sorry for not making the description clear and correct firstly. The error maybe ErrUnknownTopicOrPartition or ErrInvalidTopic or ErrTopicAuthorizationFailed or any other errors that will make the switch case continue.

I have also updated the pr description.

[andyxning]

/cc @eapache @chandradeepak @wvanbergen @burke

[eapache]

I'm not sure I agree. If the broker returns one of those errors the topic either doesn't exist or is fundamentally inaccessible. Leaving it in the metadata hash at that point would be extremely misleading to callers and would likely lead to other problems when trying to iterate over the set of topics.

But there is an situation where we may make on-demand topic metadata fetching fall back to empty once an topic error happens in metadata update.

I'm also not sure how this can happen. The refreshMetadata method already checks if the list of Topics is empty when trying to do a partial refresh: https://github.com/Shopify/sarama/blob/f7df95cff1bc4d14a4194a5be4c88a86253afb98/client.go#L654-L655

[andyxning]

@eapache

I'm also not sure how this can happen. The refreshMetadata method already checks if the list of Topics is empty when trying to do a partial refresh

Yes. That's right. But refreshMetadata will return ErrNoTopicsToUpdateMetadata forever since the topic error happens. This is a problem because the topic error may disappear, but the topic name has been removed from metadata and it will not recover automatically alongside the topic error recover. A restart of producer must be done. This hits me.

If the broker returns one of those errors the topic either doesn't exist or is fundamentally inaccessible.

The error maybe not all related with producer side configuration. Some errors are related with server side operations or status.

[eapache]

OK, I understand. As mentioned, leaving it in the metadata would cause other issues. A better solution would be to keep a separate copy of the topics when not doing full metadata, and use that (in combination with client.Topics() still) in refreshMetadata().

[andyxning]

As mentioned, leaving it in the metadata would cause other issues. A better solution would be to keep a separate copy of the topics when not doing full metadata, and use that (in combination with client.Topics() still) in refreshMetadata().

I have thought that before i propose this PR. I think directly update the client.Topics is fine. According to the discuss above, it seems that a new field should be added to store topic names whose metadata are need to be updated periodically. @eapache
Will update this later today.

[andyxning]

@eapache PTAL.

[eapache]

sarama defines a helper type called none to make uses of struct{} like this more explicit

[andyxning]

@eapache Done. PTAL.

[eapache]

thanks!