Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-17626] Count the number a build rescheduled precisely #19

Merged

Conversation

Projects
None yet
4 participants
@ikedam
Copy link
Member

commented Sep 5, 2015

JENKINS-17626

Naginator plugin 0.15 counts how many times the build is rescheduled by counting how many builds with NaginatorAction in the build history.
This results in wrong counts if there are more than one series of rescheduled builds, such as builds with different parameters.

This change stores the retrying counts in NaginatorAction and increments them when rescheduling new builds.

ikedam added some commits Sep 2, 2015

@ikedam

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2015

This is a reproduction of JENKINS-17626.

https://jenkins.ci.cloudbees.com/job/plugins/job/naginator-plugin/org.jenkins-ci.plugins$naginator/75/testReport/junit/com.chikli.hudson.plugin.naginator/NaginatorListenerTest/testCountScheduleIndependently/

junit.framework.AssertionFailedError: expected:<3> but was:<2>
    at junit.framework.Assert.fail(Assert.java:57)
    at junit.framework.Assert.failNotEquals(Assert.java:329)
    at junit.framework.Assert.assertEquals(Assert.java:78)
    at junit.framework.Assert.assertEquals(Assert.java:234)
    at junit.framework.Assert.assertEquals(Assert.java:241)
    at junit.framework.TestCase.assertEquals(TestCase.java:409)
    at com.chikli.hudson.plugin.naginator.NaginatorListenerTest.testCountScheduleIndependently(NaginatorListenerTest.java:266)

That's not the problem exactly same to described in JENKINS-17626, however, their causes are same.
The following commit will fix this issue.

@ikedam

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2015

Actually, it might better to store the retry count in NaginatorCause.
I decided not to use NaginatorCause as I found AbstractProject#scheduleBuild(int, Cause, Action ...) behaves strange when it is called with CauseAction. It results a build with two CauseActions.

  • build.getCause(NaginatorCause.class) doesn't work as expected in that case.
  • build.getAction(CauseAction.class) returns only the first CauseAction.
  • It might be fixed by passing null for Cause for scheduleBuild and adding NaginatorCause to CauseAction manually. But that requires many changes in codes (e.g. filtering out old NaginatorCause for causes passed to reschedule builds) and I worried unexpected effects with those changes.
public boolean scheduleMatrixBuild(AbstractBuild<?, ?> build, List<Combination> combinations, int n) {
NaginatorMatrixAction nma = new NaginatorMatrixAction();
return scheduleMatrixBuild(build, combinations, n, NaginatorListener.calculateRetryCount(build));
}

This comment has been minimized.

Copy link
@ikedam

ikedam Sep 5, 2015

Author Member

I think there's no reason to expose this method.
I marked this deprecated and provide no alternate public method.

static boolean scheduleBuild(final AbstractBuild<?, ?> build, final int delay) {
return scheduleBuild(build, delay, new NaginatorAction());
static boolean scheduleBuild(final AbstractBuild<?, ?> build, final int delay, int retryCount) {
return scheduleBuild(build, delay, new NaginatorAction(retryCount));

This comment has been minimized.

Copy link
@ikedam

ikedam Sep 5, 2015

Author Member

I didn't preserve compatibilities for package-scoped methods.
Let me know if I should aprovide a compatible method.

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Sep 5, 2015

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@KostyaSha

This comment has been minimized.

Copy link
Member

commented Sep 7, 2015

LGTM

@ndeloof do you maintain this plugin?

ndeloof added a commit that referenced this pull request Sep 7, 2015

Merge pull request #19 from ikedam/feature/JENKINS-17626_FixRetryCount
[JENKINS-17626] Count the number a build rescheduled precisely

@ndeloof ndeloof merged commit 6b07b08 into jenkinsci:master Sep 7, 2015

1 check passed

Jenkins This pull request looks good
Details
@KostyaSha

This comment has been minimized.

Copy link
Member

commented Sep 8, 2015

@ndeloof and what next?

@ndeloof

This comment has been minimized.

Copy link
Member

commented Sep 8, 2015

anybody in community can run a release. Do you volunteer ?

@KostyaSha

This comment has been minimized.

Copy link
Member

commented Sep 8, 2015

@ndeloof @ikedam is extending plugins to get integration between. Please assist him. Also matrix support is under question, please handle it.

@KostyaSha

This comment has been minimized.

Copy link
Member

commented Sep 16, 2015

@ikedam do you need alpha2 ?

@ikedam

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2015

Thanks, but I don't need that.
This change doesn't affect our development of build-timeout.

I plan another fix (fixing improper dependency to matrix-project), and I want a new release after that fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.