Issue 9643043: worker: new Runner type
Descriptionworker: new Runner type
This means that we can restart workers without
taking down all other workers. It also enables us
to dynamically manage a set of workers,
paving the way for some of the multitenancy work
that's on the horizon.
We don't use it yet - in an upcoming CL this
will replace runTasks in jujud.
https://code.launchpad.net/~rogpeppe/juju-core/304-jujud-tasks-package/+merge/165108
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : cmd/jujud/tasks: new package #
Total comments: 25
Patch Set 3 : cmd/jujud/tasks: new package #Patch Set 4 : cmd/jujud/tasks: new package #Patch Set 5 : worker: new Runner type #
MessagesTotal messages: 9
Please take a look.
Sign in to reply to this message.
Nice work, mostly LGTM, only small comments. https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go File cmd/jujud/tasks/runner.go (right): https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:32: isFatal func(error) bool Would prefer both funcs as own types for readability, especially in NewRunner(). https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:57: func NewRunner(isFatal func(error) bool, moreImportant func(err0, err1 error) bool) *Runner { See above. Own types would also be better documentable. https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:86: return nil Would be nice to get a possible startFunc() error returned immediately on a return channel in startReq{}. https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:147: break Wouldn't the request to start a different func with the same id be a logical error? So in case of an already running task with the wanted id this should be returned (e.g. ErrDuplicateId or ErrAlreadyRunning).
Sign in to reply to this message.
https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go File cmd/jujud/tasks/runner.go (right): https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:57: func NewRunner(isFatal func(error) bool, moreImportant func(err0, err1 error) bool) *Runner { On 2013/05/22 13:21:52, TheMue wrote: > See above. Own types would also be better documentable. I'm not sure. I think the function signatures are themselves reasonably self-explanatory, and this is the only place that they're visible in the public interface. https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:86: return nil On 2013/05/22 13:21:52, TheMue wrote: > Would be nice to get a possible startFunc() error returned immediately on a > return channel in startReq{}. i deliberately don't want to do this - task start is subject to the same restarting logic as task fail, and we don't want to wait ages here to see if it finally fails. the idea is that tasks are fire-and-forget, and the task runner exits only when we encounter a fatal error. https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:147: break On 2013/05/22 13:21:52, TheMue wrote: > Wouldn't the request to start a different func with the same id be a logical > error? So in case of an already running task with the wanted id this should be > returned (e.g. ErrDuplicateId or ErrAlreadyRunning). I think it's reasonable to say that calling StartTask of the same id twice in a row is a no-op. It simplifies the code and I can't see any particular problem with it.
Sign in to reply to this message.
This looks pretty good in general. I've got a bunch of comments though (mostly trivial and/or cosmetic). https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go File cmd/jujud/tasks/runner.go (right): https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:1: package tasks Missing copyright notice :) https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:55: // (determined by calling moreImportant) will be returned from Do we really need the flexibility that moreImportant can provide us? Will it be sufficient to start with errorPolicy = one of 'first-seen', 'last-seen' (or maybe just default to 'first-seen') or do you already envision usage for moreImportant function? https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:80: // If there is already a task with the given id, nothing will be done. Does it not make sense to allow multiple tasks with the same id, but then construct internal ids atomically to allow for multiple tasks when multitenancy comes into play. Of course, this is looking too much into future, so it's very good to leave it out of this branch (or assume that it's the responsibility of whatever sets up the runner to give unique IDs out, or perhaps you want to group them by "tenant" inside the tasks struct), but I am just wondering of the design direction you are thinking of. https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:95: // StartTask returns ErrDead if the runner is not running. s/StartTask/StopTask/ https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:152: // start function. When reading about 'tasks', I expected something different. RestartDelay confused me a bit at first, but now I see that this is more about running functions "daemonized" (i.e. run them and keep them running). I can't give a better suggestion for a name (task, to me, is more of a thing that you do and once it's done, you forget about it; granted, English is not my first language so maybe that's where the confusion comes from), but if you can come up with something that'd reflect this property as well, it'd only be for the better. https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner_test.go File cmd/jujud/tasks/runner_test.go (right): https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner_test.... cmd/jujud/tasks/runner_test.go:1: package tasks_test Missing copyright notice :)
Sign in to reply to this message.
https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go File cmd/jujud/tasks/runner.go (right): https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:57: func NewRunner(isFatal func(error) bool, moreImportant func(err0, err1 error) bool) *Runner { On 2013/05/22 13:31:59, rog wrote: > On 2013/05/22 13:21:52, TheMue wrote: > > See above. Own types would also be better documentable. > > I'm not sure. I think the function signatures are themselves > reasonably self-explanatory, and this is the only place that > they're visible in the public interface. Here at least the description of moreImportant should be extended. When shall true be returned? I think you want it in case of err0 is more important than err1, but please add a little note. https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:86: return nil On 2013/05/22 13:31:59, rog wrote: > On 2013/05/22 13:21:52, TheMue wrote: > > Would be nice to get a possible startFunc() error returned immediately on a > > return channel in startReq{}. > > i deliberately don't want to do this - task start is subject to the same > restarting logic as task fail, and we don't want to wait ages here > to see if it finally fails. > > the idea is that tasks are fire-and-forget, and the task runner exits only when > we encounter a fatal error. OK, reasonable. https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:147: break On 2013/05/22 13:31:59, rog wrote: > On 2013/05/22 13:21:52, TheMue wrote: > > Wouldn't the request to start a different func with the same id be a logical > > error? So in case of an already running task with the wanted id this should be > > returned (e.g. ErrDuplicateId or ErrAlreadyRunning). > > I think it's reasonable to say that calling StartTask of the same id twice in a > row is a no-op. It simplifies the code and I can't see any particular problem > with it. I thought of a time x where "myTask" with MyTaskX is started but at a time y later "myTask" again but with MyTaskY. Without an error one would think MyTaskY is running.
Sign in to reply to this message.
https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go File cmd/jujud/tasks/runner.go (right): https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:55: // (determined by calling moreImportant) will be returned from On 2013/05/22 13:39:16, Danilo wrote: > Do we really need the flexibility that moreImportant can provide us? Will it be > sufficient to start with errorPolicy = one of 'first-seen', 'last-seen' (or > maybe just default to 'first-seen') or do you already envision usage for > moreImportant function? we already have a use for this generality - look for moreImportant in cmd/jujud. https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:80: // If there is already a task with the given id, nothing will be done. On 2013/05/22 13:39:16, Danilo wrote: > Does it not make sense to allow multiple tasks with the same id, but then > construct internal ids atomically to allow for multiple tasks when multitenancy > comes into play. Of course, this is looking too much into future, so it's very > good to leave it out of this branch (or assume that it's the responsibility of > whatever sets up the runner to give unique IDs out, or perhaps you want to group > them by "tenant" inside the tasks struct), but I am just wondering of the design > direction you are thinking of. I am assuming that it is the responsibility of the caller to manage the id name space appropriately - the idea is that an id will imply a task and the caller uses the id to craft the function to pass along with it. https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:152: // start function. On 2013/05/22 13:39:16, Danilo wrote: > When reading about 'tasks', I expected something different. RestartDelay > confused me a bit at first, but now I see that this is more about running > functions "daemonized" (i.e. run them and keep them running). I can't give a > better suggestion for a name (task, to me, is more of a thing that you do and > once it's done, you forget about it; granted, English is not my first language > so maybe that's where the confusion comes from), but if you can come up with > something that'd reflect this property as well, it'd only be for the better. one possibility that i considered was to have the functionality of this package inside juju-core/worker. then the type would be named worker.Runner and the interface worker.Worker. does that sound better to you?
Sign in to reply to this message.
Thanks for responding, this LGTM. https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go File cmd/jujud/tasks/runner.go (right): https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:152: // start function. It does, but only if it's not too much effort (since everything else is good).
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go File cmd/jujud/tasks/runner.go (right): https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:1: package tasks On 2013/05/22 13:39:16, Danilo wrote: > Missing copyright notice :) Done. https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:95: // StartTask returns ErrDead if the runner is not running. On 2013/05/22 13:39:16, Danilo wrote: > s/StartTask/StopTask/ Done. https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:147: break On 2013/05/22 13:42:14, TheMue wrote: > On 2013/05/22 13:31:59, rog wrote: > > On 2013/05/22 13:21:52, TheMue wrote: > > > Wouldn't the request to start a different func with the same id be a logical > > > error? So in case of an already running task with the wanted id this should > be > > > returned (e.g. ErrDuplicateId or ErrAlreadyRunning). > > > > I think it's reasonable to say that calling StartTask of the same id twice in > a > > row is a no-op. It simplifies the code and I can't see any particular problem > > with it. > > I thought of a time x where "myTask" with MyTaskX is started but at a time y > later "myTask" again but with MyTaskY. Without an error one would think MyTaskY > is running. If you do that, then you're using it wrong. An id should be forever associated with a particular task. I have code in an upcoming branch that already relies on the idempotency of StartTask (now StartWorker) https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner.go#ne... cmd/jujud/tasks/runner.go:152: // start function. On 2013/05/22 14:34:41, Danilo wrote: > It does, but only if it's not too much effort (since everything else is good). Done. https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner_test.go File cmd/jujud/tasks/runner_test.go (right): https://codereview.appspot.com/9643043/diff/2001/cmd/jujud/tasks/runner_test.... cmd/jujud/tasks/runner_test.go:1: package tasks_test On 2013/05/22 13:39:16, Danilo wrote: > Missing copyright notice :) Done.
Sign in to reply to this message.
*** Submitted: worker: new Runner type This means that we can restart workers without taking down all other workers. It also enables us to dynamically manage a set of workers, paving the way for some of the multitenancy work that's on the horizon. We don't use it yet - in an upcoming CL this will replace runTasks in jujud. R=TheMue, Danilo CC= https://codereview.appspot.com/9643043
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
