Hi Brian,
Thanks for the KIP. Looks good, hope we finally get this in!
A few comments:
1) All the Admin interface methods seem to be using method names starting
with upper-case letter, should be lower-case to be follow conventions.
2) Effective quotas returns not only the actual effective quota, but also
overridden values. I don't think this works with custom quota callbacks.
3) KIP says that all quotas are currently bytes-per-second and we will use
RATE_BPS as the default. Request quotas are a percentage. So this doesn't
quite work. We also need to consider how this works with custom quota
callbacks. Can custom quota implementations define their own units?
4) We seem to be defining a new set of quota-related classes e.g. for quota
types, but we haven't considered what we do with the existing API in
org.apache.kafka.server.quota. Should we keep these consistent? Are we
planning to deprecate some of those?
Regards,
Rajini
On Wed, Jan 22, 2020 at 7:28 PM Brian Byrne <bby...@confluent.io> wrote:
> Hi Jason,
>
> I agree on (1). It was Colin's original suggestion, too, but he had changed
> his mind in preference for enums. Strings are the more generic way for now,
> so hopefully Colin can share his thinking when he's back. The QuotaFilter
> usage was an error, this has been corrected.
>
> For (2), the config-centric mode is what we have today in CommandConfig:
> reading/altering the configuration as it's described. The
> DescribeEffectiveClientQuotas would be resolving the various config entries
> to see what actually applies to a particular entity. The examples are a
> little trivial, but the resolution can become much more complicated as the
> number of config entries grows.
>
> List/describe aren't perfect either. Perhaps describe/resolve are a better
> pair, with DescribeEffectiveClientQuotas -> ResolveClientQuotas?
>
> I appreciate the feedback!
>
> Thanks,
> Brian
>
>
>
> On Tue, Jan 21, 2020 at 12:09 PM Jason Gustafson <ja...@confluent.io>
> wrote:
>
> > 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
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
