Hi, Thanks for the KIP. +1( binding).
Thanks, On Mon, Feb 25, 2019 at 6:28 PM Rajini Sivaram <rajinisiva...@gmail.com> wrote: > Thanks Mickael. > > I thought I had to list the type only once when the same field appears > twice (the field itself is listed both for cluster and topic). But you are > the second person who brought this up, so I must be mistaken. I have added > the type in both places to avoid confusion. > > On Mon, Feb 25, 2019 at 12:38 PM Mickael Maison <mickael.mai...@gmail.com> > wrote: > > > +1 (non binding) > > In the Metadata v8 section, it looks like the "authorized_operations" > > field is missing under "topic_metadata". There's only the top-level > > "authorized_operations" field. > > > > On Mon, Feb 25, 2019 at 12:11 PM Rajini Sivaram <rajinisiva...@gmail.com > > > > wrote: > > > > > > Hi Colin, > > > > > > Yes, it makes sense to reduce response size by using bit fields. > Updated > > > the KIP. > > > > > > I have also updated the KIP to say that clients will ignore any bits > set > > by > > > the broker that are unknown to the client, so there will be no UNKNOWN > > > operations in the set returned by AdminClient. Brokers may however set > > bits > > > regardless of client version. Does that match your expectation? > > > > > > Thank you, > > > > > > Rajini > > > > > > > > > On Sat, Feb 23, 2019 at 1:03 AM Colin McCabe <cmcc...@apache.org> > wrote: > > > > > > > Hi Rajini, > > > > > > > > Thanks for the explanations. > > > > > > > > On Fri, Feb 22, 2019, at 11:59, Rajini Sivaram wrote: > > > > > Hi Colin, > > > > > > > > > > Thanks for the review. Sorry I meant that an array of INT8's, each > of > > > > which > > > > > is an AclOperation code will be returned. I have clarified that in > > the > > > > KIP. > > > > > > > > Do you think it's worth considering a bitfield here still? An array > > will > > > > take up at least 4 bytes for the length, plus whatever length the > > elements > > > > are. A 32-bit bitfield would pretty much always take up less space. > > And > > > > we can have a new version of the RPC with 64 bits or whatever if we > > outgrow > > > > 32 operations. MetadataResponse for a big cluster could contain > quite > > a > > > > lot of topics, tens or hundreds of thousands. So the space savings > > could > > > > be considerable. > > > > > > > > > > > > > > All permitted operations will be returned from the set of supported > > > > > operations on each resource. This is regardless of whether the > > access was > > > > > implicitly or explicitly granted. Have clarified that in the KIP. > > > > > > > > Thanks. > > > > > > > > > > > > > > Since the values returned are INT8 codes, clients can simply ignore > > any > > > > > they don't recognize. Java clients convert these into > > > > AclOperation.UNKNOWN. > > > > > That way we don't need to update Metadata/describe request versions > > when > > > > > new operations are added to a resource. This is consistent with > > > > > DescribeAcls behaviour. Have added this to the compatibility > section > > of > > > > the > > > > > KIP. > > > > > > > > Displaying "unknown" for new AclOperations made sense for > DescribeAcls, > > > > since the ACL is explicitly referencing the new AclOperation. For > > > > example, if you upgrade your Kafka cluster to a new version that > > supports > > > > DESCRIBE_CONFIGS, your old ACLs still don't reference > DESCRIBE_CONFIGS. > > > > > > > > In contrast, in the case here, existing topics (or other resources) > > might > > > > pick up the new ACLOperation just by upgrading Kafka. For example, > if > > you > > > > had ALL permission on a topic and you upgrade to a new version with > > > > DESCRIBE_CONFIGS, you now have DESCRIBE_CONFIGS permission on that > > topic. > > > > This would result in a lot of "unknowns" being displayed here, which > > might > > > > not be ideal. > > > > > > > > Also, there is an argument from intent-- the intention here is to let > > you > > > > know what you can do with a resource that already exists. Knowing > > that you > > > > can do an unknown thing isn't very useful. In contrast, for > > DescribeAcls, > > > > knowing that an ACL references an operation your software is too old > to > > > > understand is useful (you may choose not to modify that ACL, since > you > > > > don't know what it does, for example.) What do you think? > > > > > > > > cheers, > > > > Colin > > > > > > > > > > > > > > > > > > Thank you, > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > On Fri, Feb 22, 2019 at 6:46 PM Colin McCabe <cmcc...@apache.org> > > wrote: > > > > > > > > > > > Hi Rajini, > > > > > > > > > > > > Thanks for the KIP! > > > > > > > > > > > > The KIP specifies that "Authorized operations will be returned as > > [an] > > > > > > INT8 consistent with [the] AclOperation used in ACL requests and > > > > > > responses." But there may be more than one AclOperation that is > > > > applied to > > > > > > a given resource. For example, a principal may have both READ > and > > > > WRITE > > > > > > permission on a topic. > > > > > > > > > > > > One option for representing this would be a bitfield. A 32-bit > > > > bitfield > > > > > > could have the appropriate bits set. For example, if READ and > > WRITE > > > > > > operations were permitted, bits 3 and 4 could be set. > > > > > > > > > > > > Another thing to think about here is that certain AclOperations > > imply > > > > > > certain others. For example, having WRITE on a topic gives you > > > > DESCRIBE on > > > > > > that topic as well automatically. Does that mean that a topic > with > > > > WRITE > > > > > > on it should automatically get DESCRIBE set in the bitfield? I > > would > > > > argue > > > > > > that the answer is yes, for consistency's sake. > > > > > > > > > > > > We will inevitably add new AclOperations over time, and we have > to > > > > think > > > > > > about how to do this in a compatible way. The simplest approach > > would > > > > be > > > > > > to just leave out the new AclOperations when a describe request > > comes > > > > in > > > > > > from an older version client. This should be spelled out in the > > > > > > compatibility section. > > > > > > > > > > > > best, > > > > > > Colin > > > > > > > > > > > > > > > > > > On Thu, Feb 21, 2019, at 02:28, Rajini Sivaram wrote: > > > > > > > I would like to start vote on KIP-430 to optionally obtain > > authorized > > > > > > > operations when describing resources: > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-430+-+Return+Authorized+Operations+in+Describe+Responses > > > > > > > > > > > > > > Thank you, > > > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > > > > > >
