stmtdiagnostics: remove conditional request from registry after completion #89131
Conversation
|
This change is |
yuzefovich
force-pushed
the
stmt-diag-remove
branch
2 times, most recently
from
3dc64fe to
6cbd7c5
Compare
| expired := req.isExpired(timeutil.Now()) | ||
| shouldRemove := expired | ||
| defer func() { | ||
| if shouldRemove { |
There was a problem hiding this comment.
[tiny nit] Make this if !shouldRemove { return } to outdent the block below.
| // the logic of RemoveOngoing. | ||
| // latency satisfies the request's condition. The request is automatically | ||
| // removed from the registry (unless the continuous collection is enabled and | ||
| // the request hasn't expired). |
There was a problem hiding this comment.
This inlining of logic (before this PR) reads confusingly. Can it be pulled out into the one caller instead?
| func (r *Registry) IsExecLatencyConditionMet( | ||
| requestID RequestID, req Request, execLatency time.Duration, | ||
| ) bool { | ||
| expired := req.isExpired(timeutil.Now()) |
There was a problem hiding this comment.
[tiny nit] Skip this holdover variable expired, it's used just once below.
| // continueCollecting returns true if we want to continue collecting bundles for | ||
| // this request. Notably it doesn't check whether the request has expired. | ||
| func (r *Request) continueCollecting(st *cluster.Settings) bool { | ||
| return collectUntilExpiration.Get(&st.SV) && r.samplingProbability != 0 && !r.expiresAt.IsZero() |
There was a problem hiding this comment.
I'm to blame for this unfortunate leakage. Is there a way to assert closer to where samplingProbability is set that expiresAt is non-zero? (Or is that already done?)) That way this validation code doesn't have to leak elsewhere.
yuzefovich
force-pushed
the
stmt-diag-remove
branch
from
6cbd7c5 to
e97beb5
Compare
…etion Previously, we had a minor bug in how we handle the conditional diagnostics requests when we got a bundle that satisfied the condition - we correctly updated the corresponding system table, but we forgot to remove the request from the local registry. As a result, we would continue collecting conditional bundles until the local node polls the system table and updates its registry (every 10 seconds by default). This commit fixes that issue. Additionally, this commit updates the tests to enforce that the in-memory registry doesn't contain completed requests. Release note: None
yuzefovich
force-pushed
the
stmt-diag-remove
branch
from
e97beb5 to
bfbef33
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @irfansharif)
pkg/sql/stmtdiagnostics/statement_diagnostics.go line 134 at r1 (raw file):
Previously, irfansharif (irfan sharif) wrote…
I'm to blame for this unfortunate leakage. Is there a way to assert closer to where samplingProbability is set that expiresAt is non-zero? (Or is that already done?)) That way this validation code doesn't have to leak elsewhere.
Actually the leakage was pre-existing - it was introduced when we added the conditional diagnostics about a year ago.
pkg/sql/stmtdiagnostics/statement_diagnostics.go line 422 at r1 (raw file):
Previously, irfansharif (irfan sharif) wrote…
This inlining of logic (before this PR) reads confusingly. Can it be pulled out into the one caller instead?
Done.
pkg/sql/stmtdiagnostics/statement_diagnostics.go line 426 at r1 (raw file):
Previously, irfansharif (irfan sharif) wrote…
[tiny nit] Skip this holdover variable
expired, it's used just once below.
Done.
pkg/sql/stmtdiagnostics/statement_diagnostics.go line 429 at r1 (raw file):
Previously, irfansharif (irfan sharif) wrote…
[tiny nit] Make this
if !shouldRemove { return }to outdent the block below.
Refactored.
|
TFTR! bors r+ |
|
Build succeeded: |
Previously, we had a minor bug in how we handle the conditional
diagnostics requests when we got a bundle that satisfied the condition - we
correctly updated the corresponding system table, but we forgot to remove
the request from the local registry. As a result, we would continue
collecting conditional bundles until the local node polls the system
table and updates its registry (every 10 seconds by default). This
commit fixes that issue. Additionally, this commit updates the tests to
enforce that the in-memory registry doesn't contain completed requests.
Release note: None