Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
986553002:
A simple memcache lock for appengine. (Closed)
DescriptionA simple memcache lock for appengine.
R=vadimsh@chromium.org
BUG=
Committed: https://chromium.googlesource.com/infra/infra/+/5e8bdbba1774434edaf506ad0dc719ee9c439ace
Patch Set 1 #
Total comments: 23
Patch Set 2 : total refactor #Patch Set 3 : and fix a boog #
Total comments: 8
Patch Set 4 : fix reentrancy issue #Patch Set 5 : Add memlock tests! #
Total comments: 17
Patch Set 6 : Rebase, use context #Patch Set 7 : working (fast) tests :) #Patch Set 8 : Rebase + refactor #Patch Set 9 : remove unnecessary log, use blackhole logger #
Total comments: 6
Patch Set 10 : use new Field functionality in logging #Patch Set 11 : add DEPS roll for luci-go to pick up logging.Fields #Patch Set 12 : rebase #
Total comments: 1
Messages
Total messages: 18 (3 generated)
https://codereview.chromium.org/986553002/diff/1/go/src/infra/gae/libs/mlock/... File go/src/infra/gae/libs/mlock/mlock.go (right): https://codereview.chromium.org/986553002/diff/1/go/src/infra/gae/libs/mlock/... go/src/infra/gae/libs/mlock/mlock.go:20: ErrUnableToLock = fmt.Errorf("mlock: unable to lock") mlock is also syscall... maybe pick some different name? memlock https://codereview.chromium.org/986553002/diff/1/go/src/infra/gae/libs/mlock/... go/src/infra/gae/libs/mlock/mlock.go:26: const MemcacheLockTime = (time.Second << 6) // 64 seconds omg, why not "64 * time.Second" and remove two comments about 64 seconds? also it seems way too much, considering you ping the lock each second. Though stupid RPCs sometimes hang for extended periods of time. https://codereview.chromium.org/986553002/diff/1/go/src/infra/gae/libs/mlock/... go/src/infra/gae/libs/mlock/mlock.go:64: func (m *MemcacheLock) Obtain() bool { nit: TryLock() is more canonical name, imho. TryLock()\Unlock(). Obtain()\Release() is more appropriate for mutex (you obtain a mutex, but you lock a lock). Anyhow, 'sync' packages uses Lock()\Unlock(). https://codereview.chromium.org/986553002/diff/1/go/src/infra/gae/libs/mlock/... go/src/infra/gae/libs/mlock/mlock.go:66: Key: m.key, Expiration: MemcacheLockTime, Value: m.clientID}) nit: namespace lock keys, e.g Key: "mlock:" + m.key https://codereview.chromium.org/986553002/diff/1/go/src/infra/gae/libs/mlock/... go/src/infra/gae/libs/mlock/mlock.go:79: time.Sleep(time.Second) this can be a problem if all goroutines must complete before request handler finishes. Can you rewrite it to use conditional variable or channel? So it doesn't sit and wait for nothing 1 sec. https://codereview.chromium.org/986553002/diff/1/go/src/infra/gae/libs/mlock/... go/src/infra/gae/libs/mlock/mlock.go:91: // Check ensures that we still have control of the lock. Returns true if we nit: Check returns true if we ... "ensures" implies it actively does something https://codereview.chromium.org/986553002/diff/1/go/src/infra/gae/libs/mlock/... go/src/infra/gae/libs/mlock/mlock.go:119: op = "refresh" nit: move "op" to argument instead of "refresh". checkAndRefresh("release") is more readable than checkAndRefresh(false) https://codereview.chromium.org/986553002/diff/1/go/src/infra/gae/libs/mlock/... go/src/infra/gae/libs/mlock/mlock.go:121: itm.Value = []byte{} at this point you can compare itm.Value to empty byte buffer and return early if it is, not attempting to swap it. https://codereview.chromium.org/986553002/diff/1/go/src/infra/gae/libs/mlock/... go/src/infra/gae/libs/mlock/mlock.go:126: err = memcache.CompareAndSwap(m.ctx, itm) CompareAndSwap taking single argument blows my mind :) https://codereview.chromium.org/986553002/diff/1/go/src/infra/gae/libs/mlock/... go/src/infra/gae/libs/mlock/mlock.go:128: m.ctx.Warningf("failed to %s lock (%s:%s): %s", should a race condition on a lock (when compare and swap fails "compare" stage) be logged as warning (it is now)? https://codereview.chromium.org/986553002/diff/1/go/src/infra/gae/libs/mlock/... go/src/infra/gae/libs/mlock/mlock.go:133: m.ctx.Infof("%sed lock (%s:%s)", op, m.key, string(m.clientID)) clientID is ugly byte string. Perhaps %v instead of %s?
https://codereview.chromium.org/986553002/diff/1/go/src/infra/gae/libs/mlock/... File go/src/infra/gae/libs/mlock/mlock.go (right): https://codereview.chromium.org/986553002/diff/1/go/src/infra/gae/libs/mlock/... go/src/infra/gae/libs/mlock/mlock.go:64: func (m *MemcacheLock) Obtain() bool { On 2015/03/06 03:39:26, Vadim Sh. wrote: > nit: TryLock() is more canonical name, imho. TryLock()\Unlock(). > > Obtain()\Release() is more appropriate for mutex (you obtain a mutex, but you > lock a lock). Anyhow, 'sync' packages uses Lock()\Unlock(). For completness you can also implement "Lock()" that busy waits on "TryLock()" until locked. Then your lock will implement sync.Locker :)
PTAnL https://chromiumcodereview.appspot.com/986553002/diff/1/go/src/infra/gae/libs... File go/src/infra/gae/libs/mlock/mlock.go (right): https://chromiumcodereview.appspot.com/986553002/diff/1/go/src/infra/gae/libs... go/src/infra/gae/libs/mlock/mlock.go:20: ErrUnableToLock = fmt.Errorf("mlock: unable to lock") On 2015/03/06 at 03:39:26, Vadim Sh. wrote: > mlock is also syscall... maybe pick some different name? memlock done https://chromiumcodereview.appspot.com/986553002/diff/1/go/src/infra/gae/libs... go/src/infra/gae/libs/mlock/mlock.go:26: const MemcacheLockTime = (time.Second << 6) // 64 seconds On 2015/03/06 at 03:39:26, Vadim Sh. wrote: > omg, why not "64 * time.Second" and remove two comments about 64 seconds? > haha :P. It used to be `time.Duration(numSecs) * time.Second` which was annoying. And now it's a constant, so it doesn't matter. > also it seems way too much, considering you ping the lock each second. Though stupid RPCs sometimes hang for extended periods of time. Yeah, hanging and also goroutine scheduling starvation. Maybe I'll make it 16 secs? https://chromiumcodereview.appspot.com/986553002/diff/1/go/src/infra/gae/libs... go/src/infra/gae/libs/mlock/mlock.go:64: func (m *MemcacheLock) Obtain() bool { On 2015/03/06 at 03:46:30, Vadim Sh. wrote: > On 2015/03/06 03:39:26, Vadim Sh. wrote: > > nit: TryLock() is more canonical name, imho. TryLock()\Unlock(). > > SGTM > > Obtain()\Release() is more appropriate for mutex (you obtain a mutex, but you > > lock a lock). Anyhow, 'sync' packages uses Lock()\Unlock(). > > For completness you can also implement "Lock()" that busy waits on "TryLock()" until locked. Then your lock will implement sync.Locker :) Yeah, ok, thought about this. I'll do it. https://chromiumcodereview.appspot.com/986553002/diff/1/go/src/infra/gae/libs... go/src/infra/gae/libs/mlock/mlock.go:66: Key: m.key, Expiration: MemcacheLockTime, Value: m.clientID}) On 2015/03/06 at 03:39:26, Vadim Sh. wrote: > nit: namespace lock keys, e.g Key: "mlock:" + m.key done. https://chromiumcodereview.appspot.com/986553002/diff/1/go/src/infra/gae/libs... go/src/infra/gae/libs/mlock/mlock.go:79: time.Sleep(time.Second) On 2015/03/06 at 03:39:26, Vadim Sh. wrote: > this can be a problem if all goroutines must complete before request handler finishes. Can you rewrite it to use conditional variable or channel? So it doesn't sit and wait for nothing 1 sec. OK, done. I should have done that from the beginning... https://chromiumcodereview.appspot.com/986553002/diff/1/go/src/infra/gae/libs... go/src/infra/gae/libs/mlock/mlock.go:91: // Check ensures that we still have control of the lock. Returns true if we On 2015/03/06 at 03:39:26, Vadim Sh. wrote: > nit: Check returns true if we ... > > "ensures" implies it actively does something good point https://chromiumcodereview.appspot.com/986553002/diff/1/go/src/infra/gae/libs... go/src/infra/gae/libs/mlock/mlock.go:119: op = "refresh" On 2015/03/06 at 03:39:26, Vadim Sh. wrote: > nit: move "op" to argument instead of "refresh". > > checkAndRefresh("release") is more readable than checkAndRefresh(false) and then make it module-level constants? k :) https://chromiumcodereview.appspot.com/986553002/diff/1/go/src/infra/gae/libs... go/src/infra/gae/libs/mlock/mlock.go:121: itm.Value = []byte{} On 2015/03/06 at 03:39:26, Vadim Sh. wrote: > at this point you can compare itm.Value to empty byte buffer and return early if it is, not attempting to swap it. Ah, sure. Done. https://chromiumcodereview.appspot.com/986553002/diff/1/go/src/infra/gae/libs... go/src/infra/gae/libs/mlock/mlock.go:126: err = memcache.CompareAndSwap(m.ctx, itm) On 2015/03/06 at 03:39:26, Vadim Sh. wrote: > CompareAndSwap taking single argument blows my mind :) Yeah, it's a really weird API. AND! The Item object has hidden state, so not only do you have to do a Get before, but you can only CAS with the SAME Item that Get returned. :( https://chromiumcodereview.appspot.com/986553002/diff/1/go/src/infra/gae/libs... go/src/infra/gae/libs/mlock/mlock.go:128: m.ctx.Warningf("failed to %s lock (%s:%s): %s", On 2015/03/06 at 03:39:26, Vadim Sh. wrote: > should a race condition on a lock (when compare and swap fails "compare" stage) be logged as warning (it is now)? shrug. It's true that it failed to lock though. At this point we nominally hold the lock. https://chromiumcodereview.appspot.com/986553002/diff/1/go/src/infra/gae/libs... go/src/infra/gae/libs/mlock/mlock.go:133: m.ctx.Infof("%sed lock (%s:%s)", op, m.key, string(m.clientID)) On 2015/03/06 at 03:39:26, Vadim Sh. wrote: > clientID is ugly byte string. Perhaps %v instead of %s? it's usually not though. Maybe %q, then we'll see escapes if it's really non-utf8?
mostly lgtm https://codereview.chromium.org/986553002/diff/40001/go/src/infra/gae/libs/me... File go/src/infra/gae/libs/memlock/memlock.go (right): https://codereview.chromium.org/986553002/diff/40001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:76: if !m.checkAnd(refresh) { actually, it makes the lock reentrant (same gorouting can call TryWithLock while already in TryWithLock), and it will break outer TryWithLock when inner one ends. Assuming "random clientID" is most commonly used option, can you put some protection against reentry? (e.g. if m.held { panic("OMG!! panic!") }). There's no way to prevent reentry from same process if it uses two different instances of MemcacheLock with identical clientID :( (Unless you want to add a registry of some sort, but it is an overkill). https://codereview.chromium.org/986553002/diff/40001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:81: stopChan := make(chan struct{}, 1) these "-chan" make the impression I'm watching some product of japanese entertainment industry... https://codereview.chromium.org/986553002/diff/40001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:89: stopChan <- struct{}{} I think "idiomatic" (I start to not like this word) way of signaling a channel is closing it? https://codereview.chromium.org/986553002/diff/40001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:99: break checkLoop do you really need a label here?
iannucci@chromium.org changed reviewers: + maruel@chromium.org
+maruel for an example of how the proposed context library would be used to implement tests for something (in this case, for a memcache-based "mutex"). https://codereview.chromium.org/986553002/diff/40001/go/src/infra/gae/libs/me... File go/src/infra/gae/libs/memlock/memlock.go (right): https://codereview.chromium.org/986553002/diff/40001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:76: if !m.checkAnd(refresh) { On 2015/03/07 at 02:55:50, Vadim Sh. wrote: > actually, it makes the lock reentrant (same gorouting can call TryWithLock while already in TryWithLock), and it will break outer TryWithLock when inner one ends. > > Assuming "random clientID" is most commonly used option, can you put some protection against reentry? (e.g. if m.held { panic("OMG!! panic!") }). > > There's no way to prevent reentry from same process if it uses two different instances of MemcacheLock with identical clientID :( (Unless you want to add a registry of some sort, but it is an overkill). oh, yeah, good point... I'll make it prevent this. https://codereview.chromium.org/986553002/diff/40001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:81: stopChan := make(chan struct{}, 1) On 2015/03/07 at 02:55:50, Vadim Sh. wrote: > these "-chan" make the impression I'm watching some product of japanese entertainment industry... Chan-chan! https://codereview.chromium.org/986553002/diff/40001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:89: stopChan <- struct{}{} On 2015/03/07 at 02:55:50, Vadim Sh. wrote: > I think "idiomatic" (I start to not like this word) way of signaling a channel is closing it? Hm, ok I guess that'll work. https://codereview.chromium.org/986553002/diff/40001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:99: break checkLoop On 2015/03/07 at 02:55:50, Vadim Sh. wrote: > do you really need a label here? yeah. a plain break stops the select statement (which, of course, is useless).
https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... File go/src/infra/gae/libs/memlock/doc.go (right): https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/doc.go:1: package memlock I assume doc and copyrights are TODOs? :) https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... File go/src/infra/gae/libs/memlock/memlock.go (right): https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:23: var delay = time.Second You can use a const. https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:26: release checkOp = "release" releaseOp refreshOp https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:32: var memcacheLockTime = 16 * time.Second You can use a const. 16s is hell of a long time https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:34: type Context interface { If it's exported, document it. https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:54: // this client (lock-holder). If it's empty then New() will generate a random Why generate a random one? What's the use case? https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:66: return nil, err In this case specifically (using crypto/rand) I think it's fine to panic. Note that I'm generally against using panic but here it's just noise. If crypto/rand is empty, you have bigger problems https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:81: func (m *MemcacheLock) TryWithLock(f func(check func() bool)) bool { The main issue with that is that you force the caller to use a polling loop and cannot use a select() flow anymore. It depends, sometimes a channel is more appropriate, sometimes a loop is better. In practice in GAE, using a channel is likely more often the right design. https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:86: m.held = true To double check; MemcacheLock is not thread safe. https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:119: break checkLoop use return instead, this removes need for goto https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:125: break remove https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:129: m.checkAnd(release) why not in defer? https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:137: func (m *MemcacheLock) logf(f func(string, ...interface{}), fmt string, args ...interface{}) { I personally do not see much value in this, as it obfuscate the code to save a few (8) copy paste. https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:142: func (m *MemcacheLock) checkAnd(op checkOp) bool { It's more like compareAndExchange(), which would be clearer to me. https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:150: m.logf(m.ctx.Infof, "lock owned by %q", string(itm.Value)) This will become very verbose eventually. https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:167: err = m.ctx.CompareAndSwap(itm) I don't like that it's doing 2 memcache operations on normal state, it should aim at doing the fast path first. https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... go/src/infra/gae/libs/memlock/memlock.go:173: m.logf(m.ctx.Infof, "%sed lock", op) Remove.
Please take another look (now that it's been like a month). I tried to address all the comments from before too. > https://codereview.chromium.org/986553002/diff/80001/go/src/infra/gae/libs/me... > go/src/infra/gae/libs/memlock/memlock.go:167: err = m.ctx.CompareAndSwap(itm) > I don't like that it's doing 2 memcache operations on normal state, it should > aim at doing the fast path first. I think you're referring to doing a Get and then CompareAndSwap? That's how the memcache api works... you have to Get before the CAS (because it keeps a hidden counter ID thing in the *memcache.Item). This surprised me as well. checkAnd always needs to do a CAS (even in the normal case) to reset the memcache expiration for the item. This means that in the common case we must do Get+CAS. In the failure case we only do a Get.
https://chromiumcodereview.appspot.com/986553002/diff/160001/go/src/infra/gae... File go/src/infra/gae/libs/wrapper/memory/memcache.go (left): https://chromiumcodereview.appspot.com/986553002/diff/160001/go/src/infra/gae... go/src/infra/gae/libs/wrapper/memory/memcache.go:31: // TODO(riannucci): bind+use namespace too this comment was wrong (we already bind the namespace in the useMC method), and having the BrokenFeatures object on memcacheImpl was a bug. Basically you could break a particular Impl instance, but that means that the test code can't actually cause the Impl that the code-under-test gets to have broken features. I moved this into the memcacheData, which is shared between Impl's (and adjusted the test accordingly, it was wrong before.)
lgtm with optional change. https://chromiumcodereview.appspot.com/986553002/diff/160001/go/src/infra/gae... File go/src/infra/gae/libs/memlock/memlock.go (right): https://chromiumcodereview.appspot.com/986553002/diff/160001/go/src/infra/gae... go/src/infra/gae/libs/memlock/memlock.go:37: // var so we can override it in the tests delay is a var so ... https://chromiumcodereview.appspot.com/986553002/diff/160001/go/src/infra/gae... go/src/infra/gae/libs/memlock/memlock.go:61: func TryWithLock(c context.Context, key, clientID string, f func(check func() bool) error) error { A nicer way IMHO would be to have this signature instead: // Returns nil if failed to get the lock. func WithLock(c context.Context, key, clientID string) func() { ... } so at call sites, it'd do: if release := WithLock(...); release != nil { defer release() .. run with lock } I don't see what's the usecase of the check function. https://chromiumcodereview.appspot.com/986553002/diff/160001/go/src/infra/gae... go/src/infra/gae/libs/memlock/memlock.go:76: args = append([]interface{}{key, clientID}, args...) wouldn't it be simpler to do: return f(fmt.Sprintf("%s:%q):", key, clientID) + fmt.Sprintf(fmt, args...)) Go strings are pascal style so concatenating strings is faster than in C.
thanks :) does my explanation of the goofy function interface make sense? https://chromiumcodereview.appspot.com/986553002/diff/160001/go/src/infra/gae... File go/src/infra/gae/libs/memlock/memlock.go (right): https://chromiumcodereview.appspot.com/986553002/diff/160001/go/src/infra/gae... go/src/infra/gae/libs/memlock/memlock.go:61: func TryWithLock(c context.Context, key, clientID string, f func(check func() bool) error) error { On 2015/05/31 20:35:11, M-A Ruel wrote: > A nicer way IMHO would be to have this signature instead: > > // Returns nil if failed to get the lock. > func WithLock(c context.Context, key, clientID string) func() { > ... > } > > so at call sites, it'd do: > > if release := WithLock(...); release != nil { > defer release() > .. run with lock > } > > I don't see what's the usecase of the check function. The reason for the check function is so the code running with the lock 'held' can be informed as early as possible that the lock has been lost (e.g. due to memcache flush or whatever). Some of the users of this lock do a couple loops over the datastore while holding the lock (e.g. in a backend), and so it's handy to check if we still have it (and safer than giving up the lock and then trying to re-acquire it). This is also why I can't implement sync.Locker safely, because unlike a regular in-memory mutex, losing the lock during execution is actually pretty common. This thing is less like a lock and more like a 'probably locked, but maybe not' thing. But if it were really reliable, then memcache would be really really slow :). So... tradeoffs. https://chromiumcodereview.appspot.com/986553002/diff/160001/go/src/infra/gae... go/src/infra/gae/libs/memlock/memlock.go:76: args = append([]interface{}{key, clientID}, args...) On 2015/05/31 20:35:11, M-A Ruel wrote: > wouldn't it be simpler to do: > > return f(fmt.Sprintf("%s:%q):", key, clientID) + fmt.Sprintf(fmt, args...)) > > Go strings are pascal style so concatenating strings is faster than in C. This function is actually going away... I'm standardizing a way to add 'context fields' to a Logger and let the actual Logger implementation handle it. I'll post a CL for that todayish.
The CQ bit was checked by iannucci@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org, maruel@chromium.org Link to the patchset: https://codereview.chromium.org/986553002/#ps220001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/986553002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/infra/infra/+/5e8bdbba1774434edaf506ad0dc71...
Message was sent while issue was closed.
https://codereview.chromium.org/986553002/diff/220001/DEPS File DEPS (right): https://codereview.chromium.org/986553002/diff/220001/DEPS#newcode18 DEPS:18: "@cb0fa575526ca58047408e4b0854b7ae50e151e5"), Oh next time please use: roll-dep infra/go/src/github.com/luci/luci-go in a fresh branch to roll a DEPS, it'll create a nicer CL. In particular it's not noted in the CL description that the dependency is updated. I should have noted this.
Message was sent while issue was closed.
On 2015/06/01 13:57:13, M-A Ruel wrote: > https://codereview.chromium.org/986553002/diff/220001/DEPS > File DEPS (right): > > https://codereview.chromium.org/986553002/diff/220001/DEPS#newcode18 > DEPS:18: "@cb0fa575526ca58047408e4b0854b7ae50e151e5"), > Oh next time please use: > roll-dep infra/go/src/github.com/luci/luci-go > > in a fresh branch to roll a DEPS, it'll create a nicer CL. In particular it's > not noted in the CL description that the dependency is updated. I should have > noted this. Ah, sorry, I'll do this next time. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
