On Wed, Sep 26, 2012 at 9:54 AM, Iustin Pop <[email protected]> wrote: > On Tue, Sep 25, 2012 at 06:43:52PM +0200, Agata Murawska wrote: >> The tests we currently have assume, that all the data required for >> running the query is available - once we add live data, this will no >> longer be the case. >> >> This patch adds boolean parameter to query function, which tells it >> whether to ignore live parameters gathering. > > I would have split this in two parts: first, the addition of the live > field, and then the integration, as they are two separate things. Ack, will send two separate patches with fixes as mentioned below.
> >> +-- | Collect live data from RPC query if enabled. >> +maybeCollectLiveData:: Bool -> ConfigData -> [Node] -> IO [(Node, >> NodeRuntime)] >> + >> +maybeCollectLiveData False _ nodes = >> + return $ zip nodes (repeat $ Left (RpcResultError "Live data disabled")) >> + >> +maybeCollectLiveData True cfg nodes = do >> + let vgs = [clusterVolumeGroupName $ configCluster cfg] >> + hvs = clusterEnabledHypervisors $ configCluster cfg >> + executeRpcCall nodes (RpcCallNodeInfo vgs hvs) > > This is the part which we discussed offline that can be improved. > maybeCollectLiveData should take the list of requested fields, and > decide whether to ask for the hv info and/or vg info based on the > requested fields. Please add a FIXME for this. Ack > >> +-- | Check whether list of queried fields contains live fields. >> +needsLiveData :: [FieldGetter a b] -> Bool >> +needsLiveData [] = False >> +needsLiveData (FieldRuntime _:_) = True >> +needsLiveData (_:xs) = needsLiveData xs > > Uh… you seem to really like explicit recursion, which is badstyle™: > > needsLiveData = any (\getter -> case getter of > FieldRuntime _ -> True > _ -> False) Guilty as charged ;) > > Rest looks good. > > thanks, > iustin
