HPCC-32873 Prevent concurrent write to same file when spraying/despraying #19240

jakesmith · 2024-10-25T13:16:13Z

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:

Smoketest:

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

Testing:

github-actions · 2024-10-25T13:17:36Z

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

Jirabot Action Result:
Assigning user: [email protected]
Workflow Transition To: Merge Pending
Updated PR

ghalliday · 2024-10-31T17:07:28Z

system/jlib/jfile.cpp

+                            values[FileSyncWriteClose] = v != unsetPlaneAttrValue ? 1 : 0;
+                        }
+                        else
+                            values[propNum] = false;


= unsetPlaneAttrValue

jakesmith · 2024-11-01T00:32:07Z

@ghalliday - please see 2nd commit.

ghalliday

A few comments. The only one that I think needs addressing is push single file if target does not support multiple writes will pull. Probably not a major problem, although inefficient. The logging could be confusing though.

ghalliday · 2024-11-01T10:24:07Z

dali/ft/filecopy.cpp

+    else
+        targetSupportsConcurrentWrite = true;
+    if (!targetPlane.isEmpty())
+        targetSupportsConcurrentWrite = 0 != getPlaneAttributeValue(targetPlane, ConcurrentWriteSupport, targetSupportsConcurrentWrite ? 1 : 0);


for future: getPlaneAttributeBool() would clean the code up

ghalliday · 2024-11-01T10:25:25Z

dali/ft/filecopy.cpp

+        // always pull
+        if (pushRequested)
+            IWARNLOG("Ignoring push option as targets < sources and target does not support concurrent write");
+        return true; // could be push if equal # of soruces and targets but no point


trivial: typo "soruces"

ghalliday · 2024-11-01T10:28:03Z

dali/ft/filecopy.cpp

-        return false;
+        // always pull
+        if (pushRequested)
+            IWARNLOG("Ignoring push option as targets < sources and target does not support concurrent write");


I think this could occur if targets > sources - including sources==1 where push may make sense, especially if it is small.

Multiple writes could be involved even if targets < sources (see comment on line 4072)

if >1 source , and pushing, then each source pusher may write to same target, e.g. 2 source pushing to 3 targets, each source will push to the middle target ?

sources == 1 .. ok that is special and will only ever be pushing 1 partitions to single target ?

so special case source==1 here and leave with push preference if specified, otherwise, calc size and push if < threshold and pull otherwise?

ghalliday · 2024-11-01T10:31:00Z

dali/ft/filecopy.cpp

+        bool wantPush = false;
+        if (targets.ordinality() < sources.ordinality()) // implying multiple writes to same target, force pull, and will common up on matching target filenames
+            wantPull = true;
+        else if (targets.ordinality() > sources.ordinality()) // targets > sources. i.e. multiple splits of source files for each target


If doing an M to N copy (e.g. 20 to 30) you could still have multiple writes to the same target. Probably an edge case we don't care about - since that is legal in this branch.

this is when targetSupportsConcurrentWrite = true only (e.g. BM not cloud), so it's okay / desirable (or at least maintaining previous behaviour) to continue with push even if multiple sources being pushed to same target in this case?

ghalliday · 2024-11-01T10:33:15Z

system/jlib/jfile.cpp

