Merge job started messages based on aggregation interval #313

Jimilian · 2017-05-22T09:59:26Z

The main goal of this PR is to reduce number of notification user receives from Gerrit Trigger Plugin.
It allows users to specify "aggregation interval", so all jobs that were started in this interval would merge buildStarted message in one.
I.e. you do aggregation interval 10 seconds, after that 3 builds are started:

1. build1 at 00:00
2. build2 at 00:09
3. build3 at 00:11

PR will merge build1 and build2, so, plugin will send

Build started:
build1_url
build2_url

After that build3 will be sent separately:

Build started
build3_url

We already are using this feature in production (since previous week), looks good.

rsandell

Halfway through.

rsandell · 2017-06-09T10:38:23Z

...ava/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/GerritMessageProvider.java

@@ -70,7 +70,7 @@ public String getBuildStartedMessage(Run build) {
     */
    @Deprecated
    public String getBuildStartedMessage(AbstractBuild build) {
-        if (thisOverrides("getBuildStartedMessage", Run.class)) {
+        if (thisOverrides("getBuildsStartedMessage", Run.class)) {


This looks unrelated and wrong.

Or maybe not when reading the rest of the PR, but it does still look wrong, since the check is if a specific method is overridden and in that case that method should be called, but the method checked for is not the method getting called.

rsandell · 2017-06-09T14:03:02Z

...in/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpander.java

+                minVerified,
+                Notify.ALL.name());
+
+        parameters.put("BUILDURL", "");


I guess it would have been better if we could change the command templates and separate the message part with the actual command part. And then in case of a compound message like this the message part would be just two messages correctly expanded with BUILDURL and all and added to the one command.
This seems a bit hacky to me as it omits certain variables in case an admin has changed the message format.

I did it, because it was already like that implemented for Completed message (it just ignored BUILDURL) completely :). I don't want to touch template part in my PR, but I can add more details in help section.

rsandell · 2017-06-09T14:04:16Z

...in/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpander.java

+            if (entry == null) {
+                continue;
+            }
+            runs.add(entry.getBuild());


getBuild() can also be null if jobs have been triggered but not yet started.
Even though you take care of potential null entries below it is safest if we can omit them all together in the source in case some other caller isn't that foreseeing.

rsandell · 2017-06-09T14:07:31Z

...in/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpander.java

     * @param listener listener
     * @param event event
     * @param stats stats
     * @return the message for the build started command.
     */
-    public String getBuildStartedMessage(Run build, TaskListener listener, ChangeBasedEvent event,


I'm still nervous of removing signatures like this, even though it should be safe in this context as only careless plugina uthors would rely on this code.

If you want, I can restore this one and mark like @Depracated

neonman63 · 2017-07-03T13:31:08Z

Hello, we need same behavior not only for "started message", but with entire build triggers, because with topic upload to the gerrit had triggers many jobs with same changesets. May be I can open a feature request in JIRA?

Jimilian · 2017-07-20T21:19:49Z

@neonman63, what do you mean by "entire build triggers"? In our case we received two types of messages:

job started (once per job)
final aggregated result

neonman63 · 2017-07-25T09:32:53Z

We push code with repo tool in 4 project with topic and this triggers 4 jobs. In each job we use python-script, which get code in each project by topic (rest api requests to gerrit for getting depended changesets). Finally, it sets 4 +1 votes (or -1) in each changesets with same topic and with same build results.

Jimilian · 2017-10-25T16:06:29Z

@rsandell can we follow up on it? Our devs are happy to have less messages in Gerrit.

rsandell · 2017-10-26T09:04:36Z

@Jimilian yea, the PR is just so big I have a hard time finding gaps in my day big enough to review it :'(

Jimilian · 2018-01-25T14:34:35Z

@neonman63 btw, there is a new feature "abort same topic".

panicking · 2018-02-25T07:27:07Z

@Jimilian Abort topic insist on the same git repo right? I mean if you have multiple patches on different repo with the same topic multiple job are anyway triggered or it's a way to define one job?

Jimilian · 2018-02-26T08:38:22Z

@panicking current implementation aborts all executions regardless project, because it assumes that topic used at moment of start is out-dated already.
In my company we are using Gerrit Trigger Plugin with "native" topic support that triggers all jobs for all changes with respected context. I.e.

topic contains changes in projectA and projectB.
there is three jobs: jobA (configured to be triggered on change in projectA), jobA (projectB) and jobAB (both projects)
user pushes first change (in projectA): only jobA and jobAB are triggered
user pushes second change (in projectB): both executions are aborted, three jobs are triggered (jobA, jobAB, jobB)
once all jobs are completed user receives feedback from jobA/jobAB for projectA and from jobB/jobAB for projectB

Abort same topic is only one part of this big picture.

panicking · 2018-03-12T23:06:43Z

@Jimilian So we can't use this plugin as it is to run one job for same topic but you need to have a modified version of it

If multiple jobs are configured to be triggered by GTP it can be too annoyning to get feedback/notification from each of them.

rsandell reviewed Jun 9, 2017

View reviewed changes

Jimilian force-pushed the merge_job_started_messages branch from 1b0d3e2 to f3ed5d0 Compare September 4, 2017 15:44

Jimilian force-pushed the merge_job_started_messages branch 2 times, most recently from 1edbeab to 5a07dfe Compare October 26, 2017 08:44

Jimilian force-pushed the merge_job_started_messages branch from 5a07dfe to a080c07 Compare December 7, 2017 08:18

Aggregate buildStarted messages based on interval provided by user

4bdda1f

If multiple jobs are configured to be triggered by GTP it can be too annoyning to get feedback/notification from each of them.

Jimilian force-pushed the merge_job_started_messages branch from a080c07 to 4bdda1f Compare May 15, 2018 11:35

Jimilian mentioned this pull request May 15, 2018

Fix race condtion in ssh key generation #362

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge job started messages based on aggregation interval #313

Merge job started messages based on aggregation interval #313

Jimilian commented May 22, 2017

rsandell left a comment

rsandell Jun 9, 2017

Jimilian Jul 3, 2017

rsandell Jun 9, 2017

Jimilian Jul 3, 2017

rsandell Jun 9, 2017

Jimilian Jul 3, 2017

rsandell Jun 9, 2017

Jimilian Jul 3, 2017

neonman63 commented Jul 3, 2017

Jimilian commented Jul 20, 2017

neonman63 commented Jul 25, 2017

Jimilian commented Oct 25, 2017

rsandell commented Oct 26, 2017

Jimilian commented Jan 25, 2018

panicking commented Feb 25, 2018

Jimilian commented Feb 26, 2018

panicking commented Mar 12, 2018

Merge job started messages based on aggregation interval #313

Are you sure you want to change the base?

Merge job started messages based on aggregation interval #313

Conversation

Jimilian commented May 22, 2017

rsandell left a 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

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

neonman63 commented Jul 3, 2017

Jimilian commented Jul 20, 2017

neonman63 commented Jul 25, 2017

Jimilian commented Oct 25, 2017

rsandell commented Oct 26, 2017

Jimilian commented Jan 25, 2018

panicking commented Feb 25, 2018

Jimilian commented Feb 26, 2018

panicking commented Mar 12, 2018