fix: pusher attempt delete #2974
Conversation
|
I'm not entirely sure that this is desirable behavior. At some point we've turned on the skip list for successful pushes too, which should in theory result in (some) disjoint path traversal for repeated successful pushes of the same chunk. The key doesn't leak since it has a TTL and there are timely cleanups on the skip-list. |
@acud I think you were looking at a closed PR. This PR is related to the pusher package :) |
|
oops wrong pacakge. sorry you're right. i'll review again |
| if po < d && s.attempts.try(addr) { | ||
| s.metrics.ShallowReceiptDepth.WithLabelValues(strconv.Itoa(int(po))).Inc() | ||
| return fmt.Errorf("pusher: shallow receipt depth %d, want at least %d", po, d) | ||
| } | ||
| s.logger.Tracef("pusher: pushed chunk %s to node %s, receipt depth %d", addr, peer, po) | ||
| s.metrics.ReceiptDepth.WithLabelValues(strconv.Itoa(int(po))).Inc() | ||
| s.attempts.delete(addr) |
There was a problem hiding this comment.
since checking the receipt is not synchronized with setting the chunk as synced, this might result in the chunk being pushed again in the timeframe between the delete and the setting of the chunk. not critical but can result in a bit of redundant operations
Checklist
If a non-shallow receipt is received before the allowed 6 attempts, then with the current implementation, the key never gets deleted causing a leak.
Description
Motivation and context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):
This change is