[JENKINS-30424] Move build.save to the migration thread #8

amuniz · 2015-10-21T20:21:53Z

Move build.save to the migration thread (avoiding to save the build before it is loaded), and keep the save on Id.onLoad but limit its effect if the migration thread did not finish yet.

Test added. Confirmed the test failing with current master with the following exception (and working after this fix):

Exception in thread "unique-id background migration thread" org.jenkinsci.plugins.uniqueid.impl.IdStoreMigratorV1ToV2$IDStoreMigrationException: Failure whilst migrating folderWithID » jobWithID #2
    at org.jenkinsci.plugins.uniqueid.impl.IdStoreMigratorV1ToV2.migrate(IdStoreMigratorV1ToV2.java:100)
    at org.jenkinsci.plugins.uniqueid.impl.Id.onLoad(Id.java:78)
    at hudson.model.Run.onLoad(Run.java:345)
    at org.jenkinsci.plugins.uniqueid.impl.LongLoadingBuild.onLoad(LongLoadingBuild.java:29)
    at hudson.model.RunMap.retrieve(RunMap.java:223)
    at hudson.model.RunMap.retrieve(RunMap.java:59)
    at jenkins.model.lazy.AbstractLazyLoadRunMap.load(AbstractLazyLoadRunMap.java:687)
    at jenkins.model.lazy.AbstractLazyLoadRunMap.load(AbstractLazyLoadRunMap.java:670)
    at jenkins.model.lazy.AbstractLazyLoadRunMap.all(AbstractLazyLoadRunMap.java:622)
    at jenkins.model.lazy.AbstractLazyLoadRunMap.entrySet(AbstractLazyLoadRunMap.java:277)
    at java.util.AbstractMap$2$1.<init>(AbstractMap.java:389)
    at java.util.AbstractMap$2.iterator(AbstractMap.java:388)
    at hudson.util.RunList.iterator(RunList.java:97)
    at org.jenkinsci.plugins.uniqueid.impl.IdStoreMigratorV1ToV2$RunIDMigrationThread.run(IdStoreMigratorV1ToV2.java:134)
    at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.NullPointerException
    at org.jenkinsci.plugins.uniqueid.impl.LongLoadingBuild.save(LongLoadingBuild.java:23)
    at org.jenkinsci.plugins.uniqueid.impl.RunIdStore.remove(RunIdStore.java:33)

@reviewbybees (esp. @jtnord)

…save the build before it is loaded)

ghost · 2015-10-21T20:25:04Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

recena · 2015-10-21T20:30:37Z

src/main/java/org/jenkinsci/plugins/uniqueid/impl/RunIdStore.java

