[FIXED JENKINS-28843] Stored the unique value in the job configuration #1

jtnord · 2015-06-11T03:36:24Z

unique id plugin stored the unique value in the job configuration.

@reviewbybees

oleg-nenashev · 2015-06-11T06:02:25Z

src/main/java/org/jenkinsci/plugins/uniqueid/IdStore.java

     */
-    public abstract void make(T object);
+    public abstract void make(T object) throws Exception;


It causes a binary compatibility issue, but I doubt anybody uses the plugin's API actively. Let's bump the major version digit BTW

yes - but the alternative is to make this return a boolean and have callers check.
Neither are good in my opinion - I could change this to be IOException (but that is specific to our implementation )

oleg-nenashev · 2015-06-11T07:13:46Z

I'm working on the failing test

stephenc · 2015-06-11T08:14:06Z

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

+        if (jenkins == null) {
+            throw new IllegalStateException("Jenkins is null, so it is impossible to migrate the IDs");
+        }
+        File marker = new File(jenkins.getRootDir(), "IdStoreMigration.txt");


could we use a name that makes it clear what plugin the file is for, e.g. unique-id-migration.txt

Perhaps make this a hidden file i.e. named something like .IdStoreMigrationCompleted.

will change. I'm not convinced about hiding the file so will go with @stephenc's suggestion

stephenc · 2015-06-11T08:17:50Z

otherwise looks ok from a code review PoV

oleg-nenashev · 2015-06-11T08:26:50Z

src/main/java/org/jenkinsci/plugins/uniqueid/IdStore.java

     */
-    public static String getId(Object object) {
+    public static String getId(Object object) throws IllegalArgumentException, Exception {


I think a fallback to a legacy store is required here (as I've understood initial test suites)

No - there should be no fallback at all.
The legacy store must be migrated at first run and then removed if it exists.

tfennelly · 2015-06-11T09:25:33Z

Are the binary compatibility changes here not a bit risky? Even the fact that there's no longer an Id action on these objects. I would not be at all surprised if external processes have developed dependencies on the Id action and its presence.

Maybe I don't understand the problem fully but would it not be possible to leave all the IdStore impls as they are and just change the Id action impl and how it works. Instead of it storing an id, it instead stores the path to the object, from which the location of the unique-id.txt file can be resolved - store, read etc. I suppose that breaks after a move of the object. Hmmm....

tfennelly · 2015-06-11T09:27:46Z

There's no funky Jenkins way for an Action instance to get a handle on the model object(s) with which it is associated?

oleg-nenashev · 2015-06-11T10:07:22Z

Added jtnord#1 for post-restart migrations. It also fixes the failing IdTest

Change the filenames so that they are more easily identified with this plugin. Fix some typos.

tfennelly · 2015-06-11T11:39:15Z

So there's no possibility of a post copy/replicate task extension point? Seems like a general thing that would make sense.

If there was, maybe a DeleteUniqueId implementation could clear the unique id on the target after copying.

jtnord · 2015-06-11T12:18:19Z

after discussion with the plugin maintainer it has been decided to perform migration of IDs for Runs after startup to not block startup for so long. Also Runs do not need special handling and can use the same text marker as jobs/folders.

jglick · 2015-06-11T12:24:16Z

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

+import org.jenkinsci.plugins.uniqueid.IdStore;
+
+/**
+ * Stores id's for runs as an action on the Run.


This Javadoc is obsolete I guess.

Use standardCharset to avoid UTF-8 related issues.

tfennelly · 2015-06-11T14:59:38Z

@jtnord I raised some questions/points that were not answered/addressed. Maybe that's coz they were considered to be stupid, but maybe they should be answered in some way (even if they're called out as being stupid questions 😄 ).

jtnord · 2015-06-11T15:06:23Z

@tfennelly sorry I thought I had addressed or replied to them all.

So there's no possibility of a post copy/replicate task extension point? Seems like a general thing that would make sense.

If there was, maybe a DeleteUniqueId implementation could clear the unique id on the target after copying.

The internal implementation can filter out files - so it would just filter out these files.
if we want something else I think we can evolve the API in the future?

oleg-nenashev · 2015-06-11T16:07:40Z

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

+            // all done...
+            final long duration = System.currentTimeMillis() - startTime;
+            final long minutes = TimeUnit.MILLISECONDS.toMinutes(System.currentTimeMillis() - startTime);
+            final long seconds = TimeUnit.MILLISECONDS.toMinutes(System.currentTimeMillis() - startTime - TimeUnit.HOURS.toMillis(minutes));


Should be TimeUnit.MILLISECONDS.toSeconds()

TimeUnit.MINUTES at the end?

Fixed - good spot.

oleg-nenashev · 2015-06-11T16:44:31Z

👍

recampbell · 2015-06-11T17:02:50Z

👍

[FIXED JENKINS-28843] No longer stored the unique value in the items configuration.

jglick · 2015-06-11T18:14:51Z

pom.xml

  </scm>

+  <properties>
+     <maven.compiler.source>1.7</maven.compiler.source>
+     <maven.compiler.target>1.7</maven.compiler.target>


May also want to change the Animal Sniffer execution.

Apparently even the current plugin POM does not define java.level like Jenkins core itself does. Something to be fixed in a future version of the POM.

it's currently working. I'll leave that to someone for a future excesize (along with using a recent core)

jglick · 2015-06-11T18:35:07Z

A bit late to the party but: 👍, with some low-importance comments that could be addressed later.

Tweak logging levels andload actions to force lazy loading.

jtnord added 4 commits June 10, 2015 17:41

this is not a tag

2706618

update to maven-release-plugin 2.5.1.

18da636

add .gitignore

19e7474

[JENKINS-28843] Initial implementation of ID migrator.

b5b5f08

oleg-nenashev reviewed Jun 11, 2015
View reviewed changes

stephenc reviewed Jun 11, 2015
View reviewed changes

oleg-nenashev reviewed Jun 11, 2015
View reviewed changes

jtnord added 2 commits June 11, 2015 11:39

Remove file that should not have been checked in

7762111

make filenames more appropriate.

1de8efd

Change the filenames so that they are more easily identified with this plugin. Fix some typos.

oleg-nenashev mentioned this pull request Jun 11, 2015

Bump dependencies #2

Merged

Merge unique-id-plugin#2 into JENKINS-28843

1991180

jglick changed the title ~~[WIP] Jenkins-28843~~ [WIP] [JENKINS-28843] Stored the unique value in the job configuration Jun 11, 2015

jglick reviewed Jun 11, 2015
View reviewed changes

jtnord added 2 commits June 11, 2015 15:54

Cleanup code with Java7 APIs.

07f5de0

Use standardCharset to avoid UTF-8 related issues.

update javadoc for RunIdStore

cbee5b5

jtnord changed the title ~~[WIP] [JENKINS-28843] Stored the unique value in the job configuration~~ [JENKINS-28843] Stored the unique value in the job configuration Jun 11, 2015

jtnord changed the title ~~[JENKINS-28843] Stored the unique value in the job configuration~~ [FIXED JENKINS-28843] Stored the unique value in the job configuration Jun 11, 2015

Access something from the build to ensure loading happens.

c38cbb0

oleg-nenashev reviewed Jun 11, 2015
View reviewed changes

jtnord added 2 commits June 11, 2015 17:18

Fix incorrect time conversion.

4258879

rename variable for oleg

9cb4df5

switch to use Id for the build

f4700e8

jtnord added a commit that referenced this pull request Jun 11, 2015

Merge pull request #1 from jtnord/JENKINS-28843

ea9a8df

[FIXED JENKINS-28843] No longer stored the unique value in the items configuration.

jtnord merged commit ea9a8df into jenkinsci:master Jun 11, 2015

jglick reviewed Jun 11, 2015
View reviewed changes

jtnord mentioned this pull request Jun 11, 2015

[Jenkins 28843] fix scm and review comments. #3

Merged

jtnord added a commit that referenced this pull request Jun 11, 2015

Update comments from jglick in #1

98c8f56

Tweak logging levels andload actions to force lazy loading.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIXED JENKINS-28843] Stored the unique value in the job configuration #1

[FIXED JENKINS-28843] Stored the unique value in the job configuration #1

jtnord commented Jun 11, 2015

oleg-nenashev Jun 11, 2015

jtnord Jun 11, 2015

oleg-nenashev commented Jun 11, 2015

stephenc Jun 11, 2015

tfennelly Jun 11, 2015

jtnord Jun 11, 2015

stephenc commented Jun 11, 2015

oleg-nenashev Jun 11, 2015

jtnord Jun 11, 2015

tfennelly commented Jun 11, 2015

tfennelly commented Jun 11, 2015

oleg-nenashev commented Jun 11, 2015

tfennelly commented Jun 11, 2015

jtnord commented Jun 11, 2015

jglick Jun 11, 2015

tfennelly commented Jun 11, 2015

jtnord commented Jun 11, 2015

oleg-nenashev Jun 11, 2015

oleg-nenashev Jun 11, 2015

jtnord Jun 11, 2015

oleg-nenashev commented Jun 11, 2015

recampbell commented Jun 11, 2015

jglick Jun 11, 2015

jtnord Jun 11, 2015

jglick commented Jun 11, 2015

[FIXED JENKINS-28843] Stored the unique value in the job configuration #1

[FIXED JENKINS-28843] Stored the unique value in the job configuration #1

Conversation

jtnord commented Jun 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented Jun 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenc commented Jun 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfennelly commented Jun 11, 2015

tfennelly commented Jun 11, 2015

oleg-nenashev commented Jun 11, 2015

tfennelly commented Jun 11, 2015

jtnord commented Jun 11, 2015

Choose a reason for hiding this comment

tfennelly commented Jun 11, 2015

jtnord commented Jun 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented Jun 11, 2015

recampbell commented Jun 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Jun 11, 2015