KIP-546 (1/2): Implements describeClientQuotas() and alterClientQuotas().

[bdbyrne]

Implements describeClientQuotas() and alterClientQuotas() to match current
functionality, i.e. extensible entity types and other new quotas features
like resolving quotas is not yet supported.

This is the minimal functionality necessary for KIP-500, and converts the
ConfigCommand to use the client quotas APIs. resolveClientQuotas() and the
corresponding client quotas command will be in a future PR.

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[cmccabe]

org.apache.kafka.common.quota needs its own entry in import-control.xml

[cmccabe]

    public final static String USER_DEFAULT = "<default>";
    public final static String CLIENT_ID_DEFAULT = "<default>";

I don't remember discussing reserving a special string for default values. It seems like treating default as null, and then maybe printing it out in a special way in the toString, would be clearer.

What is an "unspecified entity type"? Is it the same as a default?

    private static final String MATCH_SPECIFIED = "<specified>";

Seems like we don't need angle brackets here, since the quota filter type names are ours to choose.

[bdbyrne]

org.apache.kafka.common.quota needs its own entry in import-control.xml

Done.

    public final static String USER_DEFAULT = "<default>";
    public final static String CLIENT_ID_DEFAULT = "<default>";

I don't remember discussing reserving a special string for default values. It seems like treating default as null, and then maybe printing it out in a special way in the toString, would be clearer.

What is an "unspecified entity type"? Is it the same as a default?

    private static final String MATCH_SPECIFIED = "<specified>";

Seems like we don't need angle brackets here, since the quota filter type names are ours to choose.

The default string "" is currently defined in DynamicConfigManager$ConfigEntityType, and is used for storing the configs under ZK ("" is reserved for this purpose), so I chose to keep it the same. I can make it null, but chose to use "" to keep it consistent with the filters.

For filters, the rules are:

• null means don't match anything (user=null means the entity type must have no user specified)
• DEFAULT means match the default exactly (must contain user=)
• SPECIFIED means only match if the type is specified (user= means it matches entity with {user=foo} and {user=bar, client-id=cid}, but not {client-id=cid}).

It's a bit of a mess, but unfortunately necessary. Let me know if you prefer I name things differently.

[cmccabe]

Can we use Optional<String> for this? Using magic string values feels messy, and will leak into the API

[bdbyrne]

Done.

[cmccabe]

I'd really rather not see the double equals used with strings. I know Scala has special semantics for this, but it always looks wrong to people familiar with Java. Maybe test isEmpty instead?

[bdbyrne]

Done.

[cmccabe]

If it's not supported, it shouldn't silently return incorrect data. It should throw an UnsupportedVersionException

[bdbyrne]

Done.

[cmccabe]

Need to update this JavaDoc parameter to match the new name of the function parameter.

[bdbyrne]

Done.

[cmccabe]

I think we can have DescribeQuotasOptions contain a "filter mode" that determines if the supplied filters are treated as "AND" or as "OR". So maybe:

enum ClientQuotaFilterMode {
  OR
  AND
}

What do you think? I think this is clearer than the boolean we had there earlier.

I don't understand QuotaFilter#matchNone. The name suggests that it doesn't match anything, but that behavior would be useless, right? What was the intention for this?

How do I filter to see just quotas on the default user (or default client id, etc.)? If we're going to use a constant for this, it should be in QuotaFilter somewhere, I think. Or we could use a factory function, I guess.

[bdbyrne]

I think we can have DescribeQuotasOptions contain a "filter mode" that determines if the supplied filters are treated as "AND" or as "OR". So maybe:

enum ClientQuotaFilterMode {
  OR
  AND
}

What do you think? I think this is clearer than the boolean we had there earlier.

I don't understand QuotaFilter#matchNone. The name suggests that it doesn't match anything, but that behavior would be useless, right? What was the intention for this?

How do I filter to see just quotas on the default user (or default client id, etc.)? If we're going to use a constant for this, it should be in QuotaFilter somewhere, I think. Or we could use a factory function, I guess.

Perhaps think of matchNone as matchUnspecified. So maybe it should be either matchSpecified/matchUnspecified, or matchSome/matchNone. So {matchExact(user=foo), matchNone(client-id)} would return just {user=foo}, or alternatively {matchNone(client-id)} would return all users with no client ID provided.

To see quotas on default users, you'd do matchExact(user=QuoteEntity.USER_DEFAULT). I can move this to QuotaFilter, but the issue is that they're really specific to the entity type, i.e. not all entity types may contain a default value.

I don't believe AND/OR is able to handle unspecified types when they're present. OR seems useful within the same entity type, not so much across entity types. I'm not entirely sold on this approach, I'll give it some thought.

[cmccabe]

LGTM.

Thanks, @bdbyrne. I think this slightly modified API is a lot more intuitive. Can you update the KIP to reflect the small changes here, and post to the mailing list?