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 <bby...@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 <rajinisiva...@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 <bby...@confluent.io> wrote:
> >
> > > Thanks again, Rajini,
> > >
> > > Units will have to be implemented on a per-config basis, then. I've
> > removed
> > > all language reference to units and replaced QuotaKey -> String (config
> > > name). I've also renamed DescribeEffective -> Resolve, and replaced
> > --list
> > > with --describe, and --describe to --resolve to be consistent with the
> > > config command and clear about what functionality is "new".
> > >
> > > Thanks,
> > > Brian
> > >
> > > On Fri, Jan 24, 2020 at 2:27 AM Rajini Sivaram <
> rajinisiva...@gmail.com>
> > > wrote:
> > >
> > > > Hi Brian,
> > > >
> > > > Thanks for the responses.
> > > >
> > > > 4) Yes, agree that it would be simpler to leave units out of the
> > initial
> > > > design. We currently have units that are interpreted by the
> > configurable
> > > > callback. The default callback interprets the value as
> > > > per-broker-bytes-per-second and per-broker-percentage-cores. But
> > > callbacks
> > > > using partition-based throughput quotas for example would interpret
> the
> > > > value as cluster-wide-bytes-per-second. We could update callbacks to
> > work
> > > > with units, but as you said, it may be better to leave it out
> initially
> > > and
> > > > address later.
> > > >
> > > >
> > > >
> > > > On Thu, Jan 23, 2020 at 6:29 PM Brian Byrne <bby...@confluent.io>
> > wrote:
> > > >
> > > > > Thanks Rajini,
> > > > >
> > > > > 1) Good catch, fixed.
> > > > >
> > > > > 2) You're right. We'd need to extend ClientQuotaCallback#quotaLimit
> > or
> > > > add
> > > > > an alternate function. For the sake of an initial implementation,
> I'm
> > > > going
> > > > > to remove '--show-overridden', and a subsequent KIP will have to
> > > propose
> > > > an
> > > > > extents to ClientQuotaCallback to return more detailed information.
> > > > >
> > > > > 3) You're correct. I've removed the default.
> > > > >
> > > > > 4) The idea of the first iteration is be compatible with the
> existing
> > > > API,
> > > > > so no modification to start. The APIs should be kept consistent.
> If a
> > > > user
> > > > > wants to add custom functionality, say an entity type, they'll need
> > to
> > > > > update their ConfigEntityType any way, and the quotas APIs are
> meant
> > to
> > > > > handle that gracefully by accepting a String which can be
> propagated.
> > > > >
> > > > > The catch is 'units'. Part of the reason for having a default unit
> > was
> > > > for
> > > > > backwards compatibility, but maybe it's best to leave units out of
> > the
> > > > > initial design. This might lead to adding more configuration
> entries,
> > > but
> > > > > it's also the most flexible option. Thoughts?
> > > > >
> > > > > Thanks,
> > > > > Brian
> > > > >
> > > > >
> > > > > On Thu, Jan 23, 2020 at 4:57 AM Rajini Sivaram <
> > > rajinisiva...@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > Hi Brian,
> > > > > >
> > > > > > Thanks for the KIP. Looks good, hope we finally get this in!
> > > > > >
> > > > > > A few comments:
> > > > > >
> > > > > > 1) All the Admin interface methods seem to be using method names
> > > > starting
> > > > > > with upper-case letter, should be lower-case to be follow
> > > conventions.
> > > > > > 2) Effective quotas returns not only the actual effective quota,
> > but
> > > > also
> > > > > > overridden values. I don't think this works with custom quota
> > > > callbacks.
> > > > > > 3) KIP says that all quotas are currently bytes-per-second and we
> > > will
> > > > > use
> > > > > > RATE_BPS as the default. Request quotas are a percentage. So this
> > > > doesn't
> > > > > > quite work. We also need to consider how this works with custom
> > quota
> > > > > > callbacks. Can custom quota implementations define their own
> units?
> > > > > > 4) We seem to be defining a new set of quota-related classes e.g.
> > for
> > > > > quota
> > > > > > types, but we haven't considered what we do with the existing API
> > in
> > > > > > org.apache.kafka.server.quota. Should we keep these consistent?
> Are
> > > we
> > > > > > planning to deprecate some of those?
> > > > > >
> > > > > >
> > > > > > Regards,
> > > > > >
> > > > > > Rajini
> > > > > >
> > > > > >
> > > > > > On Wed, Jan 22, 2020 at 7:28 PM Brian Byrne <bby...@confluent.io
> >
> > > > wrote:
> > > > > >
> > > > > > > Hi Jason,
> > > > > > >
> > > > > > > I agree on (1). It was Colin's original suggestion, too, but he
> > had
> > > > > > changed
> > > > > > > his mind in preference for enums. Strings are the more generic
> > way
> > > > for
> > > > > > now,
> > > > > > > so hopefully Colin can share his thinking when he's back. The
> > > > > QuotaFilter
> > > > > > > usage was an error, this has been corrected.
> > > > > > >
> > > > > > > For (2), the config-centric mode is what we have today in
> > > > > CommandConfig:
> > > > > > > reading/altering the configuration as it's described. The
> > > > > > > DescribeEffectiveClientQuotas would be resolving the various
> > config
> > > > > > entries
> > > > > > > to see what actually applies to a particular entity. The
> examples
> > > > are a
> > > > > > > little trivial, but the resolution can become much more
> > complicated
> > > > as
> > > > > > the
> > > > > > > number of config entries grows.
> > > > > > >
> > > > > > > List/describe aren't perfect either. Perhaps describe/resolve
> > are a
> > > > > > better
> > > > > > > pair, with DescribeEffectiveClientQuotas ->
> ResolveClientQuotas?
> > > > > > >
> > > > > > > I appreciate the feedback!
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Brian
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On Tue, Jan 21, 2020 at 12:09 PM Jason Gustafson <
> > > ja...@confluent.io
> > > > >
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Hi Brian,
> > > > > > > >
> > > > > > > > Thanks for the proposal! I have a couple comments/questions:
> > > > > > > >
> > > > > > > > 1. I'm having a hard time understanding the point of
> > > > > > `QuotaEntity.Type`.
> > > > > > > It
> > > > > > > > sounds like this might be just for convenience since the APIs
> > are
> > > > > using
> > > > > > > > string types. If so, I think it's a bit misleading to
> represent
> > > it
> > > > as
> > > > > > an
> > > > > > > > enum. In particular, I cannot see how the UNKNOWN type would
> be
> > > > used.
> > > > > > The
> > > > > > > > `PrincipalBuilder` plugin allows users to provide their own
> > > > principal
> > > > > > > type,
> > > > > > > > so I think the API should be usable even for unknown entity
> > > types.
> > > > > Note
> > > > > > > > also that we appear to be relying on this enum in
> `QuotaFilter`
> > > > > class.
> > > > > > I
> > > > > > > > think that should be changed to just a string?
> > > > > > > >
> > > > > > > > 2. It's a little annoying that we have two separate APIs to
> > > > describe
> > > > > > > client
> > > > > > > > quotas. The names do not really make it clear which API
> someone
> > > > > should
> > > > > > > use.
> > > > > > > > It might just be a naming problem. In the command utility, it
> > > looks
> > > > > > like
> > > > > > > > you are using --list and --describe to distinguish the two.
> > > Perhaps
> > > > > the
> > > > > > > > APIs can be named similarly: e.g. ListClientQuotas and
> > > > > > > > DescribeClientQuotas. However, looking at the examples, it's
> > > still
> > > > > not
> > > > > > > very
> > > > > > > > clear to me why we need both options. Basically I'm finding
> the
> > > > > > > > "config-centric" mode not very intuitive.
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Jason
> > > > > > > >
> > > > > > > >
> > > > > > > > On Fri, Jan 17, 2020 at 2:14 PM Brian Byrne <
> > bby...@confluent.io
> > > >
> > > > > > wrote:
> > > > > > > >
> > > > > > > > > Thanks Colin, I've updated the KIP with the relevant
> changes.
> > > > > > > > >
> > > > > > > > > On Fri, Jan 17, 2020 at 10:17 AM Colin McCabe <
> > > > cmcc...@apache.org>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > I thought about this a little bit more, and maybe we can
> > > leave
> > > > in
> > > > > > the
> > > > > > > > > > enums rather than going with strings. But we need to
> have
> > an
> > > > > > > "UNKNOWN"
> > > > > > > > > > value for all the enums, so that if a value that the
> client
> > > > > doesn't
> > > > > > > > > > understand is returned, it can get translated to that.
> > This
> > > is
> > > > > > what
> > > > > > > we
> > > > > > > > > did
> > > > > > > > > > with the ACLs API, and it worked out well.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Done. One thing I omitted here was that the API still
> > > > > accepts/returns
> > > > > > > > > Strings, since there may be plugins that specify their own
> > > > > > types/units.
> > > > > > > > If
> > > > > > > > > we'd like to keep it this way, then the UNKNOWN may be
> > > > unnecessary.
> > > > > > Let
> > > > > > > > me
> > > > > > > > > know how you'd feel this is best resolved.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > On balance, I think we should leave in "units." It could
> > be
> > > > > useful
> > > > > > > for
> > > > > > > > > > future-proofing.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Done. Also added a comment in the ClientQuotaCommand to
> > default
> > > > to
> > > > > > > > RATE_BPS
> > > > > > > > > if no unit is supplied to ease adoption.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > Also, since there are other kinds of quotas not covered
> by
> > > this
> > > > > > API,
> > > > > > > we
> > > > > > > > > > should rename DescribeQuotas -> DescribeClientQuotas,
> > > > AlterQuotas
> > > > > > ->
> > > > > > > > > > AlterClientQuotas, etc. etc.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Done. Updated command and script name, too.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > Maybe QuotaFilter doesn't need a "rule" argument to its
> > > > > constructor
> > > > > > > > right
> > > > > > > > > > now. We can just do literal matching for everything.
> > Like I
> > > > > said
> > > > > > > > > earlier,
> > > > > > > > > > I don't think people do a lot of prefixing of principal
> > > names.
> > > > > > When
> > > > > > > we
> > > > > > > > > > added the "prefix matching" stuff for ACLs, it was mostly
> > to
> > > > let
> > > > > > > people
> > > > > > > > > do
> > > > > > > > > > it for topics. Then we made it more generic because it
> was
> > > > easy
> > > > > to
> > > > > > > do
> > > > > > > > > so.
> > > > > > > > > > In this case, the API is probably easier to understand if
> > we
> > > > just
> > > > > > do
> > > > > > > a
> > > > > > > > > > literal match. We can always have a follow-on KIP to add
> > > > fancier
> > > > > > > > > filtering
> > > > > > > > > > if needed.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Done.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > For DescribeEffectiveQuotasResult, if you request all
> > > relevant
> > > > > > > quotas,
> > > > > > > > it
> > > > > > > > > > would be nice to see which ones apply and which ones
> don't.
> > > > > Right
> > > > > > > now,
> > > > > > > > > you
> > > > > > > > > > just get a map, but you don't know which quotas are
> > actually
> > > in
> > > > > > > force,
> > > > > > > > > and
> > > > > > > > > > which are not relevant but might be in the future if a
> > > > different
> > > > > > > quota
> > > > > > > > > gets
> > > > > > > > > > deleted. One way to do this would be to have two maps--
> > one
> > > > for
> > > > > > > > > applicable
> > > > > > > > > > quotas and one for shadowed quotas.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > So the way it's specified is that it maps QuotaKey ->
> Value,
> > > > > however
> > > > > > > > Value
> > > > > > > > > is actually defined to have two parts: the entry, and a
> list
> > of
> > > > > > > > overridden
> > > > > > > > > entries (where an entry is the value, along with the
> source).
> > > > > Perhaps
> > > > > > > the
> > > > > > > > > Value is poorly named, or maybe there's a simpler structure
> > to
> > > be
> > > > > > had?
> > > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > > Brian
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > On Tue, Jan 14, 2020, at 13:32, Brian Byrne wrote:
> > > > > > > > > > > Hi Colin,
> > > > > > > > > > >
> > > > > > > > > > > Your feedback is appreciated, thank you.
> > > > > > > > > > >
> > > > > > > > > > > On Tue, Jan 14, 2020 at 11:34 AM Colin McCabe <
> > > > > > cmcc...@apache.org>
> > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > > This is probably a nitpick, but it would be nice to
> > > specify
> > > > > > that
> > > > > > > > this
> > > > > > > > > > list
> > > > > > > > > > > > is in order of highest priority to lowest.
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Done.
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > Hmm. Maybe --show-overridden or --include-overridden
> > is
> > > a
> > > > > > better
> > > > > > > > > flag
> > > > > > > > > > > > name?
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Done (--show-overridden).
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > I think it would be nice to avoid using enums for
> > > > > > > QuotaEntity#Type,
> > > > > > > > > > > > QuotaKey#Type, and QuotaFilter#Rule. With enums, we
> > have
> > > > to
> > > > > > > worry
> > > > > > > > > > about
> > > > > > > > > > > > forwards and backwards compatibility problems. For
> > > > example,
> > > > > > what
> > > > > > > > do
> > > > > > > > > > you do
> > > > > > > > > > > > when you're querying a broker that has a new value
> for
> > > one
> > > > of
> > > > > > > > these,
> > > > > > > > > > that
> > > > > > > > > > > > is not in your enum? In the past, we've created an
> > > > UNKNOWN
> > > > > > > value
> > > > > > > > > for
> > > > > > > > > > enum
> > > > > > > > > > > > types to solve this conundrum, but I'm not sure the
> > extra
> > > > > > > > complexity
> > > > > > > > > is
> > > > > > > > > > > > worth it here. We can jut make them strings and
> avoid
> > > > > worrying
> > > > > > > > about
> > > > > > > > > > the
> > > > > > > > > > > > compatibility issues.
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Makes sense. Done.
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > Is QuotaKey#Units really needed? It seems like
> perhaps
> > > > > > > > QuotaKey#Type
> > > > > > > > > > > > could imply the units used.
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Possibly, maybe. It depends on how much structure is
> > > useful,
> > > > > > which
> > > > > > > > > > > influences the implementation in the broker. For
> example,
> > > for
> > > > > the
> > > > > > > > > > existing
> > > > > > > > > > > (global) bytes-per-second types (e.g. consumer byte
> > rate),
> > > it
> > > > > may
> > > > > > > be
> > > > > > > > > > useful
> > > > > > > > > > > to define them on a per-broker BPS basis, and in some
> > > cases,
> > > > in
> > > > > > > terms
> > > > > > > > > of
> > > > > > > > > > > shares. The question becomes whether it'd be better to
> > > have a
> > > > > > > module
> > > > > > > > in
> > > > > > > > > > the
> > > > > > > > > > > broker that is capable of deducing the effective quota
> > > > > > > automatically
> > > > > > > > > > among
> > > > > > > > > > > different units for the same quota type, or whether
> each
> > > > should
> > > > > > be
> > > > > > > > > > handled
> > > > > > > > > > > individually.
> > > > > > > > > > >
> > > > > > > > > > > Given we don't expect many units, and some units may be
> > > > > > > incompatible
> > > > > > > > > with
> > > > > > > > > > > others, perhaps it is best to have the unit implicit in
> > the
> > > > > type
> > > > > > > > > string,
> > > > > > > > > > to
> > > > > > > > > > > be handled by the broker appropriately.
> > > > > > > > > > >
> > > > > > > > > > > I've updated the KIP to reflect this change, which
> > factors
> > > > out
> > > > > > the
> > > > > > > > > > > QuotaKey. Let me know your thoughts.
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > How common is the prefix matching use-case? I
> haven't
> > > > heard
> > > > > > > about
> > > > > > > > > > people
> > > > > > > > > > > > setting up principal names with a common prefix or
> > > anything
> > > > > > like
> > > > > > > > > > that-- is
> > > > > > > > > > > > that commonly done?
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > It was, in part, for exposition, but would handle a
> case
> > > > where
> > > > > > > > > principals
> > > > > > > > > > > could be prefixed by organization/team name, numbered,
> or
> > > the
> > > > > > like.
> > > > > > > > If
> > > > > > > > > > you
> > > > > > > > > > > prefer I remove the rules and just accept a pattern,
> > that's
> > > > > also
> > > > > > an
> > > > > > > > > > option.
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > I sort of feel like maybe we could have a simpler API
> > for
> > > > > > > > > > describeQuotas
> > > > > > > > > > > > where it takes a map of quota entity type to value,
> and
> > > we
> > > > > do a
> > > > > > > > > > logical AND
> > > > > > > > > > > > On that. I'm not sure if there's really a reason why
> > it
> > > > > needs
> > > > > > to
> > > > > > > > be
> > > > > > > > > a
> > > > > > > > > > > > collection rather than a set, in other words...
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > For clarification, are you suggesting an interface
> where
> > > the
> > > > > user
> > > > > > > > might
> > > > > > > > > > > provide {type=user, name=x} and it would return all
> > > entities
> > > > > that
> > > > > > > > > match,
> > > > > > > > > > > with their resulting quota values? Should I scrap
> pattern
> > > > > > matching
> > > > > > > > for
> > > > > > > > > > now,
> > > > > > > > > > > since it's a simple extension that can be done at a
> > future
> > > > > time?
> > > > > > > > > > >
> > > > > > > > > > > Thanks,
> > > > > > > > > > > Brian
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > On Wed, Dec 11, 2019, at 15:30, Brian Byrne wrote:
> > > > > > > > > > > > > Hello all,
> > > > > > > > > > > > >
> > > > > > > > > > > > > I'm reviving the discussion for adding a quotas API
> > to
> > > > the
> > > > > > > admin
> > > > > > > > > > client
> > > > > > > > > > > > by
> > > > > > > > > > > > > submitting a new proposal. There are some notable
> > > changes
> > > > > > from
> > > > > > > > > > previous
> > > > > > > > > > > > > attempts, namely a way to deduce the effective
> quota
> > > for
> > > > a
> > > > > > > client
> > > > > > > > > > > > (entity),
> > > > > > > > > > > > > a way to query for configured quotas, and the
> concept
> > > of
> > > > > > > "units"
> > > > > > > > on
> > > > > > > > > > > > quotas,
> > > > > > > > > > > > > among other minor updates.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Please take a look, and I'll be happy to make any
> > > > > > > clarifications
> > > > > > > > > and
> > > > > > > > > > > > > modifications in regards to feedback.
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-546%3A+Add+quota-specific+APIs+to+the+Admin+Client%2C+redux
> > > > > > > > > > > > >
> > > > > > > > > > > > > Thanks,
> > > > > > > > > > > > > Brian
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
