Issue 6495079: mstate/presence: foundation for presence handling
Descriptionmstate/presence: foundation for presence handling
https://code.launchpad.net/~niemeyer/juju-core/mstate-presence/+merge/122607
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : mstate/presence: foundation for presence handling #
Total comments: 73
Patch Set 3 : mstate/presence: foundation for presence handling #
Total comments: 2
Patch Set 4 : mstate/presence: foundation for presence handling #
MessagesTotal messages: 14
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
very nice design. lots of comments but nothing really substantial. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go File mstate/presence/presence.go (right): https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:8: // sequence number so that watchers can tell it is alive. I think most of this paragraph can be moved to the internal implementation documentation. Something like this should be sufficient: "A pinger works by periodically updating a document so that watchers can tell it is alive." https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:25: // "alive": { hex(<pinger seq> / 63) : (<pinger seq> % 63 | <other pingers>) }, presumably: 1 << (<pinger seq> % 63) | <other pingers> ? https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:33: // documents (the last two time slots). could we summarise the other documents (seqs, beings, pings) somewhere too? https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:36: type Watcher struct { i'd like to see some documentation on the fields in this structure. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:111: // event will be sent onto ch to report the initially known status An event will be sent onto ch to report the initial status for the key, ... https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:116: w.request <- reqAdd{key, ch} What if the watcher is dying? I think all request sends should select on Dying too. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:127: // ForceRefresh returns. i'm not sure what "but it is fine to consume the events after ForceRefresh returns" means. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:132: case <-done: If the watcher did close(all refresh channels) when finishing, there would be no need for this select, i think. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:142: select { no need for this select - we know that the watcher is processing this request. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:185: func (w *Watcher) flush() { a possible alternative phrasing of this loop that might make the logic clearer: // flush sends all pending events to their respective channels. func (w *Watcher) flush() { loop: for _, e := range w.pending { if e.ch == nil { continue // removed meanwhile } for { select { case <-w.tomb.Dying(): return case req := <-w.request: w.handle(req) case e.ch <- Change{e.key, e.alive}: continue loop } } } w.pending = w.pending[:0] } https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:230: } is the ordering of the watches channels important? if not, then something like this seems a little simpler and avoids the slice copy: for i, wch := range w.watches { if wch == ch { w.watches[i] = w.watches[len(w.watches)-1] w.watches = w.watches[0:len(w.watches)-1] break } } https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:240: case r.result <- alive: no need for this select - we know the request is waiting for our reply. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:244: panic(fmt.Errorf("unknown request: %T", req)) %#v ? not that it's gonna happen, of course :-) https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:260: // queues events to observing channels. The process is done by "It fetches the last two time slots and compares the union of both to the in-memory state." ? https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:273: var being beingInfo move above the later loop where it's actually used? https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:279: if _, err := fmt.Sscanf(key, "%x", &k); err != nil { k, err := strconv.ParseInt(key, 16, 64) ? it would save the declaration of k above. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:283: for i := int64(0); i < 64 && value > 0; i++ { i < 63 ? https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:302: if _, err := fmt.Sscanf(key, "%x", &k); err != nil { strconv.ParseInt ? https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:306: for i := int64(0); i < 64 && value > 0; i++ { i < 63 ? https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:321: } else if err != nil { s/else/\n/ ? https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:381: // Start starts periodically reporting that p's key is alive. s/periodically// ? https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:520: if _, err := p.pings.UpsertId(slot, bson.D{{"$inc", bson.D{{"alive." + p.fieldKey, p.fieldBit}}}}); err != nil { does the pings collection grow without bound? might that be a problem? https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:533: before := time.Now().UTC() why bother with the .UTC()? https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:535: after := time.Now().UTC() ditto
Sign in to reply to this message.
Pretty complex stuff, need to read it multiple times. So far only few comments. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go File mstate/presence/presence.go (right): https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:2: // The watcher package implements an interface for observing liveness s/watcher/presence/ https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:8: // sequence number so that watchers can tell it is alive. On 2012/09/04 08:28:02, rog wrote: > I think most of this paragraph can be moved to the internal implementation > documentation. > > Something like this should be sufficient: > > "A pinger works by periodically updating a document so that watchers can tell it > is alive." +1 https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:142: select { On 2012/09/04 08:28:02, rog wrote: > no need for this select - we know that the watcher is processing this request. Sure? If the method is called while the watcher is dying? https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:198: case e.ch <- Change{e.key, e.alive}: Thinking about a per key/channel pair timeout to avoid blocking when the owner of ch is blocked or dead. Problem here are optimal timeout values as well as the correct error handling (e.g. removing the key/channel pair after a number of errors). But that would lead to other problems, so don't care for my words. I'm just thinking loud. ;) https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:230: } On 2012/09/04 08:28:02, rog wrote: > is the ordering of the watches channels important? if not, then something like > this seems a little simpler and avoids the slice copy: > > for i, wch := range w.watches { > if wch == ch { > w.watches[i] = w.watches[len(w.watches)-1] > w.watches = w.watches[0:len(w.watches)-1] > break > } > } Alternatively, also to make tests more simple, a channel set could be introduced: type chanSet map[chan Change]bool func (s chanSet) add(ch chan Change) { s[ch] = true } func (s chanSet) remove(ch chan Change) { delete(s, ch) } To me w.watches[r.key].remove(r.ch) reads nice, especially in one or two year when we are in maintenance mode. ;) https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:533: before := time.Now().UTC() On 2012/09/04 08:28:02, rog wrote: > why bother with the .UTC()? Mongo uses UTC. So not using it server.Sub(before) would use the wrong location.
Sign in to reply to this message.
https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go File mstate/presence/presence.go (right): https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:142: select { On 2012/09/04 09:56:05, TheMue wrote: > On 2012/09/04 08:28:02, rog wrote: > > no need for this select - we know that the watcher is processing this request. > > Sure? If the method is called while the watcher is dying? yes, totally sure. the request channel is unbuffered, so if we manage to send the value, we know that the loop goroutine *must* have entered the <-w.request branch of the select and therefore the request *will* be processed and the reply will be sent. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:198: case e.ch <- Change{e.key, e.alive}: On 2012/09/04 09:56:05, TheMue wrote: > Thinking about a per key/channel pair timeout to avoid blocking when the owner > of ch is blocked or dead. Problem here are optimal timeout values as well as the > correct error handling (e.g. removing the key/channel pair after a number of > errors). > > But that would lead to other problems, so don't care for my words. I'm just > thinking loud. ;) This is my main pause for thought with this implementation too. To do it right, I think you'd have a goroutine per client always reading from the watcher and trying to write to the client. In practice, I don't think it matters though. All our current clients are always ready to receive AFAIK, so blocking until a client is ready is ok. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:533: before := time.Now().UTC() On 2012/09/04 09:56:05, TheMue wrote: > On 2012/09/04 08:28:02, rog wrote: > > why bother with the .UTC()? > > Mongo uses UTC. So not using it server.Sub(before) would use the wrong location. The time zone of a time doesn't make any difference to the time instant held in it. t.UTC().Sub(t) is always zero.
Sign in to reply to this message.
https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go File mstate/presence/presence.go (right): https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:142: select { On 2012/09/04 10:14:08, rog wrote: > On 2012/09/04 09:56:05, TheMue wrote: > > On 2012/09/04 08:28:02, rog wrote: > > > no need for this select - we know that the watcher is processing this > request. > > > > Sure? If the method is called while the watcher is dying? > > yes, totally sure. the request channel is unbuffered, so if we manage to send > the value, we know that the loop goroutine *must* have entered the <-w.request > branch of the select and therefore the request *will* be processed and the reply > will be sent. My thought has been: What happens if the send is already blocking, because the watcher loop is dying and doesn't receive anymore? https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:198: case e.ch <- Change{e.key, e.alive}: On 2012/09/04 10:14:08, rog wrote: > On 2012/09/04 09:56:05, TheMue wrote: > > Thinking about a per key/channel pair timeout to avoid blocking when the owner > > of ch is blocked or dead. Problem here are optimal timeout values as well as > the > > correct error handling (e.g. removing the key/channel pair after a number of > > errors). > > > > But that would lead to other problems, so don't care for my words. I'm just > > thinking loud. ;) > > This is my main pause for thought with this implementation too. To do it right, > I think you'd have a goroutine per client always reading from the watcher and > trying to write to the client. In practice, I don't think it matters though. All > our current clients are always ready to receive AFAIK, so blocking until a > client is ready is ok. Blocking until it's ready, yes. Only dead channel owners may become a problem. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:533: before := time.Now().UTC() On 2012/09/04 10:14:08, rog wrote: > On 2012/09/04 09:56:05, TheMue wrote: > > On 2012/09/04 08:28:02, rog wrote: > > > why bother with the .UTC()? > > > > Mongo uses UTC. So not using it server.Sub(before) would use the wrong > location. > > The time zone of a time doesn't make any difference to the time instant held in > it. t.UTC().Sub(t) is always zero. Oh, yes, took a look into the code. You're right, thx.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go File mstate/presence/presence.go (right): https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:2: // The watcher package implements an interface for observing liveness On 2012/09/04 09:56:05, TheMue wrote: > s/watcher/presence/ Done. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:8: // sequence number so that watchers can tell it is alive. On 2012/09/04 08:28:02, rog wrote: > I think most of this paragraph can be moved to the internal implementation > documentation. > > Something like this should be sufficient: > > "A pinger works by periodically updating a document so that watchers can tell it > is alive." I'll simplify it further. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:25: // "alive": { hex(<pinger seq> / 63) : (<pinger seq> % 63 | <other pingers>) }, On 2012/09/04 08:28:02, rog wrote: > presumably: 1 << (<pinger seq> % 63) | <other pingers> ? Done. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:33: // documents (the last two time slots). On 2012/09/04 08:28:02, rog wrote: > could we summarise the other documents (seqs, beings, pings) somewhere too? Good idea. Done. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:36: type Watcher struct { On 2012/09/04 08:28:02, rog wrote: > i'd like to see some documentation on the fields in this structure. Good point. Done. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:111: // event will be sent onto ch to report the initially known status On 2012/09/04 08:28:02, rog wrote: > An event will be sent onto ch to report the initial status for the key, ... Done. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:116: w.request <- reqAdd{key, ch} On 2012/09/04 08:28:02, rog wrote: > What if the watcher is dying? > I think all request sends should select on Dying too. Good point, done. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:127: // ForceRefresh returns. On 2012/09/04 08:28:02, rog wrote: > i'm not sure what "but it is fine to consume the events after ForceRefresh > returns" means. Clarified. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:132: case <-done: On 2012/09/04 08:28:02, rog wrote: > If the watcher did close(all refresh channels) when finishing, there would be no > need for this select, i think. With this select, there's no need to close all channels, though. :-) https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:142: select { On 2012/09/04 08:28:02, rog wrote: > no need for this select - we know that the watcher is processing this request. You're right, but I'd rather preserve this, so we don't have to comment on the other side that we're trusting on particular details of the implementation (e.g. if the other side decides to abort in the middle of handling this due to Dying, this will never return). https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:185: func (w *Watcher) flush() { On 2012/09/04 08:28:02, rog wrote: > a possible alternative phrasing of this loop that might make the logic clearer: Doesn't work. Consider what happens if w.pending gets a new entry while iterating. The current incarnation seems pleasantly straightforward, to be honest. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:198: case e.ch <- Change{e.key, e.alive}: On 2012/09/04 10:14:08, rog wrote: > On 2012/09/04 09:56:05, TheMue wrote: > > Thinking about a per key/channel pair timeout to avoid blocking when the owner > > of ch is blocked or dead. Problem here are optimal timeout values as well as > the > > correct error handling (e.g. removing the key/channel pair after a number of > > errors). > > > > But that would lead to other problems, so don't care for my words. I'm just > > thinking loud. ;) > > This is my main pause for thought with this implementation too. To do it right, > I think you'd have a goroutine per client always reading from the watcher and > trying to write to the client. In practice, I don't think it matters though. All > our current clients are always ready to receive AFAIK, so blocking until a > client is ready is ok. Exactly. It's convenient, efficient in multiple ways (e.g. if everything else is blocked, no point in spinning) and an easy property for us to build upon *now* (would be a bad afterthought). https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:230: } On 2012/09/04 08:28:02, rog wrote: > is the ordering of the watches channels important? if not, then something like > this seems a little simpler and avoids the slice copy: > > for i, wch := range w.watches { > if wch == ch { > w.watches[i] = w.watches[len(w.watches)-1] > w.watches = w.watches[0:len(w.watches)-1] > break > } > } Nice idea. We're not yet specifying the ordering among channels, so we may as well not trust it. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:230: } On 2012/09/04 09:56:05, TheMue wrote: > To me w.watches[r.key].remove(r.ch) reads nice, especially in one or two year > when we are in maintenance mode. ;) Not only the manipulation of these values is currently trivial, but in one or two years I'll still remember that a map per-key would be a very poor choice for the design. Consider the overall algorithm, how these values are used, what is the cost of a map and in which circumstances, and then consider in which directions the logic has to scale in all of those axis. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:240: case r.result <- alive: On 2012/09/04 08:28:02, rog wrote: > no need for this select - we know the request is waiting for our reply. We actually do not. Dying will correctly make it depart early. I've now buffered the channel, though, so we can avoid the select here. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:244: panic(fmt.Errorf("unknown request: %T", req)) On 2012/09/04 08:28:02, rog wrote: > %#v ? not that it's gonna happen, of course :-) Besides being hopefully extremely rare, the type is what it takes to understand the bug. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:260: // queues events to observing channels. The process is done by On 2012/09/04 08:28:02, rog wrote: > "It fetches the last two time slots and compares the union > of both to the in-memory state." ? Done. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:273: var being beingInfo On 2012/09/04 08:28:02, rog wrote: > move above the later loop where it's actually used? Done. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:279: if _, err := fmt.Sscanf(key, "%x", &k); err != nil { On 2012/09/04 08:28:02, rog wrote: > k, err := strconv.ParseInt(key, 16, 64) ? > it would save the declaration of k above. Aha, brilliant! I couldn't find it in a quick scan, and was too lazy to implement it myself thankfully. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:283: for i := int64(0); i < 64 && value > 0; i++ { On 2012/09/04 08:28:02, rog wrote: > i < 63 ? Indeed, thanks. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:302: if _, err := fmt.Sscanf(key, "%x", &k); err != nil { On 2012/09/04 08:28:02, rog wrote: > strconv.ParseInt ? Done. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:306: for i := int64(0); i < 64 && value > 0; i++ { On 2012/09/04 08:28:02, rog wrote: > i < 63 ? Indeed, thanks. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:321: } else if err != nil { On 2012/09/04 08:28:02, rog wrote: > s/else/\n/ ? Looks like pretty usual code to me. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:381: // Start starts periodically reporting that p's key is alive. On 2012/09/04 08:28:02, rog wrote: > s/periodically// ? That feels like useful information about the behavior. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:520: if _, err := p.pings.UpsertId(slot, bson.D{{"$inc", bson.D{{"alive." + p.fieldKey, p.fieldBit}}}}); err != nil { On 2012/09/04 08:28:02, rog wrote: > does the pings collection grow without bound? might that be a problem? Mongo 2.2 introduces TTL collections which can expire documents automatically, and I was planning to use it, but turns out it won't play well with the upserting nature of the collection use. I'll add a BUG comment for us to fix this in a follow up by sporadically cleaning both the beings and pings collections. It should be trivial as both have monotonically growing primary keys. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:533: before := time.Now().UTC() On 2012/09/04 08:28:02, rog wrote: > why bother with the .UTC()? Done. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:535: after := time.Now().UTC() On 2012/09/04 08:28:02, rog wrote: > ditto Done.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go File mstate/presence/presence.go (right): https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:142: select { On 2012/09/04 10:37:46, niemeyer wrote: > On 2012/09/04 08:28:02, rog wrote: > > no need for this select - we know that the watcher is processing this request. > > You're right, but I'd rather preserve this, so we don't have to comment on the > other side that we're trusting on particular details of the implementation (e.g. > if the other side decides to abort in the middle of handling this due to Dying, > this will never return). The invariant is easy to maintain - if you receive a request, you are bound to act on it. It makes both sides easier (no need to select on receive, no need to have a buffered reply channel), and marginally more correct (if the watcher is killed while the request is being processed, the reply will not be discarded). This is one of the things that makes synchronous channels so nice. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:185: func (w *Watcher) flush() { On 2012/09/04 10:37:46, niemeyer wrote: > On 2012/09/04 08:28:02, rog wrote: > > a possible alternative phrasing of this loop that might make the logic > clearer: > > Doesn't work. Consider what happens if w.pending gets a new entry while > iterating. > > The current incarnation seems pleasantly straightforward, to be honest. Of course. I hadn't appreciated that w.pending could be changed by calling w.handle. A comment might be worth it. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:321: } else if err != nil { On 2012/09/04 10:37:46, niemeyer wrote: > On 2012/09/04 08:28:02, rog wrote: > > s/else/\n/ ? > > Looks like pretty usual code to me. I prefer to see necessarily sequential code laid out like that. If there's an "else" it makes me look again to see if there's something I'm missing. It also makes the code denser for no particular reason. I realise that YMMV of course :-) https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:381: // Start starts periodically reporting that p's key is alive. On 2012/09/04 10:37:46, niemeyer wrote: > On 2012/09/04 08:28:02, rog wrote: > > s/periodically// ? > > That feels like useful information about the behavior. Ok. I was actually trying to get around the slightly awkward phrasing of the doc comment. Perhaps: // Start starts a goroutine that periodically reports that // p's. ? Or "starts a process". https://codereview.appspot.com/6495079/diff/8001/mstate/presence/presence.go File mstate/presence/presence.go (right): https://codereview.appspot.com/6495079/diff/8001/mstate/presence/presence.go#... mstate/presence/presence.go:39: // helper collection. That sequence number is then inserted into the s/a helper collection/the "seqs" collection/ ? https://codereview.appspot.com/6495079/diff/8001/mstate/presence/presence.go#... mstate/presence/presence.go:160: // have been prepared, but unblocks before changes are sent onto the s/unblocks/returns/
Sign in to reply to this message.
https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go File mstate/presence/presence.go (right): https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:142: select { On 2012/09/04 10:55:52, rog wrote: > The invariant is easy to maintain - if you receive a request, you are bound to We already have cases where the invariant isn't valid, so this is of course not true. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:185: func (w *Watcher) flush() { On 2012/09/04 10:55:52, rog wrote: > On 2012/09/04 10:37:46, niemeyer wrote: > > On 2012/09/04 08:28:02, rog wrote: > > > a possible alternative phrasing of this loop that might make the logic > > clearer: > > > > Doesn't work. Consider what happens if w.pending gets a new entry while > > iterating. > > > > The current incarnation seems pleasantly straightforward, to be honest. > > Of course. I hadn't appreciated that w.pending could be changed by calling > w.handle. A comment might be worth it. Done. https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:381: // Start starts periodically reporting that p's key is alive. On 2012/09/04 10:55:52, rog wrote: > On 2012/09/04 10:37:46, niemeyer wrote: > > On 2012/09/04 08:28:02, rog wrote: > > > s/periodically// ? > > > > That feels like useful information about the behavior. > > Ok. I was actually trying to get around the slightly awkward phrasing of the doc > comment. Perhaps: > > // Start starts a goroutine that periodically reports that > // p's. > > ? > Or "starts a process". I don't understand how that's related to the fact we say it "periodically" reports it. We don't explicitly tell whenever we have a background goroutine in other places.
Sign in to reply to this message.
https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go File mstate/presence/presence.go (right): https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:142: select { On 2012/09/04 11:07:18, niemeyer wrote: > On 2012/09/04 10:55:52, rog wrote: > > The invariant is easy to maintain - if you receive a request, you are bound to > > We already have cases where the invariant isn't valid, so this is of course not > true. interesting. i can't see any such cases, but i'm probably looking in the wrong place. (i don't count refresh, because it's not a synchronous reply - "acting on the request" in this case is simply adding the request to the queue).
Sign in to reply to this message.
https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go File mstate/presence/presence.go (right): https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:142: select { On 2012/09/04 11:56:18, rog wrote: > On 2012/09/04 11:07:18, niemeyer wrote: > > On 2012/09/04 10:55:52, rog wrote: > > > The invariant is easy to maintain - if you receive a request, you are bound > to > > > > We already have cases where the invariant isn't valid, so this is of course > not > > true. > > interesting. i can't see any such cases, but i'm probably looking in the wrong > place. (i don't count refresh, because it's not a "There's that invariant, which is actually not an invariant, but I don't care." -- not a line of thinking that is easy to correlate to.
Sign in to reply to this message.
*** Submitted: mstate/presence: foundation for presence handling R=rog, TheMue CC= https://codereview.appspot.com/6495079
Sign in to reply to this message.
https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go File mstate/presence/presence.go (right): https://codereview.appspot.com/6495079/diff/3001/mstate/presence/presence.go#... mstate/presence/presence.go:142: select { On 2012/09/04 12:01:38, niemeyer wrote: > On 2012/09/04 11:56:18, rog wrote: > > On 2012/09/04 11:07:18, niemeyer wrote: > > > On 2012/09/04 10:55:52, rog wrote: > > > > The invariant is easy to maintain - if you receive a request, you are > bound > > to > > > > > > We already have cases where the invariant isn't valid, so this is of course > > not > > > true. > > > > interesting. i can't see any such cases, but i'm probably looking in the wrong > > place. (i don't count refresh, because it's not a > > "There's that invariant, which is actually not an invariant, but I don't care." > -- not a line of thinking that is easy to correlate to. ... or *make* it an invariant by closing all done channels when quitting. i'll stop now :-)
Sign in to reply to this message.
On 2012/09/04 12:12:12, rog wrote: > ... or *make* it an invariant by closing all done channels when quitting. i'll > stop now :-) No no, please continue diverting that trivial point into other directions until one of them makes sense. I'm sure our day will be hugely improved.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
