HPCC-19337 Add streaming remote (dafilesrv) disk read support #10979

jakesmith · 2018-03-20T22:58:24Z

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:

Testing:

Run through hthor regression suite with forceRemotePattern on for all hthor files.

hpcc-jirabot · 2018-03-20T22:58:34Z

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

jakesmith · 2018-03-21T11:53:39Z

NB: ValueSetTest::testKeyed2, ValueSetTest::testFilter, ValueSetTest::testKeyed1 fail at the moment, e.g. testKeyed(rows, record, "f1=[5]") fails in deserializeFieldFilter.
I think this is related to changes Gavin made to the filter format (?) - @ghalliday - can you check/comment?

@mckellyln @ghalliday @richardkchapman - please review some/all of the changes.

richardkchapman · 2018-04-10T09:43:06Z

rtl/eclrtl/rtlnewkey.cpp

@@ -1632,6 +1649,16 @@ void RowFilter::addFilter(const IFieldFilter & filter)
        numFieldsRequired = fieldNum+1;
 }

+void RowFilter::addFilter(const RtlRecord & record, const char * filterText)
+{
+    //assertex(filter.queryField() == filters.ordinality()); //MORE - fill with wild filters and replace existing wild


Is this comment still accurate? Is there a plan to address it? A Jira? Should it be fixed in this PR?

I think the comment can be deleted - it was related to when they are used for keys, and that needs more work anyway.

ghalliday

A first review.
Main comments are to do with the remote activity implementation - particularly group handling.

ghalliday · 2018-04-10T10:07:24Z

common/remote/sockfile.cpp

-        if (cmd > RFCmaxnormal)
-            cmd = RFCmaxnormal;
+        unsigned elems = sizeof(RFCStrings) / sizeof(RFCStrings[0]);
+        if (cmd >= (sizeof(RFCStrings) / sizeof(RFCStrings[0])))


I think you meant to use elems

yes. I'll change.

ghalliday · 2018-04-10T10:22:45Z

common/remote/sockfile.cpp

+            cursorInfo.append("\"\n");
+            mrequest.append(cursorInfo.length(), cursorInfo.str());
+        }
+        DBGLOG("req = <%s}>", request.str());


Does this need to be less frequent - the logging may get excessive.

yes, I'll make it based on dafilesrv conditional tracing level.

ghalliday · 2018-04-10T10:36:27Z

common/remote/sockfile.cpp

+    mutable bool eogPending = false;
+    mutable bool someInGroup = false;
+    const RtlRecord *record = nullptr;
+    RtlDynRow *filterRow = nullptr;


I think this probably leaks.

yes, will add delete to dtor.

ghalliday · 2018-04-10T10:38:03Z

common/remote/sockfile.cpp

+                const byte *next = prefetchBuffer.queryRow();
+                bool eog = false;
+                if (inputGrouped)
+                    prefetchBuffer.read(sizeof(eog), &eog);


this could invalidate next. Need to call queryRow() after this line.

ok, will change.

ghalliday · 2018-04-10T10:38:12Z

common/remote/sockfile.cpp

+                bool eog = false;
+                if (inputGrouped)
+                    prefetchBuffer.read(sizeof(eog), &eog);
+                prefetchBuffer.finishedRow();


this could also invalidate next, needs to happen after the transform

ok, will change.

ghalliday · 2018-04-10T10:53:35Z

common/remote/sockfile.cpp

+        if (!filterRow)
+            filterRow = new RtlDynRow(*record);
+    }
+// IRemoteActivity impl.
    virtual const void *nextRow(size32_t &retSz) override


This should really take outBuilder as a parameter - otherwise it is very confusing that the buffer is updated as a side-effect of the call.

ghalliday · 2018-04-10T10:57:14Z

common/remote/sockfile.cpp

                for (unsigned __int64 i=0; i<defaultDaFSNumRecs; i++)
                {
+                    bool *eog;
+                    if (grouped && outputActivity->queryProcessed()) // i.e. not 1st row


I would expect this logic to be executed after reading enough rows (since returning null is the normal way of indicating end of group), rather than each row.

Using a local eog variable appended to the buffer at the correct point.

ghalliday · 2018-04-10T10:59:54Z

common/remote/sockfile.cpp

                    size32_t rowSz;
-                    const void *row = outputActivity->nextRow(rowSz);
+                    const void *row = outputActivity->nextRow(rowSz); // NB: row builder writes directly to reply buffer for effiency


This potentially invalidates eog pointer - so could corrupt memory.

(E.g., if a complex row is then filtered, followed by end of group).

ghalliday · 2018-04-10T11:02:24Z

common/remote/sockfile.cpp

-                        break;
+                        if (grouped)
+                        {
+                            if (outputActivity->queryProcessed()) // i.e. not 1st row


This seems a strange test to see if it is the first row - I would expect testing a local variable e.g., i != 0 or

ghalliday · 2018-04-10T11:04:31Z

common/remote/sockfile.cpp

    virtual void serializeCursor(MemoryBuffer &tgt) const override
    {
        tgt.append(prefetchBuffer.tell());
        tgt.append(processed);
+        tgt.append(someInGroup);
+        tgt.append(eogPending);


is it possible for eogPending to ever be true?

ghalliday · 2018-04-10T11:26:16Z

@jakesmith some initial comments

jakesmith · 2018-04-11T08:47:26Z

forgot to tag you @ghalliday

ghalliday

Looks very close, a few comments.

ghalliday · 2018-04-11T08:58:32Z

common/remote/sockfile.cpp

+    }
+    virtual unsigned __int64 getStatistic(StatisticKind kind)
+    {
+        UNIMPLEMENTED;


Will this cause problems? Should it be implemented in some way in a later PR?

should at least return 0, as other implementations do that don't return stats.

ghalliday · 2018-04-11T09:19:42Z

common/remote/sockfile.cpp

+                        responseWriter->outputBeginNested("Row", true);
+                        out->toXML((const byte *)row, *responseWriter);
+                        responseWriter->outputEndNested("Row");
+                        pastFirstRow = true;


Worth resetting outBuilder buffer, so it doesn't keep growing (and in the else below)?

oh yes, well spotted.

ghalliday · 2018-04-11T09:25:00Z

rtl/eclrtl/rtlnewkey.cpp

@@ -1632,6 +1649,16 @@ void RowFilter::addFilter(const IFieldFilter & filter)
        numFieldsRequired = fieldNum+1;
 }

+void RowFilter::addFilter(const RtlRecord & record, const char * filterText)
+{
+    //assertex(filter.queryField() == filters.ordinality()); //MORE - fill with wild filters and replace existing wild


I think the comment can be deleted - it was related to when they are used for keys, and that needs more work anyway.

ghalliday · 2018-04-11T09:26:16Z

common/remote/sockfile.cpp

        checkOpen();
-        if (needTransform)
+        while (!eofSeen && ((chooseN == 0) || (processed < chooseN)))


Are you sure the chooseN == 0 check is correct?

Well it was code copied from hthor and matches thor behaviour too.
But it turns out 0 should mean 0 and not all as it is currently implemented here, in hthor and thor.
The behaviour was changed some years ago, but it looks like hthor and thor were never changed, however they get away with it, because the codegen always generates a subsequent choosen activity.
I've opened a separate JIRA to change hthor and thor choosen==0 behaviour : https://track.hpccsystems.com/browse/HPCC-19440

ghalliday · 2018-04-11T09:36:02Z

ecl/hthor/hthor.cpp

-                    inputfile.setown(createIFile(rfilename));   
-                    if(compressed)
+                    inputfile.setown(createIFile(rfilename));
+                    if (rfilename.isLocal() && (!canSerialize || !readRemote(rfilename)))


Is this test correct?
Currently xml/csv needs to be read local - even if it is not a local filename
Similarly if you cannot serialize then it needs to be local.

Possibly:
if (!isBinary() || !canSerialize || (isLocal && !readRemote())

Agreed, also going to change readRemote() to forceRemote() for clarity.

ghalliday · 2018-04-11T09:37:40Z

ecl/hthor/hthor.cpp

-            calcFixedDiskRecordSize();
+                actualFilter.appendFilters(fieldFilters);
+
+            bool canSerialize = actualDiskMeta->queryTypeInfo()->canSerialize() && projectedDiskMeta->queryTypeInfo()->canSerialize();


minor: canSerializeTypeInfo would be a better indication of what couldn't be serialized.

will change.

ghalliday · 2018-04-11T09:40:03Z

@jakesmith

jakesmith · 2018-04-11T11:15:07Z

@ghalliday - please see changes following last review.

HPCCSmoketest · 2018-04-11T12:07:09Z

Automated Smoketest: ✅
OS: centos 7.2.1511 (Linux 3.10.0-327.28.3.el7.x86_64)
Sha: b46bda9
Build: success
Install hpccsystems-platform-community_6.5.0-trunk0.el7.x86_64.rpm
HPCC Start: OK

Unit tests result:

Test	total	passed
unittest	84	84
wutoolTest(Dali)	19	19
wutoolTest(Cassandra)	19	19

Regression test result:

phase	total	pass
setup (hthor)	11	11
setup (thor)	11	11
setup (roxie)	11	11
test (hthor)	759	759
test (thor)	686	686
test (roxie)	799	799

HPCC Stop: OK
HPCC Uninstall: OK
Time stats:

Prep time	Build time	Package time	Install time	Start time	Test time	Stop time	Summary
17 sec (00:00:17)	182 sec (00:03:02)	56 sec (00:00:56)	6 sec (00:00:06)	22 sec (00:00:22)	1105 sec (00:18:25)	16 sec (00:00:16)	1404 sec (00:23:24)

Implementation of a streaming remote project/filtering disk i/o class for use by engines, plus initial use by hthor. NB: This commit contains various fixes to rtl, hthor, dafilesrv discovered/required during implementation. It also adds: 1) remote compression support 2) remote filtering 3) remote grouped file support 4) Tidies up the dafilesrv exception handling and improves the error messages fed back to the client. Signed-off-by: Jake Smith <jake.smith@lexisnexisrisk.com>

jakesmith force-pushed the hpcc-19337 branch from 4b331df to 8f952b5 Compare March 21, 2018 11:44

jakesmith force-pushed the hpcc-19337 branch from 8f952b5 to 9e82bfb Compare April 10, 2018 09:21

richardkchapman reviewed Apr 10, 2018

View reviewed changes

ghalliday requested changes Apr 10, 2018

View reviewed changes

ghalliday reviewed Apr 11, 2018

View reviewed changes

jakesmith force-pushed the hpcc-19337 branch from b46bda9 to f40b4fc Compare April 11, 2018 16:11

ghalliday approved these changes Apr 11, 2018

View reviewed changes

ghalliday merged commit 5a00c4e into hpcc-systems:master Apr 11, 2018

HPCC-19337 Add streaming remote (dafilesrv) disk read support #10979

HPCC-19337 Add streaming remote (dafilesrv) disk read support #10979

Conversation

jakesmith commented Mar 20, 2018 • edited

Type of change:

Checklist:

Testing:

hpcc-jirabot commented Mar 20, 2018

jakesmith commented Mar 21, 2018

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

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 commented Apr 10, 2018

jakesmith commented Apr 11, 2018

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

ghalliday commented Apr 11, 2018

jakesmith commented Apr 11, 2018

HPCCSmoketest commented Apr 11, 2018

jakesmith commented Mar 20, 2018 •

edited