@@ -38,4 +41,18 @@ public void remove(Run run) throws IOException {
    public String get(Run thing) {
        return Id.getId((Actionable) thing);
    }
+
+    private void saveIfNeeded(Run run) throws IOException {


The method name seems clear, but a javadoc will be nice.

It's private so Javadocs are not really useful. There are some inline comments inside for developers.

I did not know this measure to add javadoc

recena · 2015-10-21T20:33:38Z

LGTM

jglick · 2015-10-21T20:34:16Z

src/main/java/org/jenkinsci/plugins/uniqueid/impl/Id.java

@@ -71,7 +71,7 @@ public void onAttached(Run<?, ?> r) {
        // NO-OP
    }

-    /** 
+    /**


Could revert.

jglick · 2015-10-21T20:37:27Z

🐝, kudos on finding a way to reproduce this in a test.

jtnord · 2015-10-21T20:38:32Z

src/test/java/org/jenkinsci/plugins/uniqueid/impl/LongLoadingAction.java

+
+    public void onLoad(Run<?, ?> r) {
+        try {
+            Thread.sleep(1000);


Sleeping in tests isn't generally a good idea, ca. A wait/semaphore be used?

Didn't think about it. It would be far more complex than this.
What's exactly wrong with Thread.sleep here?

I can see how sleeping indefinitely (inside a loop) is a bad thing, but this seems fine to me, even if I don't know why it's there (mimic a load delay or something?).

Yes, it's simulating a load delay (which is what causes the original issue actually).

Ok, having looked at the test code I can now see why you're using the 1s delay. I can see now that this could (potentially) cause some flaky test behaviour, but I guess @amuniz is the one to decide if that's realistically possible or not. If it is possible it should be easy to fix by adding a flag on LongLoadingAction that gets toggled once the test is where it needs to be, telling LongLoadingAction.onLoad that it's ok to finish now.

The question is what happens if the machine running the test is heavily loaded. If that causes a test failure when the code is good, that is a problem. If it just causes the test to pass even when the code regressed, that is OK, since this will not usually happen and we will find the failure soon.

amuniz · 2015-10-22T08:48:39Z

@jglick @jtnord comments addressed.

tfennelly · 2015-10-22T09:34:42Z

src/main/java/org/jenkinsci/plugins/uniqueid/impl/IdStoreMigratorV1ToV2.java

+                                // Save the run here so its storage is updated (after being migrated in Id.onLoad(Run))
+                                run.save();
+                            } catch (IOException e) {
+                                LOGGER.log(Level.WARNING, "Can not save build {0} on job {1}", new Object[] { run.getNumber(), job.getName() });


How about including the IOException in the log?

I would not like to pollute the logs with stacktraces, since if this migration fails it will be addressed later by Id.onLoad on demand.
Moreover, all Level.WARNING logs in this plugin are not printing stacktraces, I don't want to break predefined style.

Fair enough.

🐜 This exception would be unexpected and logging it would probably be wise.

The other loggers should log the message. If not please raise another jira

tfennelly · 2015-10-22T09:43:28Z

🐝

jglick · 2015-10-22T15:36:50Z

🐝

kwhetstone · 2015-10-23T02:26:03Z

It appears this solves another test failure in unique-id that periodically shows up if you're unlucky in threading. This will be very helpful in clearing our compat failures.

amuniz · 2015-10-23T08:12:26Z

@reviewbybees done

stephenc · 2015-10-23T08:33:38Z

🐝

[JENKINS-30424] Move build.save to the migration thread

[JENKINS-30424] Move build.save to the migration thread (avoiding to …

c2552cd

…save the build before it is loaded)

recena reviewed Oct 21, 2015
View reviewed changes

jglick reviewed Oct 21, 2015
View reviewed changes

jtnord reviewed Oct 21, 2015
View reviewed changes

Fixed @jglick's suggestions

40f4a44

amuniz force-pushed the JENKINS-30424 branch 2 times, most recently from 1075b13 to 1ad1a6f Compare October 22, 2015 08:02

Fixed @jtnord's suggestions

be2ed91

amuniz force-pushed the JENKINS-30424 branch from 1ad1a6f to be2ed91 Compare October 22, 2015 08:03

tfennelly reviewed Oct 22, 2015
View reviewed changes

Add license headers to new files

548ccda

Print exception stacktrace on build save error

a16425f

stephenc added a commit that referenced this pull request Oct 23, 2015

Merge pull request #8 from amuniz/JENKINS-30424

62ae665

[JENKINS-30424] Move build.save to the migration thread

stephenc merged commit 62ae665 into jenkinsci:master Oct 23, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-30424] Move build.save to the migration thread #8

[JENKINS-30424] Move build.save to the migration thread #8

amuniz commented Oct 21, 2015

ghost commented Oct 21, 2015

recena Oct 21, 2015

amuniz Oct 21, 2015

recena Oct 21, 2015

recena commented Oct 21, 2015

jglick Oct 21, 2015

jglick commented Oct 21, 2015

jtnord Oct 21, 2015

amuniz Oct 21, 2015

tfennelly Oct 22, 2015

amuniz Oct 22, 2015

tfennelly Oct 22, 2015

jglick Oct 22, 2015

amuniz commented Oct 22, 2015

tfennelly Oct 22, 2015

amuniz Oct 22, 2015

tfennelly Oct 22, 2015

jglick Oct 22, 2015

jtnord Oct 22, 2015

amuniz Oct 22, 2015

tfennelly commented Oct 22, 2015

jglick commented Oct 22, 2015

kwhetstone commented Oct 23, 2015

amuniz commented Oct 23, 2015

stephenc commented Oct 23, 2015

[JENKINS-30424] Move build.save to the migration thread #8

[JENKINS-30424] Move build.save to the migration thread #8

Conversation

amuniz commented Oct 21, 2015

ghost commented Oct 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

recena commented Oct 21, 2015

Choose a reason for hiding this comment

jglick commented Oct 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amuniz commented Oct 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfennelly commented Oct 22, 2015

jglick commented Oct 22, 2015

kwhetstone commented Oct 23, 2015

amuniz commented Oct 23, 2015

stephenc commented Oct 23, 2015