Issue 11003044: Implemented service.EnsureMinUnits().

dave_cheney.net

https://codereview.appspot.com/11003044/diff/4001/state/minimumunits.go File state/minimumunits.go (right): https://codereview.appspot.com/11003044/diff/4001/state/minimumunits.go#newcode133 state/minimumunits.go:133: s.doc.UnitCount = service.doc.UnitCount i'm not a fan of this ...

10 years, 9 months ago (2013-07-08 23:29:59 UTC) #3

fwereade

looking really good; sorry to leave you hanging, just a couple of preliminary comments here, ...

10 years, 9 months ago (2013-07-09 18:48:04 UTC) #4

fwereade

Sorry, I think this needs a couple of tweaks. https://codereview.appspot.com/11003044/diff/4001/state/minimumunits.go File state/minimumunits.go (right): https://codereview.appspot.com/11003044/diff/4001/state/minimumunits.go#newcode171 state/minimumunits.go:171: ...

10 years, 9 months ago (2013-07-10 08:33:44 UTC) #5

Sorry, I think this needs a couple of tweaks.

https://codereview.appspot.com/11003044/diff/4001/state/minimumunits.go
File state/minimumunits.go (right):

https://codereview.appspot.com/11003044/diff/4001/state/minimumunits.go#newco...
state/minimumunits.go:171: continue loop
Ha, I think there's a race here. Consider the following:

  * min units 5, alive units 3
  * run transaction: success, the service has not changed
  * someone sets min units to less than 5 at exactly the wrong moment
  * refresh service, picking up new txn-revno but not checking min units
  * run transaction: success, the service has not changed (as far as we're
aware)
  * whoops, 5 units

If you were to assert on minunits as well you'd dodge that case, but you'd also
need to keep track of whether the value had changed from whatever it was when
you calculated `missing`. I *think* that ends up tangling the logic a bit too
much for comfort; but I think we can safely drop the inner loop and get a
slightly clearer flow with something like:

service := &Service{st: s.st, doc: s.doc}
for {
    // calculate missing as above
    if missing == 0 {
        return nil
    }
    // get ops as above (modulo assert tweak mentioned below?)
    switch err := s.st.runTransaction(ops); err {
    case nil:
        // assign unit as above
        if missing == 1 {
            return nil
        }
    case txn.ErrAborted:
    default:
        return err
    }
    if err := service.Refresh(); err != nil {
        return err
    }
}
panic("unreachable")

Does that seem correct to you?

https://codereview.appspot.com/11003044/diff/4001/state/minimumunits.go#newco...
state/minimumunits.go:187: return nil
On 2013/07/08 23:29:59, dfc wrote:
> the set the fields here, rather than in the defer.

Even with the break above, that's one of 3 possible nil returns in which the
fields may have changed.

I think I'd still contend that nobody cares enough about the value of those
fields for it to be worth setting them, though :).

https://codereview.appspot.com/11003044/diff/4001/state/minimumunits.go#newco...
state/minimumunits.go:207: Assert: D{{"txn-revno", service.doc.TxnRevno}},
While this does clearly work, I think I'd prefer to pass an `asserts D` into
service.addUnitOps and append them onto the isAlive assert we already apply to
the service doc. Spreading asserts on a single doc out through the txn feels
unhelpful to me.

Sign in to reply to this message.

frankban

Please take a look. https://codereview.appspot.com/11003044/diff/4001/state/minimumunits.go File state/minimumunits.go (right): https://codereview.appspot.com/11003044/diff/4001/state/minimumunits.go#newcode133 state/minimumunits.go:133: s.doc.UnitCount = service.doc.UnitCount On 2013/07/09 ...

10 years, 9 months ago (2013-07-11 07:58:43 UTC) #6

Please take a look.

https://codereview.appspot.com/11003044/diff/4001/state/minimumunits.go
File state/minimumunits.go (right):

https://codereview.appspot.com/11003044/diff/4001/state/minimumunits.go#newco...
state/minimumunits.go:133: s.doc.UnitCount = service.doc.UnitCount
On 2013/07/09 18:48:04, fwereade wrote:
> On 2013/07/08 23:29:59, dfc wrote:
> > i'm not a fan of this use of defer
> 
> I don't think it's so bad really; but I'm also not sure it's necessary. Is
> anything else going to be depending on those values? If not, I'd prefer to
drop
> it.

Removed it.

https://codereview.appspot.com/11003044/diff/4001/state/minimumunits.go#newco...
state/minimumunits.go:171: continue loop
On 2013/07/10 08:33:45, fwereade wrote:
> Ha, I think there's a race here. Consider the following:
> 
>   * min units 5, alive units 3
>   * run transaction: success, the service has not changed
>   * someone sets min units to less than 5 at exactly the wrong moment
>   * refresh service, picking up new txn-revno but not checking min units
>   * run transaction: success, the service has not changed (as far as we're
> aware)
>   * whoops, 5 units
> 
> If you were to assert on minunits as well you'd dodge that case, but you'd
also
> need to keep track of whether the value had changed from whatever it was when
> you calculated `missing`. I *think* that ends up tangling the logic a bit too
> much for comfort; but I think we can safely drop the inner loop and get a
> slightly clearer flow with something like:
> 
> service := &Service{st: s.st, doc: s.doc}
> for {
>     // calculate missing as above
>     if missing == 0 {
>         return nil
>     }
>     // get ops as above (modulo assert tweak mentioned below?)
>     switch err := s.st.runTransaction(ops); err {
>     case nil:
>         // assign unit as above
>         if missing == 1 {
>             return nil
>         }
>     case txn.ErrAborted:
>     default:
>         return err
>     }
>     if err := service.Refresh(); err != nil {
>         return err
>     }
> }
> panic("unreachable")
> 
> Does that seem correct to you?

Yes it does: good catch and thanks for this great solution!
Fixed and added a test for this race.

https://codereview.appspot.com/11003044/diff/4001/state/minimumunits.go#newco...
state/minimumunits.go:185: return nil
On 2013/07/08 23:29:59, dfc wrote:
> what about a break here

The code has been refactored so that this is no longer required.

https://codereview.appspot.com/11003044/diff/4001/state/minimumunits.go#newco...
state/minimumunits.go:187: return nil
On 2013/07/10 08:33:45, fwereade wrote:
> On 2013/07/08 23:29:59, dfc wrote:
> > the set the fields here, rather than in the defer.
> 
> Even with the break above, that's one of 3 possible nil returns in which the
> fields may have changed.
> 
> I think I'd still contend that nobody cares enough about the value of those
> fields for it to be worth setting them, though :).

Agreed.

Sign in to reply to this message.

dimitern

Looks promising, but I think it needs some improvements. https://codereview.appspot.com/11003044/diff/11001/state/export_test.go File state/export_test.go (right): https://codereview.appspot.com/11003044/diff/11001/state/export_test.go#newcode71 state/export_test.go:71: ...

10 years, 9 months ago (2013-07-11 20:05:11 UTC) #7

frankban

Please take a look. https://codereview.appspot.com/11003044/diff/11001/state/export_test.go File state/export_test.go (right): https://codereview.appspot.com/11003044/diff/11001/state/export_test.go#newcode71 state/export_test.go:71: // immediately after the next ...

10 years, 9 months ago (2013-07-12 10:07:22 UTC) #8

Please take a look.

https://codereview.appspot.com/11003044/diff/11001/state/export_test.go
File state/export_test.go (right):

https://codereview.appspot.com/11003044/diff/11001/state/export_test.go#newco...
state/export_test.go:71: // immediately after the next N transactions.
On 2013/07/11 20:05:12, dimitern wrote:
> needs better comment I think: run all N functions after all N transactions are
> done, or run 1 function after 1 transaction?

Fixed the comment. Please note that I basically used the comment already present
for SetBeforeHooks, so I changed the comment there too.

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go
File state/minimumunits.go (right):

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go#newc...
state/minimumunits.go:129: defer utils.ErrorContextf(&err,
On 2013/07/11 20:05:12, dimitern wrote:
> please, either put the whole statement on one line (possibly defining the
error
> message beforehand

I'd be happier to do this after understanding the reason for this kind of
requests. Is there a codestyle guideline for Go? Or for this project? Is
avoiding new lines when calling functions a team preference? Is it just your own
personal preference? Do I need to buy a wider screen? ;-) I believe I can learn
something here.

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go#newc...
state/minimumunits.go:134: if service.doc.Life != Alive {
On 2013/07/11 20:05:12, dimitern wrote:
> this should be an transaction assert op, rather than like this, relying on a
> cached (possibly stale) doc field.

The assertion is already included in the transaction. The check here gives us
the opportunity to exit without hitting the db in the case the service is not
alive. In this case, the stale document is not a problem: AFAICT if a service is
dying or dead, there is nothing  that could bring it back to life.
Moreover, I may be wrong but I think it's responsibility of the caller to make
this function receive a properly refreshed/not stale service.
What do you think?

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go#newc...
state/minimumunits.go:138: if service.doc.MinUnits == 0 {
On 2013/07/11 20:05:12, dimitern wrote:
> ditto

See the comment above.

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go#newc...
state/minimumunits.go:142: aliveUnits, err := aliveUnitsCount(service)
On 2013/07/11 20:05:12, dimitern wrote:
> ditto

See the comment above.

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go#newc...
state/minimumunits.go:147: missing := service.doc.MinUnits - aliveUnits
On 2013/07/11 20:05:12, dimitern wrote:
> ditto

See the comment above.

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go#newc...
state/minimumunits.go:173: return err
On 2013/07/11 20:05:12, dimitern wrote:
> I don't think this is right - the transaction can be aborted because of
asserts
> in the ops, and this have to be handled here, possibly refreshing the service
> doc and retrying, rather than just returning an error.

If I read this lines correctly, this is exactly what the code does: ErrAborted
errors are never returned, the loop proceeds and the service is refreshed by the
lines you can find below. AFAIK in the Go switch/case statements the code does
not fall through by default. Am I missing something?

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go#newc...
state/minimumunits.go:184: query := D{{"service", service.doc.Name}, {"life",
Alive}}
On 2013/07/11 20:05:12, dimitern wrote:
> this needs to be an assert op, take a look at service.SetCharm for example for
> more complicated op with multiple asserts.

Could you please be more specific? Why do I need an assertion when executing a
count query?

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits_test.go
File state/minimumunits_test.go (right):

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits_test.go...
state/minimumunits_test.go:255: for i, t := range []struct {
On 2013/07/11 20:05:12, dimitern wrote:
> please put a one-line summary of fields (esp. destroy) in a comment

Done.

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits_test.go...
state/minimumunits_test.go:328: expectedErr := `cannot ensure minimum units for
service "dummy-service": service is no longer alive`
On 2013/07/11 20:05:12, dimitern wrote:
> s/no longer/not/

Done.

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits_test.go...
state/minimumunits_test.go:343: defer state.SetRetryHooks(c, s.State, func() {
On 2013/07/11 20:05:12, dimitern wrote:
> not really keen on this defer block - hard to read; can you transform it to a
> normal (non-defer block), possibly moving part (or all) of it after the Assert
> below, and making the Assert a Check, so it won't return on failure, allowing
> you to do the cleanup?
> 
> Or possibly, just assigning the result to a func var and calling defer
> checkFunc() instead.

This is how those hook helpers are used elsewhere in the code (see
machine_test.go or unit_test.go or this file itself). I'd like be consistent
here and reuse the established pattern. Thoughts?

https://codereview.appspot.com/11003044/diff/11001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/11003044/diff/11001/state/service.go#newcode44
state/service.go:44: TxnRevno      int64 `bson:"txn-revno"`
On 2013/07/11 20:05:12, dimitern wrote:
> why do you need this?

It is used in state/minimumunits.go@ensureMinUnitsOps.

https://codereview.appspot.com/11003044/diff/11001/state/service.go#newcode545
state/service.go:545: // to include additional assertions for the service
document.
On 2013/07/11 20:05:12, dimitern wrote:
> please note that adding additional asserts calls for careful inspection of all
> state calls using addUnitOps and making sure you'll account for the additional
> asserts so there won't be unexpected ErrAborted errors or
ErrExcessiveContention
> (happens when an assert is not accounted for and it leads to multiple retries,
> all of which return ErrAborted, eventually getting out of the for loop
> (typically after 3 or 5 retries in most state calls) and returning that
error).

Are you suggesting to add this comment here?

Sign in to reply to this message.

fwereade

LGTM, but I'm interested to hear dimitern's further thoughts. https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go File state/minimumunits.go (right): https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go#newcode134 state/minimumunits.go:134: ...

10 years, 9 months ago (2013-07-15 11:02:53 UTC) #9

LGTM, but I'm interested to hear dimitern's further thoughts.

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go
File state/minimumunits.go (right):

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go#newc...
state/minimumunits.go:134: if service.doc.Life != Alive {
On 2013/07/12 10:07:22, frankban wrote:
> On 2013/07/11 20:05:12, dimitern wrote:
> > this should be an transaction assert op, rather than like this, relying on a
> > cached (possibly stale) doc field.
> 
> The assertion is already included in the transaction. The check here gives us
> the opportunity to exit without hitting the db in the case the service is not
> alive. In this case, the stale document is not a problem: AFAICT if a service
is
> dying or dead, there is nothing  that could bring it back to life.
> Moreover, I may be wrong but I think it's responsibility of the caller to make
> this function receive a properly refreshed/not stale service.
> What do you think?

+1 to frankban. It's annoying that we have to define asserts that duplicate the
check we do against in-memory state, but AFAICT it's necessary until someone's
smart enough to come up with an abstraction that lets us express both
requirements with the same code.

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go#newc...
state/minimumunits.go:173: return err
On 2013/07/12 10:07:22, frankban wrote:
> On 2013/07/11 20:05:12, dimitern wrote:
> > I don't think this is right - the transaction can be aborted because of
> asserts
> > in the ops, and this have to be handled here, possibly refreshing the
service
> > doc and retrying, rather than just returning an error.
> 
> If I read this lines correctly, this is exactly what the code does: ErrAborted
> errors are never returned, the loop proceeds and the service is refreshed by
the
> lines you can find below. AFAIK in the Go switch/case statements the code does
> not fall through by default. Am I missing something?

frankban's right, but I agree that a casual reading is misleading. You *can*
safely put the default case wherever you like, though, IIRC:

switch err {
default:
    return err
case nil:
    // blah blah
case txn.ErrAborted:
}

...but even that is potentially surprising for some readers. Dealer's choice.

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go#newc...
state/minimumunits.go:184: query := D{{"service", service.doc.Name}, {"life",
Alive}}
On 2013/07/12 10:07:22, frankban wrote:
> On 2013/07/11 20:05:12, dimitern wrote:
> > this needs to be an assert op, take a look at service.SetCharm for example
for
> > more complicated op with multiple asserts.
> 
> Could you please be more specific? Why do I need an assertion when executing a
> count query?

I don't think you do, but I'm interested to hear dimitern's thinking. We haven't
made a habit of this sort of code before and he may well have spotted something
we've missed.

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits_test.go
File state/minimumunits_test.go (right):

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits_test.go...
state/minimumunits_test.go:343: defer state.SetRetryHooks(c, s.State, func() {
On 2013/07/12 10:07:22, frankban wrote:
> On 2013/07/11 20:05:12, dimitern wrote:
> > not really keen on this defer block - hard to read; can you transform it to
a
> > normal (non-defer block), possibly moving part (or all) of it after the
Assert
> > below, and making the Assert a Check, so it won't return on failure,
allowing
> > you to do the cleanup?
> > 
> > Or possibly, just assigning the result to a func var and calling defer
> > checkFunc() instead.
> 
> This is how those hook helpers are used elsewhere in the code (see
> machine_test.go or unit_test.go or this file itself). I'd like be consistent
> here and reuse the established pattern. Thoughts?

I'm comfortable with the existing arrangements, but I wrote most of them, so I
*would* say that. Dimitern, would you expand on your objections a little please?

https://codereview.appspot.com/11003044/diff/11001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/11003044/diff/11001/state/service.go#newcode545
state/service.go:545: // to include additional assertions for the service
document.
On 2013/07/12 10:07:22, frankban wrote:
> On 2013/07/11 20:05:12, dimitern wrote:
> > please note that adding additional asserts calls for careful inspection of
all
> > state calls using addUnitOps and making sure you'll account for the
additional
> > asserts so there won't be unexpected ErrAborted errors or
> ErrExcessiveContention
> > (happens when an assert is not accounted for and it leads to multiple
retries,
> > all of which return ErrAborted, eventually getting out of the for loop
> > (typically after 3 or 5 retries in most state calls) and returning that
> error).
> 
> Are you suggesting to add this comment here?

I think it's implicit that additional asserts can cause new failures, and that
code that builds up transactions in this way demands special care. I think it's
a global issue that's maybe best addressed in the hacking-state.txt document?

Sign in to reply to this message.

dimitern

LGTM I should've read more carefully in some places, sorry about the unnecessary ranting :) ...

10 years, 9 months ago (2013-07-15 11:28:23 UTC) #10

LGTM

I should've read more carefully in some places, sorry about the unnecessary
ranting :)

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go
File state/minimumunits.go (right):

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go#newc...
state/minimumunits.go:129: defer utils.ErrorContextf(&err,
On 2013/07/12 10:07:22, frankban wrote:
> On 2013/07/11 20:05:12, dimitern wrote:
> > please, either put the whole statement on one line (possibly defining the
> error
> > message beforehand
> 
> I'd be happier to do this after understanding the reason for this kind of
> requests. Is there a codestyle guideline for Go? Or for this project? Is
> avoiding new lines when calling functions a team preference? Is it just your
own
> personal preference? Do I need to buy a wider screen? ;-) I believe I can
learn
> something here.

Sorry, probably personal preference and also makes it easier to read, I think.
You can put the message in a variable/const before that so the line doesn't get
too long. YMMV

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go#newc...
state/minimumunits.go:134: if service.doc.Life != Alive {
On 2013/07/15 11:02:53, fwereade wrote:
> On 2013/07/12 10:07:22, frankban wrote:
> > On 2013/07/11 20:05:12, dimitern wrote:
> > > this should be an transaction assert op, rather than like this, relying on
a
> > > cached (possibly stale) doc field.
> > 
> > The assertion is already included in the transaction. The check here gives
us
> > the opportunity to exit without hitting the db in the case the service is
not
> > alive. In this case, the stale document is not a problem: AFAICT if a
service
> is
> > dying or dead, there is nothing  that could bring it back to life.
> > Moreover, I may be wrong but I think it's responsibility of the caller to
make
> > this function receive a properly refreshed/not stale service.
> > What do you think?
> 
> +1 to frankban. It's annoying that we have to define asserts that duplicate
the
> check we do against in-memory state, but AFAICT it's necessary until someone's
> smart enough to come up with an abstraction that lets us express both
> requirements with the same code.

OK, good point objection withdrawn.

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go#newc...
state/minimumunits.go:173: return err
On 2013/07/15 11:02:53, fwereade wrote:
> On 2013/07/12 10:07:22, frankban wrote:
> > On 2013/07/11 20:05:12, dimitern wrote:
> > > I don't think this is right - the transaction can be aborted because of
> > asserts
> > > in the ops, and this have to be handled here, possibly refreshing the
> service
> > > doc and retrying, rather than just returning an error.
> > 
> > If I read this lines correctly, this is exactly what the code does:
ErrAborted
> > errors are never returned, the loop proceeds and the service is refreshed by
> the
> > lines you can find below. AFAIK in the Go switch/case statements the code
does
> > not fall through by default. Am I missing something?
> 
> frankban's right, but I agree that a casual reading is misleading. You *can*
> safely put the default case wherever you like, though, IIRC:
> 
> switch err {
> default:
>     return err
> case nil:
>     // blah blah
> case txn.ErrAborted:
> }
> 
> ...but even that is potentially surprising for some readers. Dealer's choice.

Sorry, I read it incorrectly (that's why you shouldn't review stuff in 2am I
suppose).

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go#newc...
state/minimumunits.go:184: query := D{{"service", service.doc.Name}, {"life",
Alive}}
On 2013/07/15 11:02:53, fwereade wrote:
> On 2013/07/12 10:07:22, frankban wrote:
> > On 2013/07/11 20:05:12, dimitern wrote:
> > > this needs to be an assert op, take a look at service.SetCharm for example
> for
> > > more complicated op with multiple asserts.
> > 
> > Could you please be more specific? Why do I need an assertion when executing
a
> > count query?
> 
> I don't think you do, but I'm interested to hear dimitern's thinking. We
haven't
> made a habit of this sort of code before and he may well have spotted
something
> we've missed.

See the above comment and just ignore me, sorry :/

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits_test.go
File state/minimumunits_test.go (right):

https://codereview.appspot.com/11003044/diff/11001/state/minimumunits_test.go...
state/minimumunits_test.go:343: defer state.SetRetryHooks(c, s.State, func() {
On 2013/07/15 11:02:53, fwereade wrote:
> On 2013/07/12 10:07:22, frankban wrote:
> > On 2013/07/11 20:05:12, dimitern wrote:
> > > not really keen on this defer block - hard to read; can you transform it
to
> a
> > > normal (non-defer block), possibly moving part (or all) of it after the
> Assert
> > > below, and making the Assert a Check, so it won't return on failure,
> allowing
> > > you to do the cleanup?
> > > 
> > > Or possibly, just assigning the result to a func var and calling defer
> > > checkFunc() instead.
> > 
> > This is how those hook helpers are used elsewhere in the code (see
> > machine_test.go or unit_test.go or this file itself). I'd like be consistent
> > here and reuse the established pattern. Thoughts?
> 
> I'm comfortable with the existing arrangements, but I wrote most of them, so I
> *would* say that. Dimitern, would you expand on your objections a little
please?

My objection is that it's a bit hard to follow all that from "defer" to
".Check()" and introducing intermediate function variables and passing them as
arguments to SetRetryHooks, so the defer and Check() will be closer together.

https://codereview.appspot.com/11003044/diff/11001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/11003044/diff/11001/state/service.go#newcode545
state/service.go:545: // to include additional assertions for the service
document.
On 2013/07/15 11:02:53, fwereade wrote:
> On 2013/07/12 10:07:22, frankban wrote:
> > On 2013/07/11 20:05:12, dimitern wrote:
> > > please note that adding additional asserts calls for careful inspection of
> all
> > > state calls using addUnitOps and making sure you'll account for the
> additional
> > > asserts so there won't be unexpected ErrAborted errors or
> > ErrExcessiveContention
> > > (happens when an assert is not accounted for and it leads to multiple
> retries,
> > > all of which return ErrAborted, eventually getting out of the for loop
> > > (typically after 3 or 5 retries in most state calls) and returning that
> > error).
> > 
> > Are you suggesting to add this comment here?
> 
> I think it's implicit that additional asserts can cause new failures, and that
> code that builds up transactions in this way demands special care. I think
it's
> a global issue that's maybe best addressed in the hacking-state.txt document?

Good point, my comment was more of a warning to make sure you haven't missed
cases where addUnitOps is called with additional asserts which are not handled.
But I can see the places where it was used it's called with nil for asserts, so
it should be fine. So no problem really.

Sign in to reply to this message.

frankban

On 2013/07/15 11:28:23, dimitern wrote: > LGTM > > I should've read more carefully in ...

10 years, 9 months ago (2013-07-15 12:10:52 UTC) #11

On 2013/07/15 11:28:23, dimitern wrote:
> LGTM
> 
> I should've read more carefully in some places, sorry about the unnecessary
> ranting :)

No worries. Thanks Dimiter and William for your reviews and suggestions.

>
https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go#newc...
> state/minimumunits.go:129: defer utils.ErrorContextf(&err,
> On 2013/07/12 10:07:22, frankban wrote:
> > On 2013/07/11 20:05:12, dimitern wrote:
> > > please, either put the whole statement on one line (possibly defining the
> > error
> > > message beforehand
> > 
> > I'd be happier to do this after understanding the reason for this kind of
> > requests. Is there a codestyle guideline for Go? Or for this project? Is
> > avoiding new lines when calling functions a team preference? Is it just your
> own
> > personal preference? Do I need to buy a wider screen? ;-) I believe I can
> learn
> > something here.
> 
> Sorry, probably personal preference and also makes it easier to read, I think.
> You can put the message in a variable/const before that so the line doesn't
get
> too long. YMMV

Fixed.

>
https://codereview.appspot.com/11003044/diff/11001/state/minimumunits.go#newc...
> state/minimumunits.go:173: return err
> On 2013/07/15 11:02:53, fwereade wrote:
> > On 2013/07/12 10:07:22, frankban wrote:
> > > On 2013/07/11 20:05:12, dimitern wrote:
> > > > I don't think this is right - the transaction can be aborted because of
> > > asserts
> > > > in the ops, and this have to be handled here, possibly refreshing the
> > service
> > > > doc and retrying, rather than just returning an error.
> > > 
> > > If I read this lines correctly, this is exactly what the code does:
> ErrAborted
> > > errors are never returned, the loop proceeds and the service is refreshed
by
> > the
> > > lines you can find below. AFAIK in the Go switch/case statements the code
> does
> > > not fall through by default. Am I missing something?
> > 
> > frankban's right, but I agree that a casual reading is misleading. You *can*
> > safely put the default case wherever you like, though, IIRC:
> > 
> > switch err {
> > default:
> >     return err
> > case nil:
> >     // blah blah
> > case txn.ErrAborted:
> > }
> > 
> > ...but even that is potentially surprising for some readers. Dealer's
choice.
> 
> Sorry, I read it incorrectly (that's why you shouldn't review stuff in 2am I
> suppose).

Heh. Anyway, I added a comment inside the txn.ErrAborted that should make the
flow
clearer.

>
https://codereview.appspot.com/11003044/diff/11001/state/minimumunits_test.go...
> state/minimumunits_test.go:343: defer state.SetRetryHooks(c, s.State, func() {
> On 2013/07/15 11:02:53, fwereade wrote:
> > On 2013/07/12 10:07:22, frankban wrote:
> > > On 2013/07/11 20:05:12, dimitern wrote:
> > > > not really keen on this defer block - hard to read; can you transform it
> to
> > a
> > > > normal (non-defer block), possibly moving part (or all) of it after the
> > Assert
> > > > below, and making the Assert a Check, so it won't return on failure,
> > allowing
> > > > you to do the cleanup?
> > > > 
> > > > Or possibly, just assigning the result to a func var and calling defer
> > > > checkFunc() instead.
> > > 
> > > This is how those hook helpers are used elsewhere in the code (see
> > > machine_test.go or unit_test.go or this file itself). I'd like be
consistent
> > > here and reuse the established pattern. Thoughts?
> > 
> > I'm comfortable with the existing arrangements, but I wrote most of them, so
I
> > *would* say that. Dimitern, would you expand on your objections a little
> please?
> 
> My objection is that it's a bit hard to follow all that from "defer" to
> ".Check()" and introducing intermediate function variables and passing them as
> arguments to SetRetryHooks, so the defer and Check() will be closer together.

Maybe it would be cool to fix this here and elsewhere in a separate branch.

Sign in to reply to this message.

Please take a look.

Sign in to reply to this message.

Issue 11003044: Implemented service.EnsureMinUnits().

Description

Patch Set 1 #

Patch Set 2 : Implemented service.EnsureMinUnits(). #

Patch Set 3 : Implemented service.EnsureMinUnits(). #

Patch Set 4 : Implemented service.EnsureMinUnits(). #

Patch Set 5 : Implemented service.EnsureMinUnits(). #

Messages