jobs: add a span to resumed jobs (for /debug/requests goodness)#27422
jobs: add a span to resumed jobs (for /debug/requests goodness)#27422craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
benesch
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (and 1 stale)
pkg/sql/jobs/registry.go, line 23 at r1 (raw file):
"time" "github.com/cockroachdb/cockroach/pkg/settings"
If you happen to go through CI again, the pkg/settings import should be in the next block.
Release note: None
danhhz
left a comment
There was a problem hiding this comment.
Thanks for the review!
bors r=benesch
Reviewable status:
pkg/sql/jobs/registry.go, line 23 at r1 (raw file):
Previously, benesch (Nikhil Benesch) wrote…
If you happen to go through CI again, the pkg/settings import should be in the next block.
Done.
benesch
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status:
pkg/sql/jobs/registry_external_test.go, line 89 at r2 (raw file):
const adoptInterval = time.Nanosecond ac := log.AmbientContext{Tracer: tracing.NewTracer()}
Doesn't this need to be r.settings.Tracer? At least, it is in every other non-test AmbientContext. I'm not familiar with tracing at all though to know exactly what might go wrong if you create a fresh tracer.
Build succeeded |
danhhz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/jobs/registry_external_test.go, line 89 at r2 (raw file):
Previously, benesch (Nikhil Benesch) wrote…
Doesn't this need to be
r.settings.Tracer? At least, it is in every other non-test AmbientContext. I'm not familiar with tracing at all though to know exactly what might go wrong if you create a fresh tracer.
Argh, sorry. I'd kicked off bors and didn't see this before it merged. There's quite a bit of precedent for log.AmbientContext{Tracer: tracing.NewTracer()} in tests and I only added this to fix a panic.
benesch
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/jobs/registry_external_test.go, line 89 at r2 (raw file):
Previously, danhhz (Daniel Harrison) wrote…
Argh, sorry. I'd kicked off bors and didn't see this before it merged. There's quite a bit of precedent for
log.AmbientContext{Tracer: tracing.NewTracer()}in tests and I only added this to fix a panic.
No worries! It didn't seem like a serious enough problem to cancel your merge! I'm not even sure if it's a problem at all.
Release note: None