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.

Sign up for GitHub

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-21605] Further reduce number of recorded upstream causes #9248

Merged
merged 2 commits into from
May 10, 2024
Merged

[JENKINS-21605] Further reduce number of recorded upstream causes #9248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b276a4
Fix non-determinism in nested cause tests
cottsay May 1, 2024
625abf4
Re-work nested cause trimming algorithm
cottsay May 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions core/src/main/java/hudson/model/Cause.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,10 @@ public UpstreamCause(Run<?, ?> up) {
upstreamCauses = new ArrayList<>();
Set<String> traversed = new HashSet<>();
for (Cause c : up.getCauses()) {
if (traversed.size() >= MAX_LEAF) {
upstreamCauses.add(new DeeplyNestedUpstreamCause());
break;
}
upstreamCauses.add(trim(c, MAX_DEPTH, traversed));
}
}
Expand Down Expand Up @@ -239,14 +243,16 @@ public int hashCode() {
}
UpstreamCause uc = (UpstreamCause) c;
List<Cause> cs = new ArrayList<>();
if (depth > 0) {
if (traversed.add(uc.upstreamUrl + uc.upstreamBuild)) {
for (Cause c2 : uc.upstreamCauses) {
cs.add(trim(c2, depth - 1, traversed));
if (traversed.add(uc.upstreamUrl + uc.upstreamBuild)) {
for (Cause c2 : uc.upstreamCauses) {
if (depth <= 0 || traversed.size() >= MAX_LEAF) {
cs.add(new DeeplyNestedUpstreamCause());
break;
}
cs.add(trim(c2, depth - 1, traversed));
}
} else if (traversed.size() < MAX_LEAF) {
cs.add(new DeeplyNestedUpstreamCause());
} else {
traversed.add(uc.upstreamUrl + uc.upstreamBuild + '#' + traversed.size());
}
return new UpstreamCause(uc.upstreamProject, uc.upstreamBuild, uc.upstreamUrl, cs);
}
Expand Down
25 changes: 11 additions & 14 deletions test/src/test/java/hudson/model/CauseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ public class CauseTest {
FreeStyleProject b = j.createFreeStyleProject("b");
FreeStyleBuild early = null;
FreeStyleBuild last = null;
List<QueueTaskFuture<FreeStyleBuild>> futures = new ArrayList<>();
for (int i = 1; i <= 15; i++) {
last = recordFuture(b.scheduleBuild2(0, new Cause.UpstreamCause(recordFuture(a.scheduleBuild2(0, last == null ? null : new Cause.UpstreamCause(last)), futures).get())), futures).get();
last = j.waitForCompletion(a.scheduleBuild2(0, last == null ? null : new Cause.UpstreamCause(last)).get());
last = j.waitForCompletion(b.scheduleBuild2(0, new Cause.UpstreamCause(last)).get());
if (i == 5) {
early = last;
}
Expand All @@ -74,9 +74,6 @@ public class CauseTest {
assertTrue("keeps full history:\n" + buildXml, buildXml.contains("<upstreamBuild>1</upstreamBuild>"));
buildXml = new XmlFile(Run.XSTREAM, new File(last.getRootDir(), "build.xml")).asString();
assertFalse("too big:\n" + buildXml, buildXml.contains("<upstreamBuild>1</upstreamBuild>"));
for (QueueTaskFuture<FreeStyleBuild> future : futures) {
j.assertBuildStatusSuccess(j.waitForCompletion(future.waitForStart()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way we do require that all scheduled or started builds are completed (or canceled before they start) before the end of the test; otherwise there can be a mess cleaning up after the test’s temp dir. I think you are doing that here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 105 handles all of the builds in this test as well as the extra handling added by the looks of it

Copy link
Contributor Author

@cottsay cottsay May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For deeplyNestedCauses, every scheduled build is now passed to j.waitForCompletion immediately so this snippet is no longer necessary.

For broadlyNestedCauses, as @timja mentioned, line 105 will clean up any lingering scheduled builds, however every single build was either:

  1. scheduled with a quiet period of 1 and recorded in futures
  2. scheduled with a quiet period of 0 and immediately awaited for completion

Additionally, each case of (1) is followed by an instance of (2) so the aforementioned cleanup on line 105 should never actually do anything. Though some builds were scheduled without immediately awaiting, there is always a subsequent scheduling of that same job that does immediately await.

I left the cleanup only as a backstop.

}
}

@Issue("JENKINS-15747")
Expand All @@ -88,16 +85,16 @@ public class CauseTest {
Run<?, ?> last = null;
for (int i = 1; i <= 10; i++) {
Cause cause = last == null ? null : new Cause.UpstreamCause(last);
QueueTaskFuture<FreeStyleBuild> next1 = recordFuture(a.scheduleBuild2(0, cause), futures);
recordFuture(a.scheduleBuild2(0, cause), futures);
cause = new Cause.UpstreamCause(next1.get());
QueueTaskFuture<FreeStyleBuild> next2 = recordFuture(b.scheduleBuild2(0, cause), futures);
recordFuture(b.scheduleBuild2(0, cause), futures);
cause = new Cause.UpstreamCause(next2.get());
QueueTaskFuture<FreeStyleBuild> next3 = recordFuture(c.scheduleBuild2(0, cause), futures);
recordFuture(c.scheduleBuild2(0, cause), futures);
last = next3.get();
last = j.waitForCompletion(a.scheduleBuild2(0, cause).get());
recordFuture(b.scheduleBuild2(1, cause), futures);
cause = new Cause.UpstreamCause(last);
last = j.waitForCompletion(b.scheduleBuild2(0, cause).get());
recordFuture(c.scheduleBuild2(1, cause), futures);
cause = new Cause.UpstreamCause(last);
last = j.waitForCompletion(c.scheduleBuild2(0, cause).get());
recordFuture(a.scheduleBuild2(1, cause), futures);
}
last = j.waitForCompletion(a.scheduleBuild2(0, new Cause.UpstreamCause(last)).get());
int count = new XmlFile(Run.XSTREAM, new File(last.getRootDir(), "build.xml")).asString().split(Pattern.quote("<hudson.model.Cause_-UpstreamCause")).length;
assertFalse("too big at " + count, count > 100);
//j.interactiveBreak();
Expand Down
Loading