go/worker/txnscheduler: Support multiple txnscheduler workers #3184

ptrus · 2020-08-11T15:47:32Z

Fixes: #1654
Fixes: #1677

Adds support for multiple txnschedulers in runtime committees:

txnscheduler grpc endpoint removed, the per-runtime libp2p gossipsub is used for submitting transactions
current implementation: ~~added LeadersInterval runtime parameter~~ - txnschedulers switch between leaders in round-robin fashion (switching on each runtime round)
leader role removed from scheduler api
txnscheduler worker removed (merged into exectuor)
txnscheduler committee removed

TODO:

actually following on go/roothash: Drop support for multiple committees #3179 drop support for txnschedulers
cleanup - see comments in code.
byzantine-node tests
~~add support for roothash timeout in case the current leader is not submitting batches~~ - open issue, will handle in separate PR
- Support roothash timeout triggered by executors #3199

go/registry/api/api.go

kostko

One thing to note, the commitment pool (pool.go) needs to make sure that the batch was proposed by the leader at the time the first commitment was received. I don't see any modifications to the consensus code here.

go/worker/compute/txnscheduler/committee/node.go

go/worker/common/p2p/types.go

go/worker/common/p2p/dispatch.go

go/worker/common/p2p/p2p.go

go/worker/common/p2p/dispatch.go

go/worker/common/p2p/noop.go

go/worker/common/p2p/peermgmt.go

go/worker/compute/txnscheduler/committee/node.go

codecov · 2020-08-15T07:17:21Z

Codecov Report

Merging #3184 into master will increase coverage by 0.01%.
The diff coverage is 72.84%.

@@            Coverage Diff             @@
##           master    #3184      +/-   ##
==========================================
+ Coverage   67.45%   67.46%   +0.01%     
==========================================
  Files         380      371       -9     
  Lines       36474    36107     -367     
==========================================
- Hits        24603    24361     -242     
+ Misses       8701     8580     -121     
+ Partials     3170     3166       -4

Impacted Files	Coverage Δ
.../consensus/tendermint/apps/roothash/state/state.go	`72.54% <ø> (+3.92%)`	⬆️
go/registry/api/runtime.go	`54.00% <ø> (ø)`
go/runtime/committee/committee.go	`55.55% <ø> (-23.24%)`	⬇️
go/worker/common/p2p/peermgmt.go	`75.38% <ø> (-0.74%)`	⬇️
go/worker/compute/executor/committee/state.go	`85.18% <ø> (ø)`
go/worker/keymanager/worker.go	`63.73% <0.00%> (ø)`
go/registry/api/api.go	`42.29% <12.00%> (-0.82%)`	⬇️
go/oasis-node/cmd/registry/runtime/runtime.go	`57.53% <33.33%> (-0.61%)`	⬇️
...consensus/tendermint/apps/roothash/transactions.go	`53.65% <37.50%> (-5.81%)`	⬇️
go/oasis-node/cmd/node/node.go	`55.82% <37.50%> (-0.31%)`	⬇️
... and 69 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 2667194...16499ba. Read the comment docs.

kostko

So this removes the transaction scheduler worker but not the transaction scheduler committee? There is no need for a separate committee AFAIK, it makes more sense that one of the executor nodes is the transaction scheduler (proposer).

go/consensus/tendermint/apps/roothash/transactions.go

go/consensus/tendermint/apps/scheduler/scheduler.go

go/runtime/client/watcher.go

go/worker/common/committee/node.go

ptrus · 2020-08-16T12:26:05Z

So this removes the transaction scheduler worker but not the transaction scheduler committee? There is no need for a separate committee AFAIK, it makes more sense that one of the executor nodes is the transaction scheduler (proposer).

yeah but removing the committee will be straightforward so i left it for after i have the rest done :-)

go/roothash/api/commitment/pool.go

go/worker/compute/executor/committee/node.go

go/worker/compute/executor/committee/txnscheduling/algorithm/batching/batching.go

go/consensus/tendermint/apps/roothash/transactions.go

go/registry/api/runtime.go

go/roothash/api/commitment/pool.go

go/runtime/client/client.go

docs/runtime/index.md

