Skip to content
Permalink
Browse files
[FIXED JENKINS-33467] Do not store redundant copies of Cause in Cause…
…Action.

(cherry picked from commit 6ae54ad)
  • Loading branch information
jglick authored and olivergondza committed Apr 25, 2016
1 parent 35cbc5c commit 916e759f576fd9aec7eb563f71f6541ceb37f641
Showing with 55 additions and 28 deletions.
  1. +48 −22 core/src/main/java/hudson/model/CauseAction.java
  2. +7 −6 test/src/test/java/hudson/model/QueueTest.java
@@ -34,6 +34,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
@@ -47,35 +48,55 @@
@Deprecated
// there can be multiple causes, so this is deprecated
private transient Cause cause;

private List<Cause> causes = new ArrayList<Cause>();

/** @deprecated JENKINS-33467 inefficient */
@Deprecated
private transient List<Cause> causes;

private Map<Cause,Integer> causeBag = new LinkedHashMap<>();

public CauseAction(Cause c) {
this.causes.add(c);
this.causeBag.put(c, 1);
}

private void addCause(Cause c) {
synchronized (causeBag) {
Integer cnt = causeBag.get(c);
causeBag.put(c, cnt == null ? 1 : cnt + 1);
}
}
private void addCauses(Collection<? extends Cause> causes) {
for (Cause cause : causes) {
addCause(cause);
}
}

public CauseAction(Cause... c) {
this(Arrays.asList(c));
}

public CauseAction(Collection<? extends Cause> causes) {
this.causes.addAll(causes);
addCauses(causes);
}

public CauseAction(CauseAction ca) {
this.causes.addAll(ca.causes);
addCauses(ca.getCauses());
}

@Exported(visibility=2)
public List<Cause> getCauses() {
return causes;
List<Cause> r = new ArrayList<>();
for (Map.Entry<Cause,Integer> entry : causeBag.entrySet()) {
r.addAll(Collections.nCopies(entry.getValue(), entry.getKey()));
}
return r;
}

/**
* Finds the cause of the specific type.
*/
public <T extends Cause> T findCause(Class<T> type) {
for (Cause c : causes)
for (Cause c : causeBag.keySet())
if (type.isInstance(c))
return type.cast(c);
return null;
@@ -99,14 +120,7 @@ public String getUrlName() {
* @return Map of Cause to number of occurrences of that Cause
*/
public Map<Cause,Integer> getCauseCounts() {
Map<Cause,Integer> result = new LinkedHashMap<Cause,Integer>();
for (Cause c : causes) {
if (c != null) {
Integer i = result.get(c);
result.put(c, i == null ? 1 : i.intValue() + 1);
}
}
return result;
return Collections.unmodifiableMap(causeBag);
}

/**
@@ -115,12 +129,14 @@ public String getUrlName() {
*/
@Deprecated
public String getShortDescription() {
if(causes.isEmpty()) return "N/A";
return causes.get(0).getShortDescription();
if (causeBag.isEmpty()) {
return "N/A";
}
return causeBag.keySet().iterator().next().getShortDescription();
}

@Override public void onLoad(Run<?,?> owner) {
for (Cause c : causes) {
for (Cause c : causeBag.keySet()) {
if (c != null) {
c.onLoad(owner);
}
@@ -131,7 +147,7 @@ public String getShortDescription() {
* When hooked up to build, notify {@link Cause}s.
*/
@Override public void onAttached(Run<?,?> owner) {
for (Cause c : causes) {
for (Cause c : causeBag.keySet()) {
if (c != null) {
c.onAddedTo(owner);
}
@@ -141,7 +157,7 @@ public String getShortDescription() {
public void foldIntoExisting(hudson.model.Queue.Item item, Task owner, List<Action> otherActions) {
CauseAction existing = item.getAction(CauseAction.class);
if (existing!=null) {
existing.causes.addAll(this.causes);
existing.addCauses(getCauses());
return;
}
// no CauseAction found, so add a copy of this one
@@ -153,9 +169,19 @@ public void foldIntoExisting(hudson.model.Queue.Item item, Task owner, List<Acti
@Override protected void callback(CauseAction ca, UnmarshallingContext context) {
// if we are being read in from an older version
if (ca.cause != null) {
if (ca.causes == null) ca.causes = new ArrayList<Cause>();
ca.causes.add(ca.cause);
if (ca.causeBag == null) {
ca.causeBag = new LinkedHashMap<>();
}
ca.addCause(ca.cause);
OldDataMonitor.report(context, "1.288");
ca.cause = null;
} else if (ca.causes != null) {
if (ca.causeBag == null) {
ca.causeBag = new LinkedHashMap<>();
}
ca.addCauses(ca.causes);
OldDataMonitor.report(context, "1.653");

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck May 9, 2016

Member

@jglick @olivergondza We need to be careful when backporting something like this. Do we want to change this still for 1.651.2?

This comment has been minimized.

Copy link
@aheritier

aheritier May 9, 2016

Member

It is really misleading like this. The Manage Old Data is proposing to you to upgrade data for 1.653 while you are on 1.651.2
screenshot 2016-05-09 10 15 03

This comment has been minimized.

Copy link
@olivergondza

olivergondza May 9, 2016

Member

Replacing this with 1.651.2 should do if I am not mistaken. Though, I prefer reverting as this is not a bugfix. Thanks for spotting that.

This comment has been minimized.

Copy link
@olivergondza

olivergondza May 9, 2016

Member

Since the release is scheduled in 2 days this targets .3: https://issues.jenkins-ci.org/browse/JENKINS-34695

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck May 24, 2016

Member

https://issues.jenkins-ci.org/browse/JENKINS-35087 notes a possible problem upgrading, which I believe to be only a perceived problem caused by lazy build record loading.

ca.causes = null;
}
}
}
@@ -305,6 +305,7 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
}
}

@Issue("JENKINS-33467")
@Test public void foldableCauseAction() throws Exception {
final OneShotEvent buildStarted = new OneShotEvent();
final OneShotEvent buildShouldComplete = new OneShotEvent();
@@ -348,14 +349,13 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
StringBuilder causes = new StringBuilder();
for (Cause c : ca.getCauses()) causes.append(c.getShortDescription() + "\n");
assertEquals("Build causes should have all items, even duplicates",
"Started by user SYSTEM\nStarted by an SCM change\n"
+ "Started by user SYSTEM\nStarted by timer\n"
"Started by user SYSTEM\nStarted by user SYSTEM\n"
+ "Started by an SCM change\nStarted by an SCM change\nStarted by an SCM change\n"
+ "Started by timer\nStarted by timer\n"
+ "Started by remote host 1.2.3.4 with note: test\n"
+ "Started by remote host 4.3.2.1 with note: test\n"
+ "Started by an SCM change\n"
+ "Started by remote host 1.2.3.4 with note: test\n"
+ "Started by remote host 1.2.3.4 with note: foo\n"
+ "Started by an SCM change\nStarted by timer\n",
+ "Started by remote host 4.3.2.1 with note: test\n"
+ "Started by remote host 1.2.3.4 with note: foo\n",
causes.toString());

// View for build should group duplicates
@@ -369,6 +369,7 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
+ "Started by remote host 1.2.3.4 with note: test (2 times) "
+ "Started by remote host 4.3.2.1 with note: test "
+ "Started by remote host 1.2.3.4 with note: foo"));
System.out.println(new XmlFile(new File(build.getRootDir(), "build.xml")).asString());
}

@Issue("JENKINS-8790")

0 comments on commit 916e759

Please sign in to comment.