LGTM, thanks
On Tue, Mar 18, 2014 at 11:49 AM, Petr Pudlák <[email protected]> wrote: > 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 >> > > -- -- 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
