Prevent DEVICE_OVERLOAD during legacy sync and make remove spentExtraRecs field in favor of more robust checks#528
Conversation
|
🤖 Claude Code Review Status: Complete Current Review: This PR removes the Architecture Change: Key Changes:
Trade-offs:
Analysis: No critical issues found. The implementation appears sound with proper error handling and context management. The trade-off of read overhead for correctness is appropriate given the production failures this fixes. Note: Previous inline comments about |
…/legacy-unspend-context
…unspend->respend actions with contexts being cancelled from causing drift in this incremented value
oskarszoon
changed the title
| g, gCtx := errgroup.WithContext(context.Background()) // we don't want the tracing to be linked to these calls | ||
| util.SafeSetLimit(g, spendBatcherSize*spendBatcherConcurrency) // we limit the number of concurrent requests, to not overload Aerospike | ||
| g, gCtx := errgroup.WithContext(context.Background()) // we don't want the tracing to be linked to these calls | ||
| util.SafeSetLimit(g, spendBatcherSize*2) // keep enough goroutines in flight to feed the batcher without overwhelming Aerospike |
There was a problem hiding this comment.
Can we make this more dynamic? Why not just reduce the spendBatcherConcurrency?
| g, gCtx := errgroup.WithContext(context.Background()) // we don't want the tracing to be linked to these calls | ||
| util.SafeSetLimit(g, spendBatcherSize*spendBatcherConcurrency) // we limit the number of concurrent requests, to not overload Aerospike | ||
| g, gCtx := errgroup.WithContext(context.Background()) // we don't want the tracing to be linked to these calls | ||
| util.SafeSetLimit(g, spendBatcherSize*2) // keep enough goroutines in flight to feed the batcher without overwhelming Aerospike |
There was a problem hiding this comment.
Can we make this more dynamic? Why not just reduce the spendBatcherConcurrency?
stores/utxo/aerospike/un_spend.go
Outdated
| // - prometheusUtxoMapReset: Successful unspends | ||
| // - prometheusUtxoMapErrors: Failed operations | ||
| func (s *Store) unspendLua(ctx context.Context, spend *utxo.Spend) error { | ||
| func (s *Store) unspendLua(spend *utxo.Spend) error { |
There was a problem hiding this comment.
Shouldn't remove the context from the function call, even if it is not being used now, convention. Should add a listener that stops execution when the context cancels.
|
3 participants
No description provided.