[blockchain] v2 Routines by brapse · Pull Request #3878 · tendermint/tendermint

brapse · 2019-08-03T07:21:26Z

The architecture outlined in ADR-043 describes the interaction between concurrent routines including the scheduler and processor. These routines are aimed to allow better encapsulation of component business logic as well as provide consistent expectations around lifecycle management and concurrent message delivery. This PR includes an implementation of the Routine which is inspired by BaseService with certain key differences but shaped to fulfil some of the capabilities of the v1 fsm

Instead of a Start method which is expected to launch internal goroutines, Routines expose a Start method which are intended to be executed in a goroutine coordinates externally by the caller.
Routines are characterized by stateless func(event Event) (Events, error) which specify message schemes in the style of ADR-30
Routine are trySend()message and return false to indicate back pressure
Routines can be stopped gracefully, completing all sent message, or terminated, where all unprocessed messages are dropped.
Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

+ Include an implementaiton of the routines specified in ADR-43 along with a demuxer and some dummy reactor code

blockchain/v2/demuxer.go

blockchain/v2/routine.go

blockchain/v2/routine_test.go

+ `routine.send` returns false when routine is not running + this will prevent panics sending to channels which have been closed + Make output channels routine specific removing the risk of someone writting to a channel which was closed by another touine. + consistency changes between the routines and the demuxer

+ ensure that we stop accepting messages once `stop` has been called to avoid the case in which we attempt to write to a channel which has already been closed

blockchain/v2/routine.go

blockchain/v2/routine_test.go

blockchain/v2/reactor.go

blockchain/v2/types.go

blockchain/v2/demuxer.go

blockchain/v2/routine.go

blockchain/v2/routine_test.go

codecov-io · 2019-08-06T16:08:57Z

Codecov Report

Merging #3878 into master will decrease coverage by 0.14%.
The diff coverage is 48.03%.

@@            Coverage Diff             @@
##           master    #3878      +/-   ##
==========================================
- Coverage   66.86%   66.71%   -0.15%     
==========================================
  Files         221      225       +4     
  Lines       18510    18727     +217     
==========================================
+ Hits        12376    12493     +117     
- Misses       5211     5309      +98     
- Partials      923      925       +2

Impacted Files	Coverage Δ
blockchain/v2/schedule.go	`67.01% <ø> (ø)`	⬆️
blockchain/v2/metrics.go	`15.58% <15.58%> (ø)`
blockchain/v2/reactor.go	`49.15% <49.15%> (ø)`
blockchain/v2/types.go	`55.55% <55.55%> (ø)`
blockchain/v2/routine.go	`81.81% <81.81%> (ø)`
privval/signer_server.go	`95.65% <0%> (-4.35%)`	⬇️
privval/signer_endpoint.go	`81.33% <0%> (-2.67%)`	⬇️
privval/signer_dialer_endpoint.go	`100% <0%> (ø)`	⬆️
... and 8 more

+ use `trySend` the replicate peer sending + expose `next()` as a chan of events as output + expose `final()` as a chan of error, for the final error + add `ready()` as chan struct when routine is ready

blockchain/v2/demuxer.go

blockchain/v2/types.go

blockchain/v2/reactor.go

blockchain/v2/demuxer.go

blockchain/v2/reactor.go

melekes · 2019-08-09T08:07:36Z

blockchain/v2/demuxer.go

+	fin       chan error
+	stopped   chan struct{}
+	rdy       chan struct{}
+	running   *uint32


have you seen the common/BaseService struct https://godoc.org/github.com/tendermint/tendermint/libs/common#BaseService ? feels like you're doing exactly the same

I also thought routines could implement the service interface. I spent a bit of time trying trying to make it work but it felt wrong for a few reasons.

Routines expose methods associated with the lifecycle of a finite state machine while services don't. Implementing OnStart and OnReset for FSM didn't really make sense to me as we don't embed the state in the struct itself but instead close over it. This restricts how state in routines is managed but I would suggest this limitation is a good thing. We want
one routine to manage one state and the transition between states to happen from a serialized stream of events.

Routines will ideally terminate of their own accord and need to communicate their final event. This is an important requirement for the blockchain reactor as it needs to communicate when to switch to consensus.

While it would most likely be possible to modify the routine to adhere to the Service interface, i'm not sure it's worth it since I cannot envision any cases in which we would want either a Service or a Routine and not care about which.

blockchain/v2/demuxer.go

melekes · 2019-08-09T08:16:17Z

blockchain/v2/reactor.go

+	go r.processor.start()
+	go r.demuxer.start()
+
+	<-r.scheduler.ready()