go/consensus/tendermint/apps/roothash/transactions.go

kostko · 2020-08-19T16:25:24Z

go/runtime/scheduling/api/api.go

-	// Initialize initializes the internal transaction scheduler state.
-	// Algorithm should use the provided transaction dispatcher to dispatch
-	// scheduled transactions.
+// Scheduler defines an algorithm for scheduling incoming transaction.


Suggested change

// Scheduler defines an algorithm for scheduling incoming transaction.

// Scheduler defines an algorithm for scheduling incoming transactions.

Was this reverted, I remember this was already fixed :-)

Yeah me to 🤔

go/runtime/scheduling/api/api.go

go/runtime/scheduling/batching/incoming_queue.go

go/consensus/tendermint/apps/roothash/roothash.go

go/worker/compute/executor/committee/node.go

go/consensus/tendermint/apps/roothash/state/round.go

go/registry/api/runtime.go

ptrus · 2020-08-20T07:46:55Z

Did some more (minor) changes see: 5738d83 (in a separate commit for now, for easier review)

kostko · 2020-08-20T10:37:41Z

go/runtime/scheduling/api/api.go

-	// Initialize initializes the internal transaction scheduler state.
-	// Algorithm should use the provided transaction dispatcher to dispatch
-	// scheduled transactions.
+// Scheduler defines an algorithm for scheduling incoming transaction.


Was this reverted, I remember this was already fixed :-)

Transaction scheduling committee is removed and the transaction scheduler worker is merged into the executor. Transaction scheduling gRPC service is removed and runtime transaction submission is now done via libp2p's gossipsub. Each active executor worker now also acts as a transaction scheduler. Each round one of the executors acts as the scheduler and is expected to propose a batch for scheduling. Nodes switch between schedulers in round robin fashion.

ptrus · 2020-08-20T11:05:31Z

Really thanks for all the great reviews @kostko !

ptrus force-pushed the ptrus/feature/multiple-txnschedulers branch from f90d071 to 99460e4 Compare August 11, 2020 15:55

ptrus changed the title ~~go/worker/txnscheduler: Support multiple txnscheduler workers.~~ go/worker/txnscheduler: Support multiple txnscheduler workers Aug 11, 2020

ptrus force-pushed the ptrus/feature/multiple-txnschedulers branch 2 times, most recently from 88f703b to ddb3838 Compare August 11, 2020 16:00

kostko reviewed Aug 12, 2020

View reviewed changes

go/registry/api/api.go Outdated Show resolved Hide resolved

kostko reviewed Aug 12, 2020

View reviewed changes

ptrus force-pushed the ptrus/feature/multiple-txnschedulers branch 7 times, most recently from 416796c to 43fc3d4 Compare August 15, 2020 06:59

kostko reviewed Aug 16, 2020

View reviewed changes

ptrus force-pushed the ptrus/feature/multiple-txnschedulers branch from 43fc3d4 to 24b1dfb Compare August 16, 2020 12:43

kostko reviewed Aug 16, 2020

View reviewed changes

go/roothash/api/commitment/pool.go Outdated Show resolved Hide resolved

ptrus force-pushed the ptrus/feature/multiple-txnschedulers branch 2 times, most recently from 4b6f595 to cf072ef Compare August 17, 2020 08:46

kostko reviewed Aug 17, 2020

View reviewed changes

ptrus force-pushed the ptrus/feature/multiple-txnschedulers branch 6 times, most recently from 90e4f15 to 0bf97c0 Compare August 17, 2020 15:34

kostko reviewed Aug 17, 2020

View reviewed changes

go/consensus/tendermint/apps/roothash/transactions.go Outdated Show resolved Hide resolved

go/registry/api/runtime.go Outdated Show resolved Hide resolved

go/roothash/api/commitment/pool.go Outdated Show resolved Hide resolved

go/runtime/client/client.go Outdated Show resolved Hide resolved

ptrus force-pushed the ptrus/feature/multiple-txnschedulers branch 2 times, most recently from a3924b5 to cced41b Compare August 17, 2020 20:39

ptrus force-pushed the ptrus/feature/multiple-txnschedulers branch from 5428c9b to 866e82a Compare August 19, 2020 10:47

ptrus marked this pull request as ready for review August 19, 2020 11:03

ptrus requested review from peterjgilbert and pro-wh as code owners August 19, 2020 11:03

ptrus mentioned this pull request Aug 19, 2020

Support roothash timeout triggered by executors #3199

Closed

kostko reviewed Aug 19, 2020

View reviewed changes

ptrus force-pushed the ptrus/feature/multiple-txnschedulers branch from 866e82a to 4bf8ee4 Compare August 19, 2020 16:10

kostko reviewed Aug 19, 2020

View reviewed changes

ptrus force-pushed the ptrus/feature/multiple-txnschedulers branch 2 times, most recently from 78f9f41 to e71147d Compare August 19, 2020 18:04

kostko reviewed Aug 19, 2020

View reviewed changes

go/consensus/tendermint/apps/roothash/roothash.go Outdated Show resolved Hide resolved

go/worker/compute/executor/committee/node.go Show resolved Hide resolved

go/worker/compute/executor/committee/node.go Show resolved Hide resolved

ptrus force-pushed the ptrus/feature/multiple-txnschedulers branch 3 times, most recently from 58a905b to 202b578 Compare August 19, 2020 20:12

kostko reviewed Aug 19, 2020

View reviewed changes

go/consensus/tendermint/apps/roothash/state/round.go Outdated Show resolved Hide resolved

go/registry/api/runtime.go Outdated Show resolved Hide resolved

ptrus force-pushed the ptrus/feature/multiple-txnschedulers branch from 202b578 to a44b64f Compare August 19, 2020 20:57

ptrus force-pushed the ptrus/feature/multiple-txnschedulers branch 3 times, most recently from 511d8cb to 1de4b3b Compare August 20, 2020 09:23

kostko approved these changes Aug 20, 2020

View reviewed changes

ptrus force-pushed the ptrus/feature/multiple-txnschedulers branch from 1de4b3b to 72a48ae Compare August 20, 2020 10:51

ptrus added 4 commits August 20, 2020 12:51

go/registry/runtime: Validate runtime transaction scheduler parameters

8394c62

docs/runtime: Remove separate transaction scheduler committee

ed3cdf1

go/runtime/scheduling: Rename "batching" algorithm to "simple"

16499ba

ptrus force-pushed the ptrus/feature/multiple-txnschedulers branch from 72a48ae to 16499ba Compare August 20, 2020 10:52

ptrus merged commit e895f3e into master Aug 20, 2020

ptrus deleted the ptrus/feature/multiple-txnschedulers branch August 20, 2020 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go/worker/txnscheduler: Support multiple txnscheduler workers #3184

go/worker/txnscheduler: Support multiple txnscheduler workers #3184

ptrus commented Aug 11, 2020 •

edited

kostko left a comment

codecov bot commented Aug 15, 2020 •

edited

kostko left a comment

ptrus commented Aug 16, 2020

kostko Aug 19, 2020

kostko Aug 20, 2020

ptrus Aug 20, 2020

ptrus commented Aug 20, 2020

kostko Aug 20, 2020

ptrus commented Aug 20, 2020

	// Scheduler defines an algorithm for scheduling incoming transaction.
	// Scheduler defines an algorithm for scheduling incoming transactions.

go/worker/txnscheduler: Support multiple txnscheduler workers #3184

go/worker/txnscheduler: Support multiple txnscheduler workers #3184

Conversation

ptrus commented Aug 11, 2020 • edited

kostko left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 15, 2020 • edited

Codecov Report

kostko left a comment

Choose a reason for hiding this comment

ptrus commented Aug 16, 2020

kostko Aug 19, 2020

Choose a reason for hiding this comment

kostko Aug 20, 2020

Choose a reason for hiding this comment

ptrus Aug 20, 2020

Choose a reason for hiding this comment

ptrus commented Aug 20, 2020

kostko Aug 20, 2020

Choose a reason for hiding this comment

ptrus commented Aug 20, 2020

ptrus commented Aug 11, 2020 •

edited

codecov bot commented Aug 15, 2020 •

edited