Hi Brian,
Thanks for the responses.
4) Yes, agree that it would be simpler to leave units out of the initial
design. We currently have units that are interpreted by the configurable
callback. The default callback interprets the value as
per-broker-bytes-per-second and per-broker-percentage-cores. But callbacks
using partition-based throughput quotas for example would interpret the
value as cluster-wide-bytes-per-second. We could update callbacks to work
with units, but as you said, it may be better to leave it out initially and
address later.
On Thu, Jan 23, 2020 at 6:29 PM Brian Byrne <bby...@confluent.io> wrote:
> Thanks Rajini,
>
> 1) Good catch, fixed.
>
> 2) You're right. We'd need to extend ClientQuotaCallback#quotaLimit or add
> an alternate function. For the sake of an initial implementation, I'm going
> to remove '--show-overridden', and a subsequent KIP will have to propose an
> extents to ClientQuotaCallback to return more detailed information.
>
> 3) You're correct. I've removed the default.
>
> 4) The idea of the first iteration is be compatible with the existing API,
> so no modification to start. The APIs should be kept consistent. If a user
> wants to add custom functionality, say an entity type, they'll need to
> update their ConfigEntityType any way, and the quotas APIs are meant to
> handle that gracefully by accepting a String which can be propagated.
>
> The catch is 'units'. Part of the reason for having a default unit was for
> backwards compatibility, but maybe it's best to leave units out of the
> initial design. This might lead to adding more configuration entries, but
> it's also the most flexible option. Thoughts?
>
> Thanks,
> Brian
>
>
> On Thu, Jan 23, 2020 at 4:57 AM Rajini Sivaram <rajinisiva...@gmail.com>
> wrote:
>
> > 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
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
