Thanks again, Rajini,
Units will have to be implemented on a per-config basis, then. I've removed
all language reference to units and replaced QuotaKey -> String (config
name). I've also renamed DescribeEffective -> Resolve, and replaced --list
with --describe, and --describe to --resolve to be consistent with the
config command and clear about what functionality is "new".
Thanks,
Brian
On Fri, Jan 24, 2020 at 2:27 AM Rajini Sivaram <rajinisiva...@gmail.com>
wrote:
> 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
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
