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

coreapi: Swarm API #4803

Merged
merged 8 commits into from
Oct 2, 2018
Merged

coreapi: Swarm API #4803

merged 8 commits into from
Oct 2, 2018

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented Mar 10, 2018 •
edited by Stebalien
Loading

Based on #4802

Replaces #4774

TODOs:

@magik6k magik6k requested a review from Kubuxu as a code owner March 10, 2018 18:35
@ghost ghost assigned magik6k Mar 10, 2018
@ghost ghost added the status/in-progress In progress label Mar 10, 2018
@magik6k magik6k added topic/core-api Topic core-api status/blocked Unable to be worked further until needs are met labels Mar 11, 2018
@magik6k magik6k mentioned this pull request Mar 12, 2018
Open
51 tasks
@magik6k magik6k removed the status/blocked Unable to be worked further until needs are met label Mar 23, 2018
@magik6k
Copy link
Member Author

magik6k commented Mar 24, 2018

#4807 and #4804 need to get merged first

@magik6k magik6k added the status/blocked Unable to be worked further until needs are met label Mar 24, 2018
camelmasa
camelmasa reviewed Apr 5, 2018
View reviewed changes
core/coreapi/swarm.go Outdated
}

func (ci *connInfo) ID() peer.ID {
return ci.ID()
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for nitpick. This line will loop infinity 🐛

Copy link
Member

Choose a reason for hiding this comment

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

That's not what I'd call a "nitpick".

@Stebalien Stebalien removed the status/blocked Unable to be worked further until needs are met label Sep 11, 2018
@magik6k magik6k force-pushed the feat/coreapi/swarm branch 2 times, most recently from efe5033 to 8333fa8 Compare September 11, 2018 11:42
Stebalien
Stebalien reviewed Sep 11, 2018
View reviewed changes
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

I'd like to port the swarm command over to this interface before merging to make sure it does everything we need (and to test it). Will that blow up the patch too much?

core/coreapi/interface/swarm.go Outdated
Latency(context.Context) (time.Duration, error)

// Streams returns list of streams established with the peer
// TODO: should this return multicodecs?
Copy link
Member

Choose a reason for hiding this comment

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

protocol.IDs?

core/coreapi/interface/swarm.go Outdated
)

// PeerInfo contains information about a peer
type PeerInfo interface {
Copy link
Member

Choose a reason for hiding this comment

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

Is this PeerInfo or ConnectionInfo? It looks like this is a per-connection datastructure.

core/coreapi/interface/swarm.go Outdated
// SwarmAPI specifies the interface to libp2p swarm
type SwarmAPI interface {
// Connect to a given address
Connect(context.Context, ma.Multiaddr) error
Copy link
Member

Choose a reason for hiding this comment

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

Should this take a peerstore.PeerInfo? It should probably take either that or a peer.ID

@magik6k
Copy link
Member Author

magik6k commented Sep 11, 2018

I'd like to port the swarm command over to this interface before merging to make sure it does everything we need (and to test it). Will that blow up the patch too much?

I'm actually doing this right now, for that reason.

@magik6k magik6k force-pushed the feat/coreapi/swarm branch 4 times, most recently from 81d427d to d0e7d1a Compare September 18, 2018 14:28
@magik6k magik6k force-pushed the feat/coreapi/swarm branch 2 times, most recently from b4bfdd8 to 71d539b Compare September 19, 2018 10:23
@magik6k magik6k requested a review from Stebalien September 19, 2018 12:54
@magik6k
coreapi: swarm interface
1373218 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k
coreapi: implement swarm api
8358c8d 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k
fix infinite loop in connInfo.ID
e7f493b 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k
coreapi swarm: swarm refactor fixes
44bab58 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k
swarm cmd: port to new cmd lib
30d42f4 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k
coreapi swarm: rewire connect/disconnect
df9f101 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k
coreapi swarm: rewire address listing cmds
6fa2ab0 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k
Copy link
Member Author

magik6k commented Sep 26, 2018

(rebased)

Stebalien
Stebalien reviewed Sep 28, 2018
View reviewed changes
core/coreapi/interface/swarm.go
Address() ma.Multiaddr

// Direction returns which way the connection was established
Direction() net.Direction
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to have this over Stat? The nice thing about Stat is that it's extensible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Assuming you mean go-libp2p-net.Stat - ipfs swarm peers doesn't return info from the Extra map, so we would have to expose it somehow for a good implementation in go-ipfs-api.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Yeah, that may be tricky as it could have arbitrary data. We can punt on that.

core/coreapi/interface/swarm.go Outdated
Direction() net.Direction

// Latency returns last known round trip time to the peer
Latency(context.Context) (time.Duration, error)
Copy link
Member

Choose a reason for hiding this comment

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

As with the pubsub API, we need to consider what this parameter signals. Personally, when I see something taking a context, I immediately think that it does some kind of network operation.

core/coreapi/interface/swarm.go Show resolved Hide resolved
core/coreapi/swarm.go Outdated

swrm, ok := api.node.PeerHost.Network().(*swarm.Swarm)
if !ok {
return fmt.Errorf("peerhost network was not swarm")
Copy link
Member

Choose a reason for hiding this comment

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

If possible, can we just make this optional? That is, clear the backoff iff we have a swarm but don't error out.

Copy link
Member

Choose a reason for hiding this comment

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

(I realize this probably isn't what we do now but, well, no time better than now)

@magik6k
coreapi swarm: missing docs, review
b2f6c3e 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@Stebalien Stebalien merged commit 4f9bde8 into master Oct 2, 2018
@ghost ghost removed the status/in-progress In progress label Oct 2, 2018
@Stebalien Stebalien deleted the feat/coreapi/swarm branch October 2, 2018 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camelmasa camelmasa camelmasa left review comments

@Stebalien Stebalien Stebalien approved these changes

@Kubuxu Kubuxu Awaiting requested review from Kubuxu

Assignees

@magik6k magik6k

Labels
topic/core-api Topic core-api
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@magik6k @camelmasa @Stebalien