Issue 6422049: initial HookQueue implementation
|
Created:
12 years, 8 months ago by fwereade Modified:
12 years, 8 months ago Reviewers:
mp+115731 Visibility:
Public. |
Descriptioninitial HookQueue implementation
Does not yet support saving/loading/reconciling state, but should correctly
handle all known situations in-memory, assuming a sane stream of added
RelationUnitsChange events.
https://code.launchpad.net/~fwereade/juju-core/relationer-hook-queue/+merge/115731
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : initial HookQueue implementation #Patch Set 3 : initial HookQueue implementation #Patch Set 4 : initial HookQueue implementation #
Total comments: 46
Patch Set 5 : initial HookQueue implementation #
Total comments: 20
Patch Set 6 : initial HookQueue implementation #
MessagesTotal messages: 8
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Ended up a bit longer than I expected, sorry about that. :( https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.go File worker/relationer/hookqueue.go (right): https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:10: // the hook. // HookInfo holds details about a relation hook that will // be needed for executing it. We need a lot more than what this type holds, so we don't have to get into details about what else is necessary here. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:14: Members map[string]map[string]interface{} Right above Members there should be a comment explaining what it contains. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:17: // HookQueue accepts state.RelationUnitsChange events and converts them I love this comment. So terse and so clear. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:18: // into HookInfos. s/HookInfos/HookInfo values/; I suggest we preserve the real type name whenever we use a type name in docs. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:21: head *unitInfo head, tail *unitInfo https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:29: members map[string]struct{} This is acting like a boolean, and every unit we have here is also in info. Can't we just have a boolean in *unitInfo instead, and have a single map? https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:34: // create HookInfo, by providing their latest settings data. It seems to be used to do a lot more than that below. I'd restrict the comment to what it is, rather than covering all the use cases. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:40: // if appropriate, what hook should be run next for that unit, and when. // unitInfo holds unit information for management by the hook queue. I think that is the core of what is said above. The fields themselves can be documented below. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:44: settings map[string]interface{} // hook holds the current idea of what's the next hook the // unit must run, and is empty when the unit isn't queued. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:58: // Add updates and compacts the queue, ensuring that there is no more than This description is a bit too close to the problem. It gives no hints about what "adding" or "updating" or "compacting" means. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:70: if info, found := q.info[unit]; !found { I'd put that line right above so you can use "info" later. info, found := q.info[unit] if !found { info = &unitInfo{unit: unit} q.info[unit] = info https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:78: // If known, and queued for depart, clear out the old entry... The append below removes too. This might be: if settings.Version == info.version { q.remove(unit) } else { q.append(unit, "changed") } https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:87: q.info[unit].version = settings.Version info.version = info.settings = https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:97: q.remove(unit) Same as above. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:112: // is ready to run, Next will panic. Does that panicking logic benefit us? I was imagining something like: info, ok := q.Next() So that we're always reminded that there may not be a next one, and get rid of Ready. Would that be better? https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:120: q.head.hook = "changed" I suspect there's something perhaps problematic about the magic here. Imagine this scenario: 1) Unit A joined; queue: [A-joined] 2) Next() => A-joined; queue: [A-changed] 3) Unit A departed; queue: [A-departed] 4) Next() => A-departed This means we're getting a -departed hook without ever doing the changed hook we promised, and the charm may be in a state not anticipated by the author. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:132: // Prev returns a new HookInfo configured as the one previously It's slightly confusing that we have a queue with prev and last, and an interface with Prev and Last, and those two are quite unrelated to each other. We might call it something else, but I'm kind of wondering why do we need this in the hook queue at all? Can't we easily keep track of the last result of Next in the call site instead, and avoid all the logic here, and the potential of calling a method that explodes depending on internal state? https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:139: // Hey, we may be able to remove a redundant event from the queue! That's a weird side-effect of calling Prev. I'd avoid the temptation to optimize this here for now. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:162: // panic if the unit is not known. // It will panic if the unit is not in q.info. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:171: // If the queue is empty, place it at the start. I'd move this logic to the bottom, and say instead: // If the queue is empty, the tail is also the head. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:176: // Place it at the end. s/end/tail/ https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:185: // It will panic if the unit is not known. // It is fine to remove a unit that is not in the queue, // but it will panic if the unit is not in q.info. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:196: // If the info is at either end (or both ends) of the queue, I suspect we can do both in a single block: if info.prev == nil { q.head = info.next } else { info.prev.next = info.next } if info.next == nil { q.tail = info.prev } else { info.next.prev = info.prev }
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.go File worker/relationer/hookqueue.go (right): https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:10: // the hook. On 2012/07/23 16:40:12, niemeyer wrote: > // HookInfo holds details about a relation hook that will > // be needed for executing it. > > We need a lot more than what this type holds, so we don't have to get into > details about what else is necessary here. Done. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:14: Members map[string]map[string]interface{} On 2012/07/23 16:40:12, niemeyer wrote: > Right above Members there should be a comment explaining what it contains. Done. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:17: // HookQueue accepts state.RelationUnitsChange events and converts them On 2012/07/23 16:40:12, niemeyer wrote: > I love this comment. So terse and so clear. :) https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:18: // into HookInfos. On 2012/07/23 16:40:12, niemeyer wrote: > s/HookInfos/HookInfo values/; I suggest we preserve the real type name whenever > we use a type name in docs. Done. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:21: head *unitInfo On 2012/07/23 16:40:12, niemeyer wrote: > head, tail *unitInfo Done. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:29: members map[string]struct{} On 2012/07/23 16:40:12, niemeyer wrote: > This is acting like a boolean, and every unit we have here is also in info. > Can't we just have a boolean in *unitInfo instead, and have a single map? Nice. Done. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:34: // create HookInfo, by providing their latest settings data. On 2012/07/23 16:40:12, niemeyer wrote: > It seems to be used to do a lot more than that below. I'd restrict the comment > to what it is, rather than covering all the use cases. Done. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:40: // if appropriate, what hook should be run next for that unit, and when. On 2012/07/23 16:40:12, niemeyer wrote: > // unitInfo holds unit information for management by the hook queue. > > I think that is the core of what is said above. The fields themselves can be > documented below. Done. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:70: if info, found := q.info[unit]; !found { On 2012/07/23 16:40:12, niemeyer wrote: > I'd put that line right above so you can use "info" later. > > info, found := q.info[unit] > if !found { > info = &unitInfo{unit: unit} > q.info[unit] = info Done. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:78: // If known, and queued for depart, clear out the old entry... On 2012/07/23 16:40:12, niemeyer wrote: > The append below removes too. This might be: > > if settings.Version == info.version { > q.remove(unit) > } else { > q.append(unit, "changed") > } Done. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:87: q.info[unit].version = settings.Version On 2012/07/23 16:40:12, niemeyer wrote: > info.version = > info.settings = Done. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:97: q.remove(unit) On 2012/07/23 16:40:12, niemeyer wrote: > Same as above. Done. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:112: // is ready to run, Next will panic. On 2012/07/23 16:40:12, niemeyer wrote: > Does that panicking logic benefit us? > > I was imagining something like: > > info, ok := q.Next() > > So that we're always reminded that there may not be a next one, and get rid of > Ready. Would that be better? I *think* this is logically/emotionally equivalent to accessing an array out of bounds... and I *think* it will be convenient based on current speculation. Can we keep it for now please? https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:120: q.head.hook = "changed" On 2012/07/23 16:40:12, niemeyer wrote: > I suspect there's something perhaps problematic about the magic here. > > Imagine this scenario: > > 1) Unit A joined; queue: [A-joined] > 2) Next() => A-joined; queue: [A-changed] > 3) Unit A departed; queue: [A-departed] > 4) Next() => A-departed > > This means we're getting a -departed hook without ever doing the changed hook we > promised, and the charm may be in a state not anticipated by the author. Bah. This is a behaviour change from python, but it's obviously the right thing to do. Done. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:132: // Prev returns a new HookInfo configured as the one previously On 2012/07/23 16:40:12, niemeyer wrote: > It's slightly confusing that we have a queue with prev and last, and an > interface with Prev and Last, and those two are quite unrelated to each other. > We might call it something else, but I'm kind of wondering why do we need this > in the hook queue at all? Can't we easily keep track of the last result of Next > in the call site instead, and avoid all the logic here, and the potential of > calling a method that explodes depending on internal state? For posterity: we want to keep this information around here because the process might fall over after Next() but before the next Next(), in which case we should re-execute the hook; since we want to persist the rest of the queue here, it makes sense to persist the inflight hook as well. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:139: // Hey, we may be able to remove a redundant event from the queue! On 2012/07/23 16:40:12, niemeyer wrote: > That's a weird side-effect of calling Prev. I'd avoid the temptation to optimize > this here for now. I've kept it in for now, because it now lives in Done, which also needs to mess with our state in order to accommodate the always-run-changed-after-joined requirement. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:162: // panic if the unit is not known. On 2012/07/23 16:40:12, niemeyer wrote: > // It will panic if the unit is not in q.info. Done. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:171: // If the queue is empty, place it at the start. On 2012/07/23 16:40:12, niemeyer wrote: > I'd move this logic to the bottom, and say instead: > > // If the queue is empty, the tail is also the head. Done. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:176: // Place it at the end. On 2012/07/23 16:40:12, niemeyer wrote: > s/end/tail/ Done. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:185: // It will panic if the unit is not known. On 2012/07/23 16:40:12, niemeyer wrote: > // It is fine to remove a unit that is not in the queue, > // but it will panic if the unit is not in q.info. Done. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:196: // If the info is at either end (or both ends) of the queue, On 2012/07/23 16:40:12, niemeyer wrote: > I suspect we can do both in a single block: > > if info.prev == nil { > q.head = info.next > } else { > info.prev.next = info.next > } > if info.next == nil { > q.tail = info.prev > } else { > info.next.prev = info.prev > } So long as the unit was originally queued, yes, this is safe; and, nicer. Thanks. Done.
Sign in to reply to this message.
Some additional trivials and LGTM! Thanks! https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.go File worker/relationer/hookqueue.go (right): https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:112: // is ready to run, Next will panic. On 2012/07/24 11:00:49, fwereade wrote: > On 2012/07/23 16:40:12, niemeyer wrote: > > Does that panicking logic benefit us? > > > > I was imagining something like: > > > > info, ok := q.Next() > > > > So that we're always reminded that there may not be a next one, and get rid of > > Ready. Would that be better? > > I *think* this is logically/emotionally equivalent to accessing an array out of > bounds... and I *think* it will be convenient based on current speculation. Can > we keep it for now please? Sounds good. https://codereview.appspot.com/6422049/diff/3004/worker/relationer/hookqueue.... worker/relationer/hookqueue.go:139: // Hey, we may be able to remove a redundant event from the queue! On 2012/07/24 11:00:49, fwereade wrote: > On 2012/07/23 16:40:12, niemeyer wrote: > > That's a weird side-effect of calling Prev. I'd avoid the temptation to > optimize > > this here for now. > > I've kept it in for now, because it now lives in Done, which also needs to mess > with our state in order to accommodate the always-run-changed-after-joined > requirement. Sounds good too. https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue.go File worker/relationer/hookqueue.go (right): https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:8: // HookInfo holds details about a relation hook that will be required Sorry, my bad. I see the sentence is ambiguous now. Can we please invert it to make it clear: // HookInfo holds details required to execute a relation hook. https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:11: HookKind string Ah, helpful renaming. https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:14: // holding its relation settings, keyed on unit name. Nice, thanks. https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:25: // info holds information about every unit known to the queue. Might be worth clarifying that known != *in* the queue: // info holds information about all units that were added to the // queue and haven't had a "depart" event popped. This means the // unit may be in info and not currently in the queue itself. https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:43: // be run for the unit, and is empty if and only if the unit Great comment, thanks. https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:56: // Add updates the queue such that the stream of HookInfos returned from s/HookInfos/HookInfo values/ Otherwise, nice comment. Doesn't describe the algorithm but tells what it does from an API client perspective. https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:57: // next will reflect the state of the relation according to the supplied s/next/Peek and Pop/ https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:94: } else if hook == "changed" { || above? https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:102: // Pending returns true if there is a hook ready to run. That's a better name indeed. https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:107: // Next returns a HookInfo describing the next hook to run. Subsequent Head + Pop sound better to me. With logic equivalent to: foo := foo.Next() bar := foo.Next() I'd not expect foo and bar to point to the same thing. In fact, this is how mgo works: for iter.Next(&result) { ... } https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:109: // settings of Members (if they have been updated), until Done is called. It's not clear to me how Members could change. Can we please just talk about it quickly live? https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:146: // the previous call to Done. The second part of the condition covers the first part of it.
Sign in to reply to this message.
*** Submitted: initial HookQueue implementation Does not yet support saving/loading/reconciling state, but should correctly handle all known situations in-memory, assuming a sane stream of added RelationUnitsChange events. R=niemeyer CC= https://codereview.appspot.com/6422049 https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue.go File worker/relationer/hookqueue.go (right): https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:8: // HookInfo holds details about a relation hook that will be required On 2012/07/24 15:17:17, niemeyer wrote: > Sorry, my bad. I see the sentence is ambiguous now. Can we please invert it to > make it clear: > > // HookInfo holds details required to execute a relation hook. Done. https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:25: // info holds information about every unit known to the queue. On 2012/07/24 15:17:17, niemeyer wrote: > Might be worth clarifying that known != *in* the queue: > > // info holds information about all units that were added to the > // queue and haven't had a "depart" event popped. This means the > // unit may be in info and not currently in the queue itself. Done. https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:56: // Add updates the queue such that the stream of HookInfos returned from On 2012/07/24 15:17:17, niemeyer wrote: > s/HookInfos/HookInfo values/ > > Otherwise, nice comment. Doesn't describe the algorithm but tells what it does > from an API client perspective. Done. https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:57: // next will reflect the state of the relation according to the supplied On 2012/07/24 15:17:17, niemeyer wrote: > s/next/Peek and Pop/ Done. https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:94: } else if hook == "changed" { On 2012/07/24 15:17:17, niemeyer wrote: > || above? Ha, yes. Done. https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:107: // Next returns a HookInfo describing the next hook to run. Subsequent On 2012/07/24 15:17:17, niemeyer wrote: > Head + Pop sound better to me. > > With logic equivalent to: > > foo := foo.Next() > bar := foo.Next() > > I'd not expect foo and bar to point to the same thing. > > In fact, this is how mgo works: > > for iter.Next(&result) { ... } Done. https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:109: // settings of Members (if they have been updated), until Done is called. On 2012/07/24 15:17:17, niemeyer wrote: > It's not clear to me how Members could change. Can we please just talk about it > quickly live? Done. https://codereview.appspot.com/6422049/diff/10001/worker/relationer/hookqueue... worker/relationer/hookqueue.go:146: // the previous call to Done. On 2012/07/24 15:17:17, niemeyer wrote: > The second part of the condition covers the first part of it. I'm not quite sure, but I hate that sentence, so I'm happy to change it :).
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
