Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kvserver: Treat requests that were not evaluated specially in the span passed to the load-based splitter #91723

Closed
KaiSun314 opened this issue Nov 11, 2022 · 2 comments · Fixed by #91462
Labels
A-kv-distribution Relating to rebalancing and leasing. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team

Comments

@KaiSun314
Copy link
Contributor

KaiSun314 commented Nov 11, 2022

Currently, if we have a batch of requests, it is possible only a subset of requests are actually evaluated (e.g. if MaxSpanRequestKeys or TargetBytes were hit), meaning a subset of requests are not evaluated. However, the span that we record in the load splitter for this batch is the union of the true spans (calculated using the request and response's resumeSpan data) for all the requests in the batch, even the requests that were not evaluated.

There are 3 cases, at least according to collectSpansRead, where a resume span is not evaluated:

  1. Get && resumeSpan != nil
  2. Scan && reqHeader == resumeSpan
  3. Reverse scan && reqHeader == resumeSpan

For instance, if we have a request ['a', 'e') with response resume span ['a', 'e') i.e. this request was not actually evaluated, then this request would still contribute ['a', 'a') to the span union.

If we also have a request 'z' in the batch with resumeSpan != nil i.e. this request was not evaluated either, then this request would still contribute 'z to the span union.

Altogether, we would record ['a', 'z'.next) for this batch in the load splitter despite the fact that none of the requests have actually evaluated.

Note that even before we used the response resume span to get the true span (when we just used the request data), we still had this issue as we would contribute all requests to the span union including requests that ended up not being evaluated.

Jira issue: CRDB-21398

@KaiSun314 KaiSun314 added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-kv-distribution Relating to rebalancing and leasing. T-kv KV Team labels Nov 11, 2022
@nvanbenschoten
Copy link
Member

@KaiSun314 I see that we have a TODO in #91462 to address this issue. Is there a reason not to address this issue in that PR?

I believe the code would look similar to the // The request did not evaluate cases in Replica.collectSpansRead.

@KaiSun314
Copy link
Contributor Author

KaiSun314 commented Nov 14, 2022

@nvanbenschoten I was debating if we should exclude requests that were not evaluated from the true span union, my two reasons for not doing it were:

  1. Requests that did not evaluate still count as a "query" when considering QPS (at least it did in the legacy load splitter)
  2. If all requests in a batch did not evaluate, we would have an empty true span union; I don't think we would run into this situation a lot but if we did, it might reduce the number of data points passed to the load splitter

Thinking about it some more and after reading how ResumeSpan and "request did not evaluate" are done in replica_evaluate.go, I think it would make more sense to exclude requests that were not evaluated from the true span union + QPS. I updated this in the PR #91462

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-kv-distribution Relating to rebalancing and leasing. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants