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

go/worker/executor: Propose a new batch if there are message results #3771

Merged
merged 5 commits into from
Mar 16, 2021
Merged

go/worker/executor: Propose a new batch if there are message results #3771

merged 5 commits into from
Mar 16, 2021

Conversation

ptrus
Copy link
Member

@ptrus ptrus commented Mar 10, 2021

Fixes: #3645

@ptrus ptrus changed the title Ptrus/feature/runtime submit messages go/worker/executor: Propose a new batch if there are message results Mar 10, 2021
@ptrus ptrus force-pushed the ptrus/feature/runtime-submit-messages branch from 969ea21 to a230542 Compare March 10, 2021 13:54
@codecov
Copy link

codecov bot commented Mar 10, 2021 •
edited

Codecov Report

Merging #3771 (0e5e5bb) into master (7d0d54b) will increase coverage by 0.10%.
The diff coverage is 80.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3771      +/-   ##
==========================================
+ Coverage   67.32%   67.42%   +0.10%     
==========================================
  Files         402      401       -1     
  Lines       40560    40638      +78     
==========================================
+ Hits        27305    27401      +96     
+ Misses       9410     9387      -23     
- Partials     3845     3850       +5
Impacted Files Coverage Δ
go/registry/api/runtime.go 43.97% <ø> (ø)
go/worker/compute/executor/committee/batch.go 80.00% <0.00%> (+13.33%) ⬆️
...scheduling/simple/txpool/orderedmap/ordered_map.go 86.44% <40.00%> (-4.64%) ⬇️
go/consensus/tendermint/abci/mux.go 61.32% <50.00%> (+1.50%) ⬆️
go/worker/compute/executor/committee/node.go 69.15% <68.51%> (+0.87%) ⬆️
go/runtime/registry/registry.go 75.54% <94.73%> (+5.77%) ⬆️
go/oasis-node/cmd/node/control.go 62.16% <100.00%> (ø)
go/storage/client/client.go 72.68% <100.00%> (ø)
go/worker/common/committee/node.go 70.61% <100.00%> (ø)
go/worker/storage/committee/checkpoint_sync.go 71.48% <100.00%> (-2.01%) ⬇️
... and 34 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 7d0d54b...0e5e5bb. Read the comment docs.

@ptrus ptrus force-pushed the ptrus/feature/runtime-submit-messages branch 2 times, most recently from b7383e0 to d850b84 Compare March 11, 2021 14:21
@ptrus ptrus marked this pull request as ready for review March 11, 2021 14:30
@ptrus ptrus force-pushed the ptrus/feature/runtime-submit-messages branch 5 times, most recently from 1162c6f to 80ce959 Compare March 12, 2021 11:51
kostko
kostko reviewed Mar 12, 2021
View reviewed changes
go/runtime/registry/registry.go Outdated Show resolved Hide resolved
go/worker/compute/executor/committee/node.go Outdated Show resolved Hide resolved
@ptrus ptrus force-pushed the ptrus/feature/runtime-submit-messages branch from 459bdf3 to 6324652 Compare March 15, 2021 12:20
kostko
kostko reviewed Mar 15, 2021
View reviewed changes
go/runtime/registry/registry.go Outdated Show resolved Hide resolved
go/runtime/transaction/batch.go Show resolved Hide resolved
go/worker/compute/executor/committee/node.go Outdated Show resolved Hide resolved
@ptrus ptrus force-pushed the ptrus/feature/runtime-submit-messages branch 3 times, most recently from 71f3ca4 to 20b756a Compare March 15, 2021 14:28
kostko
kostko reviewed Mar 15, 2021
View reviewed changes
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.

Hm, node tests seem to hang in WorkerImplementationTests, specifically in:

oasis-core/go/worker/compute/executor/tests/tester.go

Lines 39 to 40 in 20b756a

// Wait for worker to start and register.
<-worker.Initialized()

@ptrus ptrus force-pushed the ptrus/feature/runtime-submit-messages branch 3 times, most recently from dfeceba to 8f8d2a3 Compare March 15, 2021 19:28
@ptrus
Copy link
Member Author

ptrus commented Mar 15, 2021 •
edited

Hm, node tests seem to hang in WorkerImplementationTests, specifically in:

It was due to syncCheckpoints blocking on waiting for ActiveDescriptor causing the storage worker never to initialize. Added a timeout and a fallback to RegistryDescriptor as i think we'd still want to try syncing from checkpoints even if runtime is inactive (there might be registered storage nodes running). I also wen't over other places where ActiveDescriptor is used and made sure appropriate context is used (e.g. blocking for the duration of used context is fine/desired).

kostko
kostko reviewed Mar 15, 2021
View reviewed changes
go/runtime/registry/registry.go
@@ -77,6 +79,13 @@ type Runtime interface {
// WatchRegistryDescriptor subscribes to registry descriptor updates.
WatchRegistryDescriptor() (<-chan *registry.Runtime, pubsub.ClosableSubscription, error)

// ActiveDescriptor waits for runtime to be initialized and then returns
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment here (and below) saying that this may block until the next epoch transition.

go/worker/storage/committee/checkpoint_sync.go Outdated Show resolved Hide resolved
@ptrus ptrus force-pushed the ptrus/feature/runtime-submit-messages branch from ec567cb to 4b84c14 Compare March 16, 2021 07:12
@ptrus ptrus force-pushed the ptrus/feature/runtime-submit-messages branch from 4b84c14 to ef709e4 Compare March 16, 2021 08:30
@ptrus ptrus force-pushed the ptrus/feature/runtime-submit-messages branch from ef709e4 to 0e5e5bb Compare March 16, 2021 09:32
@ptrus ptrus merged commit d06b845 into master Mar 16, 2021
@ptrus ptrus deleted the ptrus/feature/runtime-submit-messages branch March 16, 2021 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kostko kostko kostko approved these changes

@peterjgilbert peterjgilbert Awaiting requested review from peterjgilbert peterjgilbert is a code owner

@pro-wh pro-wh Awaiting requested review from pro-wh pro-wh is a code owner

@Yawning Yawning Awaiting requested review from Yawning

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Propose a new batch if there are message results
2 participants
@ptrus @kostko