-
Notifications
You must be signed in to change notification settings - Fork 666
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
Add SDK Sampling interface #1877
Changes from all commits
499a7f2
fb4573f
b4ab267
25d8e69
203dbe6
fc6b4fb
77849bb
cb1b78f
0af1473
d140e94
d437998
ad542ec
167ba94
b8324d1
2aa0d20
1809aff
da20c25
956efba
868893b
ab45557
0187c34
0ae6659
38e482d
a9c62b6
386f96a
af938d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
// Copyright (C) 2019-2023, Ava Labs, Inc. All rights reserved. | ||
// See the file LICENSE for licensing terms. | ||
|
||
package p2p | ||
|
||
import ( | ||
"context" | ||
|
||
"github.com/ava-labs/avalanchego/ids" | ||
) | ||
|
||
// NodeSampler samples nodes in network | ||
type NodeSampler interface { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems more like a node sampler to me. It samples nodes (which may or may not be connected) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah my thinking was that the terminology we use are:
so |
||
// Sample returns at most [limit] nodes. This may return fewer nodes if | ||
// fewer than [limit] are available. | ||
Sample(ctx context.Context, limit int) []ids.NodeID | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: I also considered making another type to return here like:
so we could add onto it without causing breaking changes. I felt like it was reasonable that in the future we might add some peer-level metadata to this, but it seemed premature to add for now. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
// Copyright (C) 2019-2023, Ava Labs, Inc. All rights reserved. | ||
// See the file LICENSE for licensing terms. | ||
|
||
package p2p | ||
|
||
import ( | ||
"context" | ||
"sync" | ||
|
||
"github.com/ava-labs/avalanchego/ids" | ||
"github.com/ava-labs/avalanchego/snow/validators" | ||
"github.com/ava-labs/avalanchego/utils/set" | ||
"github.com/ava-labs/avalanchego/version" | ||
) | ||
|
||
var ( | ||
_ validators.Connector = (*Peers)(nil) | ||
_ NodeSampler = (*Peers)(nil) | ||
) | ||
|
||
// Peers contains a set of nodes that we are connected to. | ||
type Peers struct { | ||
lock sync.RWMutex | ||
peers set.SampleableSet[ids.NodeID] | ||
} | ||
|
||
func (p *Peers) Connected(_ context.Context, nodeID ids.NodeID, _ *version.Application) error { | ||
p.lock.Lock() | ||
defer p.lock.Unlock() | ||
|
||
p.peers.Add(nodeID) | ||
|
||
return nil | ||
} | ||
|
||
func (p *Peers) Disconnected(_ context.Context, nodeID ids.NodeID) error { | ||
p.lock.Lock() | ||
defer p.lock.Unlock() | ||
|
||
p.peers.Remove(nodeID) | ||
|
||
return nil | ||
} | ||
|
||
func (p *Peers) Sample(_ context.Context, limit int) []ids.NodeID { | ||
p.lock.RLock() | ||
defer p.lock.RUnlock() | ||
|
||
return p.peers.Sample(limit) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,148 @@ | ||
// Copyright (C) 2019-2023, Ava Labs, Inc. All rights reserved. | ||
// See the file LICENSE for licensing terms. | ||
|
||
package p2p | ||
|
||
import ( | ||
"context" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/require" | ||
|
||
"go.uber.org/mock/gomock" | ||
|
||
"github.com/ava-labs/avalanchego/ids" | ||
"github.com/ava-labs/avalanchego/snow/engine/common" | ||
"github.com/ava-labs/avalanchego/utils/logging" | ||
"github.com/ava-labs/avalanchego/utils/math" | ||
"github.com/ava-labs/avalanchego/utils/set" | ||
) | ||
|
||
// Sample should always return up to [limit] peers, and less if fewer than | ||
// [limit] peers are available. | ||
func TestPeersSample(t *testing.T) { | ||
nodeID1 := ids.GenerateTestNodeID() | ||
nodeID2 := ids.GenerateTestNodeID() | ||
nodeID3 := ids.GenerateTestNodeID() | ||
|
||
tests := []struct { | ||
name string | ||
connected set.Set[ids.NodeID] | ||
disconnected set.Set[ids.NodeID] | ||
limit int | ||
}{ | ||
{ | ||
name: "no peers", | ||
limit: 1, | ||
}, | ||
{ | ||
name: "one peer connected", | ||
connected: set.Of(nodeID1), | ||
limit: 1, | ||
}, | ||
{ | ||
name: "multiple peers connected", | ||
connected: set.Of(nodeID1, nodeID2, nodeID3), | ||
limit: 1, | ||
}, | ||
{ | ||
name: "peer connects and disconnects - 1", | ||
connected: set.Of(nodeID1), | ||
disconnected: set.Of(nodeID1), | ||
limit: 1, | ||
}, | ||
{ | ||
name: "peer connects and disconnects - 2", | ||
connected: set.Of(nodeID1, nodeID2), | ||
disconnected: set.Of(nodeID2), | ||
limit: 1, | ||
}, | ||
{ | ||
name: "peer connects and disconnects - 2", | ||
connected: set.Of(nodeID1, nodeID2, nodeID3), | ||
disconnected: set.Of(nodeID1, nodeID2), | ||
limit: 1, | ||
}, | ||
{ | ||
name: "less than limit peers", | ||
connected: set.Of(nodeID1, nodeID2, nodeID3), | ||
limit: 4, | ||
}, | ||
{ | ||
name: "limit peers", | ||
connected: set.Of(nodeID1, nodeID2, nodeID3), | ||
limit: 3, | ||
}, | ||
{ | ||
name: "more than limit peers", | ||
connected: set.Of(nodeID1, nodeID2, nodeID3), | ||
limit: 2, | ||
}, | ||
} | ||
|
||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
require := require.New(t) | ||
peers := &Peers{} | ||
|
||
for connected := range tt.connected { | ||
require.NoError(peers.Connected(context.Background(), connected, nil)) | ||
} | ||
|
||
for disconnected := range tt.disconnected { | ||
require.NoError(peers.Disconnected(context.Background(), disconnected)) | ||
} | ||
|
||
sampleable := set.Set[ids.NodeID]{} | ||
sampleable.Union(tt.connected) | ||
sampleable.Difference(tt.disconnected) | ||
|
||
sampled := peers.Sample(context.Background(), tt.limit) | ||
require.Len(sampled, math.Min(tt.limit, len(sampleable))) | ||
require.Subset(sampleable, sampled) | ||
}) | ||
} | ||
} | ||
|
||
func TestAppRequestAnyNodeSelection(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
peers []ids.NodeID | ||
expected error | ||
}{ | ||
{ | ||
name: "no peers", | ||
expected: ErrNoPeers, | ||
}, | ||
{ | ||
name: "has peers", | ||
peers: []ids.NodeID{ids.GenerateTestNodeID()}, | ||
}, | ||
} | ||
|
||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
require := require.New(t) | ||
ctrl := gomock.NewController(t) | ||
mockAppSender := common.NewMockSender(ctrl) | ||
|
||
expectedCalls := 0 | ||
if tt.expected == nil { | ||
expectedCalls = 1 | ||
} | ||
mockAppSender.EXPECT().SendAppRequest(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(expectedCalls) | ||
|
||
r := NewRouter(logging.NoLog{}, mockAppSender) | ||
peers := &Peers{} | ||
for _, peer := range tt.peers { | ||
require.NoError(peers.Connected(context.Background(), peer, nil)) | ||
} | ||
|
||
client, err := r.RegisterAppProtocol(1, nil, peers) | ||
require.NoError(err) | ||
|
||
err = client.AppRequestAny(context.Background(), []byte("foobar"), nil) | ||
require.ErrorIs(err, tt.expected) | ||
}) | ||
} | ||
} |
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 part of this PR but should we have a
NewClient
function? If this in intended to be used only inside this package,Client
can be unexported. If it's intended to be used outside this package, then a user can't useClient
because they can't set its inner fields.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 created via
RegisterAppProtocol
. It's not possible to use aClient
without a correspondingRouter
because theRouter
holds the map of pending requests -> the callbacks that need to be fired on response. We could theoretically refactor this into aNewClient(Router, handlerID, ....)
if we felt that looked cleaner.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 just feels weird to export this if it's actually unusable outside this package
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.
Client
is usable outside of this package,RegisterAppProtocol
initializes it here.Maybe we could rename this to
NewClient
to be more verbose or remove the receiver onRouter
. This definitely might seem confusing coming from the existing paradigm of how we've developed VMs without the SDK component.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.
Ah I see. Yeah I think it might be a bit confusing to someone looking at this package how to create a new client. I think maybe even just a comment on
Client
might suffice.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 I could see an argument to make the constructor code
NewClient(sender common.AppSender, router *Router, handlerID byte) *Client
. This would have the added benefit of not having to keep around thesender
as a field inRouter
which we currently do.