Skip to content

Commit 3086264

Browse files
craig[bot]dtwenyihu6kev-cao
committed
151086: CLAUDE: reduce sycophancy r=dt a=dt Some users have reported including instructions such as these can help make claude stick to facts and merit better in its interactions. Release note: none. Epic: none. 151369: kvserver: add allocator sync r=sumeerbhola a=wenyihu6 Epic: CRDB-25222 Release note: none ---- **mmaintegration: add allocator_op.go and allocator_sync.go** This commit adds allocator_op.go and allocator_sync.go, introducing trackedAllocatorChange and basic coordination helpers for allocator sync. These helpers register changes before they are applied and post apply the effect on store pool after the change is applied. Note that allocator sync is not plumbed or used yet. Future commits would plumb it to replicate queue, lease queue, and mma store rebalancer to coordinate changes. --- **kvserver: plumb allocator sync** Previously, allocator sync files were added. This commit wires allocator sync into the lease queue, replicate queue, and mma store rebalancer. Each node has a single allocator sync instance responsible for coordinating with the corresponding mma allocator. Note that while the allocator_sync is now plumbed, they are not yet actively used. --- **kvserver: use allocator sync to update store pool** Previously, allocator sync was plumbed into the replicate queue, lease queue, and mma store rebalancer. This commit updates the lease and replicate queues to call allocator sync to register changes before they are applied, and again post apply to reflect load changes in the local store pool. For now, allocator sync is only used for updating the store pool. Future commits will extend this to coordinate with mma. --- **kvserver: register external changes with mma** Previously, allocator sync was called before and after applying changes, but only to update the store pool. This commit extends it to also register changes with the mma allocator. If mma is enabled at registration time, allocator sync tracks the change and informs mma during PostApply. If mma is disabled, it continues to only update the store pool. --- **mmaintegration: add MMAPreApply to allocator sync** Previously, allocator sync tracked external changes from the replicate and lease queues. This commit extends it to register mma driven changes as well. On PostApply, allocator sync now informs mma of the outcome. Note that it is not being used yet. Future commits will call into it from mma store rebalancer. --- **mmaintegration: integrate allocator sync to mma store rebalancer** Previously, mma PreApply was introduced to register mma changes with allocator sync. This commit integrates it into the mma store rebalancer. To handle failures that occur before a change is even registered (such as when a replica doesn’t exist), this commit also adds, MarkChangesAsFailed, in allocator sync. This function bypasses the trackedChanges map since the change is not meant to be applied. It must only be called on failure, and allocator sync is responsible for notifying MMA of the failure. --- **mmaintegration: add store load msg** This commit adds MakeStoreLoadMsg, moved from allocator_mma_integration.go in mmaprototypehelpers. Eventually, all functions from mmaprototypehelpers will be migrated to pkg/kv/kvserver/mmaintegration as part of productionizing the code. Note that no tests have been added yet - a TODO has been left in place to address this. Added to the spreadsheet as well. --- **mmaintegration: add InvalidSyncChangeID** This commit adds InvalidSyncChangeID to allocator sync, used only by asim to indicate that no change is currently in progress. This is needed because asim returns control to the tick loop rather than waiting for changes to complete during simulations. So it needs to track the current change id in progress. --- **mmaintegration: remove pkg/kv/kvserver/allocator/mmaprototypehelpers** Previously, all helper functions from pkg/kv/kvserver/allocator/mmaprototypehelpers were migrated to pkg/kv/kvserver/mmaintegration. This commit completes the transition by removing all usages of mmaprototypehelpers to pkg/kv/kvserver/mmaintegration. --- **mmaintegration: address minor code review comments** This commit address minor code review comments. --- **kvserver: update allocator sync on errors in AdminTransferLease** Previously, the replicate queue could skip updating allocator sync when rlm.AdminTransferLease failed, due to early returns on error. This commit ensures allocator sync is always updated before exiting the function, regardless of success or failure. --- **mmaintegration: add interface for store pool and mma** This commit integrates the store pool and MMA allocator into allocator sync via an interface to facilitate testing within this package. --- **mmaintegration: rename mmaAllocator to mmaState** This commit cleans up a comment and renames the mmaAllocator interface to mmaState in the mmaintegration package to avoid naming confusion as mmaAllocator repeats the word allocator. 151420: backup: handle final flush errors when compacting backups r=jeffswenson a=kev-cao When processing spans for compaction, we always perform a final flush of the SSTKeyWriter via `defer`. However, we currently do not propagate that error anywhere and it is swallowed after it is logged. This updates the compaction processor to properly handle `Flush` errors. Epic: None Release note: None Co-authored-by: David Taylor <[email protected]> Co-authored-by: wenyihu6 <[email protected]> Co-authored-by: Kevin Cao <[email protected]>
4 parents caa221d + 9a3942e + 07f90f6 + 076aca5 commit 3086264

