sql,stmtdiagnostics: fix startup bug, extract subpackage for stmtdiagnostics #46411
Conversation
|
This change is |
bba9d65 to
3b8fafd
Compare
…startup The construction of the registry was kicking off polling too soon in the case of a pre-existing gossip message. This commit also unhooks the polling from the connExecutor in anticipation of pulling the registry into a subpackage. It also lays the groundwork to move the registry into a subpackage. Fixes cockroachdb#46410. Release justification: bug fixes and low-risk updates to new functionality Release note: None
There was a problem hiding this comment.
Good catch, thanks for the fix.
LGTM
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @andreimatei, and @RaduBerinde)
pkg/server/server.go, line 893 at r1 (raw file):
s.internalExecutor = internalExecutor execCfg.InternalExecutor = internalExecutor // TODO(ajwerner): Do we definitely know our node ID yet? Should we take a
luckily I think the nodeID is not actually used, so let's get rid of it.
Otherwise, it's indeed not known at this point, I don't think.
pkg/server/server.go, line 895 at r1 (raw file):
// TODO(ajwerner): Do we definitely know our node ID yet? Should we take a // node id container instead? execCfg.StmtInfoRequestRegistry = sql.NewStmtDiagnosticsRequestRegistry(internalExecutor, s.db, s.gossip, s.NodeID())
long line :)
pkg/sql/exec_util.go, line 591 at r1 (raw file):
ProtectedTimestampProvider protectedts.Provider // StmtInfoRequestRegistry
sup with the comment?
pkg/sql/exec_util.go, line 602 at r2 (raw file):
// StmtDiagnosticsTracker is the interface into *stmtdiagnostics.Registry to // record statement diagnostics. type StmtDiagnosticsRecorder interface {
the comment name is wrong; the struggle is real. How about StmtDiagnoser
pkg/sql/statement_diagnostics.go, line 59 at r1 (raw file):
gossip *gossip.Gossip nodeID roachpb.NodeID gossipUpdateChan chan stmtDiagRequestID
pls add a comment to this
pkg/sql/statement_diagnostics.go, line 86 at r1 (raw file):
} func (r *stmtDiagnosticsRequestRegistry) run(ctx context.Context) {
I'd call this poll().
pkg/sql/statement_diagnostics.go, line 88 at r1 (raw file):
func (r *stmtDiagnosticsRequestRegistry) run(ctx context.Context) { var timer timeutil.Timer // TODO(ajwerner): Make this polling interval a cluster setting.
nit: my opinion is that a TODO like this doesn't serve any purpose; it's not telling me any interesting information. You're just washing you conscience (or, rather, mine), at the expense of noise :)
I haven't done it because doing it would mean intercepting events when it changed and acting on them.
This commit pulls the statement diagnostics infrastructure out of the sql package. This makes for a pretty clean set of boundaries. Release justification: bug fixes and low-risk updates to new functionality Release note: None
3b8fafd to
0b02cab
Compare
There was a problem hiding this comment.
Seems like maybe you missed some of the second commit?
Reviewable status:
pkg/server/server.go, line 893 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
luckily I think the
nodeIDis not actually used, so let's get rid of it.
Otherwise, it's indeed not known at this point, I don't think.
I did that in the next commit.
pkg/server/server.go, line 895 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
long line :)
Fixed in the second commit (at least, shortened to 92). Folded in the 3rd commit.
pkg/sql/exec_util.go, line 591 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
sup with the comment?
Cleaned up a tad? It was a bit stale. I need a comment on this field now that it's public. I do see that in the first commit it was incomplete.
pkg/sql/exec_util.go, line 602 at r2 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
the comment name is wrong; the struggle is real. How about
StmtDiagnoser
Meh on Diagnoser, it doesn't really diagnose anything. StmtDiagnosticsCollector I'd buy.
pkg/sql/statement_diagnostics.go, line 59 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
pls add a comment to this
Done.
pkg/sql/statement_diagnostics.go, line 86 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
I'd call this
poll().
Done.
pkg/sql/statement_diagnostics.go, line 88 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
nit: my opinion is that a TODO like this doesn't serve any purpose; it's not telling me any interesting information. You're just washing you conscience (or, rather, mine), at the expense of noise :)
I haven't done it because doing it would mean intercepting events when it changed and acting on them.
Ha I actually intended to fix it in this PR and left the TODO for me to come back to later. Done in the 3rd commit.
There was a problem hiding this comment.
LGTM
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
pkg/sql/stmtdiagnostics/statement_diagnostics.go, line 39 at r5 (raw file):
var stmtDiagnosticsPollingInterval = settings.RegisterDurationSetting( "sql.stmt_diagnostics.poll_interval", "rate at which the stmtdiagnostics.Registry polls for requests, set to non-positive to disable",
[nit] "set to zero to disable", what's the point in encouraging non-sensical usage
pkg/sql/stmtdiagnostics/statement_diagnostics_test.go, line 229 at r5 (raw file):
_, err := db.Exec("SET CLUSTER SETTING sql.stmt_diagnostics.poll_interval = '1ms'") require.NoError(t, err) waitForScans(3) // ensure several scans occurs
[nit] "occur"
pkg/sql/stmtdiagnostics/statement_diagnostics_test.go, line 229 at r5 (raw file):
_, err := db.Exec("SET CLUSTER SETTING sql.stmt_diagnostics.poll_interval = '1ms'") require.NoError(t, err) waitForScans(3) // ensure several scans occurs
I think this would still pass even if it didn't work, SucceedsSoon waits for 45s. Maybe make it 10.
This was previously a constant. Better to have some control. It's not a public setting so no release note. Release justification: bug fixes and low-risk updates to new functionality Release note: None
582453d to
4ef67a0
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/stmtdiagnostics/statement_diagnostics.go, line 39 at r5 (raw file):
Previously, RaduBerinde wrote…
[nit] "set to zero to disable", what's the point in encouraging non-sensical usage
Done.
pkg/sql/stmtdiagnostics/statement_diagnostics_test.go, line 229 at r5 (raw file):
Previously, RaduBerinde wrote…
I think this would still pass even if it didn't work, SucceedsSoon waits for 45s. Maybe make it 10.
Good point. Done
pkg/sql/stmtdiagnostics/statement_diagnostics_test.go, line 229 at r5 (raw file):
Previously, RaduBerinde wrote…
[nit] "occur"
Done.
|
TFTR! bors r+ |
Build succeeded |
This PR comes in 2 commits. The first is a critical bug fix for #46410 whereby queries could be issued before the server has started up. The second is aesthetic. The statement diagnostics registry deserves its own package; sql is a mess already, no need to make it worse.
Release justification: bug fixes and low-risk updates to new functionality