Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
116726: logictest: fix rare flake r=yuzefovich a=yuzefovich

This commit fixes a rare flake in `TestCCLLogic_crdb_internal` where the test ensures that a newly-created secondary tenant doesn't have a tenant capability by default, then grants that capability, and the in-memory cache is updated accordingly. The problem was that this test uses "can_admin_split" capability for the check, and the test was added before 879bd1c merged. That patch made it so that "can_admin_split" capability is granted by default, so it made the existing test racy - namely, the race is between the tenant capabilities watcher propagating the grant and the logic test framework querying the in-memory cache. The fix is simple - use a capability that is not granted by default ("can_view_node_info").

Fixes: cockroachdb#115028.

Release note: None

116730: kvstreamer: avoid an allocation on the hot path r=yuzefovich a=yuzefovich

This commit avoids an allocation in `logStatistics` call when we don't have verbose logging enabled (previously, we would always get an allocation in `humanizeutil.IBytes` call). Also this avoids a few atomic loads too. (Noticed this while looking at the CPU profile of one of the production clusters.)

Epic: None

Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>
  • Loading branch information
craig[bot] and yuzefovich committed Dec 19, 2023
3 parents 2228d63 + d2e63ee + 8a6e4e1 commit 6096da0
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 26 deletions.
17 changes: 9 additions & 8 deletions pkg/ccl/logictestccl/testdata/logic_test/crdb_internal
Original file line number Diff line number Diff line change
Expand Up @@ -188,21 +188,22 @@ statement ok
SELECT crdb_internal.create_tenant(5)

# Use retry because source data is eventually consistent.
query ITT colnames,retry
SELECT * FROM crdb_internal.node_tenant_capabilities_cache WHERE capability_name = 'can_admin_split'
query ITT colnames,retry,rowsort
SELECT * FROM crdb_internal.node_tenant_capabilities_cache WHERE capability_name = 'can_view_node_info'
----
tenant_id capability_name capability_value
1 can_admin_split true
tenant_id capability_name capability_value
1 can_view_node_info true
5 can_view_node_info false

statement ok
ALTER TENANT [5] GRANT CAPABILITY can_admin_split
ALTER TENANT [5] GRANT CAPABILITY can_view_node_info

# Use retry because source data is eventually consistent.
query ITT colnames,retry,rowsort
SELECT * FROM crdb_internal.node_tenant_capabilities_cache WHERE capability_name = 'can_admin_split'
SELECT * FROM crdb_internal.node_tenant_capabilities_cache WHERE capability_name = 'can_view_node_info'
----
tenant_id capability_name capability_value
1 can_admin_split true
5 can_admin_split true
1 can_view_node_info true
5 can_view_node_info true

subtest end
38 changes: 20 additions & 18 deletions pkg/kv/kvclient/kvstreamer/streamer.go
Original file line number Diff line number Diff line change
Expand Up @@ -932,24 +932,26 @@ func (w *workerCoordinator) mainLoop(ctx context.Context) {
// tracing span of the Streamer's user. Some time has been spent to figure it
// out but led to no success. This should be cleaned up.
func (w *workerCoordinator) logStatistics(ctx context.Context) {
avgResponseSize, _ := w.getAvgResponseSize()
log.VEventf(
ctx, 1,
"enqueueCalls=%d enqueuedRequests=%d enqueuedSingleRangeRequests=%d kvPairsRead=%d "+
"batchRequestsIssued=%d resumeBatchRequests=%d resumeSingleRangeRequests=%d "+
"numSpilledResults=%d emptyBatchResponses=%d droppedBatchResponses=%d avgResponseSize=%s",
w.s.enqueueCalls,
w.s.enqueuedRequests,
w.s.enqueuedSingleRangeRequests,
atomic.LoadInt64(w.s.atomics.kvPairsRead),
atomic.LoadInt64(w.s.atomics.batchRequestsIssued),
atomic.LoadInt64(&w.s.atomics.resumeBatchRequests),
atomic.LoadInt64(&w.s.atomics.resumeSingleRangeRequests),
w.s.results.numSpilledResults(),
atomic.LoadInt64(&w.s.atomics.emptyBatchResponses),
atomic.LoadInt64(&w.s.atomics.droppedBatchResponses),
humanizeutil.IBytes(avgResponseSize),
)
if log.ExpensiveLogEnabled(ctx, 1) {
avgResponseSize, _ := w.getAvgResponseSize()
log.Eventf(
ctx,
"enqueueCalls=%d enqueuedRequests=%d enqueuedSingleRangeRequests=%d kvPairsRead=%d "+
"batchRequestsIssued=%d resumeBatchRequests=%d resumeSingleRangeRequests=%d "+
"numSpilledResults=%d emptyBatchResponses=%d droppedBatchResponses=%d avgResponseSize=%s",
w.s.enqueueCalls,
w.s.enqueuedRequests,
w.s.enqueuedSingleRangeRequests,
atomic.LoadInt64(w.s.atomics.kvPairsRead),
atomic.LoadInt64(w.s.atomics.batchRequestsIssued),
atomic.LoadInt64(&w.s.atomics.resumeBatchRequests),
atomic.LoadInt64(&w.s.atomics.resumeSingleRangeRequests),
w.s.results.numSpilledResults(),
atomic.LoadInt64(&w.s.atomics.emptyBatchResponses),
atomic.LoadInt64(&w.s.atomics.droppedBatchResponses),
humanizeutil.IBytes(avgResponseSize),
)
}
}

// waitForRequests blocks until there is at least one request to be served.
Expand Down

0 comments on commit 6096da0

Please sign in to comment.