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

AbstractLazyLoadRunMap.iterator() calls .all()

    Details

    • Similar Issues:

      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

            Hide
            jglick Jesse Glick added a comment -

            Seems to me that AbstractLazyLoadRunMap.all could simply create an empty BuildReference for each ID on disk, rather than calling load. Then the entrySet would have the right keys, and if and when you actually tried to access a value for an entry, BuildReferenceMapAdapter.unwrap would call getById and thus load for that one build. Right?

            Show
            jglick Jesse Glick added a comment - Seems to me that AbstractLazyLoadRunMap.all could simply create an empty BuildReference for each ID on disk, rather than calling load . Then the entrySet would have the right keys, and if and when you actually tried to access a value for an entry, BuildReferenceMapAdapter.unwrap would call getById and thus load for that one build. Right?
            Hide
            jglick Jesse Glick added a comment -

            To answer my own question: wrong—because it is returning a map keyed by build number, which cannot be determined until records are loaded. So a fix would need to be trickier: would need to make some operations on the entry set eager (preferably ones no one is likely to call), but make .entrySet().iterator() (as used by .size() or simply anyone using a for-loop) load entries on demand in reverse numeric order.

            Show
            jglick Jesse Glick added a comment - To answer my own question: wrong—because it is returning a map keyed by build number, which cannot be determined until records are loaded. So a fix would need to be trickier: would need to make some operations on the entry set eager (preferably ones no one is likely to call), but make .entrySet().iterator() (as used by .size() or simply anyone using a for-loop) load entries on demand in reverse numeric order.
            Hide
            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
            http://jenkins-ci.org/commit/jenkins/5903870d318a337548e3f7ef75603f75a378e7ac
            Log:
            JENKINS-18065 Documenting current misbehavior in a unit test.

            Show
            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 http://jenkins-ci.org/commit/jenkins/5903870d318a337548e3f7ef75603f75a378e7ac Log: JENKINS-18065 Documenting current misbehavior in a unit test.
            Hide
            dogfood dogfood added a comment -

            Integrated in jenkins_main_trunk #2574
            JENKINS-18065 Documenting current misbehavior in a unit test. (Revision 5903870d318a337548e3f7ef75603f75a378e7ac)

            Result = SUCCESS
            Jesse Glick : 5903870d318a337548e3f7ef75603f75a378e7ac
            Files :

            • core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
            Show
            dogfood dogfood added a comment - Integrated in jenkins_main_trunk #2574 JENKINS-18065 Documenting current misbehavior in a unit test. (Revision 5903870d318a337548e3f7ef75603f75a378e7ac) Result = SUCCESS Jesse Glick : 5903870d318a337548e3f7ef75603f75a378e7ac Files : core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
            Hide
            jglick Jesse Glick added a comment -

            Seems to block the attempted fix of JENKINS-20892 from actually working, at least in the case of BuildTimelineWidget.

            Show
            jglick Jesse Glick added a comment - Seems to block the attempted fix of JENKINS-20892 from actually working, at least in the case of BuildTimelineWidget .
            Hide
            kohsuke Kohsuke Kawaguchi added a comment -

            The problematic part is this:

                at jenkins.model.lazy.AbstractLazyLoadRunMap.all(AbstractLazyLoadRunMap.java:617)
                at jenkins.model.lazy.AbstractLazyLoadRunMap.entrySet(AbstractLazyLoadRunMap.java:277)
                at java.util.AbstractMap$2$1.<init>(Unknown Source)
                at java.util.AbstractMap$2.iterator(Unknown Source)
                at hudson.util.RunList.iterator(RunList.java:97)
            

            In looking at the code, what's going on is that RunList calls job.getBuilds():

                public RunList(View view) {// this is a type unsafe operation
                    Set<Job> jobs = new HashSet<Job>();
                    for (TopLevelItem item : view.getItems())
                        jobs.addAll(item.getAllJobs());
            
                    List<Iterable<R>> runLists = new ArrayList<Iterable<R>>();
                    for (Job job : jobs) {
                        runLists.add(job.getBuilds());
                    }
                    this.base = combine(runLists);
                }
            

            and job.getBuilds() does this:

                public RunList<RunT> getBuilds() {
                    return RunList.fromRuns(_getRuns().values());
                }
            

            ... which results in RunMap.values() which is implemented by AbstractMap, which calls ALLRM.entrySet(), that calls all().

            ALLRM.entrySet() impl must be smarter, or job.getBuilds() need to avoid calling values().

            Show
            kohsuke Kohsuke Kawaguchi added a comment - The problematic part is this: at jenkins.model.lazy.AbstractLazyLoadRunMap.all(AbstractLazyLoadRunMap.java:617) at jenkins.model.lazy.AbstractLazyLoadRunMap.entrySet(AbstractLazyLoadRunMap.java:277) at java.util.AbstractMap$2$1.<init>(Unknown Source) at java.util.AbstractMap$2.iterator(Unknown Source) at hudson.util.RunList.iterator(RunList.java:97) In looking at the code, what's going on is that RunList calls job.getBuilds() : public RunList(View view) {// this is a type unsafe operation Set<Job> jobs = new HashSet<Job>(); for (TopLevelItem item : view.getItems()) jobs.addAll(item.getAllJobs()); List<Iterable<R>> runLists = new ArrayList<Iterable<R>>(); for (Job job : jobs) { runLists.add(job.getBuilds()); } this.base = combine(runLists); } and job.getBuilds() does this: public RunList<RunT> getBuilds() { return RunList.fromRuns(_getRuns().values()); } ... which results in RunMap.values() which is implemented by AbstractMap , which calls ALLRM.entrySet() , that calls all() . ALLRM.entrySet() impl must be smarter, or job.getBuilds() need to avoid calling values() .
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Kohsuke Kawaguchi
            Path:
            changelog.html
            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/54c084613f83f9b7a02957eb98dbd8dc661d1c9c
            Log:
            [FIXED JENKINS-18065] made ALLRM.entrySet() smarter

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Kohsuke Kawaguchi Path: changelog.html 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/54c084613f83f9b7a02957eb98dbd8dc661d1c9c Log: [FIXED JENKINS-18065] made ALLRM.entrySet() smarter
            Hide
            dogfood dogfood added a comment -

            Integrated in jenkins_main_trunk #3511
            [FIXED JENKINS-18065] made ALLRM.entrySet() smarter (Revision 54c084613f83f9b7a02957eb98dbd8dc661d1c9c)

            Result = SUCCESS
            kohsuke : 54c084613f83f9b7a02957eb98dbd8dc661d1c9c
            Files :

            • core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
            • core/src/main/java/hudson/util/RunList.java
            • changelog.html
            • core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java
            • core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java
            Show
            dogfood dogfood added a comment - Integrated in jenkins_main_trunk #3511 [FIXED JENKINS-18065] made ALLRM.entrySet() smarter (Revision 54c084613f83f9b7a02957eb98dbd8dc661d1c9c) Result = SUCCESS kohsuke : 54c084613f83f9b7a02957eb98dbd8dc661d1c9c Files : core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java core/src/main/java/hudson/util/RunList.java changelog.html core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java
            Hide
            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/da57ad5c30b1c9ca8365f869638abd29c8fb5896
            Log:
            JENKINS-18065 54c0846 amendment: presumably RunMap.entrySet().iterator().next().setValue(…) should be illegal.

            Compare: https://github.com/jenkinsci/jenkins/compare/54c084613f83...da57ad5c30b1

            Show
            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/da57ad5c30b1c9ca8365f869638abd29c8fb5896 Log: JENKINS-18065 54c0846 amendment: presumably RunMap.entrySet().iterator().next().setValue(…) should be illegal. Compare: https://github.com/jenkinsci/jenkins/compare/54c084613f83...da57ad5c30b1
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Jesse Glick
            Path:
            cli/pom.xml
            core/pom.xml
            plugins/pom.xml
            pom.xml
            test/pom.xml
            war/pom.xml
            http://jenkins-ci.org/commit/jenkins/0faad29a5594fc20f9b3670797a3b7ebb198b63b
            Log:
            Set version to: 1.554.3.JENKINS-18065-ALLRM-all

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: cli/pom.xml core/pom.xml plugins/pom.xml pom.xml test/pom.xml war/pom.xml http://jenkins-ci.org/commit/jenkins/0faad29a5594fc20f9b3670797a3b7ebb198b63b Log: Set version to: 1.554.3. JENKINS-18065 -ALLRM-all
            Hide
            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/b227250a63beab888a25c2a781b6fe8e493284d3
            Log:
            [FIXED JENKINS-18065] made ALLRM.entrySet() smarter

            (cherry picked from commit 54c084613f83f9b7a02957eb98dbd8dc661d1c9c)

            Conflicts:
            changelog.html

            Show
            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/b227250a63beab888a25c2a781b6fe8e493284d3 Log: [FIXED JENKINS-18065] made ALLRM.entrySet() smarter (cherry picked from commit 54c084613f83f9b7a02957eb98dbd8dc661d1c9c) Conflicts: changelog.html
            Hide
            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/525793d741c5f209b27bdc94844798b6410fce7b
            Log:
            JENKINS-18065 54c0846 amendment: presumably RunMap.entrySet().iterator().next().setValue(…) should be illegal.
            (cherry picked from commit da57ad5c30b1c9ca8365f869638abd29c8fb5896)

            Show
            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/525793d741c5f209b27bdc94844798b6410fce7b Log: JENKINS-18065 54c0846 amendment: presumably RunMap.entrySet().iterator().next().setValue(…) should be illegal. (cherry picked from commit da57ad5c30b1c9ca8365f869638abd29c8fb5896)
            Hide
            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/c3e73296b5af0a4aaa06a3bdf867ac50fe211888
            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)

            Show
            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/c3e73296b5af0a4aaa06a3bdf867ac50fe211888 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)
            Hide
            dogfood dogfood added a comment -

            Integrated in jenkins_main_trunk #3512
            JENKINS-18065 54c0846 amendment: presumably RunMap.entrySet().iterator().next().setValue(…) should be illegal. (Revision da57ad5c30b1c9ca8365f869638abd29c8fb5896)

            Result = SUCCESS
            Jesse Glick : da57ad5c30b1c9ca8365f869638abd29c8fb5896
            Files :

            • core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java
            Show
            dogfood dogfood added a comment - Integrated in jenkins_main_trunk #3512 JENKINS-18065 54c0846 amendment: presumably RunMap.entrySet().iterator().next().setValue(…) should be illegal. (Revision da57ad5c30b1c9ca8365f869638abd29c8fb5896) Result = SUCCESS Jesse Glick : da57ad5c30b1c9ca8365f869638abd29c8fb5896 Files : core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java
            Hide
            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/2f58ceb1a66c8ed621fa2562c5ed3445b29ea200
            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.

            Show
            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/2f58ceb1a66c8ed621fa2562c5ed3445b29ea200 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.
            Show
            jglick Jesse Glick added a comment - Deployed https://github.com/jenkinsci/jenkins/compare/jenkins-1.554.3...JENKINS-18065-ALLRM-all to: http://repo.jenkins-ci.org/public/org/jenkins-ci/main/jenkins-war/1.554.3.JENKINS-18065-ALLRM-all/jenkins-war-1.554.3.JENKINS-18065-ALLRM-all.war
            Hide
            dogfood dogfood added a comment -

            Integrated in jenkins_main_trunk #3514
            JENKINS-18065 Uncommenting original test. (Revision 2f58ceb1a66c8ed621fa2562c5ed3445b29ea200)

            Result = SUCCESS
            Jesse Glick : 2f58ceb1a66c8ed621fa2562c5ed3445b29ea200
            Files :

            • core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
            • core/src/test/java/jenkins/model/lazy/FakeMap.java
            Show
            dogfood dogfood added a comment - Integrated in jenkins_main_trunk #3514 JENKINS-18065 Uncommenting original test. (Revision 2f58ceb1a66c8ed621fa2562c5ed3445b29ea200) Result = SUCCESS Jesse Glick : 2f58ceb1a66c8ed621fa2562c5ed3445b29ea200 Files : core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java core/src/test/java/jenkins/model/lazy/FakeMap.java
            Hide
            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

            Show
            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
            Hide
            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

            Show
            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
            Hide
            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

            Show
            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
            Hide
            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)

            Show
            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)
            Hide
            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)

            Show
            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

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

                Dates

                • Created:
                  Updated:
                  Resolved: