rls: add API for response caching.#15483
Conversation
Initial PR to address envoyproxy#14299. Signed-off-by: Harvey Tuch <htuch@google.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
CC @yanavlasov @hzxuzhonghu @envoyproxy/api-shepherds |
mattklein123
left a comment
There was a problem hiding this comment.
Thanks this LGTM with one small comment.
/wait
| // | ||
| // If quota is available, a RLS request will not be made and the quota will be reduced by 1. | ||
| // | ||
| // If there is not sufficient quota, a RLS request will be made. When the result is returned, |
There was a problem hiding this comment.
Doesn't this mean that every Nth request will have higher latency due to requiring a round-trip with the RLS server? It seems like we should try to set things up such that the client already has the next quota before it actually needs it.
One option here would be some sort of pre-fetching mechanism, where the client can fetch a new quota when the old quota reaches some minimum number of remaining requests.
Another option would be a streaming API, where the server could update the client's quota whenever it changes without the client needing to explicitly make a request. In fact, couldn't the rate limit quota just be another xDS resource type that the client subscribes to?
There was a problem hiding this comment.
I think the client can optionally make a preemptive RLS request before quota expires. So, this is actually at the discretion of implementation.
Signed-off-by: Harvey Tuch <htuch@google.com>
|
Is this ready for non-draft review? /wait-any |
Signed-off-by: Harvey Tuch <htuch@google.com>
mattklein123
left a comment
There was a problem hiding this comment.
Thanks LGTM with small clarification request.
/wait
| // have no cached entry. This will trigger a new RLS request. | ||
| // When the result is returned, a single unit will be consumed from the quota for all | ||
| // matching descriptors. |
There was a problem hiding this comment.
What if the new request doesn't get any additional quota? Can you clarify? Same in next item?
There was a problem hiding this comment.
@yanavlasov will take this PR over as he's going to be owning the Google Envoy RLS efforts here going forward.
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
Superseded by PR #16942 |
Initial PR to address #14299.
Signed-off-by: Harvey Tuch htuch@google.com