24 files changed

+526
-496
lines changed

CLAUDE.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,3 +139,13 @@ Always run `./dev generate` after modifying `.proto` files, SQL grammar, or opti
139139
- Prefix the subject line with the package in which the bulk of the changes occur.
140140
- For multi-commit PRs, summarize each commit in the PR record.
141141
- Do not include a test plan unless explicitly asked by the user.
142+
143+
# Interaction Style
144+
145+
* Be direct and honest.
146+
* Skip unnecessary acknowledgments.
147+
* Correct me when I'm wrong and explain why.
148+
* Suggest better alternatives if my ideas can be improved.
149+
* Focus on accuracy and efficiency.
150+
* Challenge my assumptions when needed.
151+
* Prioritize quality information and directness.

pkg/BUILD.bazel

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1462,7 +1462,6 @@ GO_TARGETS = [
14621462
"//pkg/kv/kvserver/allocator/load:load_test",
14631463
"//pkg/kv/kvserver/allocator/mmaprototype:mmaprototype",
14641464
"//pkg/kv/kvserver/allocator/mmaprototype:mmaprototype_test",
1465-
"//pkg/kv/kvserver/allocator/mmaprototypehelpers:mmaprototypehelpers",
14661465
"//pkg/kv/kvserver/allocator/plan:plan",
14671466
"//pkg/kv/kvserver/allocator/plan:plan_test",
14681467
"//pkg/kv/kvserver/allocator/storepool:storepool",
@@ -1557,6 +1556,7 @@ GO_TARGETS = [
15571556
"//pkg/kv/kvserver/loqrecovery/loqrecoverypb:loqrecoverypb_test",
15581557
"//pkg/kv/kvserver/loqrecovery:loqrecovery",
15591558
"//pkg/kv/kvserver/loqrecovery:loqrecovery_test",
1559+
"//pkg/kv/kvserver/mmaintegration:mmaintegration",
15601560
"//pkg/kv/kvserver/multiqueue:multiqueue",
15611561
"//pkg/kv/kvserver/multiqueue:multiqueue_test",
15621562
"//pkg/kv/kvserver/print:print",

pkg/backup/compaction_processor.go

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@ func (p *compactBackupsProcessor) processSpanEntries(
240240
entryCh chan execinfrapb.RestoreSpanEntry,
241241
encryption *jobspb.BackupEncryptionOptions,
242242
store cloud.ExternalStorage,
243-
) error {
243+
) (err error) {
244244
var fileEncryption *kvpb.FileEncryptionOptions
245245
if encryption != nil {
246246
fileEncryption = &kvpb.FileEncryptionOptions{Key: encryption.Key}
@@ -256,12 +256,7 @@ func (p *compactBackupsProcessor) processSpanEntries(
256256
if err != nil {
257257
return err
258258
}
259-
defer func() {
260-
if err := sink.Flush(ctx); err != nil {
261-
log.Warningf(ctx, "failed to flush sink: %v", err)
262-
logClose(ctx, sink, "SST sink")
263-
}
264-
}()
259+
defer logClose(ctx, sink, "failed to close sst sink")
265260
pacer := newBackupPacer(
266261
ctx, p.FlowCtx.Cfg.AdmissionPacerFactory, p.FlowCtx.Cfg.Settings,
267262
)
@@ -274,7 +269,7 @@ func (p *compactBackupsProcessor) processSpanEntries(
274269
return ctx.Err()
275270
case entry, ok := <-entryCh:
276271
if !ok {
277-
return nil
272+
return sink.Flush(ctx)
278273
}
279274
if assigned, err := p.isAssignedEntry(entry); err != nil {
280275
return err

pkg/kv/kvserver/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,7 @@ go_library(
168168
"//pkg/kv/kvserver/load",
169169
"//pkg/kv/kvserver/lockspanset",
170170
"//pkg/kv/kvserver/logstore",
171+
"//pkg/kv/kvserver/mmaintegration",
171172
"//pkg/kv/kvserver/multiqueue",
172173
"//pkg/kv/kvserver/print",
173174
"//pkg/kv/kvserver/raftentry",

0 commit comments

Comments
 (0)