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

Merged
merged 1 commit into from
Nov 11, 2024
Merged

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
1 commit
Select commit
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);
Copy link
Member

Choose a reason for hiding this comment

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

This value is not used... I think it should be used in the next line

Copy link
Member

Choose a reason for hiding this comment

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

Add a comment to indicate that this will only add something if cost has not been set, on subsequent calls it will return 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is fixing the locking file issue. There is an issue with the use of legacy file costs which I'll be addressing in this PR: https://hpccsystems.atlassian.net/browse/HPCC-32880

subfile->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost + 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

@jakesmith jakesmith Oct 29, 2024 •
edited
Loading

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 : https://hpccsystems.atlassian.net/issues/HPCC-32901

return totalReadCost; // return the total cost of this file operation (exclude previous and legacy read costs)
}
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