sarama icon indicating copy to clipboard operation
sarama copied to clipboard

fix #2256 violates the functionality of `config.Metadata.Retry.Max`

Open napallday opened this issue 1 year ago • 4 comments

Versions

Please specify real version numbers or git SHAs, not just "Latest" since that changes fairly regularly.

Sarama Kafka Go
v1.40.1 N/A N/A
Configuration

What configuration values are you using for Sarama and Kafka?

Logs

When filing an issue please provide logs from Sarama and Kafka if at all possible. You can set sarama.Logger to a log.Logger to capture Sarama debug output.

logs: CLICK ME

Problem Description

In #2256, updateMetaDataMs is introduced to reduce the trigger of metadata refresh if there are multiple RefreshMetadata goroutines(to fix #1711).

	retry := func(err error) error {
		if attemptsRemaining > 0 {
			backoff := client.computeBackoff(attemptsRemaining)
			if pastDeadline(backoff) {
				Logger.Println("client/metadata skipping last retries as we would go past the metadata timeout")
				return err
			}
			if backoff > 0 {
				time.Sleep(backoff)
			}

**			t := atomic.LoadInt64(&client.updateMetadataMs)
**			if time.Since(time.UnixMilli(t)) < backoff {
**				return err
			}
			attemptsRemaining--
			Logger.Printf("client/metadata retrying after %dms... (%d attempts remaining)\n", backoff/time.Millisecond, attemptsRemaining)

			return client.tryRefreshMetadata(topics, attemptsRemaining, deadline)
		}
		return err

The logic above means that in one MetadataRefresh goroutine G1, during Nth retry and N+1th retry, if there is another goroutine G2 has refreshed the metadata, G1 will conclude the metadata refresh before initiating the N+1th retry, and directly returns the err gotten in the Nth retry, regardless of G2's result.

For example, even if the maximum retry count is set to 5, the RefreshMetadata might return after only the 1st retry with the corresponding error.

Therefore, I think this commit is unsuitable since it violates the intended functionality of the config.Metadata.Retry.Max configuration..

napallday avatar Jul 27 '23 14:07 napallday

hi master @dnwe , may I know if you have ideas on this issue? And do we need to revert the commit or do any further optimization?

napallday avatar Aug 11 '23 02:08 napallday

Thank you for taking the time to raise this issue. However, it has not had any activity on it in the past 90 days and will be closed in 30 days if no updates occur. Please check if the main branch has already resolved the issue since it was raised. If you believe the issue is still valid and you would like input from the maintainers then please comment to ask for it to be reviewed.

github-actions[bot] avatar Nov 09 '23 18:11 github-actions[bot]

Thank you for taking the time to raise this issue. However, it has not had any activity on it in the past 90 days and will be closed in 30 days if no updates occur. Please check if the main branch has already resolved the issue since it was raised. If you believe the issue is still valid and you would like input from the maintainers then please comment to ask for it to be reviewed.

github-actions[bot] avatar Feb 08 '24 12:02 github-actions[bot]

Thank you for taking the time to raise this issue. However, it has not had any activity on it in the past 90 days and will be closed in 30 days if no updates occur. Please check if the main branch has already resolved the issue since it was raised. If you believe the issue is still valid and you would like input from the maintainers then please comment to ask for it to be reviewed.

github-actions[bot] avatar May 12 '24 16:05 github-actions[bot]