Thanks Brian. Looks good.
Just a few minor points:
1) We can remove *public ResolveClientQuotasOptions
setOmitOverriddenValues(boolean omitOverriddenValues); *
2) Under ClientQuotasCommand, the three items are List/Describe/Alter,
rename to match the new naming for operations?
3) Request quota configs are doubles rather than long. And for
ClientQuotaCallback API, we used doubles everywhere. Wasn't sure if we
deliberately chose Longs for this API. if so, we should mention why under
Rejected Alternatives. We actually use request quotas < 1 in integration
tests to ensure we can throttle easily.
On Fri, Jan 24, 2020 at 5:28 PM Brian Byrne <bby...@confluent.io> wrote:
> 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
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
