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

merged 1 commit into from
Nov 11, 2024

Conversation

shamser
Copy link
Contributor

@shamser shamser commented Oct 24, 2024 •
edited
Loading

This fixes the issue of some file operations failing because the source file was locked when the operation was taking place. Source files may be locked if it is being processed elsewhere. Prevously, the source file was being locked to allow the source file's disk read count and read costs properties to be updated. This issue is fixed by making use of CFileAttrLock which makes it possible to update file properties without requiring the file to be locked.

Changes:

  • Make FileSprayer::updateTargetProperties update only update the target properties (it was also previously updating the source properties)
  • Create FileSprayer::updateSourceProperties to update the source properties and make it use CFileAttrLock lock the attributes, removing the need to lock the IDistributedFile.

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change improves the code (refactor or other change that does not change the functionality)
  • This change fixes warnings (the fix does not alter the functionality or the generated code)
  • This change is a breaking change (fix or feature that will cause existing behavior to change).
  • This change alters the query API (existing queries will have to be recompiled)

Checklist:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Cloud-compatibility
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately
  • This is a user interface / front-end modification
    • I have tested my changes in multiple modern browsers
    • The component(s) render as expected

Smoketest:

  • Send notifications about my Pull Request position in Smoketest queue.
  • Test my draft Pull Request.

Testing:

@github-actions GitHub Actions
Copy link

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-32803

Jirabot Action Result:
Workflow Transition To: Merge Pending
Updated PR

@shamser shamser marked this pull request as draft October 24, 2024 11:41
@shamser shamser force-pushed the issue32803 branch 2 times, most recently from 1583c1f to d7e4efb Compare October 25, 2024 11:13
@shamser
Copy link
Contributor Author

shamser commented Oct 25, 2024

HPCC-32879 PR needs to be merged first to allow the changes in this PR to work. @jakesmith

jakesmith
jakesmith requested changes Oct 29, 2024
View reviewed changes
Copy link
Member

@jakesmith jakesmith left a comment

Choose a reason for hiding this comment

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

@shamser - please see comments.

dali/ft/filecopy.cpp Outdated Show resolved Hide resolved
dali/ft/filecopy.cpp Outdated
throw error.getClear();
}

void FileSprayer::updateSourceProperties(cost_type & totalReadCost)
Copy link
Member

Choose a reason for hiding this comment

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

it would be cleaner if updateTargetProperties and updateSourceProperties returned the total (vs filled by reference)
With a comment ahead of each method saying what it returns.

dali/ft/filecopy.cpp
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

@shamser shamser force-pushed the issue32803 branch 3 times, most recently from 279edc1 to d0bb0ff Compare November 1, 2024 09:43
@shamser shamser marked this pull request as ready for review November 1, 2024 09:44
jakesmith
jakesmith approved these changes Nov 5, 2024
View reviewed changes
Copy link
Member

@jakesmith jakesmith left a comment

Choose a reason for hiding this comment

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

@shamser - looks good. Please squash.

Need to revisit super file property updates in this area (https://hpccsystems.atlassian.net/issues/HPCC-32901)

ghalliday
ghalliday reviewed Nov 7, 2024
View reviewed changes
Copy link
Member

@ghalliday ghalliday left a comment

Choose a reason for hiding this comment

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

@shamser 1 minor fix.

dali/ft/filecopy.cpp
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

dali/ft/filecopy.cpp Outdated
distributedSource->addAttrValue(getDFUQResultFieldName(DFUQRFnumDiskReads), totalNumReads);
cost_type legacyReadCost = getLegacyReadCost(distributedSource->queryAttributes(), distributedSource);
distributedSource->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost + totalReadCost);
return totalReadCost;
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
//return the total cost incurred reading the file for this operation (otherwise it is not clear why it doesn't include legacyReadCost)

ghalliday
ghalliday approved these changes Nov 8, 2024
View reviewed changes
Copy link
Member

@ghalliday ghalliday left a comment

Choose a reason for hiding this comment

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

Please squash

@shamser
HPCC-32803 In file ops, update source file props without locking sour…
899b9af 
…ce file

This fixes the issue of some file operations failing because the source file
was locked when the operation was taking place. Source files may be locked
if it is being processed elsewhere. Prevously, the source file was being
locked to allow the source file's disk read count and read costs properties
to be updated. The fix makes use of CFileAttrLock to update file properties
without requiring the file to be locked.

Changes:

- Make FileSprayer::updateTargetProperties update only update the target
properties (it was also previously updating the source properties)
- Create FileSprayer::updateSourceProperties to update the source properties
and make it use CFileAttrLock lock the attributes, removing the need to lock
the IDistributedFile.

Signed-off-by: Shamser Ahmed <shamser.ahmed@lexisnexis.com>
@shamser
Copy link
Contributor Author

shamser commented Nov 11, 2024

@ghalliday squashed.

@ghalliday ghalliday merged commit 6b9f66b into hpcc-systems:candidate-9.6.x Nov 11, 2024
51 checks passed
@github-actions GitHub Actions
Copy link

Jirabot Action Result:
Added fix version: 9.6.64
Added fix version: 9.8.38
Workflow Transition: 'Resolve issue'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakesmith jakesmith jakesmith approved these changes

@ghalliday ghalliday ghalliday approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shamser @ghalliday @jakesmith