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

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

Closed
napallday opened this issue Jul 27, 2023 · 4 comments
Closed

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

napallday opened this issue Jul 27, 2023 · 4 comments
Labels
needs-investigation Issues that require followup from maintainers stale Issues and pull requests without any recent activity

Comments

@napallday
Copy link
Contributor

napallday commented Jul 27, 2023

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
Copy link
Contributor Author

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?

This comment was marked as outdated.

@github-actions github-actions bot added the stale Issues and pull requests without any recent activity label Nov 9, 2023
@dnwe dnwe removed the stale Issues and pull requests without any recent activity label Nov 10, 2023
Copy link

github-actions bot commented Feb 8, 2024

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 github-actions bot added the stale Issues and pull requests without any recent activity label Feb 8, 2024
@dnwe dnwe added needs-investigation Issues that require followup from maintainers and removed stale Issues and pull requests without any recent activity labels Feb 12, 2024
Copy link

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 github-actions bot added the stale Issues and pull requests without any recent activity label May 12, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-investigation Issues that require followup from maintainers stale Issues and pull requests without any recent activity
Projects
None yet
Development

No branches or pull requests

2 participants