You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Mickael Maison <mi...@gmail.com> on 2020/08/26 13:36:04 UTC
Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux
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 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
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
>