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.

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

go/archive-mode: disable runtime P2P if archive mode is used #4775

Merged
merged 1 commit into from
Jun 6, 2022

Conversation

ptrus
Copy link
Member

@ptrus ptrus commented Jun 2, 2022

No description provided.

@ptrus ptrus force-pushed the ptrus/feature/archive-node-p2p branch 3 times, most recently from b46b5a2 to befd758 Compare June 3, 2022 08:04
@ptrus ptrus marked this pull request as ready for review June 3, 2022 08:05
@codecov
Copy link

codecov bot commented Jun 3, 2022

Codecov Report

Merging #4775 (6db6ce7) into master (bb628e3) will increase coverage by 0.05%.
The diff coverage is 70.17%.

❗ Current head 6db6ce7 differs from pull request most recent head 2657e28. Consider uploading reports for the commit 2657e28 to get more accurate results

@@            Coverage Diff             @@
##           master    #4775      +/-   ##
==========================================
+ Coverage   66.82%   66.88%   +0.05%     
==========================================
  Files         448      451       +3     
  Lines       50246    50294      +48     
==========================================
+ Hits        33579    33638      +59     
- Misses      12492    12495       +3     
+ Partials     4175     4161      -14     
Impacted Files Coverage Δ
go/consensus/api/api.go 69.23% <ø> (ø)
go/consensus/tendermint/full/common.go 67.58% <ø> (ø)
go/consensus/tendermint/seed/seed.go 66.00% <0.00%> (-0.67%) ⬇️
go/worker/common/p2p/rpc/nop.go 0.00% <0.00%> (ø)
go/worker/common/p2p/rpc/peermgmt.go 76.72% <0.00%> (-3.53%) ⬇️
go/worker/common/worker.go 92.59% <ø> (ø)
go/worker/compute/executor/committee/node.go 70.86% <ø> (-0.22%) ⬇️
go/worker/compute/executor/committee/p2p.go 79.16% <ø> (+4.16%) ⬆️
go/worker/keymanager/watcher.go 63.07% <ø> (ø)
go/worker/keymanager/worker.go 63.69% <ø> (ø)
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee40270...2657e28. Read the comment docs.

Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

Nice work with abstracting out the P2P interface. Some minor comments.

Comment on lines 277 to 279
if genesisDoc.Registry.Parameters.DebugAllowUnroutableAddresses {
p2p.DebugForceAllowUnroutableAddresses()
}
Copy link
Member

Choose a reason for hiding this comment

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

Why was this removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unintended, reverted

}

// NewMock creates a new no-op mock P2P node.
func NewMock() api.Service {
Copy link
Member

Choose a reason for hiding this comment

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

I would rather call it NewNop (or similar) as mock could imply it is only used in tests as it provides mock responses.


// Implements api.Service.
func (p *nopP2P) Name() string {
return "mock p2p"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return "mock p2p"
return "no-op p2p"

"github.com/oasisprotocol/oasis-core/go/worker/common/p2p/rpc"
)

// nopP2P is a no-op mock peer-to-peer node.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// nopP2P is a no-op mock peer-to-peer node.
// nopP2P is a no-op peer-to-peer node that does not propagate anything.

maxPeerResponseTime time.Duration,
opts ...CallOption,
) (PeerFeedback, error) {
return nil, fmt.Errorf("unsupported: mock p2p")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return nil, fmt.Errorf("unsupported: mock p2p")
return nil, fmt.Errorf("unsupported: p2p is disabled")

maxParallelRequests uint,
opts ...CallMultiOption,
) ([]interface{}, []PeerFeedback, error) {
return nil, nil, fmt.Errorf("unsupported: mock p2p")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return nil, nil, fmt.Errorf("unsupported: mock p2p")
return nil, nil, fmt.Errorf("unsupported: p2p is disabled")

@@ -215,6 +215,33 @@ type Client interface {
) ([]interface{}, []PeerFeedback, error)
}

type nopClient struct {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move the nop client and peer manager into a separate nop.go.

@@ -325,6 +353,10 @@ func (mgr *peerManager) peerProtocolWatcher() {

// NewPeerManager creates a new peer manager for the given protocol.
func NewPeerManager(p2p P2P, protocolID protocol.ID, stickyPeers bool) PeerManager {
if p2p.GetHost() == nil {
// Mock P2P service, use the mock peer manager.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Mock P2P service, use the mock peer manager.
// No P2P service, use the no-op peer manager.

@ptrus ptrus force-pushed the ptrus/feature/archive-node-p2p branch 2 times, most recently from 4bcec27 to 6db6ce7 Compare June 3, 2022 13:27
@ptrus ptrus requested a review from kostko June 6, 2022 05:44
go/worker/common/p2p/rpc/client.go Outdated Show resolved Hide resolved
@ptrus ptrus force-pushed the ptrus/feature/archive-node-p2p branch from 6db6ce7 to 80c39d0 Compare June 6, 2022 11:16
@ptrus ptrus enabled auto-merge June 6, 2022 11:17
@ptrus ptrus force-pushed the ptrus/feature/archive-node-p2p branch from 80c39d0 to 2657e28 Compare June 6, 2022 11:47
@ptrus ptrus merged commit 87e072b into master Jun 6, 2022
@ptrus ptrus deleted the ptrus/feature/archive-node-p2p branch June 6, 2022 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants