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: AdminClient | CreateACLs | check for error in response, return error if needed #2185

Merged
merged 3 commits into from
Mar 28, 2022

Conversation

omris94
Copy link
Contributor

@omris94 omris94 commented Mar 21, 2022

While calling to CreateACLs, the errors returned from the Kafka broker are ignored. ACL creation can fail but the function returns no error.

This PR makes the function return an error if one or more AclCreationResponse.Err is not ErrNoError.

@ghost ghost added the cla-needed label Mar 21, 2022
broker.go Outdated
Comment on lines 668 to 673
for _, res := range response.AclCreationResponses {
if !errors.Is(res.Err, ErrNoError) {
Logger.Printf("Failed creating ACL: %s\n", res.Err.Error())
return response, res.Err
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for spotting this. Rather than returning the first error, should we be returning the collection of errors?
e.g.,

	errs := make([]error, 0)
	for _, res := range response.AclCreationResponses {
		if !errors.Is(res.Err, ErrNoError) {
			errs = append(errs, err)
		}
	}
	if len(errs) > 0 {
		return Wrap(ErrCreateACLs, errs...)
	}

with errors.go

// ErrCreateACLs is the type of error returned when ACL creation failed
var ErrCreateACLs = errors.New("kafka server: failed to create one or more ACLs")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@ghost ghost removed the cla-needed label Mar 22, 2022
@omris94 omris94 requested a review from dnwe March 22, 2022 13:15
@nkostoulas
Copy link
Contributor

Could this explain #2167?

@dnwe
Copy link
Collaborator

dnwe commented Mar 22, 2022

@nkostoulas yes I believe it will

@dnwe dnwe added the fix label Mar 22, 2022
@dnwe dnwe changed the title AdminClient | CreateACLs | check for error in response, return error if needed fix: AdminClient | CreateACLs | check for error in response, return error if needed Mar 22, 2022
@omris94
Copy link
Contributor Author

omris94 commented Mar 22, 2022

I added a test

@omris94
Copy link
Contributor Author

omris94 commented Mar 28, 2022

@dnwe, I would appreciate a merge or a feedback, since we want to use the upstream instead of a branch

@dnwe
Copy link
Collaborator

dnwe commented Mar 28, 2022

@omris94 sure, changes look good to me so I’m happy to review and merge. We have a few more things I’d like to see go in before we’d cut a new version number, but I guess you’re happy to track via a commit in main until then?

@omris94
Copy link
Contributor Author

omris94 commented Mar 28, 2022

@omris94 sure, changes look good to me so I’m happy to review and merge. We have a few more things I’d like to see go in before we’d cut a new version number, but I guess you’re happy to track via a commit in main until then?

@dnwe
yap , thanks :-)

Copy link
Collaborator

@dnwe dnwe left a comment

Choose a reason for hiding this comment

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

@omris94 great catch this one, and thank you for your first contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants