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

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

      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.

          [JENKINS-45585] WorkflowRun.onLoad need not eagerly load the FlowExecution of a completed build

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

          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?

          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 ?

          Andrew Bayer added a comment -

          Ah, bugger, Run#state is private.

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

          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.

          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.

          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?

          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?

          Sam Van Oort added a comment -

          Released

          Sam Van Oort added a comment - Released

          Sam Van Oort added a comment -

          Released with v2.18 of workflow-job

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

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

              Created:
              Updated:
              Resolved: