Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux
Hi Brian, Thanks for the KIP. The KIP says: > As represented by the current ZK node structure, the order in which quotas > are matched are as follows. Note is a specified user principal, > is a specified client ID, and is a special default > user/client ID that matches to all users or clients IDs. This is probably a nitpick, but it would be nice to specify that this list is in order of highest priority to lowest. > --include-overrides: Whether to include overridden config entries. Hmm. Maybe --show-overridden or --include-overridden is a better flag name? 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. Is QuotaKey#Units really needed? It seems like perhaps QuotaKey#Type could imply the units used. >public DescribeQuotasResult(KafkaFuture Long>>> entities); 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? 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... cheers, Colin 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 >
Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux
Hi Colin,
Your feedback is appreciated, thank you.
On Tue, Jan 14, 2020 at 11:34 AM Colin McCabe 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
> >
>
Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux
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 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
Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux
Thanks Colin, I've updated the KIP with the relevant changes. On Fri, Jan 17, 2020 at 10:17 AM Colin McCabe 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 > 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 ju
Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux
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 wrote: > Thanks Colin, I've updated the KIP with the relevant changes. > > On Fri, Jan 17, 2020 at 10:17 AM Colin McCabe 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 > > 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. > > > > > > > >
Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux
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 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 wrote: > > > Thanks Colin, I've updated the KIP with the relevant changes. > > > > On Fri, Jan 17, 2020 at 10:17 AM Colin McCabe > 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
Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux
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 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 > 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 wrote: > > > > > Thanks Colin, I've updated the KIP with the relevant changes. > > > > > > On Fri, Jan 17, 2020 at 10:17 AM Colin McCabe > > 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 AC
Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux
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 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 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 > > 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 > wrote: > > > > > > > Thanks Colin, I've updated the KIP with the relevant changes. > > > > > > > > On Fri, Jan 17, 2020 at 10:17 AM Colin McCabe > > > 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 > > > > > underst
Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux
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 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 > 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 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 > > > 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 a
Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux
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 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 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 > > 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 > 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 > > > > > 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
Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux
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 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 > 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 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 > > 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
Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux
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 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 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 > > 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 > 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
Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux
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/, 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 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 > 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 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 > > 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 < > > > r
Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux
Thanks for reviewing, Anna, In the describe call, the idea is that the API will match the QuotaFilters, which can be specified in two ways for your example (filter is a type and a matching string): 1) (USER="user-1") returns both 2) (USER="user-1", CLIENT_ID="") returns just /config/users/”user-1” [For (2), it may be better to permit 'null' to indicate "omitted" instead of the empty string.] You're correct for the resolve case, although it may not be highlighting the difference between them. Let's say you had the following: /config/users/, value=100 Then --describe with (USER="user-1") returns empty since there's no config entries for /config/users/”user-1”/*, whereas --resolve with (USER="user-1") returns (value=100) since the user resolved to the default user config. In other words, --describe is like doing Admin#describeConfigs, whereas --resolve is like calling ClientQuotaCallback#quotaLimit, if that helps any. Thanks, Brian On Fri, Jan 24, 2020 at 3:19 PM Anna Povzner wrote: > 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/, 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 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 > > > 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 > 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 > > > wrote: > > > > > > > > > > > Thanks Rajini, > > > > > > > > > > > > 1) Good catch, fixed. > > > > > > > > > > > > 2) You're right. We'd need to extend > ClientQuotaCallback#quotaLimit > > > or > > > > > add > > > > > >
Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux
Hi Brian, Thanks for the KIP. I think it looks good and ready for a vote! One nitpick: we try to avoid being too Java-specific when describing the protocol. Rather than referencing java.lang.Double, it would be more helpful to say something equivalent, like "the float is serialized as a 64-bit IEEE 754 value, ordered as big-endian." That way people who are developing clients in other languages can more easily understand the protocol. best, Colin On Fri, Jan 24, 2020, at 17:18, Brian Byrne wrote: > Thanks for reviewing, Anna, > > In the describe call, the idea is that the API will match the QuotaFilters, > which can be specified in two ways for your example (filter is a type and a > matching string): > > 1) (USER="user-1") returns both > 2) (USER="user-1", CLIENT_ID="") returns just /config/users/”user-1” > > [For (2), it may be better to permit 'null' to indicate "omitted" instead > of the empty string.] > > You're correct for the resolve case, although it may not be highlighting > the difference between them. Let's say you had the following: > > /config/users/, value=100 > > Then --describe with (USER="user-1") returns empty since there's no config > entries for /config/users/”user-1”/*, whereas --resolve with > (USER="user-1") returns (value=100) since the user resolved to the default > user config. In other words, --describe is like doing > Admin#describeConfigs, whereas --resolve is like calling > ClientQuotaCallback#quotaLimit, if that helps any. > > Thanks, > Brian > > > > On Fri, Jan 24, 2020 at 3:19 PM Anna Povzner wrote: > > > 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/, 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 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 > > > > > 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 > > 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 > > >
Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux
Hi, Is anyone actively working on adding support for custom quotas callbacks? We should really try to get this in soon as otherwise this KIP is not usable in environments with custom quotas. It's unfortunate to have the capability on the client-side but no support on brokers. Thanks On Sat, Jan 25, 2020 at 10:51 AM Colin McCabe wrote: > > Hi Brian, > > Thanks for the KIP. I think it looks good and ready for a vote! > > One nitpick: we try to avoid being too Java-specific when describing the > protocol. Rather than referencing java.lang.Double, it would be more helpful > to say something equivalent, like "the float is serialized as a 64-bit IEEE > 754 value, ordered as big-endian." That way people who are developing > clients in other languages can more easily understand the protocol. > > best, > Colin > > > On Fri, Jan 24, 2020, at 17:18, Brian Byrne wrote: > > Thanks for reviewing, Anna, > > > > In the describe call, the idea is that the API will match the QuotaFilters, > > which can be specified in two ways for your example (filter is a type and a > > matching string): > > > > 1) (USER="user-1") returns both > > 2) (USER="user-1", CLIENT_ID="") returns just /config/users/”user-1” > > > > [For (2), it may be better to permit 'null' to indicate "omitted" instead > > of the empty string.] > > > > You're correct for the resolve case, although it may not be highlighting > > the difference between them. Let's say you had the following: > > > > /config/users/, value=100 > > > > Then --describe with (USER="user-1") returns empty since there's no config > > entries for /config/users/”user-1”/*, whereas --resolve with > > (USER="user-1") returns (value=100) since the user resolved to the default > > user config. In other words, --describe is like doing > > Admin#describeConfigs, whereas --resolve is like calling > > ClientQuotaCallback#quotaLimit, if that helps any. > > > > Thanks, > > Brian > > > > > > > > On Fri, Jan 24, 2020 at 3:19 PM Anna Povzner wrote: > > > > > 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/, 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 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 > > > > > > > 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 > > > 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
Re: [DISCUSS] KIP-546: Add quota-specific APIs to the Admin Client, redux
Hi Mickael, Unfortunately, it's not actively being worked on, but I welcome anyone to come forward. Otherwise it's on my list that I'll eventually get around to. Currently the logic replicates what was being done historically with the quota configuration management, and is therefore compatible with the custom ClientQuotaCallback plugins. To support custom entities, the ClientQuotaCallback would need to become more generic with respect to entity types as there's no way to signal an update to a custom type, so that's the major hangup. Either a new generic quota callback plugin must be created which can be a superset of the current one, or the current one needs to be extended. Sorry I don't have a definitive answer for when it'll be supported, but it's still on my radar. Brian On Wed, Aug 26, 2020 at 6:36 AM Mickael Maison wrote: > Hi, > > Is anyone actively working on adding support for custom quotas callbacks? > We should really try to get this in soon as otherwise this KIP is not > usable in environments with custom quotas. It's unfortunate to have > the capability on the client-side but no support on brokers. > > Thanks > > > On Sat, Jan 25, 2020 at 10:51 AM Colin McCabe wrote: > > > > Hi Brian, > > > > Thanks for the KIP. I think it looks good and ready for a vote! > > > > One nitpick: we try to avoid being too Java-specific when describing > the protocol. Rather than referencing java.lang.Double, it would be more > helpful to say something equivalent, like "the float is serialized as a > 64-bit IEEE 754 value, ordered as big-endian." That way people who are > developing clients in other languages can more easily understand the > protocol. > > > > best, > > Colin > > > > > > On Fri, Jan 24, 2020, at 17:18, Brian Byrne wrote: > > > Thanks for reviewing, Anna, > > > > > > In the describe call, the idea is that the API will match the > QuotaFilters, > > > which can be specified in two ways for your example (filter is a type > and a > > > matching string): > > > > > > 1) (USER="user-1") returns both > > > 2) (USER="user-1", CLIENT_ID="") returns just /config/users/”user-1” > > > > > > [For (2), it may be better to permit 'null' to indicate "omitted" > instead > > > of the empty string.] > > > > > > You're correct for the resolve case, although it may not be > highlighting > > > the difference between them. Let's say you had the following: > > > > > > /config/users/, value=100 > > > > > > Then --describe with (USER="user-1") returns empty since there's no > config > > > entries for /config/users/”user-1”/*, whereas --resolve with > > > (USER="user-1") returns (value=100) since the user resolved to the > default > > > user config. In other words, --describe is like doing > > > Admin#describeConfigs, whereas --resolve is like calling > > > ClientQuotaCallback#quotaLimit, if that helps any. > > > > > > Thanks, > > > Brian > > > > > > > > > > > > On Fri, Jan 24, 2020 at 3:19 PM Anna Povzner > wrote: > > > > > > > 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/, 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 > 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. > > >
