tracing: tenant redaction test uses tracing knobs#73821
tracing: tenant redaction test uses tracing knobs#73821craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
andreimatei
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @andreimatei and @dhartunian)
pkg/ccl/kvccl/kvtenantccl/tenant_trace_test.go, line 66 at r1 (raw file):
SQLExecutor: &sql.ExecutorTestingKnobs{ WithStatementTrace: func(trace tracing.Recording, stmt string) { if stmt == "CREATE TABLE kv(k STRING PRIMARY KEY, v STRING)" {
please factor out the stmt into a constant
pkg/ccl/kvccl/kvtenantccl/tenant_trace_test.go, line 74 at r1 (raw file):
}, } tc := serverutils.StartNewTestCluster(t, 1, args)
since you're here - how come this is a TestCluster with one node and node more simply a TestServer?
c7a7d25 to
1ddfb2f
Compare
dhartunian
left a comment
There was a problem hiding this comment.
Also discovered bug last night that was created in the backport that prevents the cluster setting update from getting picked up correctly. I changed this test to use the cluster setting instead of setting flag on tracer directly, and then will backport to this test change to release-21.2 which will motivate the fix.
Reviewable status:
pkg/ccl/kvccl/kvtenantccl/tenant_trace_test.go, line 66 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
please factor out the stmt into a constant
Done.
pkg/ccl/kvccl/kvtenantccl/tenant_trace_test.go, line 74 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
since you're here - how come this is a
TestClusterwith one node and node more simply aTestServer?
Done.
Previously, the tenant redaction test manually read back the results of traces in SQL. Since we have a testing knob for this purpose it makes sense to use it here and collect the traces in a more straightforward way. This test now also uses the `trace.redactability.enabled` cluster setting instead of setting the redactability flag on the tracer to test that the cluster setting is correctly connected. Release note: None
1ddfb2f to
815b9ee
Compare
|
bors r+ |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 815b9ee to blathers/backport-release-21.2-73821: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 21.2.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
This commit identifies and resolves a bug on this branch that misplaced the code that updated the `trace-redactability.enabled` cluster setting. The test change described below fails without this change. Includes backport from cockroachdb#73821: Previously, the tenant redaction test manually read back the results of traces in SQL. Since we have a testing knob for this purpose it makes sense to use it here and collect the traces in a more straightforward way. This test now also uses the `trace.redactability.enabled` cluster setting instead of setting the redactability flag on the tracer to test that the cluster setting is correctly connected. Resolves cockroachdb#73938 Release note: None
Previously, the tenant redaction test manually read back the results of
traces in SQL. Since we have a testing knob for this purpose it makes
sense to use it here and collect the traces in a more straightforward
way.
This test now also uses the
trace.redactability.enabledclustersetting instead of setting the redactability flag on the tracer to test
that the cluster setting is correctly connected.
Release note: None