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

AbstractLazyLoadRunMap.iterator() calls .all()

    XMLWordPrintable

Details

    Description

      Merely calling job.getBuilds().isEmpty() breaks lazy-loading:

      	at hudson.model.Run.onLoad(Run.java:319)
      	at hudson.model.RunMap.retrieve(RunMap.java:226)
      	at hudson.model.RunMap.retrieve(RunMap.java:59)
      	at jenkins.model.lazy.AbstractLazyLoadRunMap.load(AbstractLazyLoadRunMap.java:667)
      	at jenkins.model.lazy.AbstractLazyLoadRunMap.load(AbstractLazyLoadRunMap.java:650)
      	at jenkins.model.lazy.AbstractLazyLoadRunMap.all(AbstractLazyLoadRunMap.java:602)
      	at jenkins.model.lazy.AbstractLazyLoadRunMap.entrySet(AbstractLazyLoadRunMap.java:264)
      	at java.util.AbstractMap$2$1.<init>(AbstractMap.java:378)
      	at java.util.AbstractMap$2.iterator(AbstractMap.java:377)
      	at hudson.util.RunList$2.iterator(RunList.java:213)
      	at hudson.util.RunList.iterator(RunList.java:103)
      	at hudson.util.RunList.isEmpty(RunList.java:173)
      

      You would reasonably expect this method, and anything simply iterating builds, to be lazy, but it is eager.

      Attachments

        Issue Links

          Activity

            Code changed in jenkins
            User: Daniel Beck
            Path:
            changelog.html
            http://jenkins-ci.org/commit/jenkins/fdc0b5c5650b3ee849f02e3c5d94d23c12886adc
            Log:
            Noting #1314, #1316, #1308, JENKINS-17667, JENKINS-22395, JENKINS-18065

            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Daniel Beck Path: changelog.html http://jenkins-ci.org/commit/jenkins/fdc0b5c5650b3ee849f02e3c5d94d23c12886adc Log: Noting #1314, #1316, #1308, JENKINS-17667 , JENKINS-22395 , JENKINS-18065

            Code changed in jenkins
            User: Daniel Beck
            Path:
            changelog.html
            http://jenkins-ci.org/commit/jenkins/72f37e414533c02624f9deffc85a2b50f03905e6
            Log:
            Remove JENKINS-22395, JENKINS-18065 from 1.572 section

            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Daniel Beck Path: changelog.html http://jenkins-ci.org/commit/jenkins/72f37e414533c02624f9deffc85a2b50f03905e6 Log: Remove JENKINS-22395 , JENKINS-18065 from 1.572 section

            Code changed in jenkins
            User: Kohsuke Kawaguchi
            Path:
            core/src/main/java/hudson/util/RunList.java
            core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java
            core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java
            core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
            http://jenkins-ci.org/commit/jenkins/632d7fe0d01d492e0803495691f4397980cf44e0
            Log:
            [FIXED JENKINS-18065] made ALLRM.entrySet() smarter

            (cherry picked from commit 54c084613f83f9b7a02957eb98dbd8dc661d1c9c)

            Conflicts:
            changelog.html

            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Kohsuke Kawaguchi Path: core/src/main/java/hudson/util/RunList.java core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java http://jenkins-ci.org/commit/jenkins/632d7fe0d01d492e0803495691f4397980cf44e0 Log: [FIXED JENKINS-18065] made ALLRM.entrySet() smarter (cherry picked from commit 54c084613f83f9b7a02957eb98dbd8dc661d1c9c) Conflicts: changelog.html

            Code changed in jenkins
            User: Jesse Glick
            Path:
            core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java
            http://jenkins-ci.org/commit/jenkins/f1b0779c6d356db8fb0de6c89bd5bc03483d6b34
            Log:
            JENKINS-18065 54c0846 amendment: presumably RunMap.entrySet().iterator().next().setValue(…) should be illegal.
            (cherry picked from commit da57ad5c30b1c9ca8365f869638abd29c8fb5896)

            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java http://jenkins-ci.org/commit/jenkins/f1b0779c6d356db8fb0de6c89bd5bc03483d6b34 Log: JENKINS-18065 54c0846 amendment: presumably RunMap.entrySet().iterator().next().setValue(…) should be illegal. (cherry picked from commit da57ad5c30b1c9ca8365f869638abd29c8fb5896)

            Code changed in jenkins
            User: Jesse Glick
            Path:
            core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
            core/src/test/java/jenkins/model/lazy/FakeMap.java
            http://jenkins-ci.org/commit/jenkins/8c70a2b76875d2ca6a114c45fd54c0b1e9ecb7d2
            Log:
            JENKINS-18065 Uncommenting original test.
            The actual implementation does not behave as nicely as one would hope, for a few reasons:
            1. isEmpty actually tries to load the newestBuild, rather than simply checking whether there is some idOnDisk.
            Arguably this is necessary, since there could be an unloadable build record, in which case it would be technically correct for the map to be considered empty.
            2. Loading AbstractLazyLoadRunMap.newestBuild calls search(MAX_VALUE, DESC), which behaves oddly, by doing a binary search, and thus perhaps loading lg(|map|) entries.
            You would think that it would suffice to check for the last member of idOnDisk in index.byId.
            3. The iterator eagerly loads the next value before hasNext has even been called.
            Looks bad in a test, though it probably has little practical impact since most callers would be calling hasNext soon afterward anyway.
            Might cause one extra build record to be loaded unnecessarily from a limited RunList.

            (cherry picked from commit 2f58ceb1a66c8ed621fa2562c5ed3445b29ea200)

            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java core/src/test/java/jenkins/model/lazy/FakeMap.java http://jenkins-ci.org/commit/jenkins/8c70a2b76875d2ca6a114c45fd54c0b1e9ecb7d2 Log: JENKINS-18065 Uncommenting original test. The actual implementation does not behave as nicely as one would hope, for a few reasons: 1. isEmpty actually tries to load the newestBuild, rather than simply checking whether there is some idOnDisk. Arguably this is necessary, since there could be an unloadable build record, in which case it would be technically correct for the map to be considered empty. 2. Loading AbstractLazyLoadRunMap.newestBuild calls search(MAX_VALUE, DESC), which behaves oddly, by doing a binary search, and thus perhaps loading lg(|map|) entries. You would think that it would suffice to check for the last member of idOnDisk in index.byId. 3. The iterator eagerly loads the next value before hasNext has even been called. Looks bad in a test, though it probably has little practical impact since most callers would be calling hasNext soon afterward anyway. Might cause one extra build record to be loaded unnecessarily from a limited RunList. (cherry picked from commit 2f58ceb1a66c8ed621fa2562c5ed3445b29ea200)

            People

              kohsuke Kohsuke Kawaguchi
              jglick Jesse Glick
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: