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