You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@kafka.apache.org by Brian Byrne <bb...@confluent.io> on 2019/12/11 23:30:13 UTC

[DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux

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

Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux

Posted by Brian Byrne <bb...@confluent.io>.

Hi Mickael,

Unfortunately, it's not actively being worked on, but I welcome anyone to
come forward. Otherwise it's on my list that I'll eventually get around to.

Currently the logic replicates what was being done historically with the
quota configuration management, and is therefore compatible with the custom
ClientQuotaCallback plugins. To support custom entities, the
ClientQuotaCallback would need to become more generic with respect to
entity types as there's no way to signal an update to a custom type, so
that's the major hangup. Either a new generic quota callback plugin must be
created which can be a superset of the current one, or the current one
needs to be extended.

Sorry I don't have a definitive answer for when it'll be supported, but
it's still on my radar.

Brian

On Wed, Aug 26, 2020 at 6:36 AM Mickael Maison <mi...@gmail.com>
wrote:

> Hi,
>
> Is anyone actively working on adding support for custom quotas callbacks?
> We should really try to get this in soon as otherwise this KIP is not
> usable in environments with custom quotas. It's unfortunate to have
> the capability on the client-side but no support on brokers.
>
> Thanks
>
>
> On Sat, Jan 25, 2020 at 10:51 AM Colin McCabe <cm...@apache.org> wrote:
> >
> > Hi Brian,
> >
> > Thanks for the KIP.  I think it looks good and ready for a vote!
> >
> > One nitpick:  we try to avoid being too Java-specific when describing
> the protocol.  Rather than referencing java.lang.Double, it would be more
> helpful to say something equivalent, like "the float is serialized as a
> 64-bit IEEE 754 value, ordered as big-endian."  That way people who are
> developing clients in other languages can more easily understand the
> protocol.
> >
> > best,
> > Colin
> >
> >
> > On Fri, Jan 24, 2020, at 17:18, Brian Byrne wrote:
> > > Thanks for reviewing, Anna,
> > >
> > > In the describe call, the idea is that the API will match the
> QuotaFilters,
> > > which can be specified in two ways for your example (filter is a type
> and a
> > > matching string):
> > >
> > > 1) (USER="user-1") returns both
> > > 2) (USER="user-1", CLIENT_ID="") returns just /config/users/”user-1”
> > >
> > > [For (2), it may be better to permit 'null' to indicate "omitted"
> instead
> > > of the empty string.]
> > >
> > > You're correct for the resolve case, although it may not be
> highlighting
> > > the difference between them. Let's say you had the following:
> > >
> > > /config/users/<default>, value=100
> > >
> > > Then --describe with (USER="user-1") returns empty since there's no
> config
> > > entries for /config/users/”user-1”/*, whereas --resolve with
> > > (USER="user-1") returns (value=100) since the user resolved to the
> default
> > > user config. In other words, --describe is like doing
> > > Admin#describeConfigs, whereas --resolve is like calling
> > > ClientQuotaCallback#quotaLimit, if that helps any.
> > >
> > > Thanks,
> > > Brian
> > >
> > >
> > >
> > > On Fri, Jan 24, 2020 at 3:19 PM Anna Povzner <an...@confluent.io>
> wrote:
> > >
> > > > Hi Brian,
> > > >
> > > > The KIP looks good!
> > > >
> > > > I have one clarification question regarding the distinction between
> > > > describe and resolve API. Suppose I set request quota for
> > > > /config/users/”user-1”/clients/"client-1" to 100 and request quota
> for
> > > > /config/users/”user-1” to 200. Is this correct that
> describeClientQuotas
> > > > called with /config/users/”user-1” would return two entries in the
> > > > response?
> > > >
> > > >    -
> > > >
> > > >    /config/users/”user-1”/clients/<client-1>, request quota type,
> value =
> > > >    100
> > > >
> > > >
> > > >    -
> > > >
> > > >    /config/users/”user-1”, request quota type, value = 200
> > > >
> > > >
> > > > While resolve API for entity "/config/users/”user-1” would return
> the quota
> > > > setting specifically for /config/users/”user-1”, which is 200 in
> this case.
> > > >
> > > > Is my understanding correct?
> > > >
> > > > Thanks,
> > > >
> > > > Anna
> > > >
> > > >
> > > > On Fri, Jan 24, 2020 at 11:32 AM Brian Byrne <bb...@confluent.io>
> wrote:
> > > >
> > > > > My apologies, Rajini. My hasty edits omitted a couple spots. I've
> done a
> > > > > more thorough scan and should have cleaned up (1) and (2).
> > > > >
> > > > > For (3), Longs are chosen because that's what the ConfigCommand
> currently
> > > > > uses, and because there's no floating point type in the RPC
> protocol.
> > > > Longs
> > > > > didn't seem to be an issue for bytes-per-second values, and
> > > > > request_percentage is normalized [0-100], but you're right in that
> the
> > > > > extensions might require this.
> > > > >
> > > > > To make Double compatible with the RPC protocol, we'd need to
> serialize
> > > > the
> > > > > value into a String, and then validate the value on the receiving
> end.
> > > > > Would that be acceptable?
> > > > >
> > > > > Thanks,
> > > > > Brian
> > > > >
> > > > > On Fri, Jan 24, 2020 at 11:07 AM Rajini Sivaram <
> rajinisivaram@gmail.com
> > > > >
> > > > > wrote:
> > > > >
> > > > > > 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 <bbyrne@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 <
> > > > > rajinisivaram@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 <
> bbyrne@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 <
> > > > > > > rajinisivaram@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 <
> > > > bbyrne@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 <
> > > > > > > jason@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 <
> > > > > > bbyrne@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 <
> > > > > > > > cmccabe@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 <
> > > > > > > > > > cmccabe@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
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
>

Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux

Posted by Mickael Maison <mi...@gmail.com>.

Hi,

Is anyone actively working on adding support for custom quotas callbacks?
We should really try to get this in soon as otherwise this KIP is not
usable in environments with custom quotas. It's unfortunate to have
the capability on the client-side but no support on brokers.

Thanks


On Sat, Jan 25, 2020 at 10:51 AM Colin McCabe <cm...@apache.org> wrote:
>
> Hi Brian,
>
> Thanks for the KIP.  I think it looks good and ready for a vote!
>
> One nitpick:  we try to avoid being too Java-specific when describing the protocol.  Rather than referencing java.lang.Double, it would be more helpful to say something equivalent, like "the float is serialized as a 64-bit IEEE 754 value, ordered as big-endian."  That way people who are developing clients in other languages can more easily understand the protocol.
>
> best,
> Colin
>
>
> On Fri, Jan 24, 2020, at 17:18, Brian Byrne wrote:
> > Thanks for reviewing, Anna,
> >
> > In the describe call, the idea is that the API will match the QuotaFilters,
> > which can be specified in two ways for your example (filter is a type and a
> > matching string):
> >
> > 1) (USER="user-1") returns both
> > 2) (USER="user-1", CLIENT_ID="") returns just /config/users/”user-1”
> >
> > [For (2), it may be better to permit 'null' to indicate "omitted" instead
> > of the empty string.]
> >
> > You're correct for the resolve case, although it may not be highlighting
> > the difference between them. Let's say you had the following:
> >
> > /config/users/<default>, value=100
> >
> > Then --describe with (USER="user-1") returns empty since there's no config
> > entries for /config/users/”user-1”/*, whereas --resolve with
> > (USER="user-1") returns (value=100) since the user resolved to the default
> > user config. In other words, --describe is like doing
> > Admin#describeConfigs, whereas --resolve is like calling
> > ClientQuotaCallback#quotaLimit, if that helps any.
> >
> > Thanks,
> > Brian
> >
> >
> >
> > On Fri, Jan 24, 2020 at 3:19 PM Anna Povzner <an...@confluent.io> wrote:
> >
> > > Hi Brian,
> > >
> > > The KIP looks good!
> > >
> > > I have one clarification question regarding the distinction between
> > > describe and resolve API. Suppose I set request quota for
> > > /config/users/”user-1”/clients/"client-1" to 100 and request quota for
> > > /config/users/”user-1” to 200. Is this correct that describeClientQuotas
> > > called with /config/users/”user-1” would return two entries in the
> > > response?
> > >
> > >    -
> > >
> > >    /config/users/”user-1”/clients/<client-1>, request quota type, value =
> > >    100
> > >
> > >
> > >    -
> > >
> > >    /config/users/”user-1”, request quota type, value = 200
> > >
> > >
> > > While resolve API for entity "/config/users/”user-1” would return the quota
> > > setting specifically for /config/users/”user-1”, which is 200 in this case.
> > >
> > > Is my understanding correct?
> > >
> > > Thanks,
> > >
> > > Anna
> > >
> > >
> > > On Fri, Jan 24, 2020 at 11:32 AM Brian Byrne <bb...@confluent.io> wrote:
> > >
> > > > My apologies, Rajini. My hasty edits omitted a couple spots. I've done a
> > > > more thorough scan and should have cleaned up (1) and (2).
> > > >
> > > > For (3), Longs are chosen because that's what the ConfigCommand currently
> > > > uses, and because there's no floating point type in the RPC protocol.
> > > Longs
> > > > didn't seem to be an issue for bytes-per-second values, and
> > > > request_percentage is normalized [0-100], but you're right in that the
> > > > extensions might require this.
> > > >
> > > > To make Double compatible with the RPC protocol, we'd need to serialize
> > > the
> > > > value into a String, and then validate the value on the receiving end.
> > > > Would that be acceptable?
> > > >
> > > > Thanks,
> > > > Brian
> > > >
> > > > On Fri, Jan 24, 2020 at 11:07 AM Rajini Sivaram <rajinisivaram@gmail.com
> > > >
> > > > wrote:
> > > >
> > > > > 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 <bb...@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 <
> > > > rajinisivaram@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 <bb...@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 <
> > > > > > rajinisivaram@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 <
> > > bbyrne@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 <
> > > > > > jason@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 <
> > > > > bbyrne@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 <
> > > > > > > cmccabe@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 <
> > > > > > > > > cmccabe@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
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >

Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux

Posted by Colin McCabe <cm...@apache.org>.

Hi Brian,

Thanks for the KIP.  I think it looks good and ready for a vote!

One nitpick:  we try to avoid being too Java-specific when describing the protocol.  Rather than referencing java.lang.Double, it would be more helpful to say something equivalent, like "the float is serialized as a 64-bit IEEE 754 value, ordered as big-endian."  That way people who are developing clients in other languages can more easily understand the protocol.

best,
Colin


On Fri, Jan 24, 2020, at 17:18, Brian Byrne wrote:
> Thanks for reviewing, Anna,
> 
> In the describe call, the idea is that the API will match the QuotaFilters,
> which can be specified in two ways for your example (filter is a type and a
> matching string):
> 
> 1) (USER="user-1") returns both
> 2) (USER="user-1", CLIENT_ID="") returns just /config/users/”user-1”
> 
> [For (2), it may be better to permit 'null' to indicate "omitted" instead
> of the empty string.]
> 
> You're correct for the resolve case, although it may not be highlighting
> the difference between them. Let's say you had the following:
> 
> /config/users/<default>, value=100
> 
> Then --describe with (USER="user-1") returns empty since there's no config
> entries for /config/users/”user-1”/*, whereas --resolve with
> (USER="user-1") returns (value=100) since the user resolved to the default
> user config. In other words, --describe is like doing
> Admin#describeConfigs, whereas --resolve is like calling
> ClientQuotaCallback#quotaLimit, if that helps any.
> 
> Thanks,
> Brian
> 
> 
> 
> On Fri, Jan 24, 2020 at 3:19 PM Anna Povzner <an...@confluent.io> wrote:
> 
> > Hi Brian,
> >
> > The KIP looks good!
> >
> > I have one clarification question regarding the distinction between
> > describe and resolve API. Suppose I set request quota for
> > /config/users/”user-1”/clients/"client-1" to 100 and request quota for
> > /config/users/”user-1” to 200. Is this correct that describeClientQuotas
> > called with /config/users/”user-1” would return two entries in the
> > response?
> >
> >    -
> >
> >    /config/users/”user-1”/clients/<client-1>, request quota type, value =
> >    100
> >
> >
> >    -
> >
> >    /config/users/”user-1”, request quota type, value = 200
> >
> >
> > While resolve API for entity "/config/users/”user-1” would return the quota
> > setting specifically for /config/users/”user-1”, which is 200 in this case.
> >
> > Is my understanding correct?
> >
> > Thanks,
> >
> > Anna
> >
> >
> > On Fri, Jan 24, 2020 at 11:32 AM Brian Byrne <bb...@confluent.io> wrote:
> >
> > > My apologies, Rajini. My hasty edits omitted a couple spots. I've done a
> > > more thorough scan and should have cleaned up (1) and (2).
> > >
> > > For (3), Longs are chosen because that's what the ConfigCommand currently
> > > uses, and because there's no floating point type in the RPC protocol.
> > Longs
> > > didn't seem to be an issue for bytes-per-second values, and
> > > request_percentage is normalized [0-100], but you're right in that the
> > > extensions might require this.
> > >
> > > To make Double compatible with the RPC protocol, we'd need to serialize
> > the
> > > value into a String, and then validate the value on the receiving end.
> > > Would that be acceptable?
> > >
> > > Thanks,
> > > Brian
> > >
> > > On Fri, Jan 24, 2020 at 11:07 AM Rajini Sivaram <rajinisivaram@gmail.com
> > >
> > > wrote:
> > >
> > > > 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 <bb...@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 <
> > > rajinisivaram@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 <bb...@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 <
> > > > > rajinisivaram@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 <
> > bbyrne@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 <
> > > > > jason@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 <
> > > > bbyrne@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 <
> > > > > > cmccabe@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 <
> > > > > > > > cmccabe@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
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux

Posted by Brian Byrne <bb...@confluent.io>.

Thanks for reviewing, Anna,

In the describe call, the idea is that the API will match the QuotaFilters,
which can be specified in two ways for your example (filter is a type and a
matching string):

1) (USER="user-1") returns both
2) (USER="user-1", CLIENT_ID="") returns just /config/users/”user-1”

[For (2), it may be better to permit 'null' to indicate "omitted" instead
of the empty string.]

You're correct for the resolve case, although it may not be highlighting
the difference between them. Let's say you had the following:

/config/users/<default>, value=100

Then --describe with (USER="user-1") returns empty since there's no config
entries for /config/users/”user-1”/*, whereas --resolve with
(USER="user-1") returns (value=100) since the user resolved to the default
user config. In other words, --describe is like doing
Admin#describeConfigs, whereas --resolve is like calling
ClientQuotaCallback#quotaLimit, if that helps any.

Thanks,
Brian



On Fri, Jan 24, 2020 at 3:19 PM Anna Povzner <an...@confluent.io> wrote:

> Hi Brian,
>
> The KIP looks good!
>
> I have one clarification question regarding the distinction between
> describe and resolve API. Suppose I set request quota for
> /config/users/”user-1”/clients/"client-1" to 100 and request quota for
> /config/users/”user-1” to 200. Is this correct that describeClientQuotas
> called with /config/users/”user-1” would return two entries in the
> response?
>
>    -
>
>    /config/users/”user-1”/clients/<client-1>, request quota type, value =
>    100
>
>
>    -
>
>    /config/users/”user-1”, request quota type, value = 200
>
>
> While resolve API for entity "/config/users/”user-1” would return the quota
> setting specifically for /config/users/”user-1”, which is 200 in this case.
>
> Is my understanding correct?
>
> Thanks,
>
> Anna
>
>
> On Fri, Jan 24, 2020 at 11:32 AM Brian Byrne <bb...@confluent.io> wrote:
>
> > My apologies, Rajini. My hasty edits omitted a couple spots. I've done a
> > more thorough scan and should have cleaned up (1) and (2).
> >
> > For (3), Longs are chosen because that's what the ConfigCommand currently
> > uses, and because there's no floating point type in the RPC protocol.
> Longs
> > didn't seem to be an issue for bytes-per-second values, and
> > request_percentage is normalized [0-100], but you're right in that the
> > extensions might require this.
> >
> > To make Double compatible with the RPC protocol, we'd need to serialize
> the
> > value into a String, and then validate the value on the receiving end.
> > Would that be acceptable?
> >
> > Thanks,
> > Brian
> >
> > On Fri, Jan 24, 2020 at 11:07 AM Rajini Sivaram <rajinisivaram@gmail.com
> >
> > wrote:
> >
> > > 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 <bb...@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 <
> > rajinisivaram@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 <bb...@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 <
> > > > rajinisivaram@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 <
> bbyrne@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 <
> > > > jason@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 <
> > > bbyrne@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 <
> > > > > cmccabe@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 <
> > > > > > > cmccabe@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
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux

Posted by Anna Povzner <an...@confluent.io>.

Hi Brian,

The KIP looks good!

I have one clarification question regarding the distinction between
describe and resolve API. Suppose I set request quota for
/config/users/”user-1”/clients/"client-1" to 100 and request quota for
/config/users/”user-1” to 200. Is this correct that describeClientQuotas
called with /config/users/”user-1” would return two entries in the response?

   -

   /config/users/”user-1”/clients/<client-1>, request quota type, value =
   100


   -

   /config/users/”user-1”, request quota type, value = 200


While resolve API for entity "/config/users/”user-1” would return the quota
setting specifically for /config/users/”user-1”, which is 200 in this case.

Is my understanding correct?

Thanks,

Anna


On Fri, Jan 24, 2020 at 11:32 AM Brian Byrne <bb...@confluent.io> wrote:

> My apologies, Rajini. My hasty edits omitted a couple spots. I've done a
> more thorough scan and should have cleaned up (1) and (2).
>
> For (3), Longs are chosen because that's what the ConfigCommand currently
> uses, and because there's no floating point type in the RPC protocol. Longs
> didn't seem to be an issue for bytes-per-second values, and
> request_percentage is normalized [0-100], but you're right in that the
> extensions might require this.
>
> To make Double compatible with the RPC protocol, we'd need to serialize the
> value into a String, and then validate the value on the receiving end.
> Would that be acceptable?
>
> Thanks,
> Brian
>
> On Fri, Jan 24, 2020 at 11:07 AM Rajini Sivaram <ra...@gmail.com>
> wrote:
>
> > 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 <bb...@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 <
> rajinisivaram@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 <bb...@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 <
> > > rajinisivaram@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 <bbyrne@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 <
> > > jason@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 <
> > bbyrne@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 <
> > > > cmccabe@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 <
> > > > > > cmccabe@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
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux

Posted by Brian Byrne <bb...@confluent.io>.

My apologies, Rajini. My hasty edits omitted a couple spots. I've done a
more thorough scan and should have cleaned up (1) and (2).

For (3), Longs are chosen because that's what the ConfigCommand currently
uses, and because there's no floating point type in the RPC protocol. Longs
didn't seem to be an issue for bytes-per-second values, and
request_percentage is normalized [0-100], but you're right in that the
extensions might require this.

To make Double compatible with the RPC protocol, we'd need to serialize the
value into a String, and then validate the value on the receiving end.
Would that be acceptable?

Thanks,
Brian

On Fri, Jan 24, 2020 at 11:07 AM Rajini Sivaram <ra...@gmail.com>
wrote:

> 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 <bb...@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 <ra...@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 <bb...@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 <
> > rajinisivaram@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 <bb...@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 <
> > jason@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 <
> bbyrne@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 <
> > > cmccabe@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 <
> > > > > cmccabe@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
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux

Posted by Rajini Sivaram <ra...@gmail.com>.

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 <bb...@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 <ra...@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 <bb...@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 <
> rajinisivaram@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 <bb...@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 <
> jason@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 <bbyrne@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 <
> > cmccabe@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 <
> > > > cmccabe@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
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux

Posted by Brian Byrne <bb...@confluent.io>.

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 <ra...@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 <bb...@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 <ra...@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 <bb...@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 <jason@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 <bb...@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 <
> cmccabe@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 <
> > > cmccabe@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
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux

Posted by Rajini Sivaram <ra...@gmail.com>.

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 <bb...@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 <ra...@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 <bb...@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 <bb...@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 <cm...@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 <
> > cmccabe@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
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux

Posted by Brian Byrne <bb...@confluent.io>.

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 <ra...@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 <bb...@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 <bb...@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 <cm...@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 <
> cmccabe@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
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux

Posted by Rajini Sivaram <ra...@gmail.com>.

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 <bb...@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 <bb...@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 <cm...@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 <cm...@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
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux

Posted by Brian Byrne <bb...@confluent.io>.

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 <bb...@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 <cm...@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 <cm...@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
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux

Posted by Jason Gustafson <ja...@confluent.io>.

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 <bb...@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 <cm...@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 <cm...@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
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux

Posted by Brian Byrne <bb...@confluent.io>.

Thanks Colin, I've updated the KIP with the relevant changes.

On Fri, Jan 17, 2020 at 10:17 AM Colin McCabe <cm...@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 <cm...@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
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux

Posted by Colin McCabe <cm...@apache.org>.

Hi Brian,

Thanks again for working on this.  It's looking good.

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.

On balance, I think we should leave in "units."  It could be useful for future-proofing.

Also, since there are other kinds of quotas not covered by this API, we should rename DescribeQuotas -> DescribeClientQuotas, AlterQuotas -> AlterClientQuotas, etc. etc.

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.

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.

best,
Colin


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 <cm...@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
> > >
> >
>

Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux

Posted by Brian Byrne <bb...@confluent.io>.

Hi Colin,

Your feedback is appreciated, thank you.

On Tue, Jan 14, 2020 at 11:34 AM Colin McCabe <cm...@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
> >
>

Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux

Posted by Colin McCabe <cm...@apache.org>.

Hi Brian,

Thanks for the KIP.

The KIP says:

 > As represented by the current ZK node structure, the order in which quotas 
 > are matched are as follows. Note <user> is a specified user principal, 
 > <client-id> is a specified client ID, and <default> is a special default 
 > user/client ID that matches to all users or clients IDs.

This is probably a nitpick, but it would be nice to specify that this list is in order of highest priority to lowest.

> --include-overrides: Whether to include overridden config entries.

Hmm.  Maybe --show-overridden or --include-overridden is a better flag name?

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.

Is QuotaKey#Units really needed?  It seems like perhaps QuotaKey#Type could imply the units used.

>    public DescribeQuotasResult(KafkaFuture<Map<QuotaEntity, Map<QuotaKey, Long>>> entities);

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?

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...

cheers,
Colin

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
>