KAFKA-8866; Return exceptions as Optional<ApiException> in authorizer API #7294
Conversation
| */ | ||
| public ApiException exception() { | ||
| return exception; | ||
| public Optional<ApiException> exception() { |
There was a problem hiding this comment.
Is failed() below still useful enough?
There was a problem hiding this comment.
@ijuma Thanks for the review. Agree failed() is unnecessary, removed.
| @@ -44,8 +46,8 @@ private AclDeleteResult(Collection<AclBindingDeleteResult> deleteResults, ApiExc | |||
| /** | |||
| * Returns any exception while attempting to match ACL filter to delete ACLs. | |||
There was a problem hiding this comment.
Should we update this javadoc in the same way you did the AclCreateResult one?
| if (result.failed) { | ||
| println(s"Error while adding ACLs: ${result.exception.getMessage}") | ||
| println(Utils.stackTrace(result.exception)) | ||
| result.exception.ifPresent { exception => |
There was a problem hiding this comment.
I think this doesn't work with Scala 2.11. You probably have to convert to the Scala option before you can use lambdas. There are utility methods for that in scala-java8-compat.
There was a problem hiding this comment.
@ijuma Thank you, updated.
| public ApiException exception() { | ||
| return exception; | ||
| public Optional<ApiException> exception() { | ||
| return exception == null ? Optional.empty() : Optional.of(exception); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Should we remove deleted() also?
There was a problem hiding this comment.
@omkreddy Thanks for the review. Makes sense, removed deleted() as well.
Follow up on review comment in PR #7268
Committer Checklist (excluded from commit message)