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

Issue 8849043: Serialize the uniter hook executions.

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 11 months ago by thumper
Modified:
10 years, 11 months ago
Reviewers:
dimitern, mp+159543, fwereade, rog
Visibility:
Public.

Description

Serialize the uniter hook executions. Implement a file system lock, and use that in the uniter to serialize the hook execution across different uniters running on a single machine. https://code.launchpad.net/~thumper/juju-core/fslock-mashup/+merge/159543 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 61

Patch Set 2 : Serialize the uniter hook executions. #

Total comments: 3

Patch Set 3 : Serialize the uniter hook executions. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+708 lines, -0 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
A utils/fslock/export_test.go View 1 2 1 chunk +12 lines, -0 lines 0 comments Download
A utils/fslock/fslock.go View 1 2 1 chunk +227 lines, -0 lines 2 comments Download
A utils/fslock/fslock_test.go View 1 2 1 chunk +350 lines, -0 lines 0 comments Download
M worker/uniter/uniter.go View 1 2 6 chunks +44 lines, -0 lines 0 comments Download
M worker/uniter/uniter_test.go View 1 2 3 chunks +73 lines, -0 lines 0 comments Download

Messages

Total messages: 16
thumper
Please take a look.
10 years, 11 months ago (2013-04-18 03:33:39 UTC) #1
thumper
Comment changes have been made in the branch and pushed. Visible in the merge proposal, ...
10 years, 11 months ago (2013-04-18 04:16:51 UTC) #2
rog
a few comments on fslock https://codereview.appspot.com/8849043/diff/1/utils/fslock/fslock.go File utils/fslock/fslock.go (right): https://codereview.appspot.com/8849043/diff/1/utils/fslock/fslock.go#newcode103 utils/fslock/fslock.go:103: tempLockName := fmt.Sprintf(".%s", hex.EncodeToString(lock.nonce)) ...
10 years, 11 months ago (2013-04-18 08:20:28 UTC) #3
dimitern
Generally, LGTM, with some trivials. https://codereview.appspot.com/8849043/diff/1/utils/fslock/fslock.go File utils/fslock/fslock.go (right): https://codereview.appspot.com/8849043/diff/1/utils/fslock/fslock.go#newcode47 utils/fslock/fslock.go:47: func generateNonce() ([]byte, error) ...
10 years, 11 months ago (2013-04-18 08:37:05 UTC) #4
thumper
Please take a look. https://codereview.appspot.com/8849043/diff/1/utils/fslock/fslock.go File utils/fslock/fslock.go (right): https://codereview.appspot.com/8849043/diff/1/utils/fslock/fslock.go#newcode47 utils/fslock/fslock.go:47: func generateNonce() ([]byte, error) { ...
10 years, 11 months ago (2013-04-18 23:19:37 UTC) #5
fwereade
LGTM with some comments -- an an exhortation not to actually *land* anything until we ...
10 years, 11 months ago (2013-04-19 00:24:49 UTC) #6
thumper
On 2013/04/19 00:24:49, fwereade wrote: > LGTM with some comments -- an an exhortation not ...
10 years, 11 months ago (2013-04-19 04:43:59 UTC) #7
rog
https://codereview.appspot.com/8849043/diff/1/utils/fslock/fslock.go File utils/fslock/fslock.go (right): https://codereview.appspot.com/8849043/diff/1/utils/fslock/fslock.go#newcode109 utils/fslock/fslock.go:109: err = ioutil.WriteFile(path.Join(tempDirName, heldFilename), lock.nonce, 0755) On 2013/04/18 23:19:37, ...
10 years, 11 months ago (2013-04-19 06:26:01 UTC) #8
fwereade
On 2013/04/19 04:43:59, thumper wrote: > But that isn't what this one is testing... The ...
10 years, 11 months ago (2013-04-19 08:28:39 UTC) #9
fwereade
A few more thoughts: https://codereview.appspot.com/8849043/diff/1/utils/fslock/fslock.go File utils/fslock/fslock.go (right): https://codereview.appspot.com/8849043/diff/1/utils/fslock/fslock.go#newcode123 utils/fslock/fslock.go:123: log.Infof("Lock %q beaten to the ...
10 years, 11 months ago (2013-04-19 08:36:32 UTC) #10
rog
https://codereview.appspot.com/8849043/diff/1/utils/fslock/fslock.go File utils/fslock/fslock.go (right): https://codereview.appspot.com/8849043/diff/1/utils/fslock/fslock.go#newcode47 utils/fslock/fslock.go:47: func generateNonce() ([]byte, error) { On 2013/04/18 23:19:37, thumper ...
10 years, 11 months ago (2013-04-19 09:56:14 UTC) #11
fwereade
This branch has languished long enough, and I think Tim's more than capable of addressing ...
10 years, 11 months ago (2013-04-24 20:38:50 UTC) #12
thumper
I managed to completely misunderstand rog's earlier comment about a single internal lock function that ...
10 years, 11 months ago (2013-04-26 00:20:36 UTC) #13
thumper
https://codereview.appspot.com/8849043/diff/1/utils/fslock/fslock.go File utils/fslock/fslock.go (right): https://codereview.appspot.com/8849043/diff/1/utils/fslock/fslock.go#newcode47 utils/fslock/fslock.go:47: func generateNonce() ([]byte, error) { On 2013/04/19 09:56:14, rog ...
10 years, 11 months ago (2013-04-26 01:06:37 UTC) #14
thumper
*** Submitted: Serialize the uniter hook executions. Implement a file system lock, and use that ...
10 years, 11 months ago (2013-04-26 01:11:53 UTC) #15
rog
10 years, 11 months ago (2013-04-26 07:56:55 UTC) #16
LGTM and thanks for listening :-)

a few minor comments below that you might
want to address in another branch if you
feel like it.

it's nice to have a genuine use for LockWithFunc.

https://codereview.appspot.com/8849043/diff/21001/utils/fslock/fslock.go
File utils/fslock/fslock.go (right):

https://codereview.appspot.com/8849043/diff/21001/utils/fslock/fslock.go#newc...
utils/fslock/fslock.go:179: return lock.lockLoop(message, continueFunc)
i know you've already submitted, but one minor suggestion: given they're exactly
the same thing, why not just rename lockLoop to LockWithFunc and have the other
lock functions call that?

https://codereview.appspot.com/8849043/diff/21001/utils/fslock/fslock.go#newc...
utils/fslock/fslock.go:214: // BreakLock forcably breaks the lock that is
currently being held.
"forcibly breaks a lock if it is currently being held" i think might be more
accurate, as there's nothing that checks if the lock is held, and no error is
returned if it is not.
Sign in to reply to this message.

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