Hi Brian,
Thanks again for working on this. It's looking good.
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.
On balance, I think we should leave in "units." It could be useful for
future-proofing.
Also, since there are other kinds of quotas not covered by this API, we should
rename DescribeQuotas -> DescribeClientQuotas, AlterQuotas ->
AlterClientQuotas, etc. etc.
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.
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.
best,
Colin
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
> > >
> >
>