+                        unsigned __int64 value = plane.getPropInt64(prop.c_str(), unsetPlaneAttrValue);
+                        if (unsetPlaneAttrValue != value)
+                        {
+                            if (attrInfo.scale)


minor: For simplicity I would have coded using a scale of 1 for the booleans and unconditionally multiplied by the scale.

I thought about that, chose not to, with thought in mind that might later have strings too perhaps, and would want to only scale if explicitly made sense to.

ghalliday · 2024-11-01T10:35:19Z

system/jlib/jfile.cpp

+                        {
+                            unsigned __int64 v = plane.getPropInt64("expert/@fileSyncMaxRetrySecs", unsetPlaneAttrValue);
+                            // NB: fileSyncMaxRetrySecs==0 is treated as set/enabled
+                            value = v != unsetPlaneAttrValue ? 1 : 0;


I know this hasn't changed, but I think this should actually be

if (v != unsetPlaneAttrValue) value = 1;

but changing it for 9.6.x would probably be a mistake.

well it's all deprecated (fileSyncMaxRetrySecs) - I am just keeping what it used to be, which was that if 0, it did enable fsync with no retry.

ghalliday · 2024-11-01T10:36:22Z

system/jlib/jfile.cpp

        StringBuffer fileFlagsStr;
-        if (getComponentConfigSP()->getProp("expert/@enableIFileMask", fileFlagsStr) || getGlobalConfigSP()->getProp("expert/@enableIFileMask", fileFlagsStr))
+        if (componentConfig->getProp("expert/@enableIFileMask", fileFlagsStr) || getGlobalConfigSP()->getProp("expert/@enableIFileMask", fileFlagsStr))


minor: Could also move getGlobalConfigSP() into local to avoid multiple calls.

will change

ghalliday

Looks logically correct, but I'm not sure the code is in the right place. One trivial typo.

ghalliday · 2024-11-01T15:21:36Z

dali/ft/filecopy.cpp

+    if (targetSupportsConcurrentWrite)
+        return; // push ok
+
+    bool multilpeSourcesPerTarget = false;


typo: multiple

ghalliday · 2024-11-01T15:26:25Z

dali/ft/filecopy.cpp

@@ -3353,6 +3405,10 @@ void FileSprayer::spray()
    }
    addEmptyFilesToPartition();

+    if (!usePullOperation())
+        checkPushSupported(); // will switch to pull if push not supported


This placement doesn't seem right. Better would be for this call to be inside usePullOperation().... or in calcUsePull as suggested below

it's too early. partitions are created later, but usePullOperation (and calcUsePull) are called early.

ghalliday · 2024-11-01T15:28:50Z

dali/ft/filecopy.cpp

+        return; // push ok
+
+    IWARNLOG("Forcing pull. Multiple source partitions write to same target, and target does not support concurrent write");
+    cachedUsePull = true;


cleaner to return a boolean and only update cachedUsePull inside usePullOperation()

usePullOperation() is called too early, before the partitions are created (by e.g. calculateOne2OnePartition, calculateSprayPartition etc)

ghalliday · 2024-11-01T15:30:42Z

dali/ft/filecopy.cpp

+        else
+        {
+            // may still not be allowed. After partitioning, if multiple sources write to same target, then pull is forced (see checkPushSupported())
+            return false;


can this call checkPushSupported() at this point?

jakesmith · 2024-11-04T09:47:20Z

@ghalliday - push new commit (not tested) to discuss later.

Basically approach is:

check if pull and push are supported (checkPushAndPullSupport()) upfront based on operation - before partitioning - reject user requests if incompatible.
partition using preferred method (containerized will use push if operation supported, BM decides [unchanged] based on 'calcOutput'). Check createFormatPartitioner changes.
After partitioning, calculates whether to use push or pull (calcPushPullOperation -> calcUsePull), including considering checkFoMultipleSourcesPerTarget()

jakesmith · 2024-11-04T17:54:37Z

system/jlib/jfile.cpp

+                prop += "@" + std::string(attrInfo.name);
+                switch (attrInfo.type)
+                {
+                    case PlaneAttrType::integer:


@ghalliday: NB: it wasn't okay that was commoned up handling boolean/integer here (before last commit), because it means "true" "false" will not be treated correctly if defined in the config (because only getPropBool will look at this literals).

jakesmith · 2024-11-04T17:55:34Z

system/jlib/jfile.cpp

+                        {
+                            unsigned __int64 v = plane.getPropInt64("expert/@fileSyncMaxRetrySecs", unsetPlaneAttrValue);
+                            // NB: fileSyncMaxRetrySecs==0 is treated as set/enabled
+                            if (unsetPlaneAttrValue != v)


@ghalliday - as you pointed out before I think, this was wrong before this final commit. As it was, it caused the value to be 0 if not set (which is the default anyway, but..)

jakesmith

@ghalliday - please review. I've kept the plane config code in.
I could remove it, and hard-code getConcurrentWriteSupported to return default, and move the plane attr stuff to a new PR targeting master...

ghalliday

@jakesmith one minor comment, otherwise approved. Please squash.

ghalliday · 2024-11-05T17:28:19Z

dali/ft/filecopy.cpp

+    if (isContainerized() && noCommon)
+    {
+        IWARNLOG("Ignoring noCommon option in containerized mode");
+        noCommon = false;


has no effect (variable not used after this point) - coverity may complain.

Maybe restructure as:

if (noCommon) { if (!isContainerized()) return; IWARNLOG("Ignoring noCommon option in containerized mode"); }

This does mean small sprays on bare-metal may use lots of processes, but that will be dealt with in a later PR.

Will change noCommon test.

This does mean small sprays on bare-metal may use lots of processes, but that will be dealt with in a later PR.

yes, no change there, but should be changed when larger refactoring is done.

…ying Signed-off-by: Jake Smith <[email protected]>

jakesmith · 2024-11-05T18:58:38Z

@ghalliday - squashed.

github-actions · 2024-11-11T16:31:40Z

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

jakesmith force-pushed the HPCC-32873-spray-avoid-concurrent-write branch from ccaf0ef to fe6eb67 Compare October 29, 2024 16:43

jakesmith marked this pull request as ready for review October 29, 2024 16:45

jakesmith requested a review from ghalliday October 29, 2024 16:45

jakesmith force-pushed the HPCC-32873-spray-avoid-concurrent-write branch from fe6eb67 to c4739fc Compare October 29, 2024 18:34

ghalliday reviewed Oct 31, 2024

View reviewed changes

jakesmith requested a review from ghalliday November 1, 2024 00:31

ghalliday reviewed Nov 1, 2024

View reviewed changes

jakesmith requested a review from ghalliday November 1, 2024 13:30

ghalliday reviewed Nov 1, 2024

View reviewed changes

jakesmith marked this pull request as draft November 4, 2024 09:39

jakesmith force-pushed the HPCC-32873-spray-avoid-concurrent-write branch from 1ad9731 to f0af494 Compare November 4, 2024 17:52

jakesmith commented Nov 4, 2024

View reviewed changes

jakesmith force-pushed the HPCC-32873-spray-avoid-concurrent-write branch from f0af494 to 6a3a023 Compare November 4, 2024 17:59

jakesmith commented Nov 4, 2024

View reviewed changes

jakesmith requested a review from ghalliday November 4, 2024 18:01

jakesmith marked this pull request as ready for review November 4, 2024 20:43

ghalliday approved these changes Nov 5, 2024

View reviewed changes

HPCC-32873 Prevent concurrent write to same file when spraying/despra…

dc4b77e

…ying Signed-off-by: Jake Smith <[email protected]>

jakesmith force-pushed the HPCC-32873-spray-avoid-concurrent-write branch from d238085 to dc4b77e Compare November 5, 2024 18:57

ghalliday merged commit 230ce80 into hpcc-systems:candidate-9.8.x Nov 11, 2024
49 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-32873 Prevent concurrent write to same file when spraying/despraying #19240

HPCC-32873 Prevent concurrent write to same file when spraying/despraying #19240

jakesmith commented Oct 25, 2024 •

edited

Loading

github-actions bot commented Oct 25, 2024

ghalliday Oct 31, 2024

jakesmith commented Nov 1, 2024

ghalliday left a comment

ghalliday Nov 1, 2024

ghalliday Nov 1, 2024

jakesmith Nov 1, 2024

ghalliday Nov 1, 2024

ghalliday Nov 1, 2024

jakesmith Nov 1, 2024

ghalliday Nov 1, 2024

jakesmith Nov 1, 2024

ghalliday Nov 1, 2024

jakesmith Nov 1, 2024

ghalliday Nov 1, 2024

jakesmith Nov 1, 2024

ghalliday Nov 1, 2024

jakesmith Nov 1, 2024

ghalliday left a comment

ghalliday Nov 1, 2024

ghalliday Nov 1, 2024

jakesmith Nov 1, 2024

ghalliday Nov 1, 2024

jakesmith Nov 1, 2024

ghalliday Nov 1, 2024

jakesmith commented Nov 4, 2024

jakesmith Nov 4, 2024

jakesmith Nov 4, 2024

jakesmith left a comment

ghalliday left a comment

ghalliday Nov 5, 2024

jakesmith Nov 5, 2024

jakesmith commented Nov 5, 2024

github-actions bot commented Nov 11, 2024

HPCC-32873 Prevent concurrent write to same file when spraying/despraying #19240

HPCC-32873 Prevent concurrent write to same file when spraying/despraying #19240

Conversation

jakesmith commented Oct 25, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Oct 25, 2024

Choose a reason for hiding this comment

jakesmith commented Nov 1, 2024

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith commented Nov 5, 2024

github-actions bot commented Nov 11, 2024

jakesmith commented Oct 25, 2024 •

edited

Loading