Hi all,
I have submitted a PR to address a few review comments from Ismael. There
are a couple of interface changes in the PR, I have updated the KIP as well.
1. The `listener()` method in AuthorizableRequestContext and Endpoint
has been renamed to listenerName()
2.
Hi Jun,
Thanks for the response. If we use the existing purgatory implementation,
we should get additional purgatory metrics for ACL updates with the new
purgatory name as tag, consistent with what we have for other delayed
operations. I will add these to the KIP. We also have request metrics
Hi, Rajini, Ismael,
Yes, I can see the argument for making CreateAcls/DeleteAcls async. I am ok
with that if you feel the implementation is not too complicated. Should we
consider adding some additional metric to reflect the portion of the time
spent in waiting for the async operation to
Hi Jun,
As you say, even though the average number of operations may be low, the
request rate can be high in bursts. This can overwhelm the request queue
and cause an outage for no good reason. Or the backing storage system may
be slow for a period of time, causing a similar issue.
I've seen
Hi Jun,
Thanks for your note. Yes, agree that sync authorize() will be sufficient
in most cases based on the numbers we expect to see in the foreseeable
future.
For async CreateAcls/DeleteAcls, we will need to add a callback in
KafkaApis and add to purgatory if future is not complete. Since we
Hi, Rajini,
Thanks for the reply. The 4-step approach that you outlined seems to work.
Overall, I can see that the async authorize() api could lead to an overall
more efficient implementation. The tradeoff is that we have to code every
request with an extra stage. To me, this optimization seems
Thanks Ismael. That is good point. I have updated the javadoc.
On Sun, Sep 8, 2019 at 5:38 PM Ismael Juma wrote:
> Hi Rajini,
>
> Can you please update the javadoc for every sync method to be explicit
> about the fact that they should not block and the reasoning?
>
> Ismael
>
> On Sun, Sep 8,
Hi Rajini,
Can you please update the javadoc for every sync method to be explicit
about the fact that they should not block and the reasoning?
Ismael
On Sun, Sep 8, 2019, 9:21 AM Rajini Sivaram wrote:
> Hi all,
>
> Thanks everyone for the very useful discussion. I have updated the KIP to
>
Hi all,
Thanks everyone for the very useful discussion. I have updated the KIP to
make *createAcls()* and *deleteAcls()* asynchronous. Also added a section
under `*Rejected Alternatives*` for async *authorize()*, which we can
revisit in future if requirement arises.
Regards,
Rajini
On Fri,
It's not uncommon for people to do large numbers of admin operations per
second. We've seen it many times in production. It's often a bug, but the
system must cope with it without affecting other users of the same Kafka
cluster.
Ismael
On Fri, Sep 6, 2019, 9:32 AM Don Bosco Durai wrote:
> +1
>
Sounds good to me.
Thanks
Bosco
On 9/6/19, 11:47 AM, "Colin McCabe" wrote:
Hi Rajini,
I agree that there is probably some value in async createAcls and
deleteAcls methods. It's also a smaller change to use a futures-based
interface there, since it really only affects the
Hi Rajini,
I agree that there is probably some value in async createAcls and deleteAcls
methods. It's also a smaller change to use a futures-based interface there,
since it really only affects the CreateAcls and DeleteAcls operations. It's
probably fine for the other methods to be sync for
I would suggest that we go with synchronous authorize() and acls() method
to describe ACLs, but asynchronous createAcls() and deleteAcls() that
return CompletionStage. With KIP-500, I think async create/delete methods
will be useful. For now, we can assume that brokers are able to cache all
ACLs.
+1
I might be wrong here, but here are few of my assumptions..
1. Unlike transactional services, in Kafka most of the users are application
users, so once the connection is established, then they are going to be long
running sessions. So the caching is more predictable and heavy lifting can be
One more thing: if people have strong opinions that authorize has to be
sync, we could leave it like that for now. If needed, we can later add an
authorizeAsync with another method returning a Boolean indicating that it's
supported. That is, there is a path to evolve the interface (even if a bit
I'm on the move, but what Rajini said seems reasonable. I don't think using
SSDs solves the issue. They can still hang for seconds when they fail.
Also, many people may not have local SSDs available (remote SSDs like EBS
hang for tens of seconds when there are issues).
We are currently vulnerable
Hi Jun,
For ACLs, the size is also dependent on the number of users. So in
deployments with large number of users where some users are not active, an
LRU cache may be useful. Agree that there may be other solutions that help
avoid the requirement for an async API even for this case.
I wasn't
Hi, Ismael,
Yes, I agree that there are 2 separate discussion points, one on the API
and the other on the implementation.
First on the API, in general, my feeling is that the authorize() call is
expected to be fast to reduce request latency. Adding an async API could be
mis-leading since it
Hi Rajini
Overall, it looks good.
Thanks
Bosco
On 9/4/19, 3:39 AM, "Rajini Sivaram" wrote:
Hi Colin & Don,
I have submitted a PR to help with reviewing the new API:
https://github.com/apache/kafka/pull/7293/files.
- Authorizer.java contains the updated API and
Hi Jun,
I think it's important to discuss the pluggable API versus the calling
implementation as two separate points.
>From an API perspective, are you suggesting that we should tell users that
they cannot block in authorize? Or are you saying that it's OK to block on
authorize on occasion and
Hi, Rajini,
Thanks for the KIP. I was concerned about #4 too. If we change the handling
of all requests to use an async authorize() api, will that cause the code
much harder to understand? There are quite a few callbacks already. I am
not sure that we want to introduce more of those. The benefit
Hi Colin & Don,
I have submitted a PR to help with reviewing the new API:
https://github.com/apache/kafka/pull/7293/files.
- Authorizer.java contains the updated API and includes javadoc
update including the threading model.
- AclAuthorizer.scala changes in the PR should give an idea of
Hi Don,
Thanks for your note.
1) The intention is to avoid blocking in the calling thread. We already
have several requests that are put into a purgatory when waiting for remote
communication, for example produce request waiting for replication. Since
we have a limited number of request threads
Hi Rajini
Help me understand this a bit more.
1. For all practical purpose, without authorization you can't go to the next
step. The calling code needs to block anyway. So should we just let the
implementation code do the async part?
2. If you feel management calls need to be async, then we
Hi Rajini,
That's an interesting point. I guess I assumed that we would always cache
metadata locally, so that a synchronous operation would be OK here. But, I
suppose as time goes on, we will eventually want paged authorization metadata.
If the operations are done asynchronously, then that
Hi all,
Ismael brought up a point that it will be good to make the Authorizer
interface asynchronous to avoid blocking request threads during remote
operations.
1) Since we want to support different backends for authorization metadata,
making createAcls() and deleteAcls() asynchronous makes
Hi Rajini
Thanks for clarifying. I am good for now.
Regards
Bosco
On 8/16/19, 11:30 AM, "Rajini Sivaram" wrote:
Hi Don,
That should be fine. I guess Ranger loads policies from the database
synchronously when the authorizer is configured, similar to
SimpleAclAuthorizer
Hi Don,
That should be fine. I guess Ranger loads policies from the database
synchronously when the authorizer is configured, similar to
SimpleAclAuthorizer loading from ZooKeeper. Ranger can continue to load
synchronously from `configure()` or `start()` and return an empty map from
`start()`.
Hi Rajini
Assuming this doesn't affect custom plugins, I don't have any concerns
regarding this.
I do have one question regarding:
"For authorizers that don’t store metadata in ZooKeeper, ensure that authorizer
metadata for each listener is available before starting up the listener. This
Hi all,
I made another change to the KIP. The KIP was originally proposing to
extend SimpleAclAuthorizer to also implement the new API (in addition to
the existing API). But since we use the new API when available, this can
break custom authorizers that extend this class and override specific
Thanks Colin.
If there are no other concerns, I will start vote later today. Many thanks
to every one for the feedback.
Regards,
Rajini
On Thu, Aug 15, 2019 at 11:57 PM Colin McCabe wrote:
> Thanks, Rajini. It looks good to me.
>
> best,
> Colin
>
>
> On Thu, Aug 15, 2019, at 11:37, Rajini
Thanks, Rajini. It looks good to me.
best,
Colin
On Thu, Aug 15, 2019, at 11:37, Rajini Sivaram wrote:
> Hi Colin,
>
> Thanks for the review. I have updated the KIP to move the interfaces for
> request context and server info to the authorizer package. These are now
> called
Hi Colin,
Thanks for the review. I have updated the KIP to move the interfaces for
request context and server info to the authorizer package. These are now
called AuthorizableRequestContext and AuthorizerServerInfo. Endpoint is now
a class in org.apache.kafka.common to make it reusable since we
Hi Rajini,
I think it would be good to rename KafkaRequestContext to something like
AuthorizableRequestContext, and put it in the
org.apache.kafka.server.authorizer namespace. If we put it in the
org.apache.kafka.common namespace, then it's not really clear that it's part of
the Authorizer
Hi Rajini,
Thanks for the KIP!
I really like that authorize() will be able to take a batch of
requests, this will speed up many implementations!
On Tue, Aug 13, 2019 at 5:57 PM Rajini Sivaram wrote:
>
> Thanks David! I have fixed the typo.
>
> Also made a couple of changes to make the context
Thanks David! I have fixed the typo.
Also made a couple of changes to make the context interfaces more generic.
KafkaRequestContext now returns the 16-bit API key as Colin suggested as
well as the friendly name used in metrics which are useful in audit logs.
`Authorizer#start` is now provided a
Hi Rajini,
Thank you for the update! It looks good to me. There is a typo in the
`AuditFlag` enum: `MANDATORY_AUTHOEIZE` -> `MANDATORY_AUTHORIZE`.
Regards,
David
On Mon, Aug 12, 2019 at 2:54 PM Rajini Sivaram
wrote:
> Hi David,
>
> Thanks for reviewing the KIP! Since questions about
Hi David,
Thanks for reviewing the KIP! Since questions about `authorization mode`
and `count` have come up multiple times, I have renamed both.
1) Renamed `count` to `resourceReferenceCount`. It is the number of times
the resource being authorized is referenced within the request.
2) Renamed
Hi Rajini,
Thank you for the KIP. Overall, it looks good to me. I have few
questions/suggestions:
1. It is hard to grasp what `Action#count` is for. I guess I understand
where you want to go with it but it took me a while to figure it out.
Perhaps, we could come up with a better name than
Hi Rajini
3.2 - This makes sense. Thanks for clarifying.
Rest looks fine. Once the implementations are done, it will be more clear on
the different values RequestType and Mode.
Thanks
Bosco
On 8/9/19, 5:08 AM, "Rajini Sivaram" wrote:
Hi Don,
Thanks for the suggestions. A
Hi Colin,
Thanks for the reviewing the KIP and the suggestions to improve.
*1. Why not just pass the cluster ID directly to Authorizer#start*
I was thinking that we already have APIs that use ClusterResourceListener,
so kept it consistent. But you are right, since we have a start method, we
Hi Don,
Thanks for the suggestions. A few responses below:
3.1 Can rename and improve docs if we keep this. Let's finish the
discussion on Colin's suggestions regarding this first.
3.2 No, I was thinking of some requests that have an old way of authorizing
and a new way where we have retained
Hi Rajini
Thanks for clarifying. This is very helpful...
#1 - On the Ranger side, we should be able to handle multiple requests at the
same time. I was just not sure how much processing overhead will be there on
the Broker side to split and then consolidate the results. If it is negligible,
Hi Rajini,
Thanks for the KIP. This will be a great improvement.
Why not just pass the cluster ID directly to Authorizer#start, rather than
dealing with the ClusterResourceListener interface? That seems like it would
be simpler. If authorizers don't need that information, they can ignore
Hi Rajini,
Sure, I will start a discussion thread soon on dev mailing list.
Thanks,
Satish.
On Thu, Aug 8, 2019 at 1:29 AM Rajini Sivaram wrote:
>
> Hi Ron/Harsha/Satish,
>
> Thanks for reviewing the KIP!
>
> We should perhaps have a wider discussion outside this KIP for refactoring
> clients
Hi Don,
Thanks for reviewing the KIP.
1. I had this originally as a single Action, but thought it may be useful
to support batched authorize calls as well and keep it consistent with
other methods. Single requests can contain multiple topics. For example a
produce request can contain records for
Rajini
Thanks for putting this together. It is looking good. I have few questions...
1. List authorize(..., List actions). Do you see
a scenario where the broker will call authorize for multiple topics at the same
time? I can understand that during creating/deleting ACLS, multiple permissions
Hi Ron/Harsha/Satish,
Thanks for reviewing the KIP!
We should perhaps have a wider discussion outside this KIP for refactoring
clients so that others who are not following this KIP also notice the
discussion. Satish, would you like to start a discussion thread on dev?
Regards,
Rajini
On Wed,
I felt the same need when we want to add a pluggable API for core
server functionality. This does not need to be part of this KIP, it
can be a separate KIP. I can contribute those refactoring changes if
others are OK with that.
It is better to have a structure like below.
kafka-common:
common
Thanks for the KIP Rajini.
Quick thought, it would be good to have a common module outside of clients
that only applies to server side interfaces & changes. It looks like we are
increasingly in favor of using Java interface for pluggable modules on the
broker side.
Thanks,
Harsha
On Tue, Aug
Hi Rajini,
looks great and addresses a few gripes I had in the past, thanks for that!
One idea that I had while reading, but I am not sure if this is taking
"being flexible" a step too far maybe.. Would it make sense to make the
decision at which severity to log a decision
Looks great, Rajini — a detailed and complete KIP with a great
backwards-compatibility plan. Nothing came to mind aside from how easy it was
to read and understand. Thanks for writing it so clearly.
Ron
> On Aug 6, 2019, at 5:31 PM, Rajini Sivaram wrote:
>
> Hi all,
>
> I have created a
Hi all,
I have created a KIP to replace the Scala Authorizer API with a new Java
API:
-
https://cwiki.apache.org/confluence/display/KAFKA/KIP-504+-+Add+new+Java+Authorizer+Interface
This is replacement for KIP-50 which was accepted but never merged. Apart
from moving to a Java API
53 matches
Mail list logo
