-
Notifications
You must be signed in to change notification settings - Fork 662
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
Migrate x/sync
to p2p
#3106
base: master
Are you sure you want to change the base?
Migrate x/sync
to p2p
#3106
Conversation
x/sync/manager.go
Outdated
func (m *Manager) doWork(ctx context.Context, work *workItem) { | ||
// Backoff for failed requests accounting for time this job has already | ||
// spent waiting in the unprocessed queue | ||
backoff := calculateBackoff(work.attempt) |
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.
Not optimal since this worker goroutine is going to be waiting until this timeout passes and ideally we would prioritize requests that are always executable, but felt like something more complex would make the diff hard to review since the scope of this PR is just trying to get rid of the blocking networking calls
return client.GetRangeProof(ctx, request) | ||
} | ||
|
||
func TestGetRangeProof(t *testing.T) { |
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.
All client-side error handling cases were moved into Test_Sync_Result_Correct_Root
and server-side cases were moved into network_server_test
return client.AppRequest(ctx, set.Of(nodeID), requestBytes, onResponse) | ||
} | ||
|
||
func (m *Manager) retryWork(work *workItem) { |
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.
Another idea is to have a separate failed requests data structure. Feel like this will introduce more complexity into this PR though so for now we are just retrying by putting stuff in the work heap w/ it's own priority.
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
network/p2p/p2ptest/client_test.go
Outdated
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.
It seems a bit unusual to test our testing code... But since the tests are already written 🤷
// tracking of peers & bandwidth usage | ||
peers *p2p.PeerTracker |
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.
Is it possible to configure the p2p client to perform a similar type of peer sampling? It's a critical performance improvement to select peers based on historical bandwidth.
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.
Yeah we'll need to merge in some form of #3007
x/sync/client.go
Outdated
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.
The actual code in this file feels a bit weird. Can we move these helpers and definitions into manager.go
(where they are actually used)?
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.
Yeah. Originally did this to minimize the diff but agree this looks weird now.
x/sync/manager.go
Outdated
// spent waiting in the unprocessed queue | ||
backoff := calculateBackoff(work.attempt) | ||
if waitTime := backoff - time.Since(work.queueTime); waitTime > 0 { | ||
<-time.After(waitTime) |
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.
It feels a bit odd to me that we have access to a context but that we don't check it while we're waiting here.
db: db, | ||
log: log, | ||
func maybeBytesToMaybe(mb *pb.MaybeBytes) maybe.Maybe[[]byte] { | ||
if mb != nil && !mb.IsNothing { |
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.
I think mb.IsNothing
could just be removed right? Because mb == nil
already implies Nothing
. Should we remove this field from the type? Or is this needed for some other language?
To clarify - the mb.IsNothing
check is needed if it's on the type... But I think we could get rid of it entirely.
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.
I think this is showing up as a diff because of some code that got moved around it but this is previously existing code
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.
A future improvement then haha
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Why this should be merged
Eliminates our last usage of the blocking clients
How this works
Replaces usage of blocking client with
p2p
async clientsHow this was tested
Added unit tests