From a18be04b650fb81774139925d714ad255eb39add Mon Sep 17 00:00:00 2001 From: Shamser Ahmed Date: Mon, 18 Dec 2023 16:39:08 +0000 Subject: [PATCH] HPCC-31028 Use cost_type internally for consistency Costs have been represented as double in some places in the code and cost_type in other places. The use of both double and cost_type internally for costs has made the code more difficult to understand and maintain. It has lead to some bugs. This commit makes all internal representation of cost in the code as cost_type. The cost_type is only converted to double when returning costs externally via ESP services. Signed-off-by: Shamser Ahmed --- dali/base/dadfs.cpp | 24 +++++++++++------------ dali/base/dadfs.hpp | 12 ++++++------ dali/daliadmin/daliadmin.cpp | 6 +++--- dali/dfu/dfurun.cpp | 2 +- dali/dfu/dfuwu.cpp | 6 +++--- dali/dfu/dfuwu.hpp | 4 ++-- dali/ft/daft.hpp | 2 +- dali/ft/daftprogress.hpp | 4 ++-- dali/ft/filecopy.cpp | 6 +++--- ecl/hthor/hthor.cpp | 6 +++--- esp/clients/ws_dfsclient/ws_dfsclient.cpp | 2 +- esp/services/ws_dfu/ws_dfuService.cpp | 8 ++++---- esp/services/ws_fs/ws_fsService.cpp | 2 +- thorlcr/graph/thgraphmaster.cpp | 8 ++++---- 14 files changed, 46 insertions(+), 46 deletions(-) diff --git a/dali/base/dadfs.cpp b/dali/base/dadfs.cpp index 31c3d422a0e..195428a7e24 100644 --- a/dali/base/dadfs.cpp +++ b/dali/base/dadfs.cpp @@ -191,30 +191,30 @@ static IPropertyTree *getCostPropTree(const char *cluster) } } -extern da_decl double calcFileAtRestCost(const char * cluster, double sizeGB, double fileAgeDays) +extern da_decl cost_type calcFileAtRestCost(const char * cluster, double sizeGB, double fileAgeDays) { Owned costPT = getCostPropTree(cluster); if (costPT==nullptr) - return 0.0; + return 0; double atRestPrice = costPT->getPropReal("@storageAtRest", 0.0); double storageCostDaily = atRestPrice * 12 / 365; - return storageCostDaily * sizeGB * fileAgeDays; + return money2cost_type(storageCostDaily * sizeGB * fileAgeDays); } -extern da_decl double calcFileAccessCost(const char * cluster, __int64 numDiskWrites, __int64 numDiskReads) +extern da_decl cost_type calcFileAccessCost(const char * cluster, __int64 numDiskWrites, __int64 numDiskReads) { Owned costPT = getCostPropTree(cluster); if (costPT==nullptr) - return 0.0; + return 0; constexpr int accessPriceScalingFactor = 10000; // read/write pricing based on 10,000 operations double readPrice = costPT->getPropReal("@storageReads", 0.0); double writePrice = costPT->getPropReal("@storageWrites", 0.0); - return (readPrice * numDiskReads / accessPriceScalingFactor) + (writePrice * numDiskWrites / accessPriceScalingFactor); + return money2cost_type((readPrice * numDiskReads / accessPriceScalingFactor) + (writePrice * numDiskWrites / accessPriceScalingFactor)); } -extern da_decl double calcFileAccessCost(IDistributedFile *f, __int64 numDiskWrites, __int64 numDiskReads) +extern da_decl cost_type calcFileAccessCost(IDistributedFile *f, __int64 numDiskWrites, __int64 numDiskReads) { StringBuffer clusterName; // Should really specify the cluster number too, but this is the best we can do for now @@ -4942,7 +4942,7 @@ protected: friend class CDistributedFilePart; else return false; } - virtual void getCost(const char * cluster, double & atRestCost, double & accessCost) override + virtual void getCost(const char * cluster, cost_type & atRestCost, cost_type & accessCost) override { CDateTime dt; getModificationTime(dt); @@ -4956,7 +4956,7 @@ protected: friend class CDistributedFilePart; if (hasReadWriteCostFields(*attrs)) { // Newer files have readCost and writeCost attributes - accessCost = cost_type2money(attrs->getPropInt64(getDFUQResultFieldName(DFUQRFreadCost)) + attrs->getPropInt64(getDFUQResultFieldName(DFUQRFwriteCost))); + accessCost = attrs->getPropInt64(getDFUQResultFieldName(DFUQRFreadCost)) + attrs->getPropInt64(getDFUQResultFieldName(DFUQRFwriteCost)); } else { @@ -7041,12 +7041,12 @@ class CDistributedSuperFile: public CDistributedFileBase return false; } - virtual void getCost(const char * cluster, double & atRestCost, double & accessCost) override + virtual void getCost(const char * cluster, cost_type & atRestCost, cost_type & accessCost) override { CriticalBlock block (sect); ForEachItemIn(i,subfiles) { - double tmpAtRestcost, tmpAccessCost; + cost_type tmpAtRestcost, tmpAccessCost; IDistributedFile &f = subfiles.item(i); f.getCost(cluster, tmpAtRestcost, tmpAccessCost); atRestCost += tmpAtRestcost; @@ -13454,7 +13454,7 @@ IPropertyTreeIterator *deserializeFileAttrIterator(MemoryBuffer& mb, unsigned nu else sizeDiskSize = file->getPropInt64(getDFUQResultFieldName(DFUQRForigsize), 0); double sizeGB = sizeDiskSize / ((double)1024 * 1024 * 1024); - cost_type atRestCost = money2cost_type(calcFileAtRestCost(nodeGroup, sizeGB, fileAgeDays)); + cost_type atRestCost = calcFileAtRestCost(nodeGroup, sizeGB, fileAgeDays); file->setPropInt64(getDFUQResultFieldName(DFUQRFatRestCost), atRestCost); // Dyamically calc and set the access cost field and for legacy files set read/write cost fields diff --git a/dali/base/dadfs.hpp b/dali/base/dadfs.hpp index 6e5eff9e37e..c475f37a8ae 100644 --- a/dali/base/dadfs.hpp +++ b/dali/base/dadfs.hpp @@ -433,7 +433,7 @@ interface IDistributedFile: extends IInterface virtual bool getSkewInfo(unsigned &maxSkew, unsigned &minSkew, unsigned &maxSkewPart, unsigned &minSkewPart, bool calculateIfMissing) = 0; virtual int getExpire(StringBuffer *expirationDate) = 0; virtual void setExpire(int expireDays) = 0; - virtual void getCost(const char * cluster, double & atRestCost, double & accessCost) = 0; + virtual void getCost(const char * cluster, cost_type & atRestCost, cost_type & accessCost) = 0; }; @@ -889,9 +889,9 @@ extern da_decl void ensureFileScope(const CDfsLogicalFileName &dlfn, unsigned ti extern da_decl bool checkLogicalName(const char *lfn,IUserDescriptor *user,bool readreq,bool createreq,bool allowquery,const char *specialnotallowedmsg); -extern da_decl double calcFileAtRestCost(const char * cluster, double sizeGB, double fileAgeDays); -extern da_decl double calcFileAccessCost(const char * cluster, __int64 numDiskWrites, __int64 numDiskReads); -extern da_decl double calcFileAccessCost(IDistributedFile *f, __int64 numDiskWrites, __int64 numDiskReads); +extern da_decl cost_type calcFileAtRestCost(const char * cluster, double sizeGB, double fileAgeDays); +extern da_decl cost_type calcFileAccessCost(const char * cluster, __int64 numDiskWrites, __int64 numDiskReads); +extern da_decl cost_type calcFileAccessCost(IDistributedFile *f, __int64 numDiskWrites, __int64 numDiskReads); constexpr bool defaultPrivilegedUser = true; constexpr bool defaultNonPrivilegedUser = false; @@ -910,7 +910,7 @@ inline cost_type getLegacyReadCost(const IPropertyTree & fileAttr, Source source && !isFileKey(fileAttr)) { stat_type prevDiskReads = fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), 0); - return money2cost_type(calcFileAccessCost(source, 0, prevDiskReads)); + return 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 calcFileAccessCost(source, prevDiskWrites, 0); } else return 0; diff --git a/dali/daliadmin/daliadmin.cpp b/dali/daliadmin/daliadmin.cpp index 474580c01d4..26c002fbb0f 100644 --- a/dali/daliadmin/daliadmin.cpp +++ b/dali/daliadmin/daliadmin.cpp @@ -726,10 +726,10 @@ static void testDFSFile(IDistributedFile *legacyDfsFile, const char *logicalName PROGLOG("expire: %d", expire); try { - double atRestCost, accessCost; + cost_type atRestCost, accessCost; legacyDfsFile->getCost(clusterName.str(), atRestCost, accessCost); - PROGLOG("accessCost: %f", accessCost); - PROGLOG("atRestCost: %f", atRestCost); + PROGLOG("accessCost: %f", cost_type2money(accessCost)); + PROGLOG("atRestCost: %f", cost_type2money(atRestCost)); } catch(IException *e) { diff --git a/dali/dfu/dfurun.cpp b/dali/dfu/dfurun.cpp index 36de1b7a9eb..add2dfe81ea 100644 --- a/dali/dfu/dfurun.cpp +++ b/dali/dfu/dfurun.cpp @@ -143,7 +143,7 @@ class CDFUengine: public CInterface, implements IDFUengine DaftProgress::setRange(sizeReadBefore,totalSize,_totalNodes); progress->setTotalNodes(_totalNodes); } - void setFileAccessCost(double fileAccessCost) + void setFileAccessCost(cost_type fileAccessCost) { progress->setFileAccessCost(fileAccessCost); } diff --git a/dali/dfu/dfuwu.cpp b/dali/dfu/dfuwu.cpp index 26306e8db90..0da31062b43 100644 --- a/dali/dfu/dfuwu.cpp +++ b/dali/dfu/dfuwu.cpp @@ -779,7 +779,7 @@ class CDFUprogress: public CLinkedDFUWUchild, implements IDFUprogress queryRoot()->getProp("@subdone",str); return str; } - double getFileAccessCost() const + cost_type getFileAccessCost() const { CriticalBlock block(parent->crit); return queryRoot()->getPropInt64("@fileAccessCost"); @@ -795,10 +795,10 @@ class CDFUprogress: public CLinkedDFUWUchild, implements IDFUprogress CriticalBlock block(parent->crit); queryRoot()->setProp("@subdone",str); } - void setFileAccessCost(double fileAccessCost) + void setFileAccessCost(cost_type fileAccessCost) { CriticalBlock block(parent->crit); - queryRoot()->setPropReal("@fileAccessCost", fileAccessCost); + queryRoot()->setPropInt64("@fileAccessCost", fileAccessCost); } unsigned incPublisherTaskCount() { diff --git a/dali/dfu/dfuwu.hpp b/dali/dfu/dfuwu.hpp index 923630a719c..770af07c85a 100644 --- a/dali/dfu/dfuwu.hpp +++ b/dali/dfu/dfuwu.hpp @@ -316,7 +316,7 @@ interface IConstDFUprogress: extends IInterface virtual unsigned getTotalNodes() const = 0; virtual StringBuffer &getSubInProgress(StringBuffer &str) const = 0; // sub-DFUWUs in progress virtual StringBuffer &getSubDone(StringBuffer &str) const = 0; // sub-DFUWUs done (list) - virtual double getFileAccessCost() const = 0; + virtual cost_type getFileAccessCost() const = 0; virtual unsigned getPublisherTaskCount() const = 0; }; @@ -333,7 +333,7 @@ interface IDFUprogress: extends IConstDFUprogress virtual void setSubInProgress(const char *str) = 0; // set sub-DFUWUs in progress virtual void setSubDone(const char *str) = 0; // set sub-DFUWUs done virtual void clearProgress() = 0; - virtual void setFileAccessCost(double fileAccessCost) = 0; + virtual void setFileAccessCost(cost_type fileAccessCost) = 0; virtual unsigned incPublisherTaskCount() = 0; }; diff --git a/dali/ft/daft.hpp b/dali/ft/daft.hpp index 39147db875e..84bc0d85860 100644 --- a/dali/ft/daft.hpp +++ b/dali/ft/daft.hpp @@ -33,7 +33,7 @@ interface IDaftProgress { virtual void onProgress(unsigned __int64 sizeDone, unsigned __int64 totalSize, unsigned numNodes, unsigned __int64 numReads, unsigned __int64 numWrites) = 0; // how much has been done virtual void setRange(unsigned __int64 sizeReadBefore, unsigned __int64 totalSize, unsigned totalNodes) = 0; // how much has been done - virtual void setFileAccessCost(double fileAccessCost) = 0; + virtual void setFileAccessCost(cost_type fileAccessCost) = 0; }; interface IDaftCopyProgress diff --git a/dali/ft/daftprogress.hpp b/dali/ft/daftprogress.hpp index e6b0468002c..d76b1f24210 100644 --- a/dali/ft/daftprogress.hpp +++ b/dali/ft/daftprogress.hpp @@ -33,7 +33,7 @@ class DALIFT_API DaftProgress : public IDaftProgress unsigned numNodes, unsigned __int64 numReads, unsigned __int64 numWrites) = 0; virtual void displaySummary(const char * timeTaken, unsigned kbPerSecond) = 0; virtual void setRange(unsigned __int64 sizeReadBefore, unsigned __int64 totalSize, unsigned _totalNodes); - virtual void setFileAccessCost(double fileAccessCost) = 0; + virtual void setFileAccessCost(cost_type fileAccessCost) = 0; protected: void formatTime(char * buffer, unsigned secs); @@ -59,7 +59,7 @@ class DALIFT_API DemoProgress : public DaftProgress unsigned __int64 scaledDone, unsigned __int64 scaledTotal, const char * scale, unsigned kbPerSecondAve, unsigned kbPerSecondRate, unsigned numNodes); virtual void displaySummary(const char * timeTaken, unsigned kbPerSecond); - virtual void setFileAccessCost(double fileAccessCost) {}; + virtual void setFileAccessCost(cost_type fileAccessCost) {}; }; #endif diff --git a/dali/ft/filecopy.cpp b/dali/ft/filecopy.cpp index f8a8b9bb8bb..a54960df44e 100644 --- a/dali/ft/filecopy.cpp +++ b/dali/ft/filecopy.cpp @@ -3387,7 +3387,7 @@ void FileSprayer::spray() updateTargetProperties(); //Calculate and store file access cost - double fileAccessCost = 0.0; + cost_type fileAccessCost = 0; if (distributedTarget) { StringBuffer cluster; @@ -3592,7 +3592,7 @@ void FileSprayer::updateTargetProperties() DistributedFilePropertyLock lock(distributedTarget); IPropertyTree &curProps = lock.queryAttributes(); - cost_type writeCost = money2cost_type(calcFileAccessCost(distributedTarget, totalNumWrites, 0)); + cost_type writeCost = calcFileAccessCost(distributedTarget, totalNumWrites, 0); curProps.setPropInt64(getDFUQResultFieldName(DFUQRFwriteCost), writeCost); curProps.setPropInt64(getDFUQResultFieldName(DFUQRFnumDiskWrites), totalNumWrites); @@ -3777,7 +3777,7 @@ void FileSprayer::updateTargetProperties() { IPropertyTree & fileAttr = distributedSource->queryAttributes(); cost_type legacyReadCost = getLegacyReadCost(fileAttr, distributedSource); - cost_type curReadCost = money2cost_type(calcFileAccessCost(distributedSource, 0, totalNumReads)); + cost_type curReadCost = calcFileAccessCost(distributedSource, 0, totalNumReads); distributedSource->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost+curReadCost); distributedSource->addAttrValue(getDFUQResultFieldName(DFUQRFnumDiskReads), totalNumReads); } diff --git a/ecl/hthor/hthor.cpp b/ecl/hthor/hthor.cpp index 3951a5b632a..036d6beb263 100644 --- a/ecl/hthor/hthor.cpp +++ b/ecl/hthor/hthor.cpp @@ -789,7 +789,7 @@ void CHThorDiskWriteActivity::publish() { StringBuffer clusterName; file->getClusterName(0, clusterName); - diskAccessCost = money2cost_type(calcFileAccessCost(clusterName, numDiskWrites, 0)); + diskAccessCost = calcFileAccessCost(clusterName, numDiskWrites, 0); properties.setPropInt64(getDFUQResultFieldName(DFUQRFwriteCost), diskAccessCost); } file->attach(logicalName.get(), agent.queryCodeContext()->queryUserDescriptor()); @@ -1437,7 +1437,7 @@ void CHThorIndexWriteActivity::execute() StringBuffer clusterName; dfile->getClusterName(0, clusterName); - diskAccessCost = money2cost_type(calcFileAccessCost(clusterName, numDiskWrites, 0)); + diskAccessCost = calcFileAccessCost(clusterName, numDiskWrites, 0); properties.setPropInt64(getDFUQResultFieldName(DFUQRFwriteCost), diskAccessCost); } else @@ -8543,7 +8543,7 @@ void CHThorDiskReadBaseActivity::closepart() } IPropertyTree & fileAttr = dFile->queryAttributes(); cost_type legacyReadCost = getLegacyReadCost(fileAttr, dFile); - cost_type curReadCost = money2cost_type(calcFileAccessCost(dFile, 0, curDiskReads)); + cost_type curReadCost = calcFileAccessCost(dFile, 0, curDiskReads); dFile->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost + curReadCost); dFile->addAttrValue(getDFUQResultFieldName(DFUQRFnumDiskReads), curDiskReads); diff --git a/esp/clients/ws_dfsclient/ws_dfsclient.cpp b/esp/clients/ws_dfsclient/ws_dfsclient.cpp index 8270b912a8b..3aa39d35bbd 100644 --- a/esp/clients/ws_dfsclient/ws_dfsclient.cpp +++ b/esp/clients/ws_dfsclient/ws_dfsclient.cpp @@ -235,7 +235,7 @@ class CServiceDistributedFileBase : public CSimpleInterfaceOf virtual bool isExternal() const override { return false; } virtual bool getSkewInfo(unsigned &maxSkew, unsigned &minSkew, unsigned &maxSkewPart, unsigned &minSkewPart, bool calculateIfMissing) override { return legacyDFSFile->getSkewInfo(maxSkew, minSkew, maxSkewPart, minSkewPart, calculateIfMissing); } virtual int getExpire(StringBuffer *expirationDate) override { return legacyDFSFile->getExpire(expirationDate); } - virtual void getCost(const char * cluster, double & atRestCost, double & accessCost) override { legacyDFSFile->getCost(cluster, atRestCost, accessCost); } + virtual void getCost(const char * cluster, cost_type & atRestCost, cost_type & accessCost) override { legacyDFSFile->getCost(cluster, atRestCost, accessCost); } // setters (change file meta data) virtual void setPreferredClusters(const char *clusters) override { legacyDFSFile->setPreferredClusters(clusters); } diff --git a/esp/services/ws_dfu/ws_dfuService.cpp b/esp/services/ws_dfu/ws_dfuService.cpp index e9409c5a553..ad3696f03e5 100644 --- a/esp/services/ws_dfu/ws_dfuService.cpp +++ b/esp/services/ws_dfu/ws_dfuService.cpp @@ -2667,15 +2667,15 @@ void CWsDfuEx::doGetFileDetails(IEspContext &context, IUserDescriptor *udesc, co } if (version >= 1.60) { - double atRestCost, accessCost; + cost_type atRestCost, accessCost; df->getCost(cluster, atRestCost, accessCost); if (version <= 1.61) - FileDetails.setCost(atRestCost+accessCost); + FileDetails.setCost(cost_type2money(atRestCost+accessCost)); else { - FileDetails.setAccessCost(accessCost); - FileDetails.setAtRestCost(atRestCost); + FileDetails.setAccessCost(cost_type2money(accessCost)); + FileDetails.setAtRestCost(cost_type2money(atRestCost)); } } if (version >= 1.65) diff --git a/esp/services/ws_fs/ws_fsService.cpp b/esp/services/ws_fs/ws_fsService.cpp index 691bd2db6a1..b23d4347464 100644 --- a/esp/services/ws_fs/ws_fsService.cpp +++ b/esp/services/ws_fs/ws_fsService.cpp @@ -353,7 +353,7 @@ static void DeepAssign(IEspContext &context, IConstDFUWorkUnit *src, IEspDFUWork if(secs > 0) dest.setSecsLeft(secs); dest.setPercentDone(prog->getPercentDone()); - dest.setFileAccessCost(prog->getFileAccessCost()); + dest.setFileAccessCost(cost_type2money(prog->getFileAccessCost())); } IConstDFUoptions *options = src->queryOptions(); diff --git a/thorlcr/graph/thgraphmaster.cpp b/thorlcr/graph/thgraphmaster.cpp index 16ef52cc4fa..62cf0548102 100644 --- a/thorlcr/graph/thgraphmaster.cpp +++ b/thorlcr/graph/thgraphmaster.cpp @@ -661,7 +661,7 @@ void CMasterActivity::updateFileReadCostStats() { // Legacy file: calculate readCost using prev disk reads and new disk reads stat_type prevDiskReads = fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), 0); - legacyReadCost = money2cost_type(calcFileAccessCost(clusterName, 0, prevDiskReads)); + legacyReadCost = calcFileAccessCost(clusterName, 0, prevDiskReads); } stat_type curDiskReads = stats.getStatisticSum(StNumDiskReads); if(useJhtreeCacheStats) @@ -669,10 +669,10 @@ void CMasterActivity::updateFileReadCostStats() stat_type numActualReads = stats.getStatisticSum(StNumNodeDiskFetches) + stats.getStatisticSum(StNumLeafDiskFetches) + stats.getStatisticSum(StNumBlobDiskFetches); - curReadCost = money2cost_type(calcFileAccessCost(clusterName, 0, numActualReads)); + curReadCost = calcFileAccessCost(clusterName, 0, numActualReads); } else - curReadCost = money2cost_type(calcFileAccessCost(clusterName, 0, curDiskReads)); + curReadCost = calcFileAccessCost(clusterName, 0, curDiskReads); file->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost + curReadCost); file->addAttrValue(getDFUQResultFieldName(DFUQRFnumDiskReads), curDiskReads); return curReadCost; @@ -728,7 +728,7 @@ void CMasterActivity::updateFileWriteCostStats(IFileDescriptor & fileDesc, IProp assertex(fileDesc.numClusters()>=1); StringBuffer clusterName; fileDesc.getClusterGroupName(0, clusterName);// Note: calculating for 1st cluster. (Future: calc for >1 clusters) - cost_type writeCost = money2cost_type(calcFileAccessCost(clusterName, numDiskWrites, 0)); + cost_type writeCost = calcFileAccessCost(clusterName, numDiskWrites, 0); props.setPropInt64(getDFUQResultFieldName(DFUQRFwriteCost), writeCost); diskAccessCost = writeCost; }