Changes following review

Signed-off-by: Shamser Ahmed <[email protected]>
hpcc-systems · Dec 5, 2023 · 3cc32e5 · 3cc32e5
1 parent 0bed6f4
commit 3cc32e5
Show file tree

Hide file tree

Showing 3 changed files with 12 additions and 11 deletions.
diff --git a/dali/base/dadfs.cpp b/dali/base/dadfs.cpp
@@ -193,7 +193,7 @@ static IPropertyTree *getCostPropTree(const char *cluster)
 
 extern da_decl double calcFileAtRestCost(const char * cluster, double sizeGB, double fileAgeDays)
 {
-    Owned<IPropertyTree> costPT = getCostPropTree(cluster);
+    Owned<const IPropertyTree> costPT = getCostPropTree(cluster);
 
     if (costPT==nullptr)
         return 0.0;
@@ -204,7 +204,7 @@ extern da_decl double calcFileAtRestCost(const char * cluster, double sizeGB, do
 
 extern da_decl double calcFileAccessCost(const char * cluster, __int64 numDiskWrites, __int64 numDiskReads)
 {
-    Owned<IPropertyTree> costPT = getCostPropTree(cluster);
+    Owned<const IPropertyTree> costPT = getCostPropTree(cluster);
 
     if (costPT==nullptr)
         return 0.0;
@@ -4963,7 +4963,7 @@ protected: friend class CDistributedFilePart;
                 // Costs need to be calculated from numDiskReads and numDiskWrites for legacy files
                 numDiskWrites = attrs->getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskWrites));
                 doLegacyAccessCostCalc = true;
-                // But legacy index files numDiskReads is likely to be widely inaccurate, so don't use it
+                // NB: Costs of index reading can not be reliably estimated based on 'numDiskReads'
                 if (!isFileKey(*attrs))
                     numDiskReads = attrs->getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads));
             }
@@ -4973,10 +4973,12 @@ protected: friend class CDistributedFilePart;
             StringArray clusterNames;
             unsigned countClusters = getClusterNames(clusterNames);
             for (unsigned i = 0; i < countClusters; i++)
-            {
                 atRestCost += calcFileAtRestCost(clusterNames[i], sizeGB, fileAgeDays);
-                if (doLegacyAccessCostCalc)
-                    accessCost += calcFileAccessCost(clusterNames[i], numDiskWrites, numDiskReads);
+            if (countClusters && doLegacyAccessCostCalc)
+            {
+                // NB: numDiskReads/numDiskWrites are stored at the file level, not per cluster.
+                // So cannot calculate accessCost per cluster, assume cost is based on 1st.
+                accessCost += calcFileAccessCost(clusterNames[0], numDiskWrites, numDiskReads);
             }
         }
         else
@@ -13435,7 +13437,6 @@ IPropertyTreeIterator *deserializeFileAttrIterator(MemoryBuffer& mb, unsigned nu
         void setCost(IPropertyTree* file, const char *nodeGroup)
         {
             // Set the following dynamic fields: atRestCost, accessCost, cost and for legacy files: readCost, writeCost
-            // Dyamically calc and set the atRest cost field
             StringBuffer str;
             double fileAgeDays = 0.0;
             if (file->getProp(getDFUQResultFieldName(DFUQRFtimemodified), str))

diff --git a/dali/base/dadfs.hpp b/dali/base/dadfs.hpp
@@ -905,12 +905,12 @@ template<typename Source>
 inline cost_type getLegacyReadCost(const IPropertyTree & fileAttr, Source source)
 {
     // Legacy files do not have @readCost attribute, so calculate from numDiskRead
-    // But don't try to calc legacy cost if index file because numDiskReads is likely to be inaccurate
+    // NB: Costs of index reading can not be reliably estimated based on 'numDiskReads'
     if (!hasReadWriteCostFields(fileAttr) && fileAttr.hasProp(getDFUQResultFieldName(DFUQRFnumDiskReads))
         && !isFileKey(fileAttr))
     {
         stat_type prevDiskReads = fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), 0);
-        return  money2cost_type(calcFileAccessCost(source, 0, prevDiskReads));
+        return money2cost_type(calcFileAccessCost(source, 0, prevDiskReads));
     }
     else
         return 0;
@@ -922,7 +922,7 @@ inline cost_type getLegacyWriteCost(const IPropertyTree & fileAttr, Source sourc
     if (!hasReadWriteCostFields(fileAttr) && fileAttr.hasProp(getDFUQResultFieldName(DFUQRFnumDiskWrites)))
     {
         stat_type prevDiskWrites = fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskWrites), 0);
-        return  money2cost_type(calcFileAccessCost(source, prevDiskWrites, 0));
+        return money2cost_type(calcFileAccessCost(source, prevDiskWrites, 0));
     }
     else
         return 0;

diff --git a/thorlcr/graph/thgraphmaster.cpp b/thorlcr/graph/thgraphmaster.cpp
@@ -649,7 +649,7 @@ void CMasterActivity::done()
 
 void CMasterActivity::updateFileReadCostStats()
 {
-     // Returns updates numDiskReads & readCost in the file attributes and returns the readCost
+    // Updates numDiskReads & readCost in the file attributes and returns the readCost
     auto updateReadCosts = [](IDistributedFile *file, CThorStatsCollection &stats)
     {
         stat_type curDiskReads = stats.getStatisticSum(StNumDiskReads);