HPCC-30993 Fix file access for index read activity #18144

shamser · 2023-12-14T12:41:11Z

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:

shamser · 2023-12-14T12:42:18Z

thorlcr/activities/indexread/thindexreadslave.cpp

@@ -98,7 +98,7 @@ class CIndexReadSlaveBase : public CSlaveActivity
            if (!keyManager)
                throw MakeActivityException(&activity, 0, "Callback attempting to read blob with no key manager - index being read remotely?");
            needsBlobCleaning = true;
-            return (byte *) keyManager->loadBlob(id, dummy, &activity.contextLogger);
+            return (byte *) keyManager->loadBlob(id, dummy, nullptr);


Blob stats will be address in this ticket: https://track.hpccsystems.com/browse/HPCC-31002

shamser · 2023-12-14T12:42:35Z

thorlcr/activities/indexread/thindexreadslave.cpp

            {
-                IPartDescriptor &part = partDescs.item(p++);
+                IPartDescriptor &part = partDescs.item(p);


The changes in lines 169-171 is so that the correct contextLogger can be referenced with [p]. If p++ remains here then contextLoggers[p] cannot be used in line 306.

shamser · 2023-12-14T12:43:08Z

thorlcr/activities/indexread/thindexreadslave.cpp

@@ -273,7 +272,7 @@ class CIndexReadSlaveBase : public CSlaveActivity
                                    continue; // try next copy and ultimately failover to local when no more copies
                                }
                                ActPrintLog("[part=%d]: reading remote dafilesrv index '%s' (logical file = %s)", partNum, path.str(), logicalFilename.get());
-                                partNum = p;
+                                partNum = (p+1);


Change relates to: 55ef2b9#r1426670630

sensible change. Not new, but might be good to add a comment to clarify this is returning next part #, e.g.:

partNum = p+1; // NB: returning next part number to process.

shamser · 2023-12-14T12:43:46Z

thorlcr/activities/indexread/thindexreadslave.cpp

                    keyManager = klManager;
-                    partNum = p;
+                    partNum = (p+1);


Change relates to: 55ef2b9#r1426670630

shamser · 2023-12-14T12:44:19Z

thorlcr/activities/indexread/thindexreadslave.cpp

                    return createIndexLookup(keyManager);
                }
            }
-            keyMergerManager.setown(createKeyMerger(helper->queryDiskRecordSize()->queryRecordAccessor(true), keyIndexSet, seekGEOffset, &contextLogger, helper->hasNewSegmentMonitors(), false));
+            //Not tracking jhtree cache stats in KeyMerger at the moment.  Future: something to consider


Jira has been created to address KeyMerger stats: https://track.hpccsystems.com/browse/HPCC-30999

shamser · 2023-12-14T12:44:40Z

thorlcr/activities/indexread/thindexreadslave.cpp

-            updateStats();
-            lazyIFileIO.clear();
-        }
+        lazyIFileIO.clear();


The stats gathering has been moved to gatherActiveStats so that the stats are updated periodically whilst the activity is running.

jakesmith

@shamser - there are some merge clashes, looks like it needs rebasing.

Could you also update commit and PR message and JIRA description with some comments describing what the issue this is fixing and how. Thanks.

jakesmith

@shamser - there are some merge clashes, looks like it needs rebasing.

Could you also update commit and PR message and JIRA description with some comments describing what the issue this is fixing and how. Thanks.

@shamser - also, the PR is failing (crashing by the looks of it) some regression tests.

jakesmith

Could you also update commit and PR message and JIRA description with some comments describing what the issue this is fixing and how. Thanks.

am not seeing an updated commit + PR message, and JIRA description ?

shamser · 2023-12-22T15:54:54Z

Serialize jhtree and file io stats to thor manager.
Record jhtree and file io stats for Index Read Activity (to activity scope)

(Correct cost calculation for index reads require jhtree stats, so with
this change the file access cost for index read should be correct.)

Note: this change is dependent on HPCC-31035 Move ctx stats merging to caller from CKeyLevelManager mergeStats

jakesmith

@shamser - looks good, some minor comment/questions.

jakesmith · 2024-01-10T10:23:37Z

thorlcr/activities/indexread/thindexreadslave.cpp

@@ -719,10 +710,66 @@ class CIndexReadSlaveBase : public CSlaveActivity
    virtual void gatherActiveStats(CRuntimeStatisticCollection &activeStats) const
    {
        PARENT::gatherActiveStats(activeStats);
+        // *) jhtree stats will have been tracked as follows
+        //    - For superfiles, there will be 1 IContextLogger in contextLoggers[] for each subfile


lines 671-673 is creating 1 IContextLogger per physical part not per subfile,
which gatherActiveStats is depending on as it iterates through parts and does:

fileStatItem->merge(contextLoggers[partNum]->queryStats());

You could create only 1 IContextLogger per subfile, and then use mapSubPart to map the part# -> subfile# to reference contextLoggers[subfile]

Not sure worth it, but comment should be changed if not.

Could there be a very large number of parts? If that is the case, do you think the performance would be impacted enough to make the change worthwhile?

I'd like leave as it is unless it is really worthwhile. Although change looks trivial, it will require significant time consuming re-testing.

could be quite large, e.g. a superfile of 50+ files of 400x files, but each workers only gets a slice of those, and these CStatsContextLogger objects aren't that big.
I would merge as is, and perhaps open a low-prio. JIRA to revisit it on a rainy day.

I've created a jira to revisit this: https://track.hpccsystems.com/browse/HPCC-31103

jakesmith · 2024-01-10T10:30:52Z

thorlcr/activities/indexread/thindexreadslave.cpp

@@ -273,7 +272,7 @@ class CIndexReadSlaveBase : public CSlaveActivity
                                    continue; // try next copy and ultimately failover to local when no more copies
                                }
                                ActPrintLog("[part=%d]: reading remote dafilesrv index '%s' (logical file = %s)", partNum, path.str(), logicalFilename.get());
-                                partNum = p;
+                                partNum = (p+1);


sensible change. Not new, but might be good to add a comment to clarify this is returning next part #, e.g.:

partNum = p+1; // NB: returning next part number to process.

jakesmith · 2024-01-10T11:24:29Z

thorlcr/activities/indexread/thindexreadslave.cpp

+                IKeyManager * keyManager;
+                {
+                    CriticalBlock b(keyManagersCS);
+                    if (!keyManagers.isItem(partNum))


should there be a test (earlier) to exclude (and later handle differently) the merger case?
The test here may mean it passed for part 1, and bails out for part 2+ in the merge case, which will only have 1 IKeyManager in keyManagers.

Would merging stats even when merger is used be a problem? The merger was going to handled separately with https://track.hpccsystems.com/browse/HPCC-30999

I don't think it will cause exceptions, and it looks like stats will be correct in the non-super case.
But not clear to the reader what will happen in localMerge=true case at the moment.
I think worth a comment block that explains current caveat and that it needs refactoring, in if (localMerge) case.

Comment added line 728-730.

thorlcr/activities/indexread/thindexreadslave.cpp

shamser · 2024-01-10T13:33:55Z

@jakesmith I've push changes with comment changes. I may need to make further changes in response to my clarification request on your comments.

jakesmith

@shamser - looks good, if you could add a comment in gatherActiveStats re. localMerge caveat, and then go ahead and squash.

jakesmith

@shamser - looks good, please squash

jakesmith

@shamser - re-tagged myself for review (although approved), so that #18162 can be merged 1st, then this rebased.
Please tag me when that is merged and this PR is rebased.

@shamser

@shamser - tagging myself for review (although approved), so that #18162 can be merged 1st, then this rebased. Please tag me when that is merged and this PR is rebased.

Serialize jhtree and file io stats to thor manager. Record jhtree and file io stats for Index Read Activity (to activity scope) (Correct cost calculation for index reads require jhtree stats, so with this change the file access cost for index read should be correct.) Signed-off-by: Shamser Ahmed <[email protected]>

shamser · 2024-01-16T15:43:44Z

@jakesmith

jakesmith

@shamser - looks good.

ghalliday

@shamser the changes look good - I didn't follow all the details.
A couple of questions which would help clarify the code, but happy to merge as-is.

ghalliday · 2024-01-26T10:06:17Z

thorlcr/activities/indexread/thindexreadslave.cpp

+                {
+                    CriticalBlock b(keyManagersCS);
+                    if (!keyManagers.isItem(partNum))
+                        continue;


@shamser could this break rather than continuing? partNum will only get larger, and the test above will always fail. (If something else is adding them at the same time I guess it could succeed, but will be blank.)

There is a critical section to protect against changes to KeyManagers.

ghalliday · 2024-01-26T10:11:04Z

thorlcr/activities/indexread/thindexreadslave.cpp

+        //    - the io stats will be tracked in keyManagers[]
+        //    - (There will be exactly 1 keyManager per part)
+        // Note: the KeyManagers[], contextLoggers[] and fileStats[] are not necessarilly the same size
+        // - KeyManagers[] will always be the same size as partDescs[]


This comment doesn't seem to be quite true, since line 742 is testing it is true. Worth adding a comment where when it might not be true.

shamser commented Dec 14, 2023

View reviewed changes

shamser requested a review from jakesmith December 14, 2023 12:47

shamser marked this pull request as ready for review December 14, 2023 12:47

jakesmith reviewed Dec 20, 2023

View reviewed changes

shamser force-pushed the issue30993 branch 4 times, most recently from 961044a to 650c837 Compare December 21, 2023 14:35

shamser requested a review from jakesmith December 21, 2023 14:36

jakesmith reviewed Dec 22, 2023

View reviewed changes

shamser force-pushed the issue30993 branch from 650c837 to 21dcbb7 Compare December 22, 2023 11:38

shamser requested a review from jakesmith December 22, 2023 14:30

jakesmith reviewed Dec 22, 2023

View reviewed changes

shamser force-pushed the issue30993 branch from 21dcbb7 to 2f46590 Compare December 22, 2023 15:54

shamser requested a review from jakesmith December 22, 2023 15:55

jakesmith reviewed Jan 10, 2024

View reviewed changes

shamser requested a review from jakesmith January 10, 2024 13:32

shamser force-pushed the issue30993 branch from f1f6e93 to d0c699a Compare January 10, 2024 14:29

jakesmith reviewed Jan 11, 2024

View reviewed changes

shamser force-pushed the issue30993 branch from d0c699a to 41f6d05 Compare January 11, 2024 14:04

shamser requested a review from jakesmith January 11, 2024 14:06

jakesmith previously approved these changes Jan 12, 2024

View reviewed changes

jakesmith mentioned this pull request Jan 12, 2024

HPCC-31035 Move ctx stats merging to caller from CKeyLevelManager mergeStats #18162

Merged

39 tasks

jakesmith self-requested a review January 12, 2024 11:12

jakesmith reviewed Jan 12, 2024

View reviewed changes

shamser force-pushed the issue30993 branch from 41f6d05 to c167963 Compare January 16, 2024 11:52

shamser requested a review from jakesmith January 16, 2024 11:54

jakesmith approved these changes Jan 17, 2024

View reviewed changes

ghalliday approved these changes Jan 26, 2024

View reviewed changes

ghalliday merged commit 9391425 into hpcc-systems:candidate-9.4.x Feb 2, 2024
36 of 38 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-30993 Fix file access for index read activity #18144

HPCC-30993 Fix file access for index read activity #18144

shamser commented Dec 14, 2023 •

edited

Loading

shamser Dec 14, 2023

shamser Dec 14, 2023

shamser Dec 14, 2023 •

edited

Loading

jakesmith Jan 10, 2024

shamser Dec 14, 2023

shamser Dec 14, 2023

shamser Dec 14, 2023

jakesmith left a comment

jakesmith left a comment

jakesmith left a comment

shamser commented Dec 22, 2023 •

edited

Loading

jakesmith left a comment

jakesmith Jan 10, 2024

shamser Jan 10, 2024

jakesmith Jan 11, 2024

shamser Jan 11, 2024

jakesmith Jan 10, 2024

jakesmith Jan 10, 2024

shamser Jan 10, 2024 •

edited

Loading

jakesmith Jan 11, 2024

shamser Jan 11, 2024

shamser commented Jan 10, 2024

jakesmith left a comment

jakesmith left a comment

jakesmith left a comment

shamser commented Jan 16, 2024

jakesmith left a comment

ghalliday left a comment

ghalliday Jan 26, 2024

shamser Jan 26, 2024

ghalliday Jan 26, 2024

HPCC-30993 Fix file access for index read activity #18144

HPCC-30993 Fix file access for index read activity #18144

Conversation

shamser commented Dec 14, 2023 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamser Dec 14, 2023 • edited Loading

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

jakesmith left a comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

shamser commented Dec 22, 2023 • edited Loading

jakesmith 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

shamser Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamser commented Jan 10, 2024

jakesmith left a comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

shamser commented Jan 16, 2024

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

Choose a reason for hiding this comment

shamser commented Dec 14, 2023 •

edited

Loading

shamser Dec 14, 2023 •

edited

Loading

shamser commented Dec 22, 2023 •

edited

Loading

shamser Jan 10, 2024 •

edited

Loading