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

ClusterAdmin swallows TopicError ErrMsg #1153

Closed
dnwe opened this issue Aug 21, 2018 · 3 comments
Closed

ClusterAdmin swallows TopicError ErrMsg #1153

dnwe opened this issue Aug 21, 2018 · 3 comments

Comments

@dnwe
Copy link
Collaborator

dnwe commented Aug 21, 2018

Versions

Sarama Version: e7238b1
Kafka Version: 1.1.1
Go Version: go1.10

Configuration

N/A

Logs

N/A

Problem Description

The ClusterAdmin interface added under #1055 provides an administrative client for Kafka similar to the official Java client based on design discussions under #1048

However, in the case of a TopicError being returned by one of the calls, only the underlying KError is returned to the user as it satisfies the built-in error interface.

https://github.com/Shopify/sarama/blob/e7238b119b7daab993720f0153eafb88e2b0ac1f/admin.go#L141-L148

However, in the case of (e.g.,) sarama.ErrPolicyViolation, sarama.ErrInvalidPartitions, sarama.ErrInvalidReplicaAssignment etc. there may well be a specific error message in the TopicError ErrMsg that is being hidden from the caller.

Shouldn't TopicError itself be extended to satisfy the error interface by appending any ErrMsg to the generic one?

e.g.,

type TopicError struct {
    Err    KError
    ErrMsg *string
}

func (t *TopicError) Error() string {
	text := t.Err.Error()
	if t.ErrMsg != nil {
		text = fmt.Sprintf("%s - %s", text, *t.ErrMsg)
	}
	return text
}
@dnwe
Copy link
Collaborator Author

dnwe commented Aug 21, 2018

Returning the full TopicError would allow type assertion to extract the kError and errMsg separately if that's appropriate too

@varun06
Copy link
Contributor

varun06 commented Mar 10, 2019

@dnwe This should be fixed now right?

@dnwe
Copy link
Collaborator Author

dnwe commented Mar 21, 2019

Yep. Fixed be #1154

@dnwe dnwe closed this as completed Mar 21, 2019
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

No branches or pull requests

2 participants