[occ] Add basic worker task and scheduler shell #328

stevenlanders · 2023-10-11T12:11:37Z

Describe your changes and provide context

Adds a basic scheduler shell (see TODOs)
Adds a basic task definition with request/response/index
Listens to abort channel after an execution to determine conflict

Testing performed to validate your change

Compiles (holding off until shape is validated)
Basic Unit Test for ProcessAll

udpatil · 2023-10-12T18:12:16Z

types/occ/scheduler.go

+	Incarnation int
+	Request     types.RequestDeliverTx
+	Response    *types.ResponseDeliverTx
+}


do we need to store any data here about "dependent TX" for the ESTIMATE abort case?

yes once we do the estimate/validation pieces there are a few things we'll need to track

udpatil · 2023-10-12T18:16:58Z

types/occ/scheduler.go

+				case <-gCtx.Done():
+					return gCtx.Err()
+				case task, ok := <-ch:
+					if !ok {


can we refactor this logic under case to a function like receiveTask?

udpatil · 2023-10-12T18:18:37Z

types/occ/scheduler.go

+// TODO: error scenarios
+func (s *scheduler) executeAll(ctx sdk.Context, tasks []*Task) error {
+	ch := make(chan *Task, len(tasks))
+	grp, gCtx := errgroup.WithContext(ctx.Context())


shouldn't we create the context WithCancel so that we can kill off the workers from the scheduler? we check the context.Done case below, but we would still need to indicate Done for the context via Cancel right?

With errgroups, one can just return an error like context timeout error, and all ctx.Done()s will cancel. If something outside a task needs to cancel, then yes we can wrap errgroup with a WithCancel context, but then it's still going to be the ctx.Done() that exits those routines.

udpatil · 2023-10-12T18:19:51Z

types/occ/scheduler.go

+						return nil
+					}
+					//TODO: ensure version multi store is on context
+					//abortCh := make(chan Abort)


we can wrap mvkv here prior to calling the delivertx

udpatil · 2023-10-12T18:21:36Z

types/occ/scheduler.go

+					}
+					//TODO: ensure version multi store is on context
+					//abortCh := make(chan Abort)
+					resp := s.deliverTx(ctx, task.Request)


wouldnt it be better here to call deliverTx async, otherwise there would be a deadlock since we'd never get to the channel read below? and then below we have a select statement with abortChannel, deliverTx response, or context Canceled?

