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

Factor message packaging out of plumbing into core.Outbox #2795

Merged
merged 4 commits into from May 22, 2019

Conversation

@anorth
Copy link
Contributor

commented May 20, 2019

This is part of follow-up refactoring to the message queue that @acruikshank and I discussed a while ago. Sorry it's a little long: the delta is mostly moving and simplifying the old Sender test to exercise the outbox instead.

I intend to also:

  • move the blocktimer out of message pool (this change means one of the Add callers now knows height)
  • move the policy for handling new tipsets into Outbox

@anorth anorth requested review from acruikshank and rosalinekarr May 20, 2019

}

// Send marshals and sends a message, retaining it in the outbound message queue.
func (ob *Outbox) Send(ctx context.Context, from, to address.Address, value *types.AttoFIL,

This comment has been minimized.

Copy link
@anorth

anorth May 20, 2019

Author Contributor

This is a fairly direct move from msg.Sender, but with tighter dependencies and the message pool/publish extracted to publisher.

"github.com/stretchr/testify/require"
)

func TestOutbox(t *testing.T) {

This comment has been minimized.

Copy link
@anorth

anorth May 20, 2019

Author Contributor

These are ported from the msg.Sender tests, but with fakes instead of real dependencies (chain store, state HAMT etc).

I dropped a few internal tests for nextNonce and added some nonce checks to the existing tests, so this test is now in a _test package.

@@ -119,7 +119,7 @@ type Node struct {
// Incoming messages for block mining.
MsgPool *core.MessagePool
// Messages sent and not yet mined.
Outbox *core.MessageQueue
MsgQueue *core.MessageQueue

This comment has been minimized.

Copy link
@anorth

anorth May 20, 2019

Author Contributor

I'll replace this with the actual outbox object soon.

node/node.go Outdated Show resolved Hide resolved

@anorth anorth force-pushed the anorth/sender branch from 799464a to cdd9d24 May 20, 2019

@rosalinekarr
Copy link
Contributor

left a comment

Looks great, but I'd really rather not adding duplicate functionality with the actorProvider. Otherwise, just a few questions, but nothing blocking.

core/outbox.go Show resolved Hide resolved
node/node.go Outdated Show resolved Hide resolved
core/outbox.go Show resolved Hide resolved
if err != nil {
return nil, errors.Wrapf(err, "no actor at address %s", addr)
}
return actr, nil

This comment has been minimized.

Copy link
@rosalinekarr

rosalinekarr May 20, 2019

Contributor

With regard to using the chainFacade instead of chainProvider and actorProvider, this method is identical to plumbing/bcf/blockchain_facade.go line 89.

if err = p.network.Publish(p.topic, encoded); err != nil {
return errors.Wrap(err, "failed to publish message to network")
}
return nil

This comment has been minimized.

Copy link
@rosalinekarr

rosalinekarr May 20, 2019

Contributor

Looks great, but I'm not totally clear on why this is under node. Why not put it with the outbox? It feels like the node directory has been sort of devolving into a "misc" folder lately.

This comment has been minimized.

Copy link
@anorth

anorth May 21, 2019

Author Contributor

This is a package-level dependency injection. The outbox doesn't know or care how messages are published to the network. This change removed the import of net/pubsub by the core package.

It's "wiring" which definitely feels miscellaneous but is what the node package alone should be doing.

plumbing/msg/sender.go Show resolved Hide resolved

@anorth anorth requested a review from rosalinekarr May 21, 2019

@rosalinekarr
Copy link
Contributor

left a comment

LGTM

@acruikshank
Copy link
Contributor

left a comment

Couldn't find anything wrong. I like that we're moving this code out of plumbing. I'm with @rosalinekarr that I'm a little uneasy adding code to the node package, but I see the reasoning.

core/outbox_test.go Outdated Show resolved Hide resolved

@anorth anorth force-pushed the anorth/sender branch from fea059b to 8069cce May 22, 2019

@anorth anorth force-pushed the anorth/sender branch from 8069cce to fcb2cfe May 22, 2019

@anorth anorth merged commit 92883c2 into master May 22, 2019

5 checks passed

ci/circleci: build_linux-1 Your tests passed on CircleCI!
Details
ci/circleci: deps_linux Your tests passed on CircleCI!
Details
ci/circleci: functional_test_linux-1 Your tests passed on CircleCI!
Details
ci/circleci: integration_test_linux Your tests passed on CircleCI!
Details
ci/circleci: unit_test_linux Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.