Hi Brian,
Thanks for the proposal! I have a couple comments/questions:
1. I'm having a hard time understanding the point of `QuotaEntity.Type`. It
sounds like this might be just for convenience since the APIs are using
string types. If so, I think it's a bit misleading to represent it as an
enum. In particular, I cannot see how the UNKNOWN type would be used. The
`PrincipalBuilder` plugin allows users to provide their own principal type,
so I think the API should be usable even for unknown entity types. Note
also that we appear to be relying on this enum in `QuotaFilter` class. I
think that should be changed to just a string?
2. It's a little annoying that we have two separate APIs to describe client
quotas. The names do not really make it clear which API someone should use.
It might just be a naming problem. In the command utility, it looks like
you are using --list and --describe to distinguish the two. Perhaps the
APIs can be named similarly: e.g. ListClientQuotas and
DescribeClientQuotas. However, looking at the examples, it's still not very
clear to me why we need both options. Basically I'm finding the
"config-centric" mode not very intuitive.
Thanks,
Jason
On Fri, Jan 17, 2020 at 2:14 PM Brian Byrne <bby...@confluent.io> wrote:
> Thanks Colin, I've updated the KIP with the relevant changes.
>
> On Fri, Jan 17, 2020 at 10:17 AM Colin McCabe <cmcc...@apache.org> wrote:
>
> > I thought about this a little bit more, and maybe we can leave in the
> > enums rather than going with strings. But we need to have an "UNKNOWN"
> > value for all the enums, so that if a value that the client doesn't
> > understand is returned, it can get translated to that. This is what we
> did
> > with the ACLs API, and it worked out well.
> >
>
> Done. One thing I omitted here was that the API still accepts/returns
> Strings, since there may be plugins that specify their own types/units. If
> we'd like to keep it this way, then the UNKNOWN may be unnecessary. Let me
> know how you'd feel this is best resolved.
>
>
> > On balance, I think we should leave in "units." It could be useful for
> > future-proofing.
> >
>
> Done. Also added a comment in the ClientQuotaCommand to default to RATE_BPS
> if no unit is supplied to ease adoption.
>
>
> > Also, since there are other kinds of quotas not covered by this API, we
> > should rename DescribeQuotas -> DescribeClientQuotas, AlterQuotas ->
> > AlterClientQuotas, etc. etc.
> >
>
> Done. Updated command and script name, too.
>
>
> > Maybe QuotaFilter doesn't need a "rule" argument to its constructor right
> > now. We can just do literal matching for everything. Like I said
> earlier,
> > I don't think people do a lot of prefixing of principal names. When we
> > added the "prefix matching" stuff for ACLs, it was mostly to let people
> do
> > it for topics. Then we made it more generic because it was easy to do
> so.
> > In this case, the API is probably easier to understand if we just do a
> > literal match. We can always have a follow-on KIP to add fancier
> filtering
> > if needed.
> >
>
> Done.
>
>
> > For DescribeEffectiveQuotasResult, if you request all relevant quotas, it
> > would be nice to see which ones apply and which ones don't. Right now,
> you
> > just get a map, but you don't know which quotas are actually in force,
> and
> > which are not relevant but might be in the future if a different quota
> gets
> > deleted. One way to do this would be to have two maps-- one for
> applicable
> > quotas and one for shadowed quotas.
> >
>
> So the way it's specified is that it maps QuotaKey -> Value, however Value
> is actually defined to have two parts: the entry, and a list of overridden
> entries (where an entry is the value, along with the source). Perhaps the
> Value is poorly named, or maybe there's a simpler structure to be had?
>
> Thanks,
> Brian
>
>
>
> > On Tue, Jan 14, 2020, at 13:32, Brian Byrne wrote:
> > > Hi Colin,
> > >
> > > Your feedback is appreciated, thank you.
> > >
> > > On Tue, Jan 14, 2020 at 11:34 AM Colin McCabe <cmcc...@apache.org>
> > wrote:
> > >
> > > > This is probably a nitpick, but it would be nice to specify that this
> > list
> > > > is in order of highest priority to lowest.
> > > >
> > >
> > > Done.
> > >
> > >
> > > > Hmm. Maybe --show-overridden or --include-overridden is a better
> flag
> > > > name?
> > > >
> > >
> > > Done (--show-overridden).
> > >
> > >
> > > > I think it would be nice to avoid using enums for QuotaEntity#Type,
> > > > QuotaKey#Type, and QuotaFilter#Rule. With enums, we have to worry
> > about
> > > > forwards and backwards compatibility problems. For example, what do
> > you do
> > > > when you're querying a broker that has a new value for one of these,
> > that
> > > > is not in your enum? In the past, we've created an UNKNOWN value
> for
> > enum
> > > > types to solve this conundrum, but I'm not sure the extra complexity
> is
> > > > worth it here. We can jut make them strings and avoid worrying about
> > the
> > > > compatibility issues.
> > > >
> > >
> > > Makes sense. Done.
> > >
> > >
> > > > Is QuotaKey#Units really needed? It seems like perhaps QuotaKey#Type
> > > > could imply the units used.
> > > >
> > >
> > > Possibly, maybe. It depends on how much structure is useful, which
> > > influences the implementation in the broker. For example, for the
> > existing
> > > (global) bytes-per-second types (e.g. consumer byte rate), it may be
> > useful
> > > to define them on a per-broker BPS basis, and in some cases, in terms
> of
> > > shares. The question becomes whether it'd be better to have a module in
> > the
> > > broker that is capable of deducing the effective quota automatically
> > among
> > > different units for the same quota type, or whether each should be
> > handled
> > > individually.
> > >
> > > Given we don't expect many units, and some units may be incompatible
> with
> > > others, perhaps it is best to have the unit implicit in the type
> string,
> > to
> > > be handled by the broker appropriately.
> > >
> > > I've updated the KIP to reflect this change, which factors out the
> > > QuotaKey. Let me know your thoughts.
> > >
> > >
> > > > How common is the prefix matching use-case? I haven't heard about
> > people
> > > > setting up principal names with a common prefix or anything like
> > that-- is
> > > > that commonly done?
> > > >
> > >
> > > It was, in part, for exposition, but would handle a case where
> principals
> > > could be prefixed by organization/team name, numbered, or the like. If
> > you
> > > prefer I remove the rules and just accept a pattern, that's also an
> > option.
> > >
> > >
> > >
> > > > I sort of feel like maybe we could have a simpler API for
> > describeQuotas
> > > > where it takes a map of quota entity type to value, and we do a
> > logical AND
> > > > On that. I'm not sure if there's really a reason why it needs to be
> a
> > > > collection rather than a set, in other words...
> > > >
> > >
> > > For clarification, are you suggesting an interface where the user might
> > > provide {type=user, name=x} and it would return all entities that
> match,
> > > with their resulting quota values? Should I scrap pattern matching for
> > now,
> > > since it's a simple extension that can be done at a future time?
> > >
> > > Thanks,
> > > Brian
> > >
> > >
> > >
> > > > On Wed, Dec 11, 2019, at 15:30, Brian Byrne wrote:
> > > > > Hello all,
> > > > >
> > > > > I'm reviving the discussion for adding a quotas API to the admin
> > client
> > > > by
> > > > > submitting a new proposal. There are some notable changes from
> > previous
> > > > > attempts, namely a way to deduce the effective quota for a client
> > > > (entity),
> > > > > a way to query for configured quotas, and the concept of "units" on
> > > > quotas,
> > > > > among other minor updates.
> > > > >
> > > > > Please take a look, and I'll be happy to make any clarifications
> and
> > > > > modifications in regards to feedback.
> > > > >
> > > > >
> > > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-546%3A+Add+quota-specific+APIs+to+the+Admin+Client%2C+redux
> > > > >
> > > > > Thanks,
> > > > > Brian
> > > > >
> > > >
> > >
> >
>
