JENKINS-42319: Fix broken compareTo method of Run #2762

Merged
merged 2 commits into from Mar 18, 2017

Conversation

3 participants
@Jimilian
Contributor

Jimilian commented Feb 25, 2017

Fix broken some time ago method compareTo of Run object.
Current implementation relies on fact that users always compare run objects with same parent. But this is not always the case.

JENKINS-42319: Fix broken compareTo method of Run
Previous implementation relied on fact that users always compare run
objects with same parent. But this is not always the case.
So, it was not safe to put runs from different parents to TreeSet, i.e.
@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Feb 25, 2017

Member

This looks like a possibly significant performance regression for no clear benefit, and the issue does not explain why this is even needed. It also behaves as documented, and the PR does not update the documentation.

Member

daniel-beck commented Feb 25, 2017

This looks like a possibly significant performance regression for no clear benefit, and the issue does not explain why this is even needed. It also behaves as documented, and the PR does not update the documentation.

@Jimilian

This comment has been minimized.

Show comment
Hide comment
@Jimilian

Jimilian Feb 25, 2017

Contributor

Right now it's not possible to put to one TreeSet builds with same number from different projects. Because compareTo returns 0 even for non-equal objects. And according TreeSet implementation it always overrides equal objects - so, only last build will be present.
It breaks partially some features in some plugins. I.e. https://github.com/jenkinsci/build-failure-analyzer-plugin/blob/10f1536db3c9c94a8471dd9db1d63e974a1c53fc/src/main/java/com/sonyericsson/jenkins/plugins/bfa/model/FailureCauseBuildAction.java#L345 (of course, it must be fixed in plugin in this case as well).

Contributor

Jimilian commented Feb 25, 2017

Right now it's not possible to put to one TreeSet builds with same number from different projects. Because compareTo returns 0 even for non-equal objects. And according TreeSet implementation it always overrides equal objects - so, only last build will be present.
It breaks partially some features in some plugins. I.e. https://github.com/jenkinsci/build-failure-analyzer-plugin/blob/10f1536db3c9c94a8471dd9db1d63e974a1c53fc/src/main/java/com/sonyericsson/jenkins/plugins/bfa/model/FailureCauseBuildAction.java#L345 (of course, it must be fixed in plugin in this case as well).

- return this.number - that.number;
+ final int res = this.number - that.number;
+ if (res == 0)
+ return this.getParent().getFullName().compareTo(that.getParent().getFullName());

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Feb 25, 2017

Member

Do we really need to compare full names in this case? Maybe just a comparison of instances is enough (though there may be corner cases in such case)

@oleg-nenashev

oleg-nenashev Feb 25, 2017

Member

Do we really need to compare full names in this case? Maybe just a comparison of instances is enough (though there may be corner cases in such case)

This comment has been minimized.

@Jimilian

Jimilian Feb 25, 2017

Contributor

It can be done. My main point - it's not safe to have not-aligned compareTo and equals. Also I can make Job Comparable, and put that logic there.

@Jimilian

Jimilian Feb 25, 2017

Contributor

It can be done. My main point - it's not safe to have not-aligned compareTo and equals. Also I can make Job Comparable, and put that logic there.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Feb 25, 2017

Member

@Jimilian

Right now it's not possible to put to one TreeSet builds with same number from different projects.

Yes, I understand. But in what case is this actually needed? This has no clear benefit to me but a risk for breaking something that currently works.

Member

daniel-beck commented Feb 25, 2017

@Jimilian

Right now it's not possible to put to one TreeSet builds with same number from different projects.

Yes, I understand. But in what case is this actually needed? This has no clear benefit to me but a risk for breaking something that currently works.

@Jimilian

This comment has been minimized.

Show comment
Hide comment
@Jimilian

Jimilian Feb 25, 2017

Contributor

@daniel-beck I don't see how it could break anything. According JavaDoc

It is strongly recommended, but not strictly required that (x.compareTo(y)==0) == (x.equals(y)). Generally speaking, any class that implements the Comparable interface and violates this condition should clearly indicate this fact. The recommended language is "Note: this class has a natural ordering that is inconsistent with equals."

