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 > > > > > > > > > > > > > > > > > > >
