Hey Colin,
good KIP!
Some comments:
1a. For operation, permission_type and resource_type: is there any reason
for having the any and unknown enums as negative values?
Since neither of these fields has an integer significance (unlike for
example offsets which use negative offsets for logical offsets) I dont
really see a reason to do this. It might also trick client developers to
make assumptions on future negative values (if resource_type < 0:
treat_as_invalid()...), unless that's the reason :). This might be my
personal preference but encoding extended meaning into types should be
avoided unless needed, and I dont think it is needed for enums.
but..
1b. Since most clients implementing the ACL requests probably wont make
much use of this API themselves but rather treat it as a straight
pass-through between the protocol and the client's public API to the
application, could we save ourselves some work (current and future) to make
the enums as nullable_strings instead of integer enums? This would cut down
on the number of enum-to-str and vice versa conversions needed, and would
also make the APIs more future proof since an added resource_type (et.al)
would not need a client, or even tool, update, and the new type will not
show up as UNKNOWN but of its real value.
>From a broker-side verification perspective there should really be no
difference since the enum values will need to be interpreted anyway.
So instead of int enum { unknown = -2, any = -1, deny, allow }, we have {
null, "deny", "allow" }.
2. "Each of the arguments to ListAclsRequest acts as a filter."
Specify if these filters are OR:ed or AND:ed.
3. "CreateAclsRequest and CreateAclsResponse"
What happens if a client attempts to create an ACL entry which is identical
to one already created in the cluster?
Is this an error? silently ignored? resulting in duplicates?
4. "Compatibility plan"
"If we later add new resource types, operation types, and so forth, we
would like to be able to interact with them with the old AdminClient. This
is why the AclResourceType, AclOperation, and AclPermissionType enums have
UNKNOWN values. If we get an INT8 which we don't know the name for, it
gets mapped to an UNKNOWN object."
I'm not sure who "we" are. Is it the broker or the client?
(also see 1b for avoiding UNKNOWN altogether)
Regards,
Magnus
2017-04-21 22:27 GMT+02:00 Colin McCabe <cmcc...@apache.org>:
> Hi all,
>
> As part of the AdminClient work, we would like to add methods for
> adding, deleting, and listing access control lists (ACLs). I wrote up a
> KIP to discuss implementing requests for those operations, as well as
> AdminClient APIs. Take a look at:
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 140%3A+Add+administrative+RPCs+for+adding%2C+deleting%2C+and+listing+ACLs
>
> regards,
> Colin
>
