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

Issue 10148045: cleaner: implemented the cleaner worker (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 5 months ago by mue
Modified:
8 years, 4 months ago
Reviewers:
mp+168484, fwereade, rog
Visibility:
Public.

Description

cleaner: implemented the cleaner worker The cleaner worker listens to events raised by the cleaner watcher. This signals that there are documents marked for removal. So the worker calls state.Cleanup() to remove them. https://code.launchpad.net/~themue/juju-core/025-cleaner-worker/+merge/168484 Requires: https://code.launchpad.net/~themue/juju-core/024-cleanup-watcher/+merge/167748 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 11

Patch Set 2 : cleaner: implemented the cleaner worker #

Total comments: 6

Patch Set 3 : cleaner: implemented the cleaner worker #

Total comments: 12

Patch Set 4 : cleaner: implemented the cleaner worker #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+259 lines, -38 lines) Patch
A [revision details] View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M state/state.go View 1 chunk +9 lines, -0 lines 0 comments Download
M state/state_test.go View 1 2 3 2 chunks +96 lines, -34 lines 0 comments Download
M state/watcher.go View 1 1 chunk +6 lines, -4 lines 0 comments Download
A worker/cleaner/cleaner.go View 1 2 3 1 chunk +66 lines, -0 lines 0 comments Download
A worker/cleaner/cleaner_test.go View 1 2 3 1 chunk +80 lines, -0 lines 2 comments Download

Messages

Total messages: 10
mue
Please take a look.
8 years, 5 months ago (2013-06-10 16:08:29 UTC) #1
fwereade
Coming along really nicely, but the watcher implementation must be fixed (sorry, don't know how ...
8 years, 5 months ago (2013-06-11 08:50:58 UTC) #2
rog
LGTM with fwereade's suggestions, and some thoughts below https://codereview.appspot.com/10148045/diff/1/cmd/jujud/bootstrap.go File cmd/jujud/bootstrap.go (right): https://codereview.appspot.com/10148045/diff/1/cmd/jujud/bootstrap.go#newcode91 cmd/jujud/bootstrap.go:91: state.JobManageEnviron, ...
8 years, 5 months ago (2013-06-11 13:39:21 UTC) #3
mue
Please take a look. https://codereview.appspot.com/10148045/diff/1/cmd/jujud/bootstrap.go File cmd/jujud/bootstrap.go (right): https://codereview.appspot.com/10148045/diff/1/cmd/jujud/bootstrap.go#newcode91 cmd/jujud/bootstrap.go:91: state.JobManageEnviron, state.JobManageState, state.JobServeAPI, state.JobHostUnits, On ...
8 years, 5 months ago (2013-06-11 13:53:42 UTC) #4
fwereade
Couple of suggestions following IRC discussion, but it's coming along well. https://codereview.appspot.com/10148045/diff/10001/state/state_test.go File state/state_test.go (left): ...
8 years, 5 months ago (2013-06-11 14:52:11 UTC) #5
mue
Please take a look. https://codereview.appspot.com/10148045/diff/10001/state/state_test.go File state/state_test.go (left): https://codereview.appspot.com/10148045/diff/10001/state/state_test.go#oldcode1272 state/state_test.go:1272: c.Assert(err, IsNil) On 2013/06/11 14:52:11, ...
8 years, 5 months ago (2013-06-12 09:53:38 UTC) #6
fwereade
Looking nice -- just a few more suggestions for a final pass, I think. https://codereview.appspot.com/10148045/diff/16001/state/state_test.go ...
8 years, 5 months ago (2013-06-12 14:53:14 UTC) #7
mue
Please take a look. https://codereview.appspot.com/10148045/diff/16001/state/state_test.go File state/state_test.go (right): https://codereview.appspot.com/10148045/diff/16001/state/state_test.go#newcode1364 state/state_test.go:1364: // Destroying the services emits ...
8 years, 5 months ago (2013-06-13 12:01:30 UTC) #8
fwereade
LGTM with tweak in worker test. https://codereview.appspot.com/10148045/diff/23001/worker/cleaner/cleaner_test.go File worker/cleaner/cleaner_test.go (right): https://codereview.appspot.com/10148045/diff/23001/worker/cleaner/cleaner_test.go#newcode74 worker/cleaner/cleaner_test.go:74: // Cleanup is ...
8 years, 5 months ago (2013-06-13 14:33:58 UTC) #9
mue
8 years, 5 months ago (2013-06-13 15:52:11 UTC) #10
https://codereview.appspot.com/10148045/diff/23001/worker/cleaner/cleaner_tes...
File worker/cleaner/cleaner_test.go (right):

https://codereview.appspot.com/10148045/diff/23001/worker/cleaner/cleaner_tes...
worker/cleaner/cleaner_test.go:74: // Cleanup is done, so no event expected
anymore.
On 2013/06/13 14:33:58, fwereade wrote:
> There's actually a race here -- an event could be read before the final
cleanup
> above, but the cleanup could complete before the call to NeedsCleanup, and in
> that case there'd be another event in the pipe. Very unlikely, but real, so
> let's just drop the final check.

Done.
Sign in to reply to this message.

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