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.

Sign up for GitHub

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

Jump to bottom

[PeerList][Part 5] SinglePeerList #403

Merged
merged 7 commits into from
Nov 10, 2016
Merged

[PeerList][Part 5] SinglePeerList #403

merged 7 commits into from
Nov 10, 2016

Conversation

willhug
Copy link
Contributor

@willhug willhug commented Nov 2, 2016

Summary: This PR creates a concrete peerlist implementation that has a
single peer implementation.

@willhug willhug force-pushed the peerList-part4-refactor branch 3 times, most recently from 67423a5 to 9dd824b Compare November 8, 2016 02:14
@willhug willhug force-pushed the peerList-part5-singlepeerlist branch from 6d416e7 to 2b4961f Compare November 8, 2016 03:34
abhinav
abhinav approved these changes Nov 8, 2016
View reviewed changes
transport/peer/peerlist/single.go

type single struct {
peerID transport.PeerIdentifier
peer transport.Peer
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it safe to assume that peerID is irrelevant once peer is set? Maybe mention that in a comment?

transport/peer/peerlist/single.go
}

// NotifyPending when the number of Pending requests changes
func (pl *single) NotifyStatusChanged(transport.Peer) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

outdated comment

Copy link
Contributor

Choose a reason for hiding this comment

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

ping

@willhug willhug force-pushed the peerList-part5-singlepeerlist branch from ab9c17f to 8c22b83 Compare November 8, 2016 23:27
abhinav
abhinav reviewed Nov 9, 2016
View reviewed changes
transport/peer/peerlist/single.go

func (pl *single) Start() error {
if pl.started.Swap(true) {
return errors.ErrPeerListAlreadyStarted("single")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want to use a RW Mutex instead. There's a race here where if I
call Start and ChoosePeer concurrently, started is set to true by
Start but ChoosePeer returns the nil peer before Start has a chance to
fill it in.

transport/peer/peerlist/single.go
}

// NotifyPending when the number of Pending requests changes
func (pl *single) NotifyStatusChanged(transport.Peer) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

ping

transport/peer/peerlist/single_test.go
s.expectedPeerID = s.pid
s.expectedAgent = s.agent
s.expectedStarted = false
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of combining all these tests, it's okay (preferable even) to split
tests for different behaviors out into different test functions. Table tests
are for tests with enough similar structure that writing those into separate
functions would be redundant. As it stands right now, it's very difficult to
figure out what each of these tests is actually testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#422

@willhug willhug force-pushed the peerList-part5-singlepeerlist branch from 8c22b83 to a8ab052 Compare November 10, 2016 00:49
peats-bond
peats-bond approved these changes Nov 10, 2016
View reviewed changes
transport/peer/peerlist/single.go
}

// NewSingle creates a static PeerList with a single Peer
func NewSingle(pi transport.PeerIdentifier, agent transport.Agent) transport.PeerList {
Copy link
Contributor

Choose a reason for hiding this comment

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

consistency! pid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

85351-dale-shhh-gif-the-walking-dead-pzrw

transport/peer/peerlist/single_test.go
}

for _, tt := range tests {
pl := NewSingle(tt.pid, tt.agent).(*single)
Copy link
Contributor

Choose a reason for hiding this comment

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

subtests :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#422

bombela
bombela reviewed Nov 10, 2016
View reviewed changes
transport/peer/peerlist/single.go
return nil
}

func (pl *single) ChoosePeer(context.Context, *transport.Request) (transport.Peer, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do I understand that every new request will call this function first locking the mutex on the way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With a read lock yes, for this list it doesn't make as much sense, but once we start adding/removing peers I think it will be a good idea, or at least we'll need to be explicitly aware when we remove the mutexes (for performance reasons).

@willhug willhug force-pushed the peerList-part5-singlepeerlist branch from a8ab052 to 04165b8 Compare November 10, 2016 05:35
@willhug willhug force-pushed the peerList-part5-singlepeerlist branch from 04165b8 to 99078d1 Compare November 10, 2016 05:44
@willhug willhug force-pushed the peerList-part5-singlepeerlist branch from 99078d1 to 91af1f5 Compare November 10, 2016 06:29
@willhug willhug changed the base branch from peerList-part4-refactor to peerList November 10, 2016 07:18
@willhug willhug force-pushed the peerList-part5-singlepeerlist branch from 91af1f5 to 955c76f Compare November 10, 2016 07:19
@willhug willhug force-pushed the peerList-part5-singlepeerlist branch from 955c76f to e3c5ff2 Compare November 10, 2016 07:20
@willhug willhug merged commit 5615ce2 into peerList Nov 10, 2016
@willhug willhug deleted the peerList-part5-singlepeerlist branch November 10, 2016 19:12
willhug added a commit that referenced this pull request Nov 10, 2016
@willhug
[PeerList][Part 5] SinglePeerList (#403)
7435eed 
Summary: This PR creates a concrete peerlist implementation that has a
single peer implementation.  This can be used to shim the current existing http outbounds and replace them with a single PeerList
willhug added a commit that referenced this pull request Nov 10, 2016
@willhug
[PeerList][Part 5] SinglePeerList (#403)
9dcf4d9 
Summary: This PR creates a concrete peerlist implementation that has a
single peer implementation.  This can be used to shim the current existing http outbounds and replace them with a single PeerList
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhinav abhinav abhinav approved these changes

@bombela bombela bombela left review comments

@kriskowal kriskowal kriskowal approved these changes

@peats-bond peats-bond peats-bond approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@willhug @abhinav @kriskowal @bombela @peats-bond