Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HPCC-32803 In file ops, update source file props without locking source file #19232

Open
wants to merge 2 commits into
base: candidate-9.6.x
Choose a base branch
from
Open

HPCC-32803 In file ops, update source file props without locking source file #19232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f6cbe7
HPCC-32803 In file ops, update source file props without locking sour…
shamser Oct 24, 2024
d0bb0ff
HPCC-32803 Changes following review
shamser Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 23 additions & 23 deletions dali/ft/filecopy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3396,7 +3396,9 @@ void FileSprayer::spray()

//If got here then we have succeeded
//Note: On failure, costs will not be updated. Future: would be useful to have a way to update costs on failure.
updateTargetProperties();
cost_type totalWriteCost = updateTargetProperties();
cost_type totalReadCost = updateSourceProperties();
progressReport->setFileAccessCost(totalReadCost+totalWriteCost);

StringBuffer copyEventText; // [logical-source] > [logical-target]
if (distributedSource)
Expand Down Expand Up @@ -3446,13 +3448,13 @@ bool FileSprayer::isSameSizeHeaderFooter()
return retVal;
}

void FileSprayer::updateTargetProperties()
cost_type FileSprayer::updateTargetProperties()
{
TimeSection timer("FileSprayer::updateTargetProperties() time");
Owned<IException> error;
cost_type totalWriteCost = 0;
if (distributedTarget)
{
cost_type totalWriteCost = 0;
StringBuffer failedParts;
CRC32Merger partCRC;
offset_t partLength = 0;
Expand Down Expand Up @@ -3803,12 +3805,20 @@ void FileSprayer::updateTargetProperties()
int expireDays = options->getPropInt("@expireDays", -1);
if (expireDays != -1)
curProps.setPropInt("@expireDays", expireDays);
return totalWriteCost;
}
if (error)
throw error.getClear();
return 0;
}

cost_type FileSprayer::updateSourceProperties()
{
TimeSection timer("FileSprayer::updateSourceProperties() time");
// Update file readCost and numReads in file properties and do the same for subfiles
// Update totalReadCost
cost_type totalReadCost = 0;
if (distributedSource)
{
cost_type totalReadCost = 0;
IDistributedSuperFile * superSrc = distributedSource->querySuperFile();
if (superSrc && superSrc->numSubFiles() > 0)
{
Expand All @@ -3833,14 +3843,10 @@ void FileSprayer::updateTargetProperties()
// so query the first (and only) subfile
subfile = &superSrc->querySubFile(0);
}
DistributedFilePropertyLock lock(subfile);
IPropertyTree &subFileProps = lock.queryAttributes();
stat_type prevNumReads = subFileProps.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), 0);
cost_type legacyReadCost = getLegacyReadCost(subfile->queryAttributes(), subfile);
cost_type prevReadCost = subFileProps.getPropInt64(getDFUQResultFieldName(DFUQRFreadCost), 0);
cost_type curReadCost = calcFileAccessCost(subfile, 0, curProgress.numReads);
subFileProps.setPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), prevNumReads + curProgress.numReads);
subFileProps.setPropInt64(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost + prevReadCost + curReadCost);
subfile->addAttrValue(getDFUQResultFieldName(DFUQRFnumDiskReads), curProgress.numReads);
cost_type legacyReadCost = getLegacyReadCost(subfile->queryAttributes(), subfile);
subfile->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), curReadCost);
totalReadCost += curReadCost;
}
else
Expand All @@ -3854,20 +3860,14 @@ void FileSprayer::updateTargetProperties()
{
totalReadCost = calcFileAccessCost(distributedSource, 0, totalNumReads);
}
DistributedFilePropertyLock lock(distributedSource);
IPropertyTree &curProps = lock.queryAttributes();
stat_type prevNumReads = curProps.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), 0);
cost_type legacyReadCost = getLegacyReadCost(curProps, distributedSource);
cost_type prevReadCost = curProps.getPropInt64(getDFUQResultFieldName(DFUQRFreadCost), 0);
curProps.setPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), prevNumReads + totalNumReads);
curProps.setPropInt64(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost + prevReadCost + totalReadCost);
distributedSource->addAttrValue(getDFUQResultFieldName(DFUQRFnumDiskReads), totalNumReads);
cost_type legacyReadCost = getLegacyReadCost(distributedSource->queryAttributes(), distributedSource);
distributedSource->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost + totalReadCost);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed, separate topic, updating supers directly vs via updateFileAttrs
I've opened a new JIRA to revisit/tackle this separately.

return totalReadCost;
}
progressReport->setFileAccessCost(totalReadCost+totalWriteCost);
if (error)
throw error.getClear();
return 0;
}


void FileSprayer::splitAndCollectFileInfo(IPropertyTree * newRecord, RemoteFilename &remoteFileName,
bool isDistributedSource)
{
Expand Down
3 changes: 2 additions & 1 deletion dali/ft/filecopy.ipp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,8 @@ protected:
void savePartition();
void setCopyCompressedRaw();
void setSource(IFileDescriptor * source, unsigned copy, unsigned mirrorCopy = (unsigned)-1);
void updateTargetProperties();
cost_type updateTargetProperties();
cost_type updateSourceProperties();
bool usePullOperation() const;
bool usePushOperation() const;
bool usePushWholeOperation() const;
Expand Down
Loading