Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-45585

WorkflowRun.onLoad need not eagerly load the FlowExecution of a completed build

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      Currently we always load FlowExecution when loading a build even though we might not actually need it. This is thought to slow down some things, such as getBuildHealth, since we are loading executions for some historical completed builds and then not using them. The loading should be on demand only.

      Since currently FlowExecution.onComplete is called soon thereafter, we would need some other marker in WorkflowRun for a completed build. Could check for logsToCopy == null though this is deleted in JENKINS-38381. Probably better to use completed as noted here.

        Attachments

          Issue Links

            Activity

            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Jesse Glick
            Path:
            src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java
            http://jenkins-ci.org/commit/workflow-job-plugin/bbec76226dcbd01622a6f29a753d1e72af49e03f
            Log:
            Noting JENKINS-45585.

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java http://jenkins-ci.org/commit/workflow-job-plugin/bbec76226dcbd01622a6f29a753d1e72af49e03f Log: Noting JENKINS-45585 .
            Hide
            abayer Andrew Bayer added a comment -

            Jesse Glick - it looks we set state = State.COMPLETED via onEndBuilding() in finish(Result,Throwable) - could we just use that as the basis for deciding what to set completed to early in onLoad?

            Show
            abayer Andrew Bayer added a comment - Jesse Glick - it looks we set state = State.COMPLETED via onEndBuilding() in finish(Result,Throwable) - could we just use that as the basis for deciding what to set completed to early in onLoad ?
            Hide
            abayer Andrew Bayer added a comment -

            Ah, bugger, Run#state is private.

            Show
            abayer Andrew Bayer added a comment - Ah, bugger, Run#state is private.
            Hide
            abayer Andrew Bayer added a comment -

            Jesse Glick - so I may be crazy, but it feels like if we were to add CpsFlowExecution#done to CpsFlowExecution.ConverterImpl's marshal and unmarshal methods (it's unpersisted currently), we could do a CpsFlowExecution#isComplete() call in WorkflowRun#onLoad() before we end up calling CpsFlowExecution#onLoad... WorkflowRun#execution would exist correctly, though it wouldn't yet have the heads set...hrm. I dunno, actually. I may be talking myself out of this.

            Show
            abayer Andrew Bayer added a comment - Jesse Glick - so I may be crazy, but it feels like if we were to add CpsFlowExecution#done to CpsFlowExecution.ConverterImpl 's marshal and unmarshal methods (it's unpersisted currently), we could do a CpsFlowExecution#isComplete() call in WorkflowRun#onLoad() before we end up calling CpsFlowExecution#onLoad ... WorkflowRun#execution would exist correctly, though it wouldn't yet have the heads set...hrm. I dunno, actually. I may be talking myself out of this.
            Hide
            abayer Andrew Bayer added a comment -

            Thinking yet more: I think CpsFlowExecution#done should be in CpsFlowExecution.ConverterImpl#marshal and #unmarshal - it's weird that it's not persisted currently, at least to me. With that in place, CpsFlowExecution#isComplete() would be true for completed builds at load time. CpsFlowExecution#onLoad(FlowExecutionOwner) would call initializeStorage() and load the flow nodes, but not go into loadProgramAsync and onwards to parsing/transformation/etc. That at least deals with some of the issues caused by WorkflowRun#execution being eagerly loaded. So then the question is whether we want to bother with the initializeStorage() call at all in lazy-loading situations. Assuming that we don't want to do that, then we could change to something like this in WorkflowRun#onLoad:

                    FlowExecution fetchedExecution = execution;
            
                    if (fetchedExecution != null && !fetchedExecution.isComplete()) {
                        try {
                            if (getParent().isResumeBlocked() && execution instanceof BlockableResume) {
                                ((BlockableResume) execution).setResumeBlocked(true);
                            }
                            fetchedExecution.onLoad(new Owner(this));
                        } catch (Exception x) {
                            LOGGER.log(Level.WARNING, null, x);
                            execution = null; // probably too broken to use
                        }
                    }
                    fetchedExecution = execution;
                    if (fetchedExecution != null) {
            ...
            

            ...i.e., skipping calling fetchedExecution.onLoad(new Owner(this)) for a completed FlowExecution. The following if block already has a check for !fetchedExecution.isComplete() before doing some of its body, so we probably wouldn't need to do anything there, I guess?

            Show
            abayer Andrew Bayer added a comment - Thinking yet more: I think CpsFlowExecution#done should be in CpsFlowExecution.ConverterImpl#marshal and #unmarshal - it's weird that it's not persisted currently, at least to me. With that in place, CpsFlowExecution#isComplete() would be true for completed builds at load time. CpsFlowExecution#onLoad(FlowExecutionOwner) would call initializeStorage() and load the flow nodes, but not go into loadProgramAsync and onwards to parsing/transformation/etc. That at least deals with some of the issues caused by WorkflowRun#execution being eagerly loaded. So then the question is whether we want to bother with the initializeStorage() call at all in lazy-loading situations. Assuming that we don't want to do that, then we could change to something like this in WorkflowRun#onLoad : FlowExecution fetchedExecution = execution; if (fetchedExecution != null && !fetchedExecution.isComplete()) { try { if (getParent().isResumeBlocked() && execution instanceof BlockableResume) { ((BlockableResume) execution).setResumeBlocked( true ); } fetchedExecution.onLoad( new Owner( this )); } catch (Exception x) { LOGGER.log(Level.WARNING, null , x); execution = null ; // probably too broken to use } } fetchedExecution = execution; if (fetchedExecution != null ) { ... ...i.e., skipping calling fetchedExecution.onLoad(new Owner(this)) for a completed FlowExecution . The following if block already has a check for !fetchedExecution.isComplete() before doing some of its body, so we probably wouldn't need to do anything there, I guess?
            Hide
            svanoort Sam Van Oort added a comment -

            Released

            Show
            svanoort Sam Van Oort added a comment - Released
            Hide
            svanoort Sam Van Oort added a comment -

            Released with v2.18 of workflow-job

            Show
            svanoort Sam Van Oort added a comment - Released with v2.18 of workflow-job

              People

              • Assignee:
                svanoort Sam Van Oort
                Reporter:
                jglick Jesse Glick
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: