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