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

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

    XMLWordPrintable

Details

    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

            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.

            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 .
            abayer Andrew Bayer added a comment -

            jglick - 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?

            abayer Andrew Bayer added a comment - jglick - 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 ?
            abayer Andrew Bayer added a comment -

            Ah, bugger, Run#state is private.

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

            jglick - 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.

            abayer Andrew Bayer added a comment - jglick - 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.
            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?

            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?
            svanoort Sam Van Oort added a comment -

            Released

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

            Released with v2.18 of workflow-job

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

            People

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

              Dates

                Created:
                Updated:
                Resolved: