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