Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pusher: retry shallow receipts #2049

Merged
merged 1 commit into from
Jun 15, 2021
Merged

pusher: retry shallow receipts #2049

merged 1 commit into from
Jun 15, 2021

Conversation

acud
Copy link
Member

@acud acud commented Jun 10, 2021

This change makes pusher push chunks for which we've received shallow receipts multiple times to try and get them stored deeper. This is a naive implementation that can be elaborate later on to attempt different traces somehow.

unit tests still TODO


This change is Reviewable

Copy link
Contributor

@mrekucci mrekucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @acud)


pkg/pusher/pusher.go, line 161 at r1 (raw file):

						po := swarm.Proximity(ch.Address().Bytes(), storerPeer.Bytes())
						s.metrics.ReceiptDepth.WithLabelValues(strconv.Itoa(int(po))).Inc()
						delete(retryCounter, ch.Address().ByteString())

Shouldn't be the delete operation also guarded with the mtx mutex?

Copy link
Contributor

@mrekucci mrekucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @acud)

@acud acud force-pushed the pusher-retry branch 2 times, most recently from 445b47d to 5505e00 Compare June 11, 2021 13:15
pkg/pusher/pusher.go Outdated Show resolved Hide resolved
d := s.depther.NeighborhoodDepth()
if po < d {
mtx.Lock()
retryCounter[ch.Address().ByteString()]++
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

increment after the check with the counter. Otherwise you will effectively only retry retryCount-1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we already retried, this is the correct thing to do. you can double check

pkg/pusher/pusher.go Outdated Show resolved Hide resolved
@acud acud force-pushed the pusher-retry branch 5 times, most recently from 6800168 to bcb5f44 Compare June 14, 2021 11:08
@acud acud force-pushed the pusher-retry branch 2 times, most recently from c14b840 to ec483b9 Compare June 14, 2021 13:51
@janos
Copy link
Member

janos commented Jun 14, 2021

This PR somehow revealed a data race on the logger that is shared between goroutines https://github.com/ethersphere/bee/pull/2049/checks?check_run_id=2820412618#step:10:5875.

@acud acud force-pushed the pusher-retry branch 2 times, most recently from 94d4c86 to dd759ce Compare June 15, 2021 06:57
@acud acud merged commit 330ee55 into master Jun 15, 2021
@acud acud deleted the pusher-retry branch June 15, 2021 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants