Yes, I forgot about the comment. The interdiff:
diff --git a/src/Ganeti/Locking/Locks.hs b/src/Ganeti/Locking/Locks.hs
index 9705d9a..5a6b404 100644
--- a/src/Ganeti/Locking/Locks.hs
+++ b/src/Ganeti/Locking/Locks.hs
@@ -171,8 +171,8 @@ instance Lock GanetiLocks where
lockImplications (Network _) = [NetworkLockSet]
lockImplications _ = []
--- | A client is identified as a job id, thread id and path to its process
--- identifier file
+-- | A client is identified as a job id and path to its process
+-- identifier file.
data ClientId = ClientId
{ ciJobId :: Maybe JobId
, ciThreadId :: Integer
On Tue, Mar 18, 2014 at 11:14 AM, Helga Velroyen <[email protected]> wrote:
>
>
>
> On Tue, Mar 18, 2014 at 11:13 AM, Helga Velroyen <[email protected]>wrote:
>
>>
>>
>>
>> On Mon, Mar 17, 2014 at 1:46 PM, Petr Pudlak <[email protected]> wrote:
>>
>>> .. instead of using a pair.
>>>
>>> Signed-off-by: Petr Pudlak <[email protected]>
>>> ---
>>> src/Ganeti/Locking/Locks.hs | 17 +++++++++++--
>>> src/Ganeti/WConfd/Core.hs | 48
>>> +++++++++++++++++--------------------
>>> src/Ganeti/WConfd/DeathDetection.hs | 4 +++-
>>> 3 files changed, 40 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/src/Ganeti/Locking/Locks.hs b/src/Ganeti/Locking/Locks.hs
>>> index 23202c7..db81b19 100644
>>> --- a/src/Ganeti/Locking/Locks.hs
>>> +++ b/src/Ganeti/Locking/Locks.hs
>>> @@ -27,6 +27,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor,
>>> Boston, MA
>>>
>>> module Ganeti.Locking.Locks
>>> ( GanetiLocks(..)
>>> + , ClientId(..)
>>> , GanetiLockAllocation
>>> , loadLockAllocation
>>> , writeLocksAsyncTask
>>> @@ -34,7 +35,7 @@ module Ganeti.Locking.Locks
>>> , lockLevel
>>> ) where
>>>
>>> -import Control.Monad ((>=>))
>>> +import Control.Monad ((>=>), liftM)
>>> import Control.Monad.Base (MonadBase, liftBase)
>>> import Control.Monad.Error (MonadError, catchError)
>>> import Data.List (stripPrefix)
>>> @@ -170,9 +171,21 @@ instance Lock GanetiLocks where
>>> lockImplications (Network _) = [NetworkLockSet]
>>> lockImplications _ = []
>>>
>>> +-- | A client is identified as a job id, thread id and path to its
>>> process
>>> +-- identifier file
>>> +data ClientId = ClientId
>>> + { ciJobId :: JobId
>>> + , ciLockFile :: FilePath
>>> + }
>>> + deriving (Ord, Eq, Show)
>>>
>>
>> The comment mentions the thread id, but I don't see it in the data
>> structure. Is this intended?
>>
>
> I just saw that it is added in the next patch. If it is not too much
> trouble, maybe leave them out in the comment in this patch and add it in
> the next as well.
>
>
>>
>>
>>> +
>>> +instance J.JSON ClientId where
>>> + showJSON (ClientId jid lf) = J.showJSON (jid, lf)
>>> + readJSON = liftM (uncurry ClientId) . J.readJSON
>>> +
>>> -- | The type of lock Allocations in Ganeti. In Ganeti, the owner of
>>> -- locks are jobs.
>>> -type GanetiLockAllocation = LockAllocation GanetiLocks (JobId, FilePath)
>>> +type GanetiLockAllocation = LockAllocation GanetiLocks ClientId
>>>
>>> -- | Load a lock allocation from disk.
>>> https://memegen.googleplex.com/5618255569879040
>>>
>>> loadLockAllocation :: FilePath -> ResultG GanetiLockAllocation
>>> diff --git a/src/Ganeti/WConfd/Core.hs b/src/Ganeti/WConfd/Core.hs
>>> index 6160bf7..916ffa5 100644
>>> --- a/src/Ganeti/WConfd/Core.hs
>>> +++ b/src/Ganeti/WConfd/Core.hs
>>> @@ -38,8 +38,7 @@ import Language.Haskell.TH (Name)
>>>
>>> import Ganeti.BasicTypes (toErrorStr)
>>> import qualified Ganeti.Locking.Allocation as L
>>> -import Ganeti.Locking.Locks (GanetiLocks, lockLevel, LockLevel)
>>> -import Ganeti.Types (JobId)
>>> +import Ganeti.Locking.Locks (ClientId, GanetiLocks, lockLevel,
>>> LockLevel)
>>> import Ganeti.WConfd.Language
>>> import Ganeti.WConfd.Monad
>>> import Ganeti.WConfd.ConfigWriter
>>> @@ -55,53 +54,50 @@ echo = return
>>> -- ** Locking related functions
>>>
>>> -- | List the locks of a given owner (i.e., a job-id lockfile pair).
>>> -listLocks :: JobId -> FilePath -> WConfdMonad [(GanetiLocks,
>>> L.OwnerState)]
>>> -listLocks jid fpath =
>>> - liftM (M.toList . L.listLocks (jid, fpath)) readLockAllocation
>>> +listLocks :: ClientId -> WConfdMonad [(GanetiLocks, L.OwnerState)]
>>> +listLocks cid = liftM (M.toList . L.listLocks cid) readLockAllocation
>>>
>>> -- | Try to update the locks of a given owner (i.e., a job-id lockfile
>>> pair).
>>> -- This function always returns immediately. If the lock update was
>>> possible,
>>> -- the empty list is returned; otherwise, the lock status is left
>>> completly
>>> -- unchanged, and the return value is the list of jobs which need to
>>> release
>>> -- some locks before this request can succeed.
>>> -tryUpdateLocks :: JobId -> FilePath -> GanetiLockRequest -> WConfdMonad
>>> [JobId]
>>> -tryUpdateLocks jid fpath req =
>>> - liftM (S.toList . S.map fst)
>>> +tryUpdateLocks :: ClientId -> GanetiLockRequest -> WConfdMonad
>>> [ClientId]
>>> +tryUpdateLocks cid req =
>>> + liftM S.toList
>>> . (>>= toErrorStr)
>>> - $ modifyLockAllocation (L.updateLocks (jid, fpath)
>>> - (fromGanetiLockRequest req))
>>> + $ modifyLockAllocation (L.updateLocks cid (fromGanetiLockRequest req))
>>>
>>> https://memegen.googleplex.com/5618255569879040https://memegen.googleplex.com/5618255569879040
>>>
>>> -- | Free all locks of a given owner (i.e., a job-id lockfile pair).
>>> -freeLocks :: JobId -> FilePath -> WConfdMonad ()
>>> -freeLocks jid fpath =
>>> - modifyLockAllocation_ (`L.freeLocks` (jid, fpath))
>>> +freeLocks :: ClientId -> WConfdMonad ()
>>> +freeLocks cid =
>>> + modifyLockAllocation_ (`L.freeLocks` cid)
>>>
>>> -- | Free all locks of a given owner (i.e., a job-id lockfile pair)
>>> -- of a given level in the Ganeti sense (e.g., "cluster", "node").
>>> -freeLocksLevel :: JobId -> FilePath -> LockLevel -> WConfdMonad ()
>>> -freeLocksLevel jid fpath level =
>>> +freeLocksLevel :: ClientId -> LockLevel -> WConfdMonad ()
>>> +freeLocksLevel cid level =
>>> modifyLockAllocation_ (L.freeLocksPredicate ((==) level . lockLevel)
>>> - `flip` (jid, fpath))
>>> + `flip` cid)
>>>
>>> -- | Downgrade all locks of the given level to shared.
>>> -downGradeLocksLevel :: JobId -> FilePath -> LockLevel -> WConfdMonad ()
>>> -downGradeLocksLevel jid fpath level =
>>> - modifyLockAllocation_ $ L.downGradePredicate ((==) level . lockLevel)
>>> - (jid, fpath)
>>> +downGradeLocksLevel :: ClientId -> LockLevel -> WConfdMonad ()
>>> +downGradeLocksLevel cid level =
>>> + modifyLockAllocation_ $ L.downGradePredicate ((==) level . lockLevel)
>>> cid
>>>
>>> -- | Intersect the possesed locks of an owner with a given set.
>>> -intersectLocks :: JobId -> FilePath -> [GanetiLocks] -> WConfdMonad ()
>>> -intersectLocks jid fpath =
>>> - modifyLockAllocation_ . L.intersectLocks (jid,fpath)
>>> +intersectLocks :: ClientId -> [GanetiLocks] -> WConfdMonad ()
>>> +intersectLocks cid =
>>> + modifyLockAllocation_ . L.intersectLocks cid
>>>
>>> -- | Opportunistically allocate locks for a given owner.
>>> -opportunisticLockUnion :: JobId -> FilePath
>>> +opportunisticLockUnion :: ClientId
>>> -> [(GanetiLocks, L.OwnerState)]
>>> -> WConfdMonad [GanetiLocks]
>>> -opportunisticLockUnion jid fpath req =
>>> +opportunisticLockUnion cid req =
>>> liftM S.toList
>>> . modifyLockAllocation
>>> - $ L.opportunisticLockUnion (jid, fpath) req
>>> + $ L.opportunisticLockUnion cid reqhttps://
>>> memegen.googleplex.com/5618255569879040
>>>
>>>
>>> -- * The list of all functions exported to RPC.
>>>
>>> diff --git a/src/Ganeti/WConfd/DeathDetection.hs
>>> b/src/Ganeti/WConfd/DeathDetection.hs
>>> index 532a65f..e5d8a0b 100644
>>> --- a/src/Ganeti/WConfd/DeathDetection.hs
>>> +++ b/src/Ganeti/WConfd/DeathDetection.hs
>>> @@ -45,6 +45,7 @@ import System.Posix.IO
>>> import Ganeti.BasicTypes
>>> import qualified Ganeti.Constants as C
>>> import qualified Ganeti.Locking.Allocation as L
>>> +import Ganeti.Locking.Locks (ClientId(..))
>>> import Ganeti.Logging.Lifted (logDebug, logInfo)
>>> import Ganeti.WConfd.Monad
>>>
>>> @@ -70,7 +71,8 @@ cleanupLocksTask = forever . runResultT $ do
>>> logDebug "Death detection timer fired"
>>> owners <- liftM L.lockOwners readLockAllocation
>>> logDebug $ "Current lock owners: " ++ show owners
>>> - let cleanupIfDead owner@(_, fpath) = do
>>> + let cleanupIfDead owner = do
>>> + let fpath = ciLockFile owner
>>> died <- liftIO (isDead fpath)
>>> when died $ do
>>> logInfo $ show owner ++ " died, releasing locks"
>>> --
>>> 1.9.0.279.gdc9e3eb
>>>
>>>
>>
>>
> Rest LGTM
>
>
>>
>> --
>> --
>> Helga Velroyen | Software Engineer | [email protected] |
>>
>> Google Germany GmbH
>> Dienerstr. 12
>> 80331 München
>>
>> Registergericht und -nummer: Hamburg, HRB 86891
>> Sitz der Gesellschaft: Hamburg
>> Geschäftsführer: Graham Law, Christine Elizabeth Flores
>>
>
>
>
> --
> --
> Helga Velroyen | Software Engineer | [email protected] |
>
> Google Germany GmbH
> Dienerstr. 12
> 80331 München
>
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
> Geschäftsführer: Graham Law, Christine Elizabeth Flores
>
