Issue 6589045: uniter: filter type
Descriptionuniter: filter type
Differs from preview in a couple of notable respects:
* has its own tomb, and usual Stop/Wait/Dying methods: it's easier to test,
and IMO clearer and more consistent with our existing code.
* no longer expects to mess around with the uniter's tomb in any way
* thanks to unit.Service, it's much neater to construct
It's not yet integrated with the Uniter. Based on other discussions in
various places, expect it to evolve in the followup.
https://code.launchpad.net/~fwereade/juju-core/uniter-filter-type/+merge/127142
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : uniter: filter type #
Total comments: 40
Patch Set 3 : uniter: filter type #Patch Set 4 : uniter: filter type #
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.
LGTM, very nice. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go File worker/uniter/filter.go (right): https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:66: var ok bool perhaps it might look better if all these vars were in a var ( ... ) block. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:86: // names, once the first event on the corresponding *Out chan is ready. s/\*Out/out/ ? https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:127: if service == nil { save some indentation with: if service != nil { break } ? OTOH, could this go above the unit.Resolved check? it might make it slightly more obvious that this is always executed the first time we get a unitw change. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:158: if err := unit.EnsureDying(); err != nil { i'm slightly troubled by the extra watcher polling interval this implies before we get around to actually acting on the unit dying. perhaps, as discussed online, putting life into a field and adding a setLife method might be a good way to do it. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:162: return fmt.Errorf("service died unexpectedly") perhaps it might still be best to be robust and EnsureDying on the unit. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:260: var tick = struct{}{} this name confused me when i saw it in the source ("tick" is usually used for time-based events). how about "empty"? or, tbh, struct{}{} itself isn't the prettiest thing in the world, but it would only be used 4 times in this file and it's immediately obvious.
Sign in to reply to this message.
Very good, and great splitting of branches too, thank you. A few comments: https://codereview.appspot.com/6589045/diff/3001/state/unit.go File state/unit.go (right): https://codereview.appspot.com/6589045/diff/3001/state/unit.go#newcode99 state/unit.go:99: func (u *Unit) Service() (*Service, error) { In most circumstances, when we have a unit, we have its service at hand already, and it's significantly cheaper to use this service than to go remote and ask the state about the unit's service. Maybe we'll have some good use cases soon, but it feels like we might resist a bit longer without it given the uses in this branch. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go File worker/uniter/filter.go (right): https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:13: // ErrDead indicates that a uniter's unit is dead, and that the Uniter We now have a worker.ErrDead, which I think makes sense here as well. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:112: case state.Dead: Sounds like a sane approach to consider it here and drop ship, but this is an unwanted behavior that we should log at least: log.Printf("unit was found dead while still running") https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:125: // If we have not previously done so, we discover the service and Why is this not being done upfront? If we have a uniter, we have a well-defined unit and service, and the service of a unit never changes. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:134: log.Printf("watching config changes") Both this and the watching of service changes below sounds like uninteresting events to log. They are local events part of normal initialization, and even for debugging they're not great as there's a delay between actual watching and the message. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:138: log.Printf("watching service changes") Same. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:158: if err := unit.EnsureDying(); err != nil { On 2012/10/01 09:16:24, rog wrote: > i'm slightly troubled by the extra watcher polling interval this implies before > we get around to actually acting on the unit dying. perhaps, as discussed > online, putting life into a field and adding a setLife method might be a good > way to do it. Let's please not introduce these kinds of workarounds now. Besides being a non-relevant delay, there are many things we can do to improve that timing without dirtying up the high-level implementations relying on it. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:184: // On request, reactivate an out chan to ensure an event will be sent. // On request, make sure an out chan is active ... (it's not necessarily activating as it might already be active) https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:260: var tick = struct{}{} FWIW, I generally call it as "nothing".
Sign in to reply to this message.
thanks guys, good comments -- responses below. Would appreciate your thoughts. https://codereview.appspot.com/6589045/diff/3001/state/unit.go File state/unit.go (right): https://codereview.appspot.com/6589045/diff/3001/state/unit.go#newcode99 state/unit.go:99: func (u *Unit) Service() (*Service, error) { On 2012/10/01 18:55:38, niemeyer wrote: > In most circumstances, when we have a unit, we have its service at hand already, > and it's significantly cheaper to use this service than to go remote and ask the > state about the unit's service. I'm certainly not suggesting it be used in such situations. Consider Charm/CharmURL on Service -- both have their place :). > Maybe we'll have some good use cases soon, but it feels like we might resist a > bit longer without it given the uses in this branch. I think it's a very useful use, as discussed elsewhere :). https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go File worker/uniter/filter.go (right): https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:13: // ErrDead indicates that a uniter's unit is dead, and that the Uniter On 2012/10/01 18:55:38, niemeyer wrote: > We now have a worker.ErrDead, which I think makes sense here as well. Cool, I missed that. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:66: var ok bool On 2012/10/01 09:16:24, rog wrote: > perhaps it might look better if all these vars > were in a var ( > ... > ) block. Good idea. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:86: // names, once the first event on the corresponding *Out chan is ready. On 2012/10/01 09:16:24, rog wrote: > s/\*Out/out/ ? Thanks https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:112: case state.Dead: On 2012/10/01 18:55:38, niemeyer wrote: > Sounds like a sane approach to consider it here and drop ship, but this is an > unwanted behavior that we should log at least: > > log.Printf("unit was found dead while still running") I think what it's most likely to mean is "someone did remove-unit --force". https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:125: // If we have not previously done so, we discover the service and On 2012/10/01 18:55:38, niemeyer wrote: > Why is this not being done upfront? If we have a uniter, we have a well-defined > unit and service, and the service of a unit never changes. GAAH reitveld :/. Ate a long response. Bah. Ok. 1) The pre-existing unit/service are owned by the wrong goroutine, so that's a red herring. 2) If we pass in all the watchers, like the original implementation, or just start them all upfront, a correct implementation (unlike the original) has to: * handle a unit change to get a *Unit before it risks getting a service that that necessitates a unit.EnsureDying. * handle a unit change and ensure that its Life has been inspected and reacted to before we send any events (because the Modes' reactions to events sometimes depend on unit state, and I'd be keen to avoid the possibility of apparent Dying->Alive transitions). So: we can pass in, or create upfront 3, watchers (soon 4 -- relations) and avoid using them until later[0]... or we can pass in 1 watcher and make it impossible to even seek truths we can't handle. That seemed to me to be a big win... [0] using whichever technique, either basically a duplicated unit block, or nil-switching of service/config watch channels. I think I prefer the latter but am not hugely bothered. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:127: if service == nil { On 2012/10/01 09:16:24, rog wrote: > save some indentation with: > > if service != nil { > break > } > ? I vacillated; happy to change it. > OTOH, could this go above the unit.Resolved check? it might make it slightly > more obvious that this is always executed the first time we get a unitw change. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:134: log.Printf("watching config changes") On 2012/10/01 18:55:38, niemeyer wrote: > Both this and the watching of service changes below sounds like uninteresting > events to log. They are local events part of normal initialization, and even for > debugging they're not great as there's a delay between actual watching and the > message. Fair enough. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:138: log.Printf("watching service changes") On 2012/10/01 18:55:38, niemeyer wrote: > Same. How about "filter running" here, to go with s/watching unit changes/filter starting up/ at the top of the loop? https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:158: if err := unit.EnsureDying(); err != nil { On 2012/10/01 18:55:38, niemeyer wrote: > On 2012/10/01 09:16:24, rog wrote: > > i'm slightly troubled by the extra watcher polling interval this implies > before > > we get around to actually acting on the unit dying. perhaps, as discussed > > online, putting life into a field and adding a setLife method might be a good > > way to do it. > > Let's please not introduce these kinds of workarounds now. Besides being a > non-relevant delay, there are many things we can do to improve that timing > without dirtying up the high-level implementations relying on it. Yeah, I'm personally even comfortable with the delay, given that it allows us to have Just One Path that closes unitDying. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:162: return fmt.Errorf("service died unexpectedly") On 2012/10/01 09:16:24, rog wrote: > perhaps it might still be best to be robust and EnsureDying on the unit. I'm not so sure that's entirely robust -- this is a whoa-we're-totally-screwed moment and I don't think we can safely infer any action at all from a clearly-insane state. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:184: // On request, reactivate an out chan to ensure an event will be sent. On 2012/10/01 18:55:38, niemeyer wrote: > // On request, make sure an out chan is active ... > > (it's not necessarily activating as it might already be active) Better, thanks https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:260: var tick = struct{}{} On 2012/10/01 18:55:38, niemeyer wrote: > FWIW, I generally call it as "nothing". I like nothing... and I still feel it has a slight but real readability benefit.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go File worker/uniter/filter.go (right): https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:13: // ErrDead indicates that a uniter's unit is dead, and that the Uniter On 2012/10/02 06:45:19, fwereade wrote: > On 2012/10/01 18:55:38, niemeyer wrote: > > We now have a worker.ErrDead, which I think makes sense here as well. > > Cool, I missed that. Done. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:66: var ok bool On 2012/10/02 06:45:19, fwereade wrote: > On 2012/10/01 09:16:24, rog wrote: > > perhaps it might look better if all these vars > > were in a var ( > > ... > > ) block. > > Good idea. Done. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:86: // names, once the first event on the corresponding *Out chan is ready. On 2012/10/02 06:45:19, fwereade wrote: > On 2012/10/01 09:16:24, rog wrote: > > s/\*Out/out/ ? > > Thanks Done. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:112: case state.Dead: On 2012/10/02 06:45:19, fwereade wrote: > On 2012/10/01 18:55:38, niemeyer wrote: > > Sounds like a sane approach to consider it here and drop ship, but this is an > > unwanted behavior that we should log at least: > > > > log.Printf("unit was found dead while still running") > > I think what it's most likely to mean is "someone did remove-unit --force". (My point is that it's not entirely unexpected, and a simple "unit is dead" should suffice. Assuming that's OK, Done.) https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:127: if service == nil { On 2012/10/02 06:45:19, fwereade wrote: > On 2012/10/01 09:16:24, rog wrote: > > save some indentation with: > > > > if service != nil { > > break > > } > > ? > > I vacillated; happy to change it. > > > OTOH, could this go above the unit.Resolved check? it might make it slightly > > more obvious that this is always executed the first time we get a unitw > change. Done. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:134: log.Printf("watching config changes") On 2012/10/02 06:45:19, fwereade wrote: > On 2012/10/01 18:55:38, niemeyer wrote: > > Both this and the watching of service changes below sounds like uninteresting > > events to log. They are local events part of normal initialization, and even > for > > debugging they're not great as there's a delay between actual watching and the > > message. > > Fair enough. Done. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:184: // On request, reactivate an out chan to ensure an event will be sent. On 2012/10/02 06:45:19, fwereade wrote: > On 2012/10/01 18:55:38, niemeyer wrote: > > // On request, make sure an out chan is active ... > > > > (it's not necessarily activating as it might already be active) > Better, thanks Done. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:260: var tick = struct{}{} On 2012/10/02 06:45:19, fwereade wrote: > On 2012/10/01 18:55:38, niemeyer wrote: > > FWIW, I generally call it as "nothing". > > I like nothing... and I still feel it has a slight but real readability benefit. Done.
Sign in to reply to this message.
LGTM. A couple of final comments for your consideration. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go File worker/uniter/filter.go (right): https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:125: // If we have not previously done so, we discover the service and On 2012/10/02 06:45:19, fwereade wrote: > On 2012/10/01 18:55:38, niemeyer wrote: > So: we can pass in, or create upfront 3, watchers (soon 4 -- relations) and > avoid using them until later[0]... or we can pass in 1 watcher and make it > impossible to even seek truths we can't handle. That seemed to me to be a big > win... I apologize for still not understanding. It sounds so straightforward to me: 1) Load unit 2) Load service 4) Is unit or service dying? Bail if so. 3) Initialize watchers 5) Get into loop. Why is it any more complex than that? This seems like straightforward code that needs no explanation. As the logic is right now, there are heads-up comments both at the top and within the select loop describing why it's different than what one would inherently expect. I won't argue over this, though. If you still feel more comfortable as it is, let's move on. https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:138: log.Printf("watching service changes") On 2012/10/02 06:45:19, fwereade wrote: > On 2012/10/01 18:55:38, niemeyer wrote: > > Same. > > How about "filter running" here, to go with s/watching unit changes/filter > starting up/ at the top of the loop? What is a filter and why wasn't it running before? I'd drop both this and the filter starting up messages, or at least turn them into Debugf.
Sign in to reply to this message.
*** Submitted: uniter: filter type Differs from preview in a couple of notable respects: * has its own tomb, and usual Stop/Wait/Dying methods: it's easier to test, and IMO clearer and more consistent with our existing code. * no longer expects to mess around with the uniter's tomb in any way * thanks to unit.Service, it's much neater to construct It's not yet integrated with the Uniter. Based on other discussions in various places, expect it to evolve in the followup. R=rog, niemeyer CC= https://codereview.appspot.com/6589045 https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go File worker/uniter/filter.go (right): https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:125: // If we have not previously done so, we discover the service and On 2012/10/03 19:31:53, niemeyer wrote: > On 2012/10/02 06:45:19, fwereade wrote: > > On 2012/10/01 18:55:38, niemeyer wrote: > > So: we can pass in, or create upfront 3, watchers (soon 4 -- relations) and > > avoid using them until later[0]... or we can pass in 1 watcher and make it > > impossible to even seek truths we can't handle. That seemed to me to be a big > > win... > > I apologize for still not understanding. It sounds so straightforward to me: > > 1) Load unit > 2) Load service > 4) Is unit or service dying? Bail if so. > 3) Initialize watchers > 5) Get into loop. > > Why is it any more complex than that? This seems like straightforward code that > needs no explanation. As the logic is right now, there are heads-up comments > both at the top and within the select loop describing why it's different than > what one would inherently expect. > > I won't argue over this, though. If you still feel more comfortable as it is, > let's move on. I just had another go, and the duplication upset me again; I'll probably submit this as-is, and have another in the followup (which I *think* will require a *State anyway, which alters the balance of the filter a little, and feels like it might cause it to click). https://codereview.appspot.com/6589045/diff/3001/worker/uniter/filter.go#newc... worker/uniter/filter.go:138: log.Printf("watching service changes") On 2012/10/03 19:31:53, niemeyer wrote: > On 2012/10/02 06:45:19, fwereade wrote: > > On 2012/10/01 18:55:38, niemeyer wrote: > > > Same. > > > > How about "filter running" here, to go with s/watching unit changes/filter > > starting up/ at the top of the loop? > > What is a filter and why wasn't it running before? > > I'd drop both this and the filter starting up messages, or at least turn them > into Debugf. Gone :-).
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
