Issue 6503086: mstate/watcher: new foundation for watchers
Descriptionmstate/watcher: new foundation for watchers
https://code.launchpad.net/~niemeyer/juju-core/mstate-watcher-foundation/+merge/123214
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 14
Patch Set 2 : mstate/watcher: new foundation for watchers #
Total comments: 32
Patch Set 3 : mstate/watcher: new foundation for watchers #
Total comments: 4
Patch Set 4 : mstate/watcher: new foundation for watchers #
Total comments: 9
Patch Set 5 : mstate/watcher: new foundation for watchers #
MessagesTotal messages: 12
Please take a look.
Sign in to reply to this message.
looks ok, but substantial caveat below. https://codereview.appspot.com/6503086/diff/1/mstate/watcher/watcher.go File mstate/watcher/watcher.go (right): https://codereview.appspot.com/6503086/diff/1/mstate/watcher/watcher.go#newco... mstate/watcher/watcher.go:36: // the the gorotuine loop. // the goroutine loop. https://codereview.appspot.com/6503086/diff/1/mstate/watcher/watcher.go#newco... mstate/watcher/watcher.go:96: func (w *Watcher) Stop() error { Don't we need a Wait method too? Otherwise we can't ever tell if the Watcher has died with an error. https://codereview.appspot.com/6503086/diff/1/mstate/watcher/watcher.go#newco... mstate/watcher/watcher.go:119: // document, or -1 if it doesn't currently exist. If the receiver doesn't receive the Change events, all watchers will be blocked? This is my biggest worry with this design. I didn't mind too much about presence, because presence watches don't tend to interact, but this design makes it very easy to write code that's subtly vulnerable to deadlock. For example, consider this code: ch0 := make(chan watcher.Change) ch1 := make(chan watcher.Change) w.Watch("c0", id, 0, ch0) w.Watch("c1", id, 0, ch1) transfer := make(chan bool) go func() { for { v0 := <-ch0 transfer <- v0 } }() go func() { for { v1 := <-ch1 v0 := <-transfer foo(v0, v1) } } This code can deadlock, but it looks ok - and it would be ok if receiving on ch0 and ch1 were not interlinked. I feel a little anxious about this - I've been bitten by similar things in the past. Moreover, I don't think it would be too hard to fix. Oh yes, we can also deadlock if we call ForceRefresh within a watcher loop AFAICS. This feels icky. https://codereview.appspot.com/6503086/diff/1/mstate/watcher/watcher.go#newco... mstate/watcher/watcher.go:154: // identifier is an int64 in seconds. i don't see any slots. it looks to me like this could be a time.Duration; then we could have shorter latency if we wish. https://codereview.appspot.com/6503086/diff/1/mstate/watcher/watcher.go#newco... mstate/watcher/watcher.go:190: for i >= 0 { i'd be tempted to declare both i's in the loop header, leaving the final clause blank. https://codereview.appspot.com/6503086/diff/1/mstate/watcher/watcher.go#newco... mstate/watcher/watcher.go:319: var d, r []interface{} perhaps a comment might be appropriate here, e.g. // See txn.ChangeLog for documentation on the structure // of log entries.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6503086/diff/1/mstate/watcher/watcher.go File mstate/watcher/watcher.go (right): https://codereview.appspot.com/6503086/diff/1/mstate/watcher/watcher.go#newco... mstate/watcher/watcher.go:36: // the the gorotuine loop. On 2012/09/07 08:45:37, rog wrote: > // the goroutine loop. Done. https://codereview.appspot.com/6503086/diff/1/mstate/watcher/watcher.go#newco... mstate/watcher/watcher.go:96: func (w *Watcher) Stop() error { On 2012/09/07 08:45:37, rog wrote: > Don't we need a Wait method too? Otherwise we can't ever tell if the Watcher has > died with an error. Yeah, we need Wait and Dying too. I'll handle that on monday, though, if nobody else gets there before I do, as I'm stepping out to enjoy my holiday in a moment. https://codereview.appspot.com/6503086/diff/1/mstate/watcher/watcher.go#newco... mstate/watcher/watcher.go:119: // document, or -1 if it doesn't currently exist. On 2012/09/07 08:45:37, rog wrote: > If the receiver doesn't receive the Change events, all watchers will be blocked? > > This is my biggest worry with this design. I didn't mind too much about > presence, because presence watches don't tend to interact, but this design makes > it very easy to write code that's subtly vulnerable to deadlock. > > For example, consider this code: I've already explained this before in the context of the presence package, which indeed is no different (yes, they do interact), but I'm happy to explain again the motivations behind this design. There are several points here: 1) As a first point, the sample code you provide is already broken by itself, despite anything about this design, and would cause the code to deadlock or panic due to lack of consideration for dying goroutines. The answer of course is "Oh, sure but it's just a matter of...", and yes it is. The point is that your code doesn't "look ok" to me, and it is easy to get code that "looks ok" wrong if we deal careless with channels and goroutines. 2) Much more interestingly, though, the sample code is also broken in pretty subtle ways, and that I'm hoping this primitive will help us getting it right. What happens if we receive 1000 events on ch0 before we manage to send the first value onto transfer? Here is what happens: we'll send each one of the one thousand events to the other side, *after* we send it to transfer, despite the fact that the consumer has *already been told that it changed*. This is dealt with if we do not block, and properly consume events. 3) An important remark here is that I'm not suggesting we preserve this logic in the high-level interface of state. With this foundation in place, it is *trivial* for us to do something like this: case machineChange := machineWatcher.Change(): machine, err := machineChange.Machine() Not only this is pretty much what we have today, but it should not suffer the issue you describe, and by addresing point 2, it means that we'll get the *most recent machine* by the time we actually consume the event, instead of doing what we have today which loads the machine and dispatches each one of them despite the fact we haven't handled the first one yet. This not only means that blocking is fine, but it means that blocking is *beneficial*, because we're discarding events that are not interesting while we didn't handle the first event. 4) As a minor but convenient side-effect, this design means that events are ordered. If we apply transaction A and then apply transaction B, we're able to tell that when consuming events. 5) The design is also simple, and efficient. 6) Nothing is settled on stone. There was a lot of thinking gone onto how this works, but I'm not claiming to be the holder of truth. If we try it in practice and if it shows significant problems, we can tweak it until we're happy. I do think this is a pretty good start, though. > Oh yes, we can also deadlock if we call ForceRefresh within a watcher loop > AFAICS. This feels icky. No, it doesn't block (see the tests), but to be honest what would feel very icky would be calling ForceRefresh within a watcher loop. This would be pretty bad design. https://codereview.appspot.com/6503086/diff/1/mstate/watcher/watcher.go#newco... mstate/watcher/watcher.go:154: // identifier is an int64 in seconds. On 2012/09/07 08:45:37, rog wrote: > i don't see any slots. it looks to me like this could be a time.Duration; then > we could have shorter latency if we wish. Copy & paste artifact, I'll fix it. Thanks. https://codereview.appspot.com/6503086/diff/1/mstate/watcher/watcher.go#newco... mstate/watcher/watcher.go:190: for i >= 0 { On 2012/09/07 08:45:37, rog wrote: > i'd be tempted to declare both i's in the loop header, leaving the final clause > blank. Cool, but I find this more elegant. I hope you're fine with that. https://codereview.appspot.com/6503086/diff/1/mstate/watcher/watcher.go#newco... mstate/watcher/watcher.go:319: var d, r []interface{} On 2012/09/07 08:45:37, rog wrote: > perhaps a comment might be appropriate here, e.g. > // See txn.ChangeLog for documentation on the structure > // of log entries. Good idea, thanks.
Sign in to reply to this message.
AFAICS there is no way to watch over a whole collection, only over a set of known documents. I wonder why is the basic feature missing and the advanced feature present. How would you implement WatchMachines without the option of watching over a collection? https://codereview.appspot.com/6503086/diff/1/mstate/watcher/watcher.go File mstate/watcher/watcher.go (right): https://codereview.appspot.com/6503086/diff/1/mstate/watcher/watcher.go#newco... mstate/watcher/watcher.go:119: // document, or -1 if it doesn't currently exist. > No, it doesn't block (see the tests) Hmm? I called ForceRefresh twice and it blocked the whole thing. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go File mstate/watcher/watcher.go (right): https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:1: // The watcher package implements an interface for observing changes s/implements/provides/ https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:2: // to arbitrary MongoDB documents that are maintained via the s/documents/collections or documents/ https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:26: current map[watchKey]int64 s/$/ Access to the map is serialized through the main loop./ https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:65: id interface{} s|%|// optional, can be nil.| https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:79: // New returns a new Watcher observing the changelog collection. // New returns a new Watcher observing the changelog collection, // which must be a capped collection maintained by mgo/txn. Pity we can't actually validate if the collection is capped or not. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:115: // Watch includes the given collection and document id into w for monitoring. s/document id/optional document id/ https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:154: // loop implements the main watcher loop. s/implements/is/ or perhaps delete the whole comment. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:184: // Refresh events are handled backwards, to prevent moving This comment is confusing, at least to me. I suggest: // refreshEvents are stored newest first. Then it is pretty obvious why the loop is backwards. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:187: for i >= 0 { The canonical form is to put the decrement inside the for clause, not in the body. for i := len(w.refreshEvents)-1; i >=0; i-- { https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:189: for e.ch != nil { Checking e.ch works fine now, but I'm concerned that in the future we might want to run this function and the handle function concurrently and we'll forget about this. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:202: // Request events are handled forwards. Same as the comment for refreshEvents. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:204: for i < len(w.requestEvents) { Ditto as for the first loop. Alternatively, you can use range here. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:234: panic("adding channel twice for same document") Why would we be actively disallowing this? https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:237: if revno, ok := w.current[r.key]; ok && (revno > r.info.revno || revno == -1 && r.info.revno >= 0) { I think it's fine to drop the ok check here, w.current[r.key] will return 0 if r.key does not exist, and the following check will correctly fail. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:339: log.Printf("watcher: changelog has revno with type %T: %#v", r[i], r[i]) Nice, didn't know about %T. I used to use reflect.TypeOf for this.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6503086/diff/1/mstate/watcher/watcher.go File mstate/watcher/watcher.go (right): https://codereview.appspot.com/6503086/diff/1/mstate/watcher/watcher.go#newco... mstate/watcher/watcher.go:119: // document, or -1 if it doesn't currently exist. On 2012/09/08 17:50:03, aram wrote: > > No, it doesn't block (see the tests) > > Hmm? I called ForceRefresh twice and it blocked the whole thing. Yep, my apologies. You're both right. ForceRefresh did block during send, and that means it can't be used in certain circumstances. I've split the method in half and tweaked it to make the behavior more obvious. We now have StartSync and Sync with explicit properties that enable to wait, and to simply kick off a resync. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go File mstate/watcher/watcher.go (right): https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:1: // The watcher package implements an interface for observing changes On 2012/09/08 17:50:04, aram wrote: > s/implements/provides/ [niemeyer@gopher ~/src/go/src/pkg]% grep 'Package.*implements' * -r | wc -l 89 https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:2: // to arbitrary MongoDB documents that are maintained via the On 2012/09/08 17:50:04, aram wrote: > s/documents/collections or documents/ We observe changes to documents, that live in collections. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:26: current map[watchKey]int64 On 2012/09/08 17:50:04, aram wrote: > s/$/ Access to the map is serialized through the main loop./ This doesn't sound right. Access to most of the private data is serialized because it's being used from a single goroutine. This field isn't even special about that. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:65: id interface{} On 2012/09/08 17:50:04, aram wrote: > s|%|// optional, can be nil.| Done. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:79: // New returns a new Watcher observing the changelog collection. On 2012/09/08 17:50:04, aram wrote: > // New returns a new Watcher observing the changelog collection, > // which must be a capped collection maintained by mgo/txn. > > Pity we can't actually validate if the collection is capped or not. Done. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:115: // Watch includes the given collection and document id into w for monitoring. On 2012/09/08 17:50:04, aram wrote: > s/document id/optional document id/ Good point regarding the per-collection watchers, thanks. I've tried to make the suggested change to the docs, but the documentation was reading improperly. I ended up splitting out the function to clean up the API. Please see what you think. I've also tested it properly. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:154: // loop implements the main watcher loop. On 2012/09/08 17:50:04, aram wrote: > s/implements/is/ > > or perhaps delete the whole comment. Done. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:184: // Refresh events are handled backwards, to prevent moving On 2012/09/08 17:50:04, aram wrote: > This comment is confusing, at least to me. I suggest: > > // refreshEvents are stored newest first. > > Then it is pretty obvious why the loop is backwards. Done. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:187: for i >= 0 { On 2012/09/08 17:50:04, aram wrote: > The canonical form is to put the decrement inside the for clause, not in the > body. > > for i := len(w.refreshEvents)-1; i >=0; i-- { Good point. This was an inheritance from the loop copied from presence, which handles it differently. There's no reason for that after this reorganization, so I've changed it as you suggest. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:189: for e.ch != nil { On 2012/09/08 17:50:04, aram wrote: > Checking e.ch works fine now, but I'm concerned that in the future we might want > to run this function and the handle function concurrently and we'll forget about > this. If we decide to do handle it concurrently, a lot of stuff is broken. That's part of the reason why it's not concurrent. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:202: // Request events are handled forwards. On 2012/09/08 17:50:04, aram wrote: > Same as the comment for refreshEvents. Done. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:204: for i < len(w.requestEvents) { On 2012/09/08 17:50:04, aram wrote: > Ditto as for the first loop. Alternatively, you can use range here. No, range doesn't work here, as the slice can grow during the loop. I've added that to the comment you suggest above. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:234: panic("adding channel twice for same document") On 2012/09/08 17:50:04, aram wrote: > Why would we be actively disallowing this? It feels like buggy code. Why would we want to do this? https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:237: if revno, ok := w.current[r.key]; ok && (revno > r.info.revno || revno == -1 && r.info.revno >= 0) { On 2012/09/08 17:50:04, aram wrote: > I think it's fine to drop the ok check here, w.current[r.key] will return 0 if > r.key does not exist, and the following check will correctly fail. If revno == 0 and r.info.revno == -1, it sends an event, which isn't right. The check seems correct.
Sign in to reply to this message.
Thanks for looking into this, but I neither wanted or expected you to fix this in the weekend. There's a spurious test failure: http://paste.ubuntu.com/1196558/ https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go File mstate/watcher/watcher.go (right): https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:26: current map[watchKey]int64 > This doesn't sound right. Access to most of the private data is > serialized because it's being used from a single goroutine. This > field isn't even special about that. Yeah, but maps are not thread safe and when I first read the code my immediate reaction was "why isn't this protected by a mutex since this has to change at client request and obviously is read all the time by the main loop?" only later in the code I've seen that requests are only processed through the main loop instead of Watch/Unwatch modifying this field directly. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:115: // Watch includes the given collection and document id into w for monitoring. > I've tried to make the suggested change to the docs, but the > documentation was reading improperly. I ended up splitting out > the function to clean up the API. Please see what you think. Thanks, I think it's better this way. https://codereview.appspot.com/6503086/diff/4001/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:237: if revno, ok := w.current[r.key]; ok && (revno > r.info.revno || revno == -1 && r.info.revno >= 0) { > If revno == 0 and r.info.revno == -1, it sends an event, which isn't > right. The check seems correct. Ah, yes, -1 is a valid revno. https://codereview.appspot.com/6503086/diff/7002/mstate/watcher/watcher.go File mstate/watcher/watcher.go (right): https://codereview.appspot.com/6503086/diff/7002/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:168: func (w *Watcher) Sync() { I don't understand the purpose of this function. It's very easy to deadlock if misused, and I can't figure out why would I ever want to call it.
Sign in to reply to this message.
https://codereview.appspot.com/6503086/diff/7002/mstate/watcher/watcher.go File mstate/watcher/watcher.go (right): https://codereview.appspot.com/6503086/diff/7002/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:168: func (w *Watcher) Sync() { On 2012/09/10 12:47:13, aram wrote: > I don't understand the purpose of this function. It's very easy to deadlock if > misused, and I can't figure out why would I ever want to call it. Testing is its main purpose, and its semantics seem clear and well documented if nothing else. I'm happy to kill it if we end up not using it, but can we wait a bit to see how things turn out?
Sign in to reply to this message.
https://codereview.appspot.com/6503086/diff/7002/mstate/watcher/watcher.go File mstate/watcher/watcher.go (right): https://codereview.appspot.com/6503086/diff/7002/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:168: func (w *Watcher) Sync() { > Testing is its main purpose, and its semantics seem clear and > well documented if nothing else. I'm happy to kill it if we end > up not using it, but can we wait a bit to see how things turn out? Ah, sure, testing is fine, I thought it's a regular function you might want to call in normal usage.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6503086/diff/7002/mstate/watcher/watcher.go File mstate/watcher/watcher.go (right): https://codereview.appspot.com/6503086/diff/7002/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:168: func (w *Watcher) Sync() { On 2012/09/10 13:02:31, aram wrote: > > Testing is its main purpose, and its semantics seem clear and > > well documented if nothing else. I'm happy to kill it if we end > > up not using it, but can we wait a bit to see how things turn out? > > Ah, sure, testing is fine, I thought it's a regular function you > might want to call in normal usage. I believe I fixed up the spurious error you were seeing with the use of Sync, so this may be a good example.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
LGTM with a couple of final comments. I've been thinking about this a lot over the weekend, and I think this implementation has positive and negative aspects. Mostly positive, and it's nice and simple, definite bonus points! We'll see how it pans out. The API is lovely anyway. https://codereview.appspot.com/6503086/diff/2002/mstate/watcher/watcher.go File mstate/watcher/watcher.go (right): https://codereview.appspot.com/6503086/diff/2002/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:117: // parameter informs the currently known revision number for the document. s/informs/holds/ https://codereview.appspot.com/6503086/diff/2002/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:118: // Non-existing documents are represented by a -1 revno. s/Non-existing/Non-existing/ or "Documents that do not exist are..." https://codereview.appspot.com/6503086/diff/2002/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:123: select { how about: func (w *Watcher) sendReq(req interface{}) { select { case w.request <- req: case <-w.tomb.Dying(): } } then all those select statements below become simple function calls. https://codereview.appspot.com/6503086/diff/2002/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:321: iter := w.log.Find(nil).Batch(10).Sort("-$natural").Iter() i think a comment on this rather involved line might be good. // Iterate backwards in time through all events in the log. ?
Sign in to reply to this message.
*** Submitted: mstate/watcher: new foundation for watchers R=rog, aram CC= https://codereview.appspot.com/6503086 https://codereview.appspot.com/6503086/diff/2002/mstate/watcher/watcher.go File mstate/watcher/watcher.go (right): https://codereview.appspot.com/6503086/diff/2002/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:117: // parameter informs the currently known revision number for the document. On 2012/09/10 14:02:46, rog wrote: > s/informs/holds/ Done. https://codereview.appspot.com/6503086/diff/2002/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:118: // Non-existing documents are represented by a -1 revno. On 2012/09/10 14:02:46, rog wrote: > s/Non-existing/Non-existing/ > or "Documents that do not exist are..." Done. https://codereview.appspot.com/6503086/diff/2002/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:123: select { On 2012/09/10 14:02:46, rog wrote: > how about: > > func (w *Watcher) sendReq(req interface{}) { > select { > case w.request <- req: > case <-w.tomb.Dying(): > } > } > > then all those select statements below become simple > function calls. Very nice. Cheers! https://codereview.appspot.com/6503086/diff/2002/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:321: iter := w.log.Find(nil).Batch(10).Sort("-$natural").Iter() On 2012/09/10 14:02:46, rog wrote: > i think a comment on this rather involved line might be good. > // Iterate backwards in time through all events in the log. > ? Good idea. Added a note. https://codereview.appspot.com/6503086/diff/2002/mstate/watcher/watcher.go#ne... mstate/watcher/watcher.go:321: iter := w.log.Find(nil).Batch(10).Sort("-$natural").Iter() On 2012/09/10 14:02:46, rog wrote: > i think a comment on this rather involved line might be good. > // Iterate backwards in time through all events in the log. > ? Good idea. Added a note.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
