Make PulsarAuthorizationProvider#grantPermissionAsync actually async #13897
Conversation
michaeljmarshall
added
type/enhancement
michaeljmarshall
requested review from
merlimat,
freeznet,
rdhabalia,
eolivelli and
codelipenghui
|
|
||
| return result; | ||
| future.exceptionally(ex -> { | ||
| log.error("[{}] Failed to set permissions for role {} namespace {}", role, role, namespaceName, ex); |
There was a problem hiding this comment.
- it seems we need to
getCausehere. - I think
[{}] Failed to set policies while set permissions for role {} namespace {}more specific, what do you think about it?
There was a problem hiding this comment.
@mattisonchao - great catch, we should use getCause here. I'll revisit the log line too. Both of these are copied closely from #12515, so I'll take a look at modifying that code too. Thanks!
There was a problem hiding this comment.
Regarding the log line, I think the lack of clarity is because the API is a bit inconsistent by using policies and permission interchangeably. There are several instances in the file that indicate Failed to set permissions or Failed to get permissions. I think we should leave it as is for now and possibly change it in another PR. Let me know what you think. Thanks.
...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
Outdated
Show resolved
Hide resolved
...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
Outdated
Show resolved
Hide resolved
...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
Outdated
Show resolved
Hide resolved
|
@codelipenghui - I removed the |
|
@codelipenghui - since this is only an optimization and not a bug fix, I didn't expect this to get cherry picked back to any maintenance branches? How do we determine what goes into maintenance branches? Thanks. |
|
@michaeljmarshall I have removed release/2.9.3. I thought it was a fix after encountering a problem before. |
…pache#13897) ### Motivation This PR is based on an observation from apache#12515. The content is essentially the same, and the goal is to make an async method _actually_ asynchronous. ### Modifications * Make `PulsarAuthorizationProvider#grantPermissionAsync` actually async * Update the exception handling to ensure correctness * Switch from `thenRun` to `whenComplete` for handling future completion in several methods in `PulsarAuthorizationProvider`.
Motivation
This PR is based on an observation from #12515. The content is essentially the same, and the goal is to make an async method actually asynchronous.
Modifications
PulsarAuthorizationProvider#grantPermissionAsyncactually asyncthenRuntowhenCompletefor handling future completion in several methods inPulsarAuthorizationProvider.Verifying this change
There are already tests that cover this section of the code.
Does this pull request potentially affect one of the following parts:
This is only an optimization, it does not include any breaking changes.
Documentation
no-need-doc