HPCC-31062 Include dfu file op cost in workunit aggregates #18358

shamser · 2024-02-29T14:18:37Z

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

@shamser - back to you for now as discussed offline.

jakesmith · 2024-03-05T16:17:24Z

common/workunit/workunit.cpp

+            if (sst!=SSTnone)
+                setStatistic(creatorType, creator, sst, currentScope.str(), StCostFileAccess, "", value, 1, 0, StatsMergeSum);
+        }
+        setStatistic(creatorType, creator, SSTglobal, "", StCostFileAccess, "", value, 1, 0, StatsMergeSum);


Discussed offline, this would cause problems if called >once for same scope. This will happen if the duration if more than a few seconds, these mergesum figures will be wrong.

jakesmith · 2024-04-02T10:08:49Z

esp/scm/ws_fs.ecm

@@ -734,7 +734,7 @@ ESPservice [
    ESPmethod [resp_xsl_default("/esp/xslt/showresult.xslt")] ShowResult(ShowResultRequest, ShowResultResponse);
    ESPmethod [resp_xsl_default("/esp/xslt/dfuwu_search.xslt")] DFUWUSearch(DFUWUSearchRequest, DFUWUSearchResponse);
    ESPmethod [resp_xsl_default("/esp/xslt/dfu_workunits.xslt")] GetDFUWorkunits(GetDFUWorkunits, GetDFUWorkunitsResponse);
-    ESPmethod [resp_xsl_default("/esp/xslt/dfu_wuid.xslt")] GetDFUWorkunit(GetDFUWorkunit, GetDFUWorkunitResponse);
+    ESPmethod [min_ver("1.26"), resp_xsl_default("/esp/xslt/dfu_wuid.xslt")] GetDFUWorkunit(GetDFUWorkunit, GetDFUWorkunitResponse);


I'm not sure what happens, but I would think this will cause all clients that are <1.26 to fail if they call this method.

This was added because 'FileAccessCost' wasn't being returned (it's a field with min_ver=1.23).

I am not sure why the service definition doesn't have a 'default_client_version', nor am I sure what the effect should be when client code compiled with a current version (1.26) hits a server (1.26), i.e. why doesn't it treat it as a 1.26 client in the absence of 'default_client_version'.
I can understand why an arbitrary SOAP call that doesn't specify a version uses 'default_client_version', but not in the case of code based on generated ecm stubs.
Let's check/discuss with @afishbeck further.

Once #18538 is merged, this change (line 737 will not be needed)

jakesmith · 2024-04-02T10:10:39Z

common/workunit/workunit.cpp

        iter->playProperties(aggregatesLoader);
+        if (iter->getScopeType()==SSToperation && !iter->nextSibling()) // don't descend operation.  only top operation scope needed


although I agree shouldn't be necessary to recurse down. I think in general, it would be better not to special case unless necessary. In offline chat you highlighted that there's a secondary reason related to a bug.
I'd leave this code as it is, but reference the reasons and the JIRA until resolved, then this code can be revisited.

I have addressed the bug with: #18547. Once the fix is merged, the changes made in line 2697 will not be needed.

shamser · 2024-04-17T12:00:49Z

The following PRs must be merged before this one: #18538 and #18547 .

jakesmith · 2024-04-26T13:56:51Z

@shamser - can you remind me why this isn't/can't be implemented via the StatisticsAggregator mechanism?

jakesmith

@shamser see question.

shamser · 2024-04-29T09:29:45Z

@shamser - can you remind me why this isn't/can't be implemented via the StatisticsAggregator mechanism?

@jakesmith
SSTdfuworkunit statistic is updated by fileservices plugin using CLocalWorkunit::setStatistic.
To update the aggregates immediately within setStatistic, it would need to create an StatisticsAggregator object, load all of the existing statistics from global stats, mark the SSTdfuworkunit scope as "dirty", and then execute refreshAggregates to update the totals. The CStatisticCollection would need to be extended to support marking a scope as dirty. (This seems messy, slow and complicated compared to what is being done in this PR).

However, if this is not done with the expectation is that the statistic will be handled by future StatisticsAggregator, it won't be. The reason being is that there is nothing marking the SSTdfuworkunit scope as dirty and if the scope is not dirty then it wouldn't be used to calculate the aggregate.

jakesmith · 2024-04-29T11:57:58Z

To update the aggregates immediately within setStatistic, it would need to create an StatisticsAggregator object, load all of the existing statistics from global stats

That is already happening though(?), but fileservices doesn't have access to the statsAggregator.
i.e. fileservices is being called from a running workunit, which is loading all stats/calling StatisticsAggregator::updateAggregates periodically.

I think this approach is ok/pragmatic, but ideally, all stat settings would go through the aggregator that's active for the current workunit - with a similar approach to StatisticsAggregator::recordStats, which would mark dirty for a dfuscope (or arbitrary scope), and updates (to the workunit) would be a separate period task.

It would be good to add some notes into the code, to clarify why fileservices stats are not aggregated via the current workunit's StatisticsAggregator and what would have to change for that to be so, to make it clearer if this is revisited.

jakesmith · 2024-04-29T16:17:43Z

common/workunit/workunit.cpp

+    // This would be slow and unnessarily cumbersome.) Note also that if the stat is written
+    // with the expectation that the hThor/Thor stats aggregator would generate the aggregate for
+    // this stat, it would not be the case as the aggregates are only created for stats marked
+    // 'dirty'.


The comment update wasn't quite what I had in mind.
I've opened a new JIRA to potentially reviist this: https://hpccsystems.atlassian.net/browse/HPCC-31703

And say the StatsAggregator were created here, it would need to load all the global stats from dali

Each filespray service call is called from a context that already has a CStatsAggregator, and has already loaded the existing stats, and has the ability to mark target scopes dirty.
However, fileservices doesn't have access to the in flight 'statsAggregator' (perhaps could have via ICodeContext).

And, CStatsAggregator would need an alternate 'recordStats' mechanism that took an arbitrary scope, and possible a CRuntimeStatisitcsCollections for the specific stats being updated.

The advantage would be that it would share the same rollup aggregator mechanism as used elsewhere, and it would allow the stat. updates vs the aggregation and publication to be done at different times (e.g. is spraying lots of small files, the updates to Dali constantly could become expensive.

jakesmith

@shamser - approving, but instead of squashing, I think it would be better to drop the 2nd commit (comment update), see my comments.
And this can then revisited at some later date (https://hpccsystems.atlassian.net/browse/HPCC-31703)

jakesmith

@shamser - looks good.

ghalliday

@shamser I think ok to merge, but have a couple of questions.

ghalliday · 2024-05-07T14:10:14Z

common/workunit/workunit.cpp

    const unsigned numStats = mapping.numStatistics();
    for (unsigned i=0; i<numStats; ++i)
        filter.addOutputStatistic(mapping.getKind(i));
-    filter.setDepth(1,3); // 1=global, 2=workflow, 3=graph
+    filter.setDepth(0,3); // 0=global, 1=workflow, (2=graph, 3=subgraph | 2=>dfu)


I think you probably just want to delete this line. As long as you have the next line it will efficiently walk the existing global stats.

By "global", I mean the top-level scopes (e.g. parent of the workflow scopes). To iterate through the top level scopes, the depth needs to start at 0. Here is information from logging noteStatistic to record the scope names (depth 0,3):

noteStatistic Statistic CostExecute (value 415263369)
noteStatistic Statistic CostFileAccess (value 12554)
noteStatistic w1 Statistic CostExecute (value 415263369)
noteStatistic w1 Statistic CostFileAccess (value 12554)
noteStatistic w1:graph1 Statistic CostExecute (value 386667103)
noteStatistic w1:graph2 Statistic CostExecute (value 266666968)
noteStatistic w1:graph3 Statistic CostExecute (value 4056896594)
noteStatistic w1:graph3 Statistic CostFileAccess (value 323)
noteStatistic w1:graph4 Statistic CostFileAccess (value 525)
noteStatistic w1:graph4 Statistic CostExecute (value 3493194855)
noteStatistic w1:>dfu Statistic CostFileAccess (value 11706)
noteStatistic w1:>dfu:>copy Statistic CostFileAccess (value 11706)

If the depth is set to (1,3), the iterator skips the top-level and starts one level down (e.g. at the workflow scope level). Here's the results of the same workunit but with the depth set to (1,3):

noteStatistic w1 Statistic CostExecute (value 941930630)
noteStatistic w1 Statistic CostFileAccess (value 12554)
noteStatistic w1:graph1 Statistic CostExecute (value 384444878)
noteStatistic w1:graph2 Statistic CostExecute (value 246666945)
noteStatistic w1:graph3 Statistic CostExecute (value 310818807)
noteStatistic w1:graph3 Statistic CostFileAccess (value 323)
noteStatistic w1:graph4 Statistic CostFileAccess (value 525)
noteStatistic w1:graph4 Statistic CostExecute (value 1056007436)
noteStatistic w1:>dfu Statistic CostFileAccess (value 11706)
noteStatistic w1:>dfu:>copy Statistic CostFileAccess (value 11706)

I changed to 0,3 because I was seeing that the top-level totals wasn't being updated. With this change, they are.
@ghalliday

Yes, I think you need to change from 1,3 to 0,3. I think you could equally delete the filter, but I think probably no harm in leaving it here either.
The intent of the filters was that the sources etc. would be automatically deduced from the filter conditions. That logic needs updating for it to be true - and in this case the source has been explicitly selected.

ghalliday · 2024-05-07T14:12:34Z

common/workunit/workunit.cpp

@@ -8941,7 +8940,7 @@ void CLocalWorkUnit::setStatistic(StatisticCreatorType creatorType, const char *
    if (mergeAction != StatsMergeAppend)
    {
        StringBuffer xpath;
-        xpath.append("Statistic[@creator='").append(creator).append("'][@scope='").append(scope).append("'][@kind='").append(kindName).append("']");
+        xpath.append("Statistic[@scope='").append(scope).append("'][@kind='").append(kindName).append("']");


This change looks correct, but what was the reason for it? The jira or the PR should include some indication of why.

The StCostFileAccess from dfu scopes are included in the aggregation of the StCostFileAccess totals. Note that an aggregate may be written by one component and then later updated by another component. CLocalWorkUnit::setStatistic has been modified to ignore the creator when looking up the existing value for a statistic. Signed-off-by: Shamser Ahmed <[email protected]> # dockerfiles/t4.yaml

shamser · 2024-05-08T09:52:19Z

The commit message has been updated to explain the change to setStatistic.

I believe the change to the filter depth is necessary. Please see comment: #18358 (comment).

@ghalliday

shamser changed the title ~~Issue31062~~ HPCC-31062 Generates aggregates for dfu stats in workunits Feb 29, 2024

shamser marked this pull request as ready for review March 1, 2024 12:30

shamser requested a review from jakesmith March 1, 2024 12:31

jakesmith reviewed Mar 5, 2024

View reviewed changes

shamser force-pushed the issue31062 branch from 8cd1291 to 6d9edde Compare March 27, 2024 16:27

shamser changed the base branch from master to candidate-9.6.x March 27, 2024 16:29

shamser marked this pull request as draft March 27, 2024 16:30

shamser changed the title ~~HPCC-31062 Generates aggregates for dfu stats in workunits~~ HPCC-31062 Include dfu file op cost in workunit aggregates Mar 27, 2024

shamser force-pushed the issue31062 branch from 6d9edde to f67ec42 Compare March 27, 2024 16:47

shamser marked this pull request as ready for review March 27, 2024 16:50

shamser requested a review from jakesmith March 27, 2024 16:50

shamser force-pushed the issue31062 branch from f67ec42 to 11cbe77 Compare March 28, 2024 09:19

jakesmith reviewed Apr 2, 2024

View reviewed changes

shamser force-pushed the issue31062 branch from 11cbe77 to 65466a2 Compare April 17, 2024 11:59

shamser force-pushed the issue31062 branch from 65466a2 to ffd6ee1 Compare April 26, 2024 11:39

shamser requested a review from jakesmith April 26, 2024 12:03

jakesmith reviewed Apr 26, 2024

View reviewed changes

shamser requested a review from jakesmith April 29, 2024 09:30

jakesmith reviewed Apr 29, 2024

View reviewed changes

jakesmith approved these changes Apr 29, 2024

View reviewed changes

shamser force-pushed the issue31062 branch from f3c0d77 to ffd6ee1 Compare April 30, 2024 08:33

shamser requested a review from jakesmith April 30, 2024 08:33

jakesmith approved these changes Apr 30, 2024

View reviewed changes

ghalliday approved these changes May 7, 2024

View reviewed changes

shamser force-pushed the issue31062 branch from ffd6ee1 to bc9998d Compare May 8, 2024 09:49

shamser requested a review from ghalliday May 8, 2024 09:49

ghalliday approved these changes May 9, 2024

View reviewed changes

ghalliday merged commit 540d967 into hpcc-systems:candidate-9.6.x May 9, 2024
50 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-31062 Include dfu file op cost in workunit aggregates #18358

HPCC-31062 Include dfu file op cost in workunit aggregates #18358

shamser commented Feb 29, 2024 •

edited

Loading

jakesmith left a comment

jakesmith Mar 5, 2024

jakesmith Apr 2, 2024

jakesmith Apr 2, 2024

shamser Apr 16, 2024

jakesmith Apr 2, 2024

shamser Apr 17, 2024 •

edited

Loading

shamser commented Apr 17, 2024 •

edited

Loading

jakesmith commented Apr 26, 2024

jakesmith left a comment

shamser commented Apr 29, 2024 •

edited

Loading

jakesmith commented Apr 29, 2024

jakesmith Apr 29, 2024

jakesmith left a comment

jakesmith left a comment

ghalliday left a comment

ghalliday May 7, 2024

shamser May 8, 2024 •

edited

Loading

ghalliday May 9, 2024

ghalliday May 7, 2024

shamser commented May 8, 2024

		iter->playProperties(aggregatesLoader);
		if (iter->getScopeType()==SSToperation && !iter->nextSibling()) // don't descend operation. only top operation scope needed

HPCC-31062 Include dfu file op cost in workunit aggregates #18358

HPCC-31062 Include dfu file op cost in workunit aggregates #18358

Conversation

shamser commented Feb 29, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

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

shamser Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

shamser commented Apr 17, 2024 • edited Loading

jakesmith commented Apr 26, 2024

jakesmith left a comment

Choose a reason for hiding this comment

shamser commented Apr 29, 2024 • edited Loading

jakesmith commented Apr 29, 2024

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

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamser May 8, 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 May 8, 2024

shamser commented Feb 29, 2024 •

edited

Loading

shamser Apr 17, 2024 •

edited

Loading

shamser commented Apr 17, 2024 •

edited

Loading

shamser commented Apr 29, 2024 •

edited

Loading

shamser May 8, 2024 •

edited

Loading