Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1523)

Issue 6495043: mstate: integrated lifecycle into service (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 9 months ago by TheMue
Modified:
9 years, 8 months ago
Reviewers:
mp+121552
Visibility:
Public.

Description

mstate: integrated lifecycle into service Added the lifecycle methods to service and changed the service removing in state and the according tests. https://code.launchpad.net/~themue/juju-core/go-mstate-lifecycle-service/+merge/121552 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : mstate: integrated lifecycle into service #

Patch Set 3 : mstate: integrated lifecycle into service #

Total comments: 8

Patch Set 4 : mstate: integrated lifecycle into service #

Unified diffs Side-by-side diffs Delta from patch set Stats (+82 lines, -13 lines) Patch
A [revision details] View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M mstate/assign_test.go View 1 chunk +2 lines, -0 lines 0 comments Download
M mstate/life_test.go View 1 2 2 chunks +17 lines, -1 line 0 comments Download
M mstate/relation_test.go View 1 chunk +2 lines, -0 lines 0 comments Download
M mstate/service.go View 1 chunk +27 lines, -0 lines 0 comments Download
M mstate/service_test.go View 1 chunk +3 lines, -1 line 0 comments Download
M mstate/state.go View 1 2 3 3 chunks +27 lines, -11 lines 0 comments Download
M mstate/state_test.go View 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 7
TheMue
Please take a look.
9 years, 9 months ago (2012-08-28 08:01:27 UTC) #1
TheMue
Please take a look.
9 years, 9 months ago (2012-08-28 09:34:03 UTC) #2
TheMue
Please take a look.
9 years, 9 months ago (2012-08-28 09:49:40 UTC) #3
niemeyer
https://codereview.appspot.com/6495043/diff/2002/mstate/service.go File mstate/service.go (right): https://codereview.appspot.com/6495043/diff/2002/mstate/service.go#newcode45 mstate/service.go:45: s.doc.Life = Dying As discussed on IRC, we'll need ...
9 years, 9 months ago (2012-08-28 13:48:59 UTC) #4
niemeyer
LGTM after IRC conversation, please just add a note: https://codereview.appspot.com/6495043/diff/2002/mstate/state.go File mstate/state.go (right): https://codereview.appspot.com/6495043/diff/2002/mstate/state.go#newcode135 mstate/state.go:135: ...
9 years, 9 months ago (2012-08-28 13:58:12 UTC) #5
aram
https://codereview.appspot.com/6495043/diff/2002/mstate/state.go File mstate/state.go (right): https://codereview.appspot.com/6495043/diff/2002/mstate/state.go#newcode156 mstate/state.go:156: // Remove the units. Please add a TODO that ...
9 years, 9 months ago (2012-08-28 13:59:10 UTC) #6
TheMue
9 years, 8 months ago (2012-08-28 14:50:11 UTC) #7
*** Submitted:

mstate: integrated lifecycle into service

Added the lifecycle methods to service and changed
the service removing in state and the according tests.

R=niemeyer, aram
CC=
https://codereview.appspot.com/6495043

https://codereview.appspot.com/6495043/diff/2002/mstate/service.go
File mstate/service.go (right):

https://codereview.appspot.com/6495043/diff/2002/mstate/service.go#newcode45
mstate/service.go:45: s.doc.Life = Dying
On 2012/08/28 13:48:59, niemeyer wrote:
> As discussed on IRC, we'll need to fix the Kill methods.. they're changing the
> cached state to Dying irrespective of previous state. We shouldn't do Dying >
> Dead.
> 
> We can move on like this for now, though, and fix all the Kill methods in a
> follow up at once.

Yep, will get an extra CL.

https://codereview.appspot.com/6495043/diff/2002/mstate/state.go
File mstate/state.go (right):

https://codereview.appspot.com/6495043/diff/2002/mstate/state.go#newcode135
mstate/state.go:135: // TODO(mue) Will change with full txn integration.
On 2012/08/28 13:58:12, niemeyer wrote:
> // TODO: Integrate with txn and do lifecycle properly. We can't
> // remove units like this.

Done.

https://codereview.appspot.com/6495043/diff/2002/mstate/state.go#newcode156
mstate/state.go:156: // Remove the units.
On 2012/08/28 13:59:10, aram wrote:
> Please add a TODO that this is wrong, and the right thing to do is have units
> watch for their own service and kill themselves when needed.

Done.

https://codereview.appspot.com/6495043/diff/2002/mstate/state.go#newcode162
mstate/state.go:162: err = unit.Die()
On 2012/08/28 13:48:59, niemeyer wrote:
> What's the win here? You're forcefully destroying all the units.. how is this
> any different from deleting it all? What does it mean to "integrate
lifecycle"?

Discussed in IRC.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b