HPCC-27204 Support planes with multiple devices (striping) #15782

jakesmith · 2022-02-16T14:29:10Z

Signed-off-by: Jake Smith jake.smith@lexisnexisrisk.com

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:

jakesmith · 2022-02-16T14:29:41Z

@ghalliday - please review, leaving a draft for the moment, because I need to do some more testing.

github-actions · 2022-02-16T14:29:46Z

https://track.hpccsystems.com/browse/HPCC-27204
Jira updated

ghalliday

@jakesmith a few comments from an initial scan. Main comment is that a logical file/file descriptor should have a baseDevice field - generally set to a hash of the logical filename - which is added to the part number before modulus. (Then the hash code is localised to creating the file, rather than the readers.)

ghalliday · 2022-02-17T13:13:53Z

dali/base/dadfs.cpp

+                unsigned numStripedDevices = queryPartDiskMapping(cn).numStripedDevices;
+                unsigned stripeNum = 0;
+                if (numStripedDevices>1)
+                    stripeNum = (i%numStripedDevices)+1;


more: also hash of logical filename/base hash stored in the file descriptor?

I deliberately left this functionality off in this draft so far (but there is a comment somewhere), so prove the rest worked.

ghalliday · 2022-02-17T13:16:43Z

dali/base/dafdesc.cpp

+                mspec.flags &= ~CPDMSF_striped;
+#else
+            // Bare-metal can have multiple devices per plane (e.g. data + mirror), but it doesn't stripe across them
+            mspec.numStripedDevices = 1;


That is possibly a flaw. May want to revisit bare metal at some point.

agreed, it would break bare metal to consider what it thinks of multidevice planes now as striped, but we could have a switch where new plane types were flagged as striped.
As you say, for future.

ghalliday · 2022-02-17T13:20:35Z

dali/base/dafdesc.hpp

@@ -326,7 +328,8 @@ extern da_decl StringBuffer &makePhysicalPartName(
                                unsigned replicateLevel,            // uses replication directory
                                DFD_OS os,                          // os must be specified if no dir specified
                                const char *diroverride,            // override default directory
-                                bool dirPerPart);                   // generate a subdirectory per part
+                                bool dirPerPart,                    // generate a subdirectory per part
+                                unsigned stripeNum);                 // strip number


trivial: indent of comment and "strip"->"stripe"

will fix in next commit

AttilaVamos · 2022-02-22T15:33:39Z

@jakesmith The error with the failed spray_dir_test.ecl is:

<Exception><Code>1410</Code><Source>Roxie</Source><Message>Could not resolve filename regress::roxie::W20220222-125949-4::spray_test</Message></Exception>

ghalliday

@jakesmith looks good. a few relatively minor comments/questions

ghalliday · 2022-02-24T10:55:39Z

thorlcr/activities/indexwrite/thindexwriteslave.cpp

@@ -546,6 +546,7 @@ class IndexWriteSlaveActivity : public ProcessSlaveActivity, public ILookAheadSt
                        getPartFilename(*tlkDesc, l, path, true);
                        if (0 == l)
                        {
+                            ensureDirectoryForFile(path.str());


efficiency: I will open a separate PR, but in most cases it would be better to try and copy a file and only try and create the directory if that fails. Not an issue for local file systems, but more of an issue for remote.

ok, revisit this and other similar places in that new JIRA then I think.

ghalliday · 2022-02-24T11:00:29Z

common/thorhelper/roxiehelper.cpp

+    makePhysicalPartName(logicalName.get(), 0, 0, dir, 0, DFD_OSdefault, prefix, false, false);
+
+    StringBuffer fullPath;
+    makePhysicalPartName(logicalName.get(), 1, 1, fullPath, 0, DFD_OSdefault, prefix, false, 0);


Is passing 0 for stripe correct?

ghalliday · 2022-02-24T11:04:45Z

common/thorhelper/roxiehelper.cpp

@@ -2399,7 +2400,10 @@ void ClusterWriteHandler::getPhysicalName(StringBuffer & name, const char * clus
 {
    Owned<IStoragePlane> plane = getDataStoragePlane(cluster, false);
    const char * prefix = plane ? plane->queryPrefix() : nullptr;
-    makePhysicalPartName(logicalName.get(), 1, 1, name, 0, DFD_OSdefault, prefix, false);
+    unsigned stripeNum = 0;
+    if (plane)


if plane && numDevices > 1? or calcStripeNumber should return 0 if numPlanes = 1

yes, I'll add a check to calcStrripeNumber and simplify the calls

ghalliday · 2022-02-24T11:14:57Z

dali/base/dafdesc.cpp

+            lfnHash = attr->getPropInt("@lfnHash");
+        else if (tracename.length())
+        {
+            lfnHash = hashc((const unsigned char *)tracename.str(), tracename.length(), 0);


minor: More encapsulated if the function to calculate the hash was in a separately named function - so the actual hash function is isolated.

will change.

ghalliday · 2022-02-24T11:18:53Z

dali/base/dafdesc.cpp

@@ -2336,7 +2428,7 @@ IFileDescriptor *createFileDescriptor(const char *lname,IGroup *grp,IPropertyTre
        width = grp->ordinality();
    StringBuffer s;
    for (unsigned i=0;i<width;i++) {
-        makePhysicalPartName(lname, i+1, width, s.clear(), 0, os, nullptr, false);
+        makePhysicalPartName(lname, i+1, width, s.clear(), 0, os, nullptr, false, 0);


Is 0 for the stripe correct? Would be worth having a comment why. (added later?)

I don't think it is, but this version of createFileDescriptor is only called in one context in dfurun.cpp to do with keydiff.
However, a more sensible/normalized version should be called which would create an IFileDescriptor based on plane/striping.
I'll change and remove this defunct version.

ghalliday · 2022-02-24T11:21:10Z

dali/base/dautils.cpp

 #endif
+            StringBuffer descPath;
+            makePhysicalPartName(lfn.get(), 0, 0, descPath, 0, DFD_OSdefault, dir, false, false);


Last parameter should be a stripeNumber rather than a boolean. Is it worth having a special function for this since similar code occurs elsewhere (I assume it gets the base directory).

yes, I'll introduce a helper for clarify, but call common code.

ghalliday · 2022-02-24T11:23:27Z

dali/base/dautils.hpp

+extern da_decl void addStripeDirectory(StringBuffer &out, const char *directory, const char *planeName, unsigned partNum, unsigned lfnHash, unsigned numStripes);
+inline unsigned calcStripeNumber(unsigned partNum, unsigned lfnHash, unsigned numStripes)
+{
+    return ((partNum+lfnHash)%numStripes)+1;


suggestion: return 0 if numStripes <= 1? (to simplify calling code)

yep, will change to that.

jakesmith

@ghalliday - please review latest commit

ghalliday

Last changes look good and cleaned up the code a bit.

jakesmith · 2022-03-01T12:44:23Z

@richardkchapman - want me to squash?

richardkchapman · 2022-03-01T13:06:35Z

@jakesmith Yes please

Signed-off-by: Jake Smith <jake.smith@lexisnexisrisk.com>

jakesmith · 2022-03-01T13:11:28Z

@richardkchapman - squashed

HPCCSmoketest · 2022-03-01T13:50:42Z

Automated Smoketest: ✅
OS: centos 7.6.1810 (Linux 3.10.0-957.1.3.el7.x86_64)
Host: ip-10-20-0-50.ca-central-1.compute.internal
GCC: gcc (GCC) 7.3.1 20180303 (Red Hat 7.3.1-5)
Git: 2.9.5, CMake: 3.22.1, cUrl: 7.67.0, node.js: v16.13.1, npm: 8.1.2
Sha: 743e752
Containerized:False
Build: success
Milestone:Install hpccsystems-platform-community_8.6.7-closedown0.el7.x86_64.rpm
HPCC Start: OK

Unit tests result:

Test	total	passed	failed	errors	timeout	elaps
unittest	222	222	0	0	0	69 sec
wutoolTest(Dali)	19	19	0	0	0	1 sec

Regression test result:

phase	total	pass	elaps
setup (hthor)	9	9	22 sec (00:00:22)
setup (thor)	9	9	39 sec (00:00:39)
setup (roxie)	20	20	19 sec (00:00:19)
test (hthor)	984	984	895 sec (00:14:55)
test (thor)	894	894	1076 sec (00:17:56)
test (roxie)	1054	1054	903 sec (00:15:03)

HPCC Stop: OK
HPCC Uninstall: OK
Time stats:

Prep time	Build time	Package time	Install time	Start time	Test time	Stop time	Summary
18 sec (00:00:18)	450 sec (00:07:30)	111 sec (00:01:51)	21 sec (00:00:21)	16 sec (00:00:16)	1339 sec (00:22:19)	18 sec (00:00:18)	1973 sec (00:32:53)

jakesmith requested a review from ghalliday February 16, 2022 14:29

jakesmith force-pushed the HPCC-27204-numDevices-support branch 5 times, most recently from c2705da to ff6eabc Compare February 17, 2022 12:34

ghalliday reviewed Feb 17, 2022

View reviewed changes

jakesmith force-pushed the HPCC-27204-numDevices-support branch 2 times, most recently from 189ab8d to 6cc22dc Compare February 22, 2022 10:40

jakesmith marked this pull request as ready for review February 22, 2022 12:30

jakesmith force-pushed the HPCC-27204-numDevices-support branch from 6cc22dc to da24759 Compare February 23, 2022 09:40

richardkchapman self-requested a review February 23, 2022 16:12

ghalliday self-requested a review February 23, 2022 16:54

ghalliday reviewed Feb 24, 2022

View reviewed changes

jakesmith force-pushed the HPCC-27204-numDevices-support branch from 51baa32 to a7f944f Compare February 28, 2022 12:32

jakesmith requested a review from ghalliday February 28, 2022 12:39

jakesmith commented Feb 28, 2022

View reviewed changes

ghalliday approved these changes Mar 1, 2022

View reviewed changes

HPCC-27204 Support planes with multiple devices (striping)

743e752

Signed-off-by: Jake Smith <jake.smith@lexisnexisrisk.com>

jakesmith force-pushed the HPCC-27204-numDevices-support branch from a7f944f to 743e752 Compare March 1, 2022 13:11

richardkchapman approved these changes Mar 1, 2022

View reviewed changes

richardkchapman merged commit a01d88b into hpcc-systems:candidate-8.6.x Mar 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-27204 Support planes with multiple devices (striping) #15782

HPCC-27204 Support planes with multiple devices (striping) #15782

jakesmith commented Feb 16, 2022 •

edited

Loading

jakesmith commented Feb 16, 2022

github-actions bot commented Feb 16, 2022

ghalliday left a comment

ghalliday Feb 17, 2022

jakesmith Feb 17, 2022

ghalliday Feb 17, 2022

jakesmith Feb 17, 2022

ghalliday Feb 17, 2022

jakesmith Feb 17, 2022

AttilaVamos commented Feb 22, 2022

ghalliday left a comment

ghalliday Feb 24, 2022

jakesmith Feb 25, 2022

ghalliday Feb 24, 2022

ghalliday Feb 24, 2022

jakesmith Feb 25, 2022

ghalliday Feb 24, 2022

jakesmith Feb 25, 2022

ghalliday Feb 24, 2022

jakesmith Feb 25, 2022

ghalliday Feb 24, 2022

jakesmith Feb 25, 2022

ghalliday Feb 24, 2022

jakesmith Feb 25, 2022

jakesmith left a comment

ghalliday left a comment

jakesmith commented Mar 1, 2022

richardkchapman commented Mar 1, 2022

jakesmith commented Mar 1, 2022

HPCCSmoketest commented Mar 1, 2022

HPCC-27204 Support planes with multiple devices (striping) #15782

HPCC-27204 Support planes with multiple devices (striping) #15782

Conversation

jakesmith commented Feb 16, 2022 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

jakesmith commented Feb 16, 2022

github-actions bot commented Feb 16, 2022

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

AttilaVamos commented Feb 22, 2022

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

jakesmith left a comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

jakesmith commented Mar 1, 2022

richardkchapman commented Mar 1, 2022

jakesmith commented Mar 1, 2022

HPCCSmoketest commented Mar 1, 2022

jakesmith commented Feb 16, 2022 •

edited

Loading