Can't we use WaitGroup or https://godoc.org/golang.org/x/sync/errgroup here?

Definitely could use wait groups here but didn't want to include inter routine coordination in the routine. I updated the the code s/t the rdy is now closed as soon as the routine in ready, this way <-ready() will return immediately if the routine is "ready" as the name implies.

blockchain/v2/reactor.go

blockchain/v2/routine.go

melekes · 2019-08-09T08:29:58Z

blockchain/v2/routine.go

+
+type handleFunc = func(event Event) (Events, error)
+
+type Routine struct {


Could you explain the rational for this struct?

The intention for routines is to provide lifecycle management (start/stop) behaviour to adr-30 style finite state machines characterized by functions of the form:

type handleFunc = func(event Event) (Events, error)

The abstraction allows us to assert the fulfilment of event delivery guarantees without domain specific logic. It also allows us to assert the correctness of domain specific logic without coupling message delivery concerns.

+ close `rdy` channel to ensure that calls to `<-ready()` will always return if the routine is ready

ebuchman

High level thoughts from review:

Channel Capacity
Routine API
should start() panic if its already started ?

ebuchman · 2019-08-22T14:19:52Z

blockchain/v2/routine.go

+// * audit log levels
+// * Convert routine to an interface with concrete implmentation
+
+type handleFunc = func(event Event) (Events, error)


Do we need multiple events in the output?

I think you're right, switching it to single event output made it simpler.

ebuchman · 2019-08-22T14:21:06Z

blockchain/v2/routine.go

+	handle   handleFunc
+	logger   log.Logger
+	metrics  *Metrics
+	stopping *uint32


Better grouping

ebuchman · 2019-08-22T14:31:16Z

blockchain/v2/routine.go

+			rt.metrics.EventsOut.With("routine", rt.name).Add(float64(len(oEvents)))
+			rt.logger.Info(fmt.Sprintf("%s handled %d events\n", rt.name, len(oEvents)))
+			for _, event := range oEvents {
+				rt.logger.Info(fmt.Sprintln("writing back to output"))


Debug here.

Also just add the name to the logger itself.

ebuchman · 2019-08-22T14:37:36Z

blockchain/v2/routine.go

+			}
+			rt.metrics.ErrorsOut.With("routine", rt.name).Add(float64(len(oEvents)))
+			for _, event := range oEvents {
+				rt.out <- event


ebuchman · 2019-08-22T14:58:32Z

blockchain/v2/routine_test.go

+			time.Sleep(10 * time.Millisecond)
+		}
+	}()
+


Can we assert the above go routine is already running before we trySend the error ?

* Routines will now use a priority queue instead of channels to iterate over events

golangcibot · 2019-09-12T16:09:50Z

blockchain/v2/routine_test.go

+var done = fmt.Errorf("done")
+
+func simpleHandler(event Event) (Event, error) {
+	switch event.(type) {


singleCaseSwitch: should rewrite switch statement to if statement (from gocritic)

+ Simplify the design by demuxing events directly in the reactor

golangcibot · 2019-09-13T15:36:57Z

blockchain/v2/reactor.go

+}
+
+func schedulerHandle(event Event) (Event, error) {
+	switch event.(type) {


singleCaseSwitch: should rewrite switch statement to if statement (from gocritic)

golangcibot · 2019-09-13T15:36:58Z

blockchain/v2/reactor.go

+}
+
+func processorHandle(event Event) (Event, error) {
+	switch event.(type) {


singleCaseSwitch: should rewrite switch statement to if statement (from gocritic)

golangcibot · 2019-09-13T15:36:58Z

blockchain/v2/reactor.go

+			// XXX: check for backpressure
+			r.scheduler.trySend(event)
+			r.processor.trySend(event)
+		case _ = <-r.stopDemux:


S1005: '_ = <-ch' can be simplified to '<-ch' (from gosimple)

golangcibot · 2019-09-14T17:03:50Z

blockchain/v2/routine.go

+	}
+}
+
+func (rt *Routine) setLogger(logger log.Logger) {


U1000: func (*Routine).setLogger is unused (from unused)

golangcibot · 2019-09-17T21:49:36Z

blockchain/v2/types.go

+	"github.com/Workiva/go-datastructures/queue"
+)
+
+type Event queue.Item


Event redeclared in this block (from typecheck)

golangcibot · 2019-09-17T21:49:36Z

blockchain/v2/routine.go

+	if !rt.isRunning() {
+		return false
+	}
+	err := rt.queue.Put(event)


cannot use event (variable of type Event) as queue.Item value in argument to rt.queue.Put: missing method Compare (from typecheck)

ebuchman

Thanks sean. Merging for now, we'll keep iterating.

…m/tendermint/tendermint into brapse/blockchain-v2-riri-routine

blockchain v2: routines

d1671d6

+ Include an implementaiton of the routines specified in ADR-43 along with a demuxer and some dummy reactor code

brapse requested a review from ancazamfir August 3, 2019 07:21

golangcibot reviewed Aug 3, 2019

View reviewed changes

blockchain/v2/demuxer.go Outdated Show resolved Hide resolved

blockchain/v2/routine.go Outdated Show resolved Hide resolved

blockchain/v2/routine_test.go Show resolved Hide resolved

brapse added 2 commits August 6, 2019 13:27

Fix race condition in shutdown:

e4913f5

+ ensure that we stop accepting messages once `stop` has been called to avoid the case in which we attempt to write to a channel which has already been closed

golangcibot reviewed Aug 6, 2019

View reviewed changes

Solidify API:

c081b60

+ use `trySend` the replicate peer sending + expose `next()` as a chan of events as output + expose `final()` as a chan of error, for the final error + add `ready()` as chan struct when routine is ready

golangcibot reviewed Aug 8, 2019

View reviewed changes

blockchain/v2/demuxer.go Outdated Show resolved Hide resolved

blockchain/v2/types.go Outdated Show resolved Hide resolved

brapse added 2 commits August 8, 2019 15:53

cleanup events

5b880fb

demuxer cleanup

e826ca3

golangcibot reviewed Aug 8, 2019

View reviewed changes

blockchain/v2/reactor.go Show resolved Hide resolved

blockchain/v2/demuxer.go Outdated Show resolved Hide resolved

blockchain/v2/demuxer.go Outdated Show resolved Hide resolved

brapse added 2 commits August 8, 2019 16:54

typo fix

aeac474

set logger

acbfe67

golangcibot reviewed Aug 8, 2019

View reviewed changes

blockchain/v2/reactor.go Show resolved Hide resolved

linter fixes

2c8cbfc

brapse marked this pull request as ready for review August 8, 2019 15:44

brapse requested review from ebuchman, melekes and xla as code owners August 8, 2019 15:44

brapse changed the title ~~[blockchain] Routines~~ [blockchain] v2 Routines Aug 8, 2019

melekes reviewed Aug 9, 2019

View reviewed changes

brapse added 3 commits August 13, 2019 17:57

fixes based on feedback

78d4c3b

Add some docs

f81c319

Close rdy channel

9d41770

+ close `rdy` channel to ensure that calls to `<-ready()` will always return if the routine is ready

ebuchman reviewed Aug 22, 2019

View reviewed changes

melekes added the WIP label Sep 3, 2019

melekes assigned brapse Sep 3, 2019

Switch to a priority queue:

5474528

* Routines will now use a priority queue instead of channels to iterate over events

golangcibot reviewed Sep 12, 2019

View reviewed changes

brapse added 2 commits September 12, 2019 12:50

feedback tweaks

c62b7fb

Subsume the demuxer into the reactor

e7ee314

+ Simplify the design by demuxing events directly in the reactor

golangcibot reviewed Sep 13, 2019

View reviewed changes

brapse added 4 commits September 13, 2019 18:36

changes based on feedback

fbede85

rename trySend to end

9bd2c03

better debugging logging

822942a

align buffer sizes

99b7a33

golangcibot reviewed Sep 14, 2019

View reviewed changes

brapse added 2 commits September 17, 2019 15:18

tidying

d3d034e

Merge branch 'master' into brapse/blockchain-v2-riri-routine

ffb0667

golangcibot reviewed Sep 17, 2019

View reviewed changes

ebuchman approved these changes Sep 17, 2019

View reviewed changes

brapse added 4 commits September 18, 2019 15:22

merge fix

0cbf32d

Merge branch 'master' into brapse/blockchain-v2-riri-routine

211bd64

merge artifact go build file

2ae7a30

Merge branch 'brapse/blockchain-v2-riri-routine' of https://github.co…

fc77f8f

…m/tendermint/tendermint into brapse/blockchain-v2-riri-routine

brapse merged commit abab490 into master Sep 18, 2019

brapse deleted the brapse/blockchain-v2-riri-routine branch September 18, 2019 20:06


		type handleFunc = func(event Event) (Events, error)

		type Routine struct {

Conversation

brapse commented Aug 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brapse Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebuchman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebuchman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

brapse commented Aug 3, 2019 •

edited

Loading

codecov-io commented Aug 6, 2019 •

edited

Loading

brapse Aug 13, 2019 •

edited

Loading