Combine Quota functionality into the Check method.

[geeknoid]

Check is now expressed as a series of conditions to check.
It can check preconditions (what we've traditionally associated
with Check), and it can check/allocate quotas.

This approach lets us reduce the RPC overhead from 2 calls to 1 and lets Mixer
execute precondition checks and quota allocations in parallel.

THIS WILL BE A BREAKING API CHANGE.

[douglas-reid]

would desired_use_count be a better name here (to parallel the response of use_count)? or maybe just uses? amount feels very quota-centric.

[geeknoid]

Done.

[qiwzhang]

we need:

bool best_effort = 3;

[geeknoid]

Do we have use cases where best_effort=false applies?

[ZackButcher]

best_effort makes sense for quota, but not check. I already didn't love amount here since it's only used for half the call. If we want that extra info, why not:

message Condition {
  string name = 1;
  
  message QuotaParams {
    int64 amount = 1;
    bool best_effort = 2;
  }
  message CheckParams {
    // none are required today
  }

  oneof operation {
    QuotaParams quota = 2;
    CheckParams check = 3;
  }
}

[qiwzhang]

quota-prefetch will need it.

if prefetch is not enabled, best_effort = false. if prefetch is enabled, best_effort = true. users may or may not enable quota-prefetch.

[douglas-reid]

since this isn't a map of <name, condition> or similar, what is the expected behavior for multiple Conditions with the name of "precondition" with differing amounts? Does mixer take the first? the last? the minimum?

[geeknoid]

I made it a map, that way I don't need to answer you :-)

[douglas-reid]

for my understanding: are we getting rid of Quota altogether, or just for now? is the idea that resource quotas would use the unified Check() too?

[geeknoid]

Yes, I think that even resource quotas would eventually go through Check.

For the foreseeable future, I expect that the intermediated API will only support rate quotas, and resource quotas will only be exposed through the disintermediated API.

[douglas-reid]

maybe prepend valid_ here, since we no longer have the context of CachabilityInfo? so, valid_duration and valid_use_count ?

[geeknoid]

Done.

[qiwzhang]

If I understand correctly, this is an attempt to batch Check and Quota calls into one call.

If client doesn't have cache, this will save client one remote-call. But in the cost of making API looks complicated.

If client has cache for both Check and Quota, this batch will not save. The first call may be able to use it, with different expiration and granted_count, Check and Quota will make separate calls afterward.

But the API really looks complicated. Do we want to do that? Can we assume client always has cache? At least mixer client is going to have cache.

[qiwzhang]

I think status should be inside ConditionResult too? Each condition may have different status result. Server should not combine them, or client cache may not be able to use it.

[geeknoid]

My intent was to not support partial failures. If a precondition check fails, then we shouldn't allocate any quotas.

[qiwzhang]

How about precondition passed, but quota fails, how can client tell a failed status is from which condition? For just Check and Quota, if Quota failure has a fixed status code, we may be able to work around this problem. But semantic wise, a repeated conditions may means a lot of conditions to check, client like to know the result of each condition.

[geeknoid]

Updated per Doug's comments.

Wayne, you make a good point that an efficient cache will inherently reduce the RPC overhead of Check+Quota. However, the overhead will not be negligible and this new API would cut this overhead by almost 50%. Additionally, it's worth considering the behavior of the system when check and quota results aren't in the cache. In order to maintain correct quota accounting, the client will need to issue Check, wait for Check to return, and then issue Quota. This means that the latency for uncached accesses is in fact 2x the cost of a single API. Ouch. Combining these two operations together avoids this double wait.

Do you really feel the API is that complicated? CheckRequest and CheckResponse remain fairly small structure with narrow semantics.

[qiwzhang]

It is very difficult for mixer client to use this API if mixer client has check cache and quota prefetch.

They will have different cache expiration. it is very hard for one to wait for another so their remote calls can be batched into one call.

For easy implementation, I will just make two calls.

[qiwzhang]

we need:

bool best_effort = 3;

[geeknoid]

@qiwzhang In terms of usability, I think we need to discuss this in person. Previously you had to do:

1. Fill in CheckRequest
2. Make Check call
3. Wait for Check call to complete
4. Fill in QuotaRequest
5. Make Quota call
6. Repeat Update references to attributes.proto. #4 and Fix references to the other protos in service.proto. #5 for however many quotas there are
7. Wait for all Quota calls to complete

Now, you supply all the same information in a single CheckRequest and you get the same things back in response. It's actually simpler to use since there is no synchronization between two API calls.

[qiwzhang]

Your steps are without cache. With Check and Quota cache, the steps are:

1. call CheckCache(attributes, check_transport_cb, check_done_cb);
2. call QuotaCache(attiributes , quota_transport_cb, quota_done_cb);

check_transport_cb is making the actual remote call. it is cache to determine when to call it since they have different expiration for check and quota.

[ZackButcher]

Why did we decide to use number of words in the dictionary as the differentiator rather than something even marginally stronger like a checksum?

As I've said in other places, I think we need to plan for better handling of the dictionary. Why not mandate a version number for the dictionary and use that here instead?

[ZackButcher]

To close the loop, based on out of band conversations: the global dictionary is append only and does not allow deletions. Therefore number of words allows both client and server to fall back on a dictionary they agree on, by using only min(len(server.global_dict), len(client.global_dict)).

[ZackButcher]

best_effort makes sense for quota, but not check. I already didn't love amount here since it's only used for half the call. If we want that extra info, why not:

message Condition {
  string name = 1;
  
  message QuotaParams {
    int64 amount = 1;
    bool best_effort = 2;
  }
  message CheckParams {
    // none are required today
  }

  oneof operation {
    QuotaParams quota = 2;
    CheckParams check = 3;
  }
}

[geeknoid]

PTAL.

This uses a more general-purpose approach for conditions. Maps are gone, and instead simpler arrays are used instead. The best_effort functionality is back for quota.

This continues to have a single deduplication_id per request, as opposed to per condition. Similarly, this continues to have a single status result per response, instead of one per condition. I don't believe we can reasonably support partial failures of this API. As it is defined now, it's all or nothing, which is easier for consumers to deal with.

[ZackButcher]

LGTM

[ZackButcher]

To close the loop, based on out of band conversations: the global dictionary is append only and does not allow deletions. Therefore number of words allows both client and server to fall back on a dictionary they agree on, by using only min(len(server.global_dict), len(client.global_dict)).

[ZackButcher]

As I understand it, the long term goal is for Mixer to return the set of attributes it used to determine the result, and for Envoy to cache on those attributes. Should we include an Attributes value on the CheckResponse to use for that? Right now the comments on ConditionResult seem to indicate that all attributes sent to Mixer will be used to construct the cache key, which will result cache misses that could've been hits.

[geeknoid]

Yes, we'll need something like that. I'm deferring that design to a later PR.

[ZackButcher]

CheckRequest.conditions[i] corresponds to CheckResponse.results[i], given that the results have no name? We should make note of that in the comments (you allude to it but don't come out and say explicitly).

[qiwzhang]

count() and expiration() also applied to failed condition. Even a failed condition can be cached.
Is that correct?

[qiwzhang]

the confusion is: do I need to set it if quota is not used?

[geeknoid]

Yes, it needs to be set in all uses of the API. I'll probably add it to Report as well. This is just a way to ensure that mutating operations only occur once. It matters for quota, and it matters for thing like Report for billing events.

So whenever you send a message, just set the dedup id. If the RPC fails, you can retry the same message with the same dedup ID.

[qiwzhang]

if client has separate check cache and quota cache, it needs to know an error status from which condition? How can the client know which condition results in the error?

[qiwzhang]

count() and expiration() also applied to failed condition. Even a failed condition can be cached.
Is that correct?

[geeknoid]

OK, this is yet another variation of the API surface. It's simpler overall than before and supports the semantics Wayne needs for proper caching.

PTAL.

[ZackButcher]

Nit: typical pattern is to declare the new messages then use them, rather than using them then declaring all the messages at the end of the parent message.

I do like that this better represents the shape of the return (a single precondition, N quotas).

[qiwzhang]

should be granted_amount to match "amount" in the request.

[qiwzhang]

it should be 0 if best_effort = false, and amount is not enough. it can be less than "amount" if best_effort is true.

[qiwzhang]

type is int64 too to match "amount" type in the request.

[geeknoid]

Done

[geeknoid]

PTAL.