(I added a comment on this, but basically if the abort channel is buffered, writes won't block)

udpatil · 2023-10-12T18:25:14Z

types/occ/scheduler.go

+
+		// validate returns any that should be re-executed
+		// note this processes ALL tasks, not just those recently executed
+		toExecute, err = s.validateAll(ctx, tasks)


we want validation to also be ongoing and not done after all executions are done, that way if we fail validation for tx X we can immediately re-execute instead of having waited for ALL the txs to have executed before realizing the validation failed

Is it clear that as soon as validation fails is when we know it should be immediately re-executed? (If we have to prioritize re-executions?) That wasn't yet clear to me because it seems like we have to do a full-validation at the end anyway.

udpatil · 2023-10-17T13:49:20Z

tasks/scheduler.go

-	TaskStatusValidated TaskStatus = "validated"
+	// statusPending tasks are ready for execution
+	// all executing tasks are in pending state
+	statusPending status = "pending"


shouldnt these be StatusPending so theyre public?

at the moment tasks don't leak the scheduler so nothing else cares

udpatil · 2023-10-17T13:55:21Z

tasks/scheduler.go

-func (s *scheduler) validateAll(ctx sdk.Context, tasks []*Task) ([]*Task, error) {
-	var res []*Task
-	for _, t := range tasks {
+func (s *scheduler) validateAll(ctx sdk.Context, tasks []*deliverTxTask) ([]*deliverTxTask, error) {


this isnt actually doing any validation, should we rename this to something like collectTasksForValidation (since thats what it appears to be doing?)

it's going to be doing the validation, it's collecting tasks for re-execution

udpatil · 2023-10-17T13:56:40Z

types/occ/types.go

+)
+
+// Abort contains the information for a transaction's conflict
+type Abort struct {


i'm ok moving this to scheduler too if we want, otherwise if theres a concern of import cycle we can keep in types so it can be used by both store and scheduler

## Describe your changes and provide context - Adds a basic scheduler shell (see TODOs) - Adds a basic task definition with request/response/index - Listens to abort channel after an execution to determine conflict ## Testing performed to validate your change - Compiles (holding off until shape is validated) - Basic Unit Test for ProcessAll

- Adds a basic scheduler shell (see TODOs) - Adds a basic task definition with request/response/index - Listens to abort channel after an execution to determine conflict - Compiles (holding off until shape is validated) - Basic Unit Test for ProcessAll

## Describe your changes and provide context - Adds a basic scheduler shell (see TODOs) - Adds a basic task definition with request/response/index - Listens to abort channel after an execution to determine conflict ## Testing performed to validate your change - Compiles (holding off until shape is validated) - Basic Unit Test for ProcessAll

- Adds a basic scheduler shell (see TODOs) - Adds a basic task definition with request/response/index - Listens to abort channel after an execution to determine conflict - Compiles (holding off until shape is validated) - Basic Unit Test for ProcessAll

## Describe your changes and provide context - Adds a basic scheduler shell (see TODOs) - Adds a basic task definition with request/response/index - Listens to abort channel after an execution to determine conflict ## Testing performed to validate your change - Compiles (holding off until shape is validated) - Basic Unit Test for ProcessAll

stevenlanders added 3 commits October 11, 2023 08:10

add basic worker task and scheduler shell

a6a9d4a

remove ok to avoid a white-hot loop

edf17c5

fix comment spelling

8ed5ba9

stevenlanders changed the title ~~add basic worker task and scheduler shell~~ Add basic worker task and scheduler shell Oct 11, 2023

stevenlanders added 4 commits October 11, 2023 18:29

add more skeleton

459ccd5

clarify comment

b06373e

init multi-version store

244f21d

updating comment

6f18c42

stevenlanders changed the title ~~Add basic worker task and scheduler shell~~ [occ] Add basic worker task and scheduler shell Oct 11, 2023

stevenlanders added 3 commits October 11, 2023 19:07

fix import cycle

7260454

remove context item for now (import cycle), needs refactor

a4b4a7f

add basic test

e8be7d5

stevenlanders requested a review from udpatil October 12, 2023 14:26

udpatil reviewed Oct 12, 2023

View reviewed changes

stevenlanders added 5 commits October 13, 2023 09:37

move packages

37bb06e

remove excess go func

588f1f6

cleanup

aea0cf9

add clarifying comments

a16fbc2

Merge branch 'occ-main' into add-worker-task-and-shell-scheduler

c1a37b7

udpatil reviewed Oct 17, 2023

View reviewed changes

udpatil approved these changes Oct 17, 2023

View reviewed changes

stevenlanders merged commit ab38f6b into occ-main Oct 17, 2023
14 checks passed

stevenlanders deleted the add-worker-task-and-shell-scheduler branch October 17, 2023 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[occ] Add basic worker task and scheduler shell #328

[occ] Add basic worker task and scheduler shell #328

stevenlanders commented Oct 11, 2023 •

edited

Loading

udpatil Oct 12, 2023

stevenlanders Oct 12, 2023

udpatil Oct 12, 2023

udpatil Oct 12, 2023

stevenlanders Oct 12, 2023

udpatil Oct 12, 2023

udpatil Oct 12, 2023

stevenlanders Oct 13, 2023

udpatil Oct 12, 2023

stevenlanders Oct 12, 2023

udpatil Oct 17, 2023

stevenlanders Oct 17, 2023

udpatil Oct 17, 2023

stevenlanders Oct 17, 2023

udpatil Oct 17, 2023

[occ] Add basic worker task and scheduler shell #328

[occ] Add basic worker task and scheduler shell #328

Conversation

stevenlanders commented Oct 11, 2023 • edited Loading

Describe your changes and provide context

Testing performed to validate your change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenlanders commented Oct 11, 2023 •

edited

Loading