Code review comment for lp:~waigani/juju-core/get-oldConfig-from-state

https://codereview.appspot.com/70190050/diff/1/state/state.go
File state/state.go (left):

https://codereview.appspot.com/70190050/diff/1/state/state.go#oldcode282
state/state.go:282: _, err = settings.Write()
On 2014/03/06 08:17:48, waigani wrote:
> On 2014/03/06 04:19:17, axw wrote:
> > Write isn't really good enough here; this does not address the
deleted TODO.
> The
> > update to settings should assert that there has been no change to
settings
> since
> > st.EnvironConfig() was called; otherwise the validation is not
complete.

> fwereade wanted a txn retry loop, ensuring the underlying settings had
not been
> changed, in client.EnvironmentSet

(https://codereview.appspot.com/63790045/diff/40001/state/apiserver/client/client.go#newcode791).
> As this is a WIP, I have not done it yet. I can make a TODO in
> client.EnvironmentSet and write the txn retry loop in a follow up
branch?

The assertion is a prerequisite of the looping. The assertion is what
ensures that the settings haven't been changed concurrently. If the
settings are changed underfoot, then the transaction fails with
txn.ErrAborted.

The assertion must happen here, whereas looping may happen here or in
the caller. There's basically no point in making any of these changes
unless that assertion is in place.

https://codereview.appspot.com/70190050/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode304
state/state.go:304: func (st *State) SetEnvironConfig(attrs
map[string]interface{}) error {
On 2014/03/06 08:17:48, waigani wrote:
> On 2014/03/06 04:19:17, axw wrote:
> > Why do we still need SetEnvironConfig? This still has the old race
conditions.
> > *If* it's needed, then SetEnvironConfig and UpdateEnvironConfig
should use a
> > common method that takes a *state.Settings as input and does the
> > no-concurrent-change check on that.

> It was needed by testing.ChangeEnvironConfig. ChangeEnvironConfig is
only used
> in two places, but it is actually easier to use UpdateEnvironConfig
directly
> instead. So I would be in favour of deleting SetEnvironConfig and
> ChangeEnvironConfig.

ChangeEnvironConfig can be modified to compute a delta and pass it into
UpdateEnvironConfig. I know it's a bit arse about, but I don't think we
should be exposing ourselves to bugs just to simplify a test. If you can
just get rid of ChangeEnvironConfig, though, that may be better.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode333
state/state.go:333: err = st.VaidateUpdateEnvironConfig(updateAttrs,
removeAttrs, oldConfig)
On 2014/03/06 08:17:48, waigani wrote:
> On 2014/03/06 04:19:17, axw wrote:
> > Since ValidateUpdateEnvironConfig updates the config with attributes
to
> > validate, I wonder if it's worthwhile even having a separate method.
You're
> > duplicating work here by updating settings.
> >
> > I think it would be simpler to just:
> > 1. get settings, and create a config from them
> > 2. apply changes to get a new config, check validity
> > 3. do something *like* replaceSettingsOp (see settings.go), but
passing in
> > the *state.Settings

> Do you think something *like* replaceSettingsOp should happen within
> state.UpdateEnvironConfig or client.EnvironmentSet?

Certainly in state. How would you propose to do it outside anyway?

I suspect what William meant is that UpdateEnvironConfig should be
retried for some number of attempts until it returns an error !=
mgo/txn/ErrAborted; better to check with William.

https://codereview.appspot.com/70190050/