feat: Retrial behavior in retrieval

[metacertain]

This pr attempts to mirror some of the push sync behaviour changes introduced in #1662 and to enable retrying until a chunk is attempted to be retrieved from at least one peer

---

This change is 

[janos]

👍

[aloknerurkar]

Reviewed 4 of 4 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @metacertain)

[ralph-pichler]

if a chunk is already retrieved with origin false, does that singleflight here not imply that we would never try with maxPeers=8 if a origin true call happens at the same time?

[janos]

Good point, the origin flag should be part of the singleflight key.

[Eknir]

Let's merge this after the upcoming release (0.6.2)

[aloknerurkar]

Reviewable status: 2 of 6 files reviewed, 2 unresolved discussions (waiting on @aloknerurkar, @metacertain, and @ralph-pichler)

---

pkg/retrieval/retrieval.go, line 111 at r2 (raw file):

	}

	v, err, _ := s.singleflight.Do(flightRoute, func() (interface{}, error) {

Problem with this singleFlight pattern is that the parent context cancellation does not cancel the singleFlight requests which are being shared, resulting in goroutine leaks. This was inspected during #1891 and we decided to use @janos 's lib which adds context semantics correctly to singleFlight. I would recommend using that here.

---

pkg/retrieval/retrieval.go, line 187 at r2 (raw file):

				requestAttempt++
				timeNow := time.Now().Unix()
				if timeNow > lastTime {

When will this not be true?

[metacertain]

When will this not be true?

if we are in the same second still
we wait 600 miliseconds only so we should get into the next second within 2 rounds of requestAttempts

[istae]

maybe we should stay consistent and have this as a variable at the top of the file

[istae]

This would always be true, no? What is this checking?

[metacertain]

if we are in the same second still
we wait 600 miliseconds only so we should get into the next second within 2 rounds of requestAttempts

[istae]

Ok, can we add a test case that covers the area starting at requestAttempt++ and down.

[istae]

I leave the judgement to you if this is necessary

[aloknerurkar]

Reviewed 1 of 3 files at r4.
Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on @aloknerurkar, @esadakar, and @ralph-pichler)

[aloknerurkar]

---

pkg/retrieval/retrieval.go, line 188 at r3 (raw file):

Previously, metacertain wrote…

if we are in the same second still
we wait 600 miliseconds only so we should get into the next second within 2 rounds of requestAttempts

It would be good if you can add a comment here explaining this.