Thank you, much better. LGTM! On Fri, Nov 15, 2013 at 1:56 PM, Klaus Aehlig <[email protected]> wrote:
> On Fri, Nov 15, 2013 at 10:58:58AM +0100, Hrvoje Ribicic wrote: > > I believe that the logic of this function is correct, but reading this > code > > is painful enough that it might be very hard to maintain in the future. > > > > Some concrete suggestions on improving this: > > > > Within the function, i is used as a prefix indicating both instance and > > initial, and it is also the first letter of two other variables - inst > and > > idx. Could this somehow be improved? > > Renamed variables to make the function more readable > > Interdiff patch 3/6 Add function to get the moves between two > configurations > > diff --git a/src/Ganeti/HTools/Cluster.hs b/src/Ganeti/HTools/Cluster.hs > index c647221..6cb7229 100644 > --- a/src/Ganeti/HTools/Cluster.hs > +++ b/src/Ganeti/HTools/Cluster.hs > @@ -1416,17 +1416,17 @@ involvedNodes il plc = > -- | From two adjacent cluster tables get the list of moves that > transitions > -- from to the other > getMoves :: (Table, Table) -> [MoveJob] > -getMoves (Table _ iil _ iplc, Table fnl _ _ fplc) = > +getMoves (Table _ initial_il _ initial_plc, Table final_nl _ _ final_plc) > = > let > plctoMoves (plc@(idx, p, s, mv, _)) = > - let inst = Container.find idx iil > - inam = Instance.name inst > - afn = involvedNodes iil plc > - np = Node.alias $ Container.find p fnl > - ns = Node.alias $ Container.find s fnl > - (_, cmds) = computeMoves inst inam mv np ns > - in (afn, idx, mv, cmds) > - in map plctoMoves . reverse . drop (length iplc) $ reverse fplc > + let inst = Container.find idx initial_il > + inst_name = Instance.name inst > + affected = involvedNodes initial_il initial_plc > + np = Node.alias $ Container.find p final_nl > + ns = Node.alias $ Container.find s final_nl > + (_, cmds) = computeMoves inst inst_name mv np ns > + in (affected, idx, mv, cmds) > + in map plctoMoves . reverse . drop (length initial_plc) $ reverse > final_plc > > -- | Inner function for splitJobs, that either appends the next job to > -- the current jobset, or starts a new jobset. > > > -- > Klaus Aehlig > Google Germany GmbH, Dienerstr. 12, 80331 Muenchen > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > Geschaeftsfuehrer: Graham Law, Christine Elizabeth Flores > Hrvoje Ribicic Ganeti Engineering 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 Steuernummer: 48/725/00206 Umsatzsteueridentifikationsnummer: DE813741370
