fix: pullsync context cancellation #2562
Conversation
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @acud)
pkg/pullsync/metrics.go, line 13 at r1 (raw file):
type metrics struct { OfferCounter prometheus.Counter // number of chunks offered
I'd suggest to avoid the Counter suffix here and in the fields below since it is kind of implicit and we don't expect to have anything else in the metrics
pkg/pullsync/pullsync.go, line 521 at r1 (raw file):
_ = stream.Reset() if logMore { s.logger.Debugf("pullsync: peer %d failed cancelling %d: %v", p.Address.String(), c.Ruid, err)
The verb for sting (address) should be %s, not %d.
pkg/pullsync/pullsync.go, line 533 at r1 (raw file):
if logMore { s.logger.Debugf("pullsync: peer %d cancelling %d", p.Address.String(), c.Ruid)
Same here.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mrekucci)
pkg/pullsync/metrics.go, line 13 at r1 (raw file):
Previously, mrekucci wrote…
I'd suggest to avoid the
Counter suffixhere and in the fields below since it is kind of implicit and we don't expect to have anything else in the metrics
agree and done
pkg/pullsync/pullsync.go, line 521 at r1 (raw file):
Previously, mrekucci wrote…
The verb for sting (address) should be
%s, not%d.
Done.
pkg/pullsync/pullsync.go, line 533 at r1 (raw file):
Previously, mrekucci wrote…
Same here.
Done.
|
Kudos, SonarCloud Quality Gate passed!
|
Codecov Report
@@ Coverage Diff @@
## master #2562 +/- ##
=========================================
Coverage ? 63.77%
=========================================
Files ? 232
Lines ? 25797
Branches ? 0
=========================================
Hits ? 16451
Misses ? 7860
Partials ? 1486 Continue to review full report at Codecov.
|
| s.ruidMtx.Lock() | ||
| defer s.ruidMtx.Unlock() | ||
|
|
||
| if cancel, ok := s.ruidCtx[c.Ruid]; ok { | ||
| if cancel, ok := s.ruidCtx[p.Address.ByteString()][c.Ruid]; ok { |
There was a problem hiding this comment.
Still being a novice golang coder, does this handle if an unknown (s.ruidCtx[p.Address.ByteString()] is null) peer from asking us to cancel a [c.Ruid]? Or will the if try to access nil[c.Ruid]?
There was a problem hiding this comment.
https://play.golang.org/p/hm2GrMxoTJA I don't see a case where the peer address is unknown. If this happens we have a much bigger problem on our hands and I'd rather have this explicitly panic so that we know the problem is there. Otherwise as there's no pointer dereferencing here there's no chance of panicking
There was a problem hiding this comment.
Just echoing years of my C coding where you always validate the first map/subscript is not null before referencing the next level down.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @acud)
This PR changes the way we do ruid cancellations in pullsync in a way that limits the access to certain ruids only to the overlay address from which the requests came from.
This change is