From db8df4886c09901cc1dca619c0f16b1b0bc1afe6 Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Thu, 24 Jan 2019 10:11:23 +0100 Subject: [PATCH] Add Gerrit config to disable mergeability bit in ChangeInfo Computing mergeability of open changes becomes very expensive for hosts that have fast-moving branches and a lot of open changes. This commit scales down this problem by adding a config to remove the bit from ChangeInfo. We will consult the mailing list to see if we can remove this bit from ChangeInfo and the change index completely or add a new MERGEABLE list option. Change-Id: I28e90e17abaf369d03ff9d3c0d491903ecf30bf9 --- Documentation/config-gerrit.txt | 9 +++++++++ Documentation/rest-api-changes.txt | 8 +++++++- .../gerrit/extensions/client/ListChangesOption.java | 1 + java/com/google/gerrit/server/change/ChangeJson.java | 8 +++++++- .../google/gerrit/acceptance/api/change/ChangeIT.java | 8 ++++++++ 5 files changed, 32 insertions(+), 2 deletions(-) diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 066d2ffbcc..f5a226d065 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -1214,6 +1214,15 @@ performance improvements by reducing the number of refs in the repo. + Default is true. +[[change.api.excludeMergeableInChangeInfo]]change.api.excludeMergeableInChangeInfo:: ++ +If true, the mergeability bit in +link:rest-api-changes.html#change-info[ChangeInfo] will never be set. It can +be requested separately through the +link:rest-api-changes.html#get-mergeable[get-mergeable] endpoint. ++ +Default is false. + [[change.showAssigneeInChangesTable]]change.showAssigneeInChangesTable:: + Show assignee field in changes table. If set to false, assignees will diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index ea33d02e3a..935e39b9d7 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -309,6 +309,10 @@ default. Optional fields are: * `SKIP_MERGEABLE`: skip the `mergeable` field in link:#change-info[ChangeInfo]. For fast moving projects, this field must be recomputed often, which is slow for projects with big trees. ++ +This option is deprecated. In a future release, `mergeable` will not be +populated in link:#change-info[ChangeInfo]. It can be requested separately +by calling the link:#get-mergeable[get-mergeable] endpoint. -- [[submittable]] @@ -5765,7 +5769,9 @@ Not set for merged changes. |`mergeable` |optional| Whether the change is mergeable. + Not set for merged changes, if the change has not yet been tested, or -if the link:#skip_mergeable[skip_mergeable] option is set. +if the link:#skip_mergeable[skip_mergeable] option is set or when +link:config-gerrit.html#change.api.excludeMergeableInChangeInfo[change.api.excludeMergeableInChangeInfo] +is set. |`submittable` |optional| Whether the change has been approved by the project submit rules. + Only set if link:#submittable[requested]. diff --git a/java/com/google/gerrit/extensions/client/ListChangesOption.java b/java/com/google/gerrit/extensions/client/ListChangesOption.java index ffc5029f70..a3a3ccf4a2 100644 --- a/java/com/google/gerrit/extensions/client/ListChangesOption.java +++ b/java/com/google/gerrit/extensions/client/ListChangesOption.java @@ -78,6 +78,7 @@ public enum ListChangesOption { TRACKING_IDS(21), /** Skip mergeability data */ + @Deprecated SKIP_MERGEABLE(22); private final int value; diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java index 28f9618c71..fa38139492 100644 --- a/java/com/google/gerrit/server/change/ChangeJson.java +++ b/java/com/google/gerrit/server/change/ChangeJson.java @@ -83,6 +83,7 @@ import com.google.gerrit.server.ReviewerStatusUpdate; import com.google.gerrit.server.StarredChangesUtil; import com.google.gerrit.server.account.AccountInfoComparator; import com.google.gerrit.server.account.AccountLoader; +import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.TrackingFooters; import com.google.gerrit.server.index.change.ChangeField; import com.google.gerrit.server.notedb.ChangeNotes; @@ -111,6 +112,7 @@ import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.function.Supplier; +import org.eclipse.jgit.lib.Config; /** * Produces {@link ChangeInfo} (which is serialized to JSON afterwards) from {@link ChangeData}. @@ -216,6 +218,7 @@ public class ChangeJson { private final Metrics metrics; private final RevisionJson revisionJson; private final Optional pluginDefinedAttributesFactory; + private final boolean excludeMergeableInChangeInfo; private final boolean lazyLoad; private AccountLoader accountLoader; @@ -236,6 +239,7 @@ public class ChangeJson { TrackingFooters trackingFooters, Metrics metrics, RevisionJson.Factory revisionJsonFactory, + @GerritServerConfig Config cfg, @Assisted Iterable options, @Assisted Optional pluginDefinedAttributesFactory) { this.userProvider = user; @@ -252,6 +256,8 @@ public class ChangeJson { this.metrics = metrics; this.revisionJson = revisionJsonFactory.create(options); this.options = Sets.immutableEnumSet(options); + this.excludeMergeableInChangeInfo = + cfg.getBoolean("change", "api", "excludeMergeableInChangeInfo", false); this.lazyLoad = containsAnyOf(this.options, REQUIRE_LAZY_LOAD); this.pluginDefinedAttributesFactory = pluginDefinedAttributesFactory; @@ -509,7 +515,7 @@ public class ChangeJson { if (str.isOk()) { out.submitType = str.type; } - if (!has(SKIP_MERGEABLE)) { + if (!excludeMergeableInChangeInfo && !has(SKIP_MERGEABLE)) { out.mergeable = cd.isMergeable(); } if (has(SUBMITTABLE)) { diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java index 31de4cfff8..c54c410d46 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -252,6 +252,14 @@ public class ChangeIT extends AbstractDaemonTest { assertThat(c.mergeable).isTrue(); } + @Test + @GerritConfig(name = "change.api.excludeMergeableInChangeInfo", value = "true") + public void excludeMergeableInChangeInfo() throws Exception { + PushOneCommit.Result r = createChange(); + ChangeInfo c = gApi.changes().id(r.getChangeId()).get(); + assertThat(c.mergeable).isNull(); + } + @Test public void setPrivateByOwner() throws Exception { TestRepository userRepo = cloneProject(project, user);