And right now (x.compareTo(y)==0) != (x.equals(y)) and I didn't find information about that in Jenkins API documents or tests. So, if Jenkins maintainers don't want to align compareTo with equals, they should at least mention in docs that it's not safe to put builds from different parents to sorted set or sorted map and explicitly defined comparator is needed. And create a test that verifies this behavior. Right now it's (from my perspective) very hidden bug.

But in what case is this actually needed?

In case of Build Failure Analyzer Plugin you want to collect failed builds from child jobs and keep it sorted. To show nice view at the end of execution. (But I can only assume that logic - code was introduced 3 years ago)

Contributor

Jimilian commented Feb 25, 2017

@daniel-beck I don't see how it could break anything. According JavaDoc

It is strongly recommended, but not strictly required that (x.compareTo(y)==0) == (x.equals(y)). Generally speaking, any class that implements the Comparable interface and violates this condition should clearly indicate this fact. The recommended language is "Note: this class has a natural ordering that is inconsistent with equals."

And right now (x.compareTo(y)==0) != (x.equals(y)) and I didn't find information about that in Jenkins API documents or tests. So, if Jenkins maintainers don't want to align compareTo with equals, they should at least mention in docs that it's not safe to put builds from different parents to sorted set or sorted map and explicitly defined comparator is needed. And create a test that verifies this behavior. Right now it's (from my perspective) very hidden bug.

But in what case is this actually needed?

In case of Build Failure Analyzer Plugin you want to collect failed builds from child jobs and keep it sorted. To show nice view at the end of execution. (But I can only assume that logic - code was introduced 3 years ago)

@Jimilian Jimilian referenced this pull request in jenkinsci/build-failure-analyzer-plugin Feb 25, 2017

Merged

Fix collecting downstream builds #65

@oleg-nenashev

Weak 👍 from me. I am convinced the approach is formally correct, but we need to make sure that the performance in other core logic does not suffer significantly.

For such TreeSets we need to set up a custom lightweight comparator to retain the original behavior

@oleg-nenashev oleg-nenashev requested a review from daniel-beck Mar 11, 2017

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Mar 11, 2017

Member

@Jimilian WDYT about my performance-related comment above?

Member

oleg-nenashev commented Mar 11, 2017

@Jimilian WDYT about my performance-related comment above?

@Jimilian

This comment has been minimized.

Show comment
Hide comment
@Jimilian

Jimilian Mar 11, 2017

Contributor

@oleg-nenashev I think we can use hashCode of parents instead of fullName, but I doubt that current implementation of PR may affect performance. Because it should be really rare case then second computation is needed + I didn't find any usage of TreeSet<Run> or TreeSet<AbstractBuild> in JenkinsCore.

Contributor

Jimilian commented Mar 11, 2017

@oleg-nenashev I think we can use hashCode of parents instead of fullName, but I doubt that current implementation of PR may affect performance. Because it should be really rare case then second computation is needed + I didn't find any usage of TreeSet<Run> or TreeSet<AbstractBuild> in JenkinsCore.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Mar 16, 2017

Member

I doubt that current implementation of PR may affect performance. Because it should be really rare case then second computation is needed

@Jimilian Good point, I must be blind -- didn't see your second commit.

👍 from me.

Member

daniel-beck commented Mar 16, 2017

I doubt that current implementation of PR may affect performance. Because it should be really rare case then second computation is needed

@Jimilian Good point, I must be blind -- didn't see your second commit.

👍 from me.

@oleg-nenashev oleg-nenashev merged commit e8efc96 into jenkinsci:master Mar 18, 2017

2 checks passed

Jenkins This pull request looks good
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Mar 18, 2017

Member

Changelog entry: Default compareTo() method implementation for Runs now takes the parent Item group into account

Member

oleg-nenashev commented Mar 18, 2017

Changelog entry: Default compareTo() method implementation for Runs now takes the parent Item group into account

@Jimilian Jimilian deleted the Jimilian:fix_compareTo_of_run branch Mar 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment