HPCC-33568 Introduce a lingering c++ compile #19869

ghalliday · 2025-05-14T13:37:06Z

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:

ghalliday · 2025-05-14T13:37:40Z

@jakesmith code is written, compiled, but untested. Pushed for initial test.

Copilot

Pull Request Overview

This pull request implements changes to the HPCC C++ compiler infrastructure by refining containerized behavior and introducing a lingering compile server mode. Key changes include:

Adjusting job argument construction in jcontainerized.cpp to conditionally include workunit parameters.
Refactoring eclccserver.cpp to separate compilation logic into new classes (EclccCompiler, EclccLingeringCompiler) and improving behavior when running containerized compilations.
Adding a new macro and helper function in daqueue.hpp to support compile queue naming for the lingering server.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
system/jlib/jcontainerized.cpp	Updated job argument generation to conditionally include the workunit parameter.
ecl/eclccserver/eclccserver.cpp	Refactored compile thread logic with new compiler classes and adjusted K8s job flow.
dali/base/daqueue.hpp	Introduced a new compile queue extension macro and helper function.

Comments suppressed due to low confidence (1)

ecl/eclccserver/eclccserver.cpp:970

The call to runJob now passes a nullptr for the workunit identifier instead of the previous wuid. Ensure that this change is intentional and that the runJob function handles a null workunit correctly for lingering compile requests.

k8s::runJob("compile", nullptr, jobName, params);

Copilot · 2025-05-14T13:38:53Z

ecl/eclccserver/eclccserver.cpp

            //If the process is killed it is probably because it ran out of memory - so try to compile as a K8s job
-            timedOut = abortWaiter.stop() || (isContainerized() && processKilled);
+            bool timedOut = abortWaiter.stop() || (isContainerized() && processKilled);
            if (!timedOut)


[nitpick] The declaration of 'bool timedOut' within the compile method may be confusing since it replaces the previous out parameter usage. Consider updating related comments or variable names to clarify its intent.

Copilot · 2025-05-14T13:38:54Z

ecl/eclccserver/eclccserver.cpp

+
+            __uint64 executeTimeNs = waitTimer.elapsedNs() - waitTimeNs;
+            cost_type costExecute = money2cost_type(calcCostNs(costPerHour, executeTimeNs));
+            const char * username = "MORE";


[nitpick] The use of the hardcoded username 'MORE' for metrics recording might be a placeholder. Consider revisiting this value to use a dynamic or more descriptive identifier if applicable.

Suggested change

const char * username = "MORE";

const char * username = globals->queryProp("@username");

if (!username || !*username)

username = "unknown";

github-actions · 2025-05-14T13:43:37Z

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

Jirabot Action Result:
Workflow Transition To: Merge Pending
Updated PR

ecl/eclccserver/eclccserver.cpp

jakesmith

@ghalliday - looks good, didn't spot any problems, 1 minor question.

jakesmith · 2025-05-20T17:30:55Z

ecl/eclccserver/eclccserver.cpp

-                    compileViaK8sJob(true);
-                    return;
-                }
+                //NOTE: This call does not modify the workunit itself, so no need to commit afterwards


I'm not sure what this comment means.. the line below is altering the workunit and the workunit.clear() will commit won't it?

@ghalliday - still not sure re. this comment, is it misplaced? setContainerizedProcessInfo does alter the workunit..

ghalliday · 2025-05-22T15:49:56Z

I may have accidentally merged some of the follow up changes into the second commit.

github-actions · 2025-05-22T15:51:05Z

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

Jirabot Action Result:
Workflow Transition To: Merge Pending

jakesmith

@ghalliday - looks good, some v. minor comments

jakesmith · 2025-05-29T10:57:21Z

ecl/eclccserver/eclccserver.cpp

            workunit->commit();
+            result = workunit->getState();
+        }
+
        workunit.clear();


trivial: could be inside the if (workunit) block

jakesmith · 2025-05-29T10:57:55Z

ecl/eclccserver/eclccserver.cpp

            workunit->commit();
+            result = workunit->getState();


trivial: I don't think it matters that after the commit(), but as coded, makes it looks like it is significant?

jakesmith · 2025-05-29T11:00:56Z

ecl/eclccserver/eclccserver.cpp

+
+public:
+    EclccLingeringCompiler(IPropertyTree * globals, unsigned _idx)
+    : instanceName(globals->queryProp("@name")), compiler(instanceName, _idx)


I know 'globals' is in scope in the context EclccLingeringCompiler is used.. but it looks unsafe to keep pointer to @name here, for clarify, future safety, is it worth storing in a StringAttr despite not being strictly being necessary at the moment?

jakesmith · 2025-05-29T11:03:19Z

ecl/eclccserver/eclccserver.cpp

-                    compileViaK8sJob(true);
-                    return;
-                }
+                //NOTE: This call does not modify the workunit itself, so no need to commit afterwards


@ghalliday - still not sure re. this comment, is it misplaced? setContainerizedProcessInfo does alter the workunit..

Signed-off-by: Gavin Halliday <[email protected]>

github-actions · 2025-05-29T16:27:27Z

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

ghalliday requested review from jakesmith and Copilot May 14, 2025 13:37

Copilot AI reviewed May 14, 2025

View reviewed changes

ghalliday force-pushed the issue33568 branch from bfa948f to d4d88bd Compare May 14, 2025 16:14

ghalliday commented May 15, 2025

View reviewed changes

ecl/eclccserver/eclccserver.cpp Show resolved Hide resolved

jakesmith reviewed May 20, 2025

View reviewed changes

ghalliday force-pushed the issue33568 branch from 57431f1 to 91061ae Compare May 22, 2025 15:43

ghalliday changed the base branch from master to candidate-9.12.x May 22, 2025 15:45

ghalliday closed this May 22, 2025

ghalliday reopened this May 22, 2025

ghalliday requested a review from jakesmith May 22, 2025 15:45

jakesmith reviewed May 29, 2025

View reviewed changes

ghalliday force-pushed the issue33568 branch from 91061ae to 43efa10 Compare May 29, 2025 15:57

HPCC-33568 Introduce a lingering c++ compile

6f46c42

Signed-off-by: Gavin Halliday <[email protected]>

ghalliday force-pushed the issue33568 branch from 43efa10 to 6f46c42 Compare May 29, 2025 15:58

ghalliday merged commit f41231a into hpcc-systems:candidate-9.12.x May 29, 2025
22 checks passed

-            const char * username = "MORE";
+            const char * username = globals->queryProp("@username");
+            if (!username || !*username)
+                username = "unknown";

HPCC-33568 Introduce a lingering c++ compile #19869

HPCC-33568 Introduce a lingering c++ compile #19869

Uh oh!

Conversation

ghalliday commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change:

Checklist:

Smoketest:

Testing:

Uh oh!

ghalliday commented May 14, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 14, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 14, 2025

Uh oh!

Uh oh!

jakesmith left a comment

Choose a reason for hiding this comment

Uh oh!

jakesmith May 20, 2025

Choose a reason for hiding this comment

Uh oh!

jakesmith May 29, 2025

Choose a reason for hiding this comment

Uh oh!

ghalliday commented May 22, 2025

Uh oh!

github-actions bot commented May 22, 2025

Uh oh!

jakesmith left a comment

Choose a reason for hiding this comment

Uh oh!

jakesmith May 29, 2025

Choose a reason for hiding this comment

Uh oh!

jakesmith May 29, 2025

Choose a reason for hiding this comment

Uh oh!

jakesmith May 29, 2025

Choose a reason for hiding this comment

Uh oh!

jakesmith May 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented May 29, 2025

Uh oh!

Uh oh!

ghalliday commented May 14, 2025 •

edited

Loading