-
Notifications
You must be signed in to change notification settings - Fork 337
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
feat: node warmup time for pull/pushsync protocols #2050
Conversation
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.
Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @esadakar and @zelig)
pkg/node/node.go, line 182 at r1 (raw file):
warmupTime = time.Hour }
maybe we should move time.Hour
to a const maxWarmupTime
and return an error here?
pkg/puller/puller.go, line 76 at r1 (raw file):
bins: bins, warmupTime: warmupTime,
no need for the extra field, you can simply call go p.manage(warmupTime)
pkg/pusher/pusher.go, line 64 at r1 (raw file):
quit: make(chan struct{}), chunksWorkerQuitC: make(chan struct{}), warmupTime: warmupTime,
here too
pkg/pushsync/pushsync.go, line 48 at r1 (raw file):
ErrOutOfDepthReplication = errors.New("replication outside of the neighborhood") ErrNoPush = errors.New("could not push chunk") ErrWarmup = errors.New("could not push chunk")
error description should be changed
pkg/pushsync/pushsync.go, line 205 at r1 (raw file):
if time.Now().Before(ps.warmupPeriod) { _ = stream.Reset()
since the stream will be reset anyway when the defer statement at the beginning of the handler
executes, it is enough to just return ErrWarmup
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.
Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @acud and @esadakar)
pkg/node/node.go, line 178 at r1 (raw file):
// upperbound on warmupTime for push/pullsync protocols warmupTime := o.WarmupTime
why need this complexity, if some set it long its their problem
pkg/puller/puller.go, line 76 at r1 (raw file):
Previously, acud (acud) wrote…
no need for the extra field, you can simply call
go p.manage(warmupTime)
+1
pkg/puller/puller.go, line 103 at r1 (raw file):
}() // wait for warmup duration to complete
this should only apply to storer nodes. lightnodes do not need to wait to push
pkg/pusher/pusher.go, line 94 at r1 (raw file):
}() // wait for warmup duration to complete
only for storer nodes
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: 5 of 9 files reviewed, 8 unresolved discussions (waiting on @acud and @zelig)
pkg/node/node.go, line 178 at r1 (raw file):
Previously, zelig (Viktor Trón) wrote…
why need this complexity, if some set it long its their problem
Fair enough. Thanks.
pkg/node/node.go, line 182 at r1 (raw file):
Previously, acud (acud) wrote…
maybe we should move
time.Hour
to aconst maxWarmupTime
and return an error here?
Done.
pkg/puller/puller.go, line 76 at r1 (raw file):
Previously, zelig (Viktor Trón) wrote…
+1
Done.
pkg/puller/puller.go, line 103 at r1 (raw file):
Previously, zelig (Viktor Trón) wrote…
this should only apply to storer nodes. lightnodes do not need to wait to push
Done.
pkg/pusher/pusher.go, line 64 at r1 (raw file):
Previously, acud (acud) wrote…
here too
Done.
pkg/pusher/pusher.go, line 94 at r1 (raw file):
Previously, zelig (Viktor Trón) wrote…
only for storer nodes
Done.
pkg/pushsync/pushsync.go, line 48 at r1 (raw file):
Previously, acud (acud) wrote…
error description should be changed
Done.
pkg/pushsync/pushsync.go, line 205 at r1 (raw file):
Previously, acud (acud) wrote…
since the stream will be reset anyway when the defer statement at the beginning of the
handler
executes, it is enough to justreturn ErrWarmup
Done.
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.
Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @zelig)
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.
Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @zelig)
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: all files reviewed, 3 unresolved discussions (waiting on @esadakar and @zelig)
pkg/node/node.go, line 178 at r1 (raw file):
Previously, esadakar (Esad Akar) wrote…
Fair enough. Thanks.
cool now
@esadakar I think the integration tests are failing because of this right? |
Node warmup delay for pull/pushsync protocols #2048
Depends on ethersphere/beekeeper#155
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)