-
Notifications
You must be signed in to change notification settings - Fork 338
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
fix: delay concluding attempt in pushsync #2016
Conversation
pkg/pushsync/pushsync_test.go
Outdated
if triggerCount < 9 { | ||
triggerCount++ | ||
stream.Close() | ||
return errors.New("fmt") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a better error here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tag increment needs to be reverted to previous state
pkg/pushsync/pushsync.go
Outdated
@@ -470,6 +461,15 @@ func (ps *PushSync) pushPeer(ctx context.Context, peer swarm.Address, ch swarm.C | |||
return nil, true, err | |||
} | |||
|
|||
// if you manage to get a tag, just increment the respective counter | |||
t, err := ps.tagger.Get(ch.TagID()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not correct... you should increment the sent count once the chunk is sent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, it was a misunderstanding
pkg/pushsync/pushsync_test.go
Outdated
@@ -488,7 +488,7 @@ func TestPushChunkToNextClosest(t *testing.T) { | |||
if err != nil { | |||
t.Fatal(err) | |||
} | |||
if ta2.Get(tags.StateSent) != 2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems a bit wrong no? why is this change needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
restored as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @acud and @metacertain)
pkg/pushsync/pushsync_test.go, line 944 at r3 (raw file):
triggerCount := 0 var lock sync.Mutex
Does this lock suppose to guard just the triggerCount
variable? If so, I'd recommend to rename it to signal that those two are related.
9c237ed
to
d4e17c8
Compare
This PR intends to make forwarder behavior ensure that chunk is successfully written to at least one further peer before concluding attempt has been made
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)