Clean up configs around mergeability behavior
Gerrit now lets admins decide if they want to optimize speed of operations by omitting 'mergeable' from the change index and ChangeInfo in API responses. Omitting 'mergeable' from the change index also means that Gerrit will not reindex all open changes when the target ref advances. This operation is especially costly on repositories with a large number of open changes. Prior to this commit, this was controlled by the following switches: gerrit.config: - change.api.excludeChangeInMergeable: Skip 'mergable' in API responses. - index.change.indexMergeable: Skip mergeable when indexing. ListChangeOptions: - SKIP_MERGEABLE: Skip 'mergeable' on-demand in the query at hand. These three knobs make it hard to get this right. In addition, they allow for potentially dangerous behavior when change.api.excludeChangeInMergeable=false and index.change.indexMergeable=false since this forces Gerrit to recompute the 'mergeability' bit potentially many times when returning query responses. This commit cleans up this landscape by deprecating the SKIP_MERGEABLE change list option. The idea is that Gerrit admins need to be prescriptive about the performance behavior of their systems. So the behavior should be controlled system-wide instead of per-request. The two Gerrit configs are unified into an enum that allows us to get rid of the dangerous behavior. The possible states of change.mergeabilityComputationBehavior are: - API_REF_UPDATED_AND_CHANGE_REINDEX - REF_UPDATED_AND_CHANGE_REINDEX - NEVER Change-Id: I01c87f6f6d731121b51301f403fa92de3e4c72f7
This commit is contained in:
@@ -1171,15 +1171,6 @@ explicitly.
|
||||
+
|
||||
Default is `ALL`.
|
||||
|
||||
[[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.cacheAutomerge]]change.cacheAutomerge::
|
||||
+
|
||||
When reviewing diff commits, the left-hand side shows the output of the
|
||||
@@ -1232,6 +1223,34 @@ The following operations are allowed even when a change is at the limit:
|
||||
|
||||
By default 1000.
|
||||
|
||||
[[change.mergeabilityComputationBehavior]]change.mergeabilityComputationBehavior::
|
||||
+
|
||||
This setting determines when Gerrit computes if a change is mergeable or not.
|
||||
This computation is expensive, especially when the repository is large or when
|
||||
there are many open changes.
|
||||
+
|
||||
This config can have the following states:
|
||||
+
|
||||
* `API_REF_UPDATED_AND_CHANGE_REINDEX`: Gerrit indexes `mergeability` enabling
|
||||
the `is:mergeable` predicate in change search and allowing fast retrieval of
|
||||
this bit in query responses. Gerrit will always serve `mergeable` in
|
||||
link:rest-api-changes.html#change-info[ChangeInfo] objects.
|
||||
Gerrit will reindex all open changes when the target ref advances (expensive).
|
||||
* `REF_UPDATED_AND_CHANGE_REINDEX`: Gerrit indexes `mergeability` enabling the
|
||||
`is:mergeable` predicate in change search and allowing fast retrieval of this
|
||||
bit in query responses. Gerrit will never serve `mergeable` in
|
||||
link:rest-api-changes.html#change-info[ChangeInfo]
|
||||
objects. This state can be a final state for instances that only want to
|
||||
optimize the read path, but not the write path for speed or serve as an
|
||||
intermediary step for instances that want to optimize both and need to migrate
|
||||
callers of their API.
|
||||
Gerrit will reindex all open changes when the target ref advances (expensive).
|
||||
* `NEVER`: Gerrit does not index `mergeable`, so `is:mergeable` is disabled as
|
||||
query operator. Gerrit does not serve `mergeable` in
|
||||
link:rest-api-changes.html#change-info[ChangeInfo].
|
||||
|
||||
Default is `REF_UPDATED_AND_CHANGE_REINDEX`.
|
||||
|
||||
[[change.move]]change.move::
|
||||
+
|
||||
Whether the link:rest-api-changes.html#move-change[Move Change] REST
|
||||
@@ -1400,9 +1419,10 @@ the changes to auto-abandon.
|
||||
+
|
||||
By default `true`, meaning mergeable changes are auto-abandoned.
|
||||
+
|
||||
If link:#index.change.indexMergeable[`index.change.indexMergeable`]
|
||||
is disabled, setting this option to `false` has no effect and it
|
||||
behaves as though it were set to `true`.
|
||||
If
|
||||
link:#change.mergeabilityComputationBehavior[`change.mergeabilityComputationBehavior`]
|
||||
is set to `NEVER`, setting this option to `false` has no effect and it behaves
|
||||
as though it were set to `true`.
|
||||
|
||||
[[changeCleanup.cleanupAccountPatchReview]]changeCleanup.cleanupAccountPatchReview::
|
||||
+
|
||||
@@ -2776,22 +2796,6 @@ online schema upgrades.
|
||||
If not set or set to a zero, defaults to the number of logical CPUs as returned
|
||||
by the JVM. If set to a negative value, defaults to a direct executor.
|
||||
|
||||
[[index.change.indexMergeable]]index.change.indexMergeable::
|
||||
+
|
||||
Specifies if `mergeable` should be index or not. Indexing this field enables
|
||||
queries that contain the mergeability operator (`is:mergeable`). If enabled,
|
||||
Gerrit will check if the change is mergeable into the target branch when
|
||||
reindexing a change. This is an expensive operation.
|
||||
+
|
||||
If true, Gerrit will reindex all open changes when the target ref advances.
|
||||
Depending on the frequency of updates to the ref and the number of open changes,
|
||||
this can be very expensive.
|
||||
+
|
||||
When this setting is changed from `false` to `true`, all changes need to be
|
||||
reindexed.
|
||||
+
|
||||
Defaults to true.
|
||||
|
||||
[[index.onlineUpgrade]]index.onlineUpgrade::
|
||||
+
|
||||
Whether to upgrade to new index schema versions while the server is
|
||||
@@ -3102,7 +3106,7 @@ Depending on the setup, these events might get serialized using stream
|
||||
events.
|
||||
+
|
||||
This can be set to the set of minimal options that consumers of Gerrit's
|
||||
events need. A minimal set would be (`SKIP_MERGEABLE`,`SKIP_DIFFSTAT`).
|
||||
events need. A minimal set would be (`SKIP_DIFFSTAT`).
|
||||
+
|
||||
Every option that gets added here will have a performance impact. The
|
||||
general recommendation is therefore to set this to a minimal set of
|
||||
|
||||
@@ -2428,8 +2428,8 @@ that were deleted.
|
||||
|Field Name |Description
|
||||
|`change` |
|
||||
link:rest-api-changes.html#change-info[ChangeInfo] entity describing the change
|
||||
on which one or more comments was deleted. Populated with only the
|
||||
link:rest-api-changes.html#skip_mergeable[SKIP_MERGEABLE] option.
|
||||
on which one or more comments was deleted. Populated with no change list
|
||||
options.
|
||||
|`deleted` |
|
||||
List of link:rest-api-changes.html#comment-info[CommentInfo] entities for each
|
||||
comment that was deleted.
|
||||
|
||||
@@ -306,26 +306,6 @@ default. Optional fields are:
|
||||
from the change owner, i.e. this change would show up in the results of
|
||||
link:user-search.html#reviewedby[reviewedby:self].
|
||||
--
|
||||
|
||||
[[skip_mergeable]]
|
||||
--
|
||||
* `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.
|
||||
+
|
||||
When link:config-gerrit.html#change.api.excludeMergeableInChangeInfo[
|
||||
`change.api.excludeMergeableInChangeInfo`] is set in the `gerrit.config`,
|
||||
the `mergeable` field will always be omitted and `SKIP_MERGEABLE` has no
|
||||
effect.
|
||||
+
|
||||
When link:config-gerrit.html#index.change.indexMergeable[
|
||||
`index.change.indexMergeable`] is set to `false` in the `gerrit.config`,
|
||||
the `mergeable` field will always be omitted when querying changes and
|
||||
`SKIP_MERGEABLE` has no effect.
|
||||
+
|
||||
A change's mergeability can be requested separately by calling the
|
||||
link:#get-mergeable[get-mergeable] endpoint.
|
||||
--
|
||||
[[skip_diffstat]]
|
||||
--
|
||||
* `SKIP_DIFFSTAT`: skip the 'insertions' and 'deletions' field in link:#change-info[ChangeInfo].
|
||||
@@ -5964,10 +5944,9 @@ The link:config-project-config.html#submit-type[submit type] of the change. +
|
||||
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 or when
|
||||
link:config-gerrit.html#change.api.excludeMergeableInChangeInfo[change.api.excludeMergeableInChangeInfo]
|
||||
is set.
|
||||
Only set for open changes if
|
||||
link:config-gerrit.html#change.mergeabilityComputationBehavior[change.mergeabilityComputationBehavior]
|
||||
is `API_REF_UPDATED_AND_CHANGE_REINDEX`.
|
||||
|`submittable` |optional|
|
||||
Whether the change has been approved by the project submit rules. +
|
||||
Only set if link:#submittable[requested].
|
||||
|
||||
@@ -1566,10 +1566,11 @@ link:config-gerrit.html#change.submitWholeTopic[A configuration if
|
||||
the whole topic is submitted].
|
||||
|`disable_private_changes` |not set if `false`|
|
||||
Returns true if private changes are disabled.
|
||||
|`exclude_mergeable_in_change_info` |not set if `false`|
|
||||
Value of the link:config-gerrit.html#change.api.excludeMergeableInChangeInfo[
|
||||
|`mergeability_computation_behavior` ||
|
||||
Value of the link:config-gerrit.html#change.mergeabilityComputationBehavior[
|
||||
configuration parameter] that controls whether the mergeability bit in
|
||||
link:rest-api-changes.html#change-info[ChangeInfo] will never be set.
|
||||
link:rest-api-changes.html#change-info[ChangeInfo] will never be set and if the
|
||||
bit is indexed.
|
||||
|=============================
|
||||
|
||||
[[change-index-config-info]]
|
||||
|
||||
@@ -571,8 +571,8 @@ public class GerritServer implements AutoCloseable {
|
||||
cfg.setInt("sshd", null, "commandStartThreads", 1);
|
||||
cfg.setInt("receive", null, "threadPoolSize", 1);
|
||||
cfg.setInt("index", null, "threads", 1);
|
||||
if (cfg.getString("index", "change", "indexMergeable") == null) {
|
||||
cfg.setBoolean("index", "change", "indexMergeable", false);
|
||||
if (cfg.getString("index", null, "mergeabilityComputationBehavior") == null) {
|
||||
cfg.setString("index", null, "mergeabilityComputationBehavior", "NEVER");
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -49,6 +49,7 @@ import com.google.gerrit.index.query.QueryParseException;
|
||||
import com.google.gerrit.server.ReviewerByEmailSet;
|
||||
import com.google.gerrit.server.ReviewerSet;
|
||||
import com.google.gerrit.server.StarredChangesUtil;
|
||||
import com.google.gerrit.server.change.MergeabilityComputationBehavior;
|
||||
import com.google.gerrit.server.config.GerritServerConfig;
|
||||
import com.google.gerrit.server.config.SitePaths;
|
||||
import com.google.gerrit.server.index.IndexUtils;
|
||||
@@ -111,7 +112,7 @@ class ElasticChangeIndex extends AbstractElasticIndex<Change.Id, ChangeData>
|
||||
this.idField =
|
||||
this.schema.useLegacyNumericFields() ? ChangeField.LEGACY_ID : ChangeField.LEGACY_ID_STR;
|
||||
this.skipFields =
|
||||
gerritConfig.getBoolean("index", "change", "indexMergeable", true)
|
||||
MergeabilityComputationBehavior.fromConfig(gerritConfig).includeInIndex()
|
||||
? ImmutableSet.of()
|
||||
: ImmutableSet.of(ChangeField.MERGEABLE.getName());
|
||||
}
|
||||
|
||||
@@ -259,17 +259,12 @@ public interface ChangeApi {
|
||||
*
|
||||
* <ul>
|
||||
* <li>{@code CHECK} is omitted, to skip consistency checks.
|
||||
* <li>{@code SKIP_MERGEABLE} is omitted, so the {@code mergeable} bit <em>is</em> set.
|
||||
* <li>{@code SKIP_DIFFSTAT} is omitted to ensure diffstat calculations.
|
||||
* </ul>
|
||||
*/
|
||||
default ChangeInfo get() throws RestApiException {
|
||||
return get(
|
||||
EnumSet.complementOf(
|
||||
EnumSet.of(
|
||||
ListChangesOption.CHECK,
|
||||
ListChangesOption.SKIP_MERGEABLE,
|
||||
ListChangesOption.SKIP_DIFFSTAT)));
|
||||
EnumSet.complementOf(EnumSet.of(ListChangesOption.CHECK, ListChangesOption.SKIP_DIFFSTAT)));
|
||||
}
|
||||
|
||||
/** {@link #get(ListChangesOption...)} with no options included. */
|
||||
|
||||
@@ -74,7 +74,11 @@ public enum ListChangesOption implements ListOption {
|
||||
/** If tracking Ids are included, include detailed tracking Ids info. */
|
||||
TRACKING_IDS(21),
|
||||
|
||||
/** Skip mergeability data */
|
||||
/**
|
||||
* Use {@code gerrit.config} instead to turn this off for your instance. See {@code
|
||||
* change.mergeabilityComputationBehavior}.
|
||||
*/
|
||||
@Deprecated
|
||||
SKIP_MERGEABLE(22),
|
||||
|
||||
/**
|
||||
|
||||
@@ -24,5 +24,5 @@ public class ChangeConfigInfo {
|
||||
public String replyTooltip;
|
||||
public int updateDelay;
|
||||
public Boolean submitWholeTopic;
|
||||
public Boolean excludeMergeableInChangeInfo;
|
||||
public String mergeabilityComputationBehavior;
|
||||
}
|
||||
|
||||
@@ -1,22 +0,0 @@
|
||||
// Copyright (C) 2019 The Android Open Source Project
|
||||
//
|
||||
// Licensed under the Apache License, Version 2.0 (the "License");
|
||||
// you may not use this file except in compliance with the License.
|
||||
// You may obtain a copy of the License at
|
||||
//
|
||||
// http://www.apache.org/licenses/LICENSE-2.0
|
||||
//
|
||||
// Unless required by applicable law or agreed to in writing, software
|
||||
// distributed under the License is distributed on an "AS IS" BASIS,
|
||||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||
// See the License for the specific language governing permissions and
|
||||
// limitations under the License.
|
||||
|
||||
package com.google.gerrit.extensions.common;
|
||||
|
||||
/**
|
||||
* API response containing values from the {@code index.change} section of {@code gerrit.config}.
|
||||
*/
|
||||
public class ChangeIndexConfigInfo {
|
||||
public Boolean indexMergeable;
|
||||
}
|
||||
@@ -1,20 +0,0 @@
|
||||
// Copyright (C) 2019 The Android Open Source Project
|
||||
//
|
||||
// Licensed under the Apache License, Version 2.0 (the "License");
|
||||
// you may not use this file except in compliance with the License.
|
||||
// You may obtain a copy of the License at
|
||||
//
|
||||
// http://www.apache.org/licenses/LICENSE-2.0
|
||||
//
|
||||
// Unless required by applicable law or agreed to in writing, software
|
||||
// distributed under the License is distributed on an "AS IS" BASIS,
|
||||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||
// See the License for the specific language governing permissions and
|
||||
// limitations under the License.
|
||||
|
||||
package com.google.gerrit.extensions.common;
|
||||
|
||||
/** API response containing values from the {@code index} section of {@code gerrit.config}. */
|
||||
public class IndexConfigInfo {
|
||||
public ChangeIndexConfigInfo change;
|
||||
}
|
||||
@@ -21,7 +21,6 @@ public class ServerInfo {
|
||||
public ChangeConfigInfo change;
|
||||
public DownloadInfo download;
|
||||
public GerritInfo gerrit;
|
||||
public IndexConfigInfo index;
|
||||
public Boolean noteDbEnabled;
|
||||
public PluginConfigInfo plugin;
|
||||
public SshdInfo sshd;
|
||||
|
||||
@@ -55,6 +55,7 @@ import com.google.gerrit.index.query.QueryParseException;
|
||||
import com.google.gerrit.index.query.ResultSet;
|
||||
import com.google.gerrit.proto.Protos;
|
||||
import com.google.gerrit.server.StarredChangesUtil;
|
||||
import com.google.gerrit.server.change.MergeabilityComputationBehavior;
|
||||
import com.google.gerrit.server.config.GerritServerConfig;
|
||||
import com.google.gerrit.server.config.SitePaths;
|
||||
import com.google.gerrit.server.index.IndexExecutor;
|
||||
@@ -182,7 +183,7 @@ public class LuceneChangeIndex implements ChangeIndex {
|
||||
this.changeDataFactory = changeDataFactory;
|
||||
this.schema = schema;
|
||||
this.skipFields =
|
||||
cfg.getBoolean("index", "change", "indexMergeable", true)
|
||||
MergeabilityComputationBehavior.fromConfig(cfg).includeInIndex()
|
||||
? ImmutableSet.of()
|
||||
: ImmutableSet.of(ChangeField.MERGEABLE.getName());
|
||||
|
||||
|
||||
@@ -29,7 +29,6 @@ import static com.google.gerrit.extensions.client.ListChangesOption.MESSAGES;
|
||||
import static com.google.gerrit.extensions.client.ListChangesOption.REVIEWED;
|
||||
import static com.google.gerrit.extensions.client.ListChangesOption.REVIEWER_UPDATES;
|
||||
import static com.google.gerrit.extensions.client.ListChangesOption.SKIP_DIFFSTAT;
|
||||
import static com.google.gerrit.extensions.client.ListChangesOption.SKIP_MERGEABLE;
|
||||
import static com.google.gerrit.extensions.client.ListChangesOption.SUBMITTABLE;
|
||||
import static com.google.gerrit.extensions.client.ListChangesOption.TRACKING_IDS;
|
||||
import static com.google.gerrit.server.ChangeMessagesUtil.createChangeMessageInfo;
|
||||
@@ -219,7 +218,7 @@ public class ChangeJson {
|
||||
private final Metrics metrics;
|
||||
private final RevisionJson revisionJson;
|
||||
private final Optional<PluginDefinedAttributesFactory> pluginDefinedAttributesFactory;
|
||||
private final boolean excludeMergeableInChangeInfo;
|
||||
private final boolean includeMergeable;
|
||||
private final boolean lazyLoad;
|
||||
|
||||
private AccountLoader accountLoader;
|
||||
@@ -257,8 +256,7 @@ 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.includeMergeable = MergeabilityComputationBehavior.fromConfig(cfg).includeInApi();
|
||||
this.lazyLoad = containsAnyOf(this.options, REQUIRE_LAZY_LOAD);
|
||||
this.pluginDefinedAttributesFactory = pluginDefinedAttributesFactory;
|
||||
|
||||
@@ -525,7 +523,7 @@ public class ChangeJson {
|
||||
if (str.isOk()) {
|
||||
out.submitType = str.type;
|
||||
}
|
||||
if (!excludeMergeableInChangeInfo && !has(SKIP_MERGEABLE)) {
|
||||
if (includeMergeable) {
|
||||
out.mergeable = cd.isMergeable();
|
||||
}
|
||||
if (has(SUBMITTABLE)) {
|
||||
|
||||
@@ -0,0 +1,58 @@
|
||||
// Copyright (C) 2020 The Android Open Source Project
|
||||
//
|
||||
// Licensed under the Apache License, Version 2.0 (the "License");
|
||||
// you may not use this file except in compliance with the License.
|
||||
// You may obtain a copy of the License at
|
||||
//
|
||||
// http://www.apache.org/licenses/LICENSE-2.0
|
||||
//
|
||||
// Unless required by applicable law or agreed to in writing, software
|
||||
// distributed under the License is distributed on an "AS IS" BASIS,
|
||||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||
// See the License for the specific language governing permissions and
|
||||
// limitations under the License.
|
||||
|
||||
package com.google.gerrit.server.change;
|
||||
|
||||
import org.eclipse.jgit.lib.Config;
|
||||
|
||||
/**
|
||||
* State that is used to decide if {@code mergeable} should be included in the REST API or the
|
||||
* change index.
|
||||
*
|
||||
* <p>Computing mergeability of a change is an expensive operation - especially for Gerrit
|
||||
* installations with a large number of open changes or large repositories. Therefore, we want to
|
||||
* control when this computation is performed.
|
||||
*/
|
||||
public enum MergeabilityComputationBehavior {
|
||||
NEVER(false, false),
|
||||
REF_UPDATED_AND_CHANGE_REINDEX(true, false),
|
||||
API_REF_UPDATED_AND_CHANGE_REINDEX(true, true);
|
||||
|
||||
private final boolean includeInIndex;
|
||||
private final boolean includeInApi;
|
||||
|
||||
MergeabilityComputationBehavior(boolean includeInIndex, boolean includeInApi) {
|
||||
this.includeInIndex = includeInIndex;
|
||||
this.includeInApi = includeInApi;
|
||||
}
|
||||
|
||||
/** Returns a {@link MergeabilityComputationBehavior} constructed from a Gerrit server config. */
|
||||
public static MergeabilityComputationBehavior fromConfig(Config cfg) {
|
||||
return cfg.getEnum(
|
||||
"change",
|
||||
null,
|
||||
"mergeabilityComputationBehavior",
|
||||
MergeabilityComputationBehavior.API_REF_UPDATED_AND_CHANGE_REINDEX);
|
||||
}
|
||||
|
||||
/** Whether {@code mergeable} should be included in the change API. */
|
||||
public boolean includeInApi() {
|
||||
return includeInApi;
|
||||
}
|
||||
|
||||
/** Whether {@code mergeable} should be included in the change index. */
|
||||
public boolean includeInIndex() {
|
||||
return includeInIndex;
|
||||
}
|
||||
}
|
||||
@@ -17,6 +17,7 @@ package com.google.gerrit.server.config;
|
||||
import com.google.common.base.Strings;
|
||||
import com.google.common.flogger.FluentLogger;
|
||||
import com.google.gerrit.extensions.registration.DynamicItem;
|
||||
import com.google.gerrit.server.change.MergeabilityComputationBehavior;
|
||||
import com.google.gerrit.server.config.ScheduleConfig.Schedule;
|
||||
import com.google.inject.Inject;
|
||||
import com.google.inject.Singleton;
|
||||
@@ -51,7 +52,7 @@ public class ChangeCleanupConfig {
|
||||
this.urlFormatter = urlFormatter;
|
||||
schedule = ScheduleConfig.createSchedule(cfg, SECTION);
|
||||
abandonAfter = readAbandonAfter(cfg);
|
||||
boolean indexMergeable = cfg.getBoolean("index", "change", "indexMergeable", true);
|
||||
boolean indexMergeable = MergeabilityComputationBehavior.fromConfig(cfg).includeInIndex();
|
||||
if (!indexMergeable) {
|
||||
if (!readAbandonIfMergeable(cfg)) {
|
||||
logger.atWarning().log(
|
||||
|
||||
@@ -28,6 +28,7 @@ import com.google.gerrit.entities.Project;
|
||||
import com.google.gerrit.entities.RefNames;
|
||||
import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
|
||||
import com.google.gerrit.server.account.AccountCache;
|
||||
import com.google.gerrit.server.change.MergeabilityComputationBehavior;
|
||||
import com.google.gerrit.server.config.AllUsersName;
|
||||
import com.google.gerrit.server.config.GerritServerConfig;
|
||||
import com.google.gerrit.server.git.QueueProvider.QueueType;
|
||||
@@ -85,7 +86,7 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
|
||||
this.accountCache = accountCache;
|
||||
this.indexer = indexer;
|
||||
this.executor = executor;
|
||||
this.enabled = cfg.getBoolean("index", "change", "indexMergeable", true);
|
||||
this.enabled = MergeabilityComputationBehavior.fromConfig(cfg).includeInIndex();
|
||||
}
|
||||
|
||||
@Override
|
||||
|
||||
@@ -63,6 +63,7 @@ import com.google.gerrit.server.account.GroupMembers;
|
||||
import com.google.gerrit.server.account.VersionedAccountDestinations;
|
||||
import com.google.gerrit.server.account.VersionedAccountQueries;
|
||||
import com.google.gerrit.server.change.ChangeTriplet;
|
||||
import com.google.gerrit.server.change.MergeabilityComputationBehavior;
|
||||
import com.google.gerrit.server.config.AllProjectsName;
|
||||
import com.google.gerrit.server.config.AllUsersName;
|
||||
import com.google.gerrit.server.config.GerritServerConfig;
|
||||
@@ -293,7 +294,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
|
||||
groupMembers,
|
||||
anonymousUserProvider,
|
||||
operatorAliasConfig,
|
||||
gerritConfig.getBoolean("index", "change", "indexMergeable", true));
|
||||
MergeabilityComputationBehavior.fromConfig(gerritConfig).includeInIndex());
|
||||
}
|
||||
|
||||
private Arguments(
|
||||
|
||||
@@ -27,7 +27,6 @@ import com.google.gerrit.entities.PatchSet;
|
||||
import com.google.gerrit.entities.Project;
|
||||
import com.google.gerrit.extensions.api.accounts.DeleteDraftCommentsInput;
|
||||
import com.google.gerrit.extensions.api.accounts.DeletedDraftCommentInfo;
|
||||
import com.google.gerrit.extensions.client.ListChangesOption;
|
||||
import com.google.gerrit.extensions.common.CommentInfo;
|
||||
import com.google.gerrit.extensions.restapi.AuthException;
|
||||
import com.google.gerrit.extensions.restapi.BadRequestException;
|
||||
@@ -190,9 +189,7 @@ public class DeleteDraftComments
|
||||
if (dirty) {
|
||||
result = new DeletedDraftCommentInfo();
|
||||
result.change =
|
||||
changeJsonFactory
|
||||
.create(ListChangesOption.SKIP_MERGEABLE)
|
||||
.format(changeDataFactory.create(ctx.getNotes()));
|
||||
changeJsonFactory.noOptions().format(changeDataFactory.create(ctx.getNotes()));
|
||||
result.deleted = comments.build();
|
||||
}
|
||||
return dirty;
|
||||
|
||||
@@ -23,11 +23,9 @@ import com.google.gerrit.common.data.ContributorAgreement;
|
||||
import com.google.gerrit.extensions.common.AccountsInfo;
|
||||
import com.google.gerrit.extensions.common.AuthInfo;
|
||||
import com.google.gerrit.extensions.common.ChangeConfigInfo;
|
||||
import com.google.gerrit.extensions.common.ChangeIndexConfigInfo;
|
||||
import com.google.gerrit.extensions.common.DownloadInfo;
|
||||
import com.google.gerrit.extensions.common.DownloadSchemeInfo;
|
||||
import com.google.gerrit.extensions.common.GerritInfo;
|
||||
import com.google.gerrit.extensions.common.IndexConfigInfo;
|
||||
import com.google.gerrit.extensions.common.PluginConfigInfo;
|
||||
import com.google.gerrit.extensions.common.ReceiveInfo;
|
||||
import com.google.gerrit.extensions.common.ServerInfo;
|
||||
@@ -45,6 +43,7 @@ import com.google.gerrit.server.account.AccountVisibilityProvider;
|
||||
import com.google.gerrit.server.account.Realm;
|
||||
import com.google.gerrit.server.avatar.AvatarProvider;
|
||||
import com.google.gerrit.server.change.ArchiveFormat;
|
||||
import com.google.gerrit.server.change.MergeabilityComputationBehavior;
|
||||
import com.google.gerrit.server.config.AllProjectsName;
|
||||
import com.google.gerrit.server.config.AllUsersName;
|
||||
import com.google.gerrit.server.config.AnonymousCowardName;
|
||||
@@ -143,7 +142,6 @@ public class GetServerInfo implements RestReadView<ConfigResource> {
|
||||
info.change = getChangeInfo();
|
||||
info.download = getDownloadInfo();
|
||||
info.gerrit = getGerritInfo();
|
||||
info.index = getIndexInfo();
|
||||
info.noteDbEnabled = true;
|
||||
info.plugin = getPluginInfo();
|
||||
info.defaultTheme = getDefaultTheme();
|
||||
@@ -230,10 +228,10 @@ public class GetServerInfo implements RestReadView<ConfigResource> {
|
||||
info.updateDelay =
|
||||
(int) ConfigUtil.getTimeUnit(config, "change", null, "updateDelay", 300, TimeUnit.SECONDS);
|
||||
info.submitWholeTopic = toBoolean(MergeSuperSet.wholeTopicEnabled(config));
|
||||
info.excludeMergeableInChangeInfo =
|
||||
toBoolean(this.config.getBoolean("change", "api", "excludeMergeableInChangeInfo", false));
|
||||
info.disablePrivateChanges =
|
||||
toBoolean(this.config.getBoolean("change", null, "disablePrivateChanges", false));
|
||||
info.mergeabilityComputationBehavior =
|
||||
MergeabilityComputationBehavior.fromConfig(config).name();
|
||||
return info;
|
||||
}
|
||||
|
||||
@@ -299,14 +297,6 @@ public class GetServerInfo implements RestReadView<ConfigResource> {
|
||||
return info;
|
||||
}
|
||||
|
||||
private IndexConfigInfo getIndexInfo() {
|
||||
ChangeIndexConfigInfo change = new ChangeIndexConfigInfo();
|
||||
change.indexMergeable = toBoolean(config.getBoolean("index", "change", "indexMergeable", true));
|
||||
IndexConfigInfo index = new IndexConfigInfo();
|
||||
index.change = change;
|
||||
return index;
|
||||
}
|
||||
|
||||
private String getDocUrl() {
|
||||
String docUrl = config.getString("gerrit", null, "docUrl");
|
||||
if (Strings.isNullOrEmpty(docUrl)) {
|
||||
|
||||
@@ -14,6 +14,7 @@
|
||||
|
||||
package com.google.gerrit.testing;
|
||||
|
||||
import com.google.gerrit.server.change.MergeabilityComputationBehavior;
|
||||
import org.eclipse.jgit.lib.Config;
|
||||
|
||||
public class IndexConfig {
|
||||
@@ -23,9 +24,10 @@ public class IndexConfig {
|
||||
|
||||
public static Config createFromExistingConfig(Config cfg) {
|
||||
cfg.setInt("index", null, "maxPages", 10);
|
||||
// To avoid this flakiness indexMergeable is switched off for the tests as it incurs background
|
||||
// To avoid this flakiness indexing mergeable is disabled for the tests as it incurs background
|
||||
// reindex calls.
|
||||
cfg.setBoolean("index", "change", "indexMergeable", false);
|
||||
cfg.setEnum(
|
||||
"change", null, "mergeabilityComputationBehavior", MergeabilityComputationBehavior.NEVER);
|
||||
cfg.setString("trackingid", "query-bug", "footer", "Bug:");
|
||||
cfg.setString("trackingid", "query-bug", "match", "QUERY\\d{2,8}");
|
||||
cfg.setString("trackingid", "query-bug", "system", "querytests");
|
||||
|
||||
@@ -143,7 +143,9 @@ public class AbandonIT extends AbstractDaemonTest {
|
||||
@UseClockStep
|
||||
@GerritConfig(name = "changeCleanup.abandonAfter", value = "1w")
|
||||
@GerritConfig(name = "changeCleanup.abandonIfMergeable", value = "false")
|
||||
@GerritConfig(name = "index.change.indexMergeable", value = "true")
|
||||
@GerritConfig(
|
||||
name = "change.mergeabilityComputationBehavior",
|
||||
value = "API_REF_UPDATED_AND_CHANGE_REINDEX")
|
||||
public void notAbandonedIfMergeableWhenMergeableOperatorIsEnabled() throws Exception {
|
||||
ObjectId initial = repo().exactRef(HEAD).getLeaf().getObjectId();
|
||||
|
||||
@@ -180,14 +182,14 @@ public class AbandonIT extends AbstractDaemonTest {
|
||||
}
|
||||
|
||||
/**
|
||||
* When indexMergeable is disabled then the abandonIfMergeable option is ineffective and the auto
|
||||
* abandon behaves as though it were set to its default value (true).
|
||||
* When indexing mergeable is disabled then the abandonIfMergeable option is ineffective and the
|
||||
* auto abandon behaves as though it were set to its default value (true).
|
||||
*/
|
||||
@Test
|
||||
@UseClockStep
|
||||
@GerritConfig(name = "changeCleanup.abandonAfter", value = "1w")
|
||||
@GerritConfig(name = "changeCleanup.abandonIfMergeable", value = "false")
|
||||
@GerritConfig(name = "index.change.indexMergeable", value = "false")
|
||||
@GerritConfig(name = "change.mergeabilityComputationBehavior", value = "NEVER")
|
||||
public void abandonedIfMergeableWhenMergeableOperatorIsDisabled() throws Exception {
|
||||
ObjectId initial = repo().exactRef(HEAD).getLeaf().getObjectId();
|
||||
|
||||
|
||||
@@ -295,19 +295,7 @@ public class ChangeIT extends AbstractDaemonTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
public void skipMergeable() throws Exception {
|
||||
PushOneCommit.Result r = createChange();
|
||||
String triplet = project.get() + "~master~" + r.getChangeId();
|
||||
ChangeInfo c =
|
||||
gApi.changes().id(triplet).get(ImmutableList.of(ListChangesOption.SKIP_MERGEABLE));
|
||||
assertThat(c.mergeable).isNull();
|
||||
|
||||
c = gApi.changes().id(triplet).get();
|
||||
assertThat(c.mergeable).isTrue();
|
||||
}
|
||||
|
||||
@Test
|
||||
@GerritConfig(name = "change.api.excludeMergeableInChangeInfo", value = "true")
|
||||
@GerritConfig(name = "change.mergeabilityComputationBehavior", value = "NEVER")
|
||||
public void excludeMergeableInChangeInfo() throws Exception {
|
||||
PushOneCommit.Result r = createChange();
|
||||
ChangeInfo c = gApi.changes().id(r.getChangeId()).get();
|
||||
@@ -4425,7 +4413,6 @@ public class ChangeIT extends AbstractDaemonTest {
|
||||
ListChangesOption.MESSAGES,
|
||||
ListChangesOption.SUBMITTABLE,
|
||||
ListChangesOption.WEB_LINKS,
|
||||
ListChangesOption.SKIP_MERGEABLE,
|
||||
ListChangesOption.SKIP_DIFFSTAT);
|
||||
|
||||
PushOneCommit.Result change = createChange();
|
||||
@@ -4438,14 +4425,16 @@ public class ChangeIT extends AbstractDaemonTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
@GerritConfig(name = "index.change.indexMergeable", value = "true")
|
||||
@GerritConfig(
|
||||
name = "change.mergeabilityComputationBehavior",
|
||||
value = "API_REF_UPDATED_AND_CHANGE_REINDEX")
|
||||
public void changeQueryReturnsMergeableWhenGerritIndexMergeable() throws Exception {
|
||||
String changeId = createChange().getChangeId();
|
||||
assertThat(gApi.changes().query(changeId).get().get(0).mergeable).isTrue();
|
||||
}
|
||||
|
||||
@Test
|
||||
@GerritConfig(name = "index.change.indexMergeable", value = "false")
|
||||
@GerritConfig(name = "change.mergeabilityComputationBehavior", value = "NEVER")
|
||||
public void changeQueryDoesNotReturnMergeableWhenGerritDoesNotIndexMergeable() throws Exception {
|
||||
String changeId = createChange().getChangeId();
|
||||
assertThat(gApi.changes().query(changeId).get().get(0).mergeable).isNull();
|
||||
|
||||
@@ -1102,7 +1102,9 @@ public class RevisionIT extends AbstractDaemonTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
@GerritConfig(name = "index.change.indexMergeable", value = "true")
|
||||
@GerritConfig(
|
||||
name = "change.mergeabilityComputationBehavior",
|
||||
value = "API_REF_UPDATED_AND_CHANGE_REINDEX")
|
||||
public void mergeable() throws Exception {
|
||||
ObjectId initial = repo().exactRef(HEAD).getLeaf().getObjectId();
|
||||
|
||||
|
||||
@@ -1205,7 +1205,9 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
@GerritConfig(name = "index.change.indexMergeable", value = "true")
|
||||
@GerritConfig(
|
||||
name = "change.mergeabilityComputationBehavior",
|
||||
value = "API_REF_UPDATED_AND_CHANGE_REINDEX")
|
||||
public void submitSchedulesOpenChangesOfSameBranchForReindexing() throws Throwable {
|
||||
// Create a merged change.
|
||||
PushOneCommit push =
|
||||
|
||||
@@ -59,7 +59,9 @@ public class IndexChangeIT extends AbstractDaemonTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
@GerritConfig(name = "index.change.indexMergeable", value = "true")
|
||||
@GerritConfig(
|
||||
name = "change.mergeabilityComputationBehavior",
|
||||
value = "API_REF_UPDATED_AND_CHANGE_REINDEX")
|
||||
public void indexChangeAfterOwnerLosesVisibility() throws Exception {
|
||||
// Create a test group with 2 users as members
|
||||
TestAccount user2 = accountCreator.user2();
|
||||
|
||||
@@ -169,7 +169,8 @@ public class ServerInfoIT extends AbstractDaemonTest {
|
||||
assertThat(i.change.updateDelay).isEqualTo(300);
|
||||
assertThat(i.change.disablePrivateChanges).isNull();
|
||||
assertThat(i.change.submitWholeTopic).isNull();
|
||||
assertThat(i.change.excludeMergeableInChangeInfo).isNull();
|
||||
assertThat(i.change.mergeabilityComputationBehavior)
|
||||
.isEqualTo("API_REF_UPDATED_AND_CHANGE_REINDEX");
|
||||
|
||||
// download
|
||||
assertThat(i.download.archives).containsExactly("tar", "tbz2", "tgz", "txz");
|
||||
@@ -180,9 +181,6 @@ public class ServerInfoIT extends AbstractDaemonTest {
|
||||
assertThat(i.gerrit.allUsers).isEqualTo(AllUsersNameProvider.DEFAULT);
|
||||
assertThat(i.gerrit.reportBugUrl).isNull();
|
||||
|
||||
// index
|
||||
assertThat(i.index.change.indexMergeable).isNull(); // false in all tests
|
||||
|
||||
// plugin
|
||||
assertThat(i.plugin.jsResourcePaths).isEmpty();
|
||||
|
||||
@@ -204,16 +202,9 @@ public class ServerInfoIT extends AbstractDaemonTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
@GerritConfig(name = "change.api.excludeMergeableInChangeInfo", value = "true")
|
||||
public void serverConfigWithExcludeMergeableInChangeInfo() throws Exception {
|
||||
@GerritConfig(name = "change.mergeabilityComputationBehavior", value = "NEVER")
|
||||
public void mergeabilityComputationBehavior_neverCompute() throws Exception {
|
||||
ServerInfo i = gApi.config().server().getInfo();
|
||||
assertThat(i.change.excludeMergeableInChangeInfo).isTrue();
|
||||
}
|
||||
|
||||
@Test
|
||||
@GerritConfig(name = "index.change.indexMergeable", value = "true")
|
||||
public void indexMergeableIsTrueWhenTrueInConfig() throws Exception {
|
||||
ServerInfo i = gApi.config().server().getInfo();
|
||||
assertThat(i.index.change.indexMergeable).isTrue();
|
||||
assertThat(i.change.mergeabilityComputationBehavior).isEqualTo("NEVER");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -32,32 +32,26 @@ public class EventPayloadIT extends AbstractDaemonTest {
|
||||
@Test
|
||||
public void defaultOptions() throws Exception {
|
||||
RevisionCreatedListener listener =
|
||||
new RevisionCreatedListener() {
|
||||
@Override
|
||||
public void onRevisionCreated(Event event) {
|
||||
event -> {
|
||||
assertThat(event.getChange().submittable).isNotNull();
|
||||
assertThat(event.getRevision().files).isNotEmpty();
|
||||
}
|
||||
};
|
||||
try (Registration registration = extensionRegistry.newRegistration().add(listener)) {
|
||||
try (Registration ignored = extensionRegistry.newRegistration().add(listener)) {
|
||||
createChange();
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
@GerritConfig(name = "event.payload.listChangeOptions", value = "SKIP_MERGEABLE")
|
||||
@GerritConfig(name = "event.payload.listChangeOptions", value = "SKIP_DIFFSTAT")
|
||||
public void configuredOptions() throws Exception {
|
||||
RevisionCreatedListener listener =
|
||||
new RevisionCreatedListener() {
|
||||
@Override
|
||||
public void onRevisionCreated(Event event) {
|
||||
event -> {
|
||||
assertThat(event.getChange().submittable).isNull();
|
||||
assertThat(event.getChange().mergeable).isNull();
|
||||
assertThat(event.getChange().insertions).isNull();
|
||||
assertThat(event.getRevision().files).isNull();
|
||||
assertThat(event.getChange().subject).isNotEmpty();
|
||||
}
|
||||
};
|
||||
try (Registration registration = extensionRegistry.newRegistration().add(listener)) {
|
||||
try (Registration ignored = extensionRegistry.newRegistration().add(listener)) {
|
||||
createChange();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2025,7 +2025,9 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
|
||||
}
|
||||
|
||||
@Test
|
||||
@GerritConfig(name = "index.change.indexMergeable", value = "true")
|
||||
@GerritConfig(
|
||||
name = "change.mergeabilityComputationBehavior",
|
||||
value = "API_REF_UPDATED_AND_CHANGE_REINDEX")
|
||||
public void mergeable() throws Exception {
|
||||
TestRepository<Repo> repo = createProject("repo");
|
||||
RevCommit commit1 = repo.parseBody(repo.commit().add("file1", "contents1").create());
|
||||
@@ -2043,7 +2045,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
|
||||
// If a change gets submitted, the remaining open changes get reindexed asynchronously to update
|
||||
// their mergeability information. If the further assertions in this test are done before the
|
||||
// asynchronous reindex completed they fail because the mergeability information in the index
|
||||
// was not updated yet. To avoid this flakiness indexMergeable is switched off for the
|
||||
// was not updated yet. To avoid this flakiness indexing mergeable is switched off for the
|
||||
// tests and we index change2 synchronously here.
|
||||
gApi.changes().id(change2.getChangeId()).index();
|
||||
|
||||
@@ -3083,7 +3085,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
|
||||
}
|
||||
|
||||
@Test
|
||||
@GerritConfig(name = "index.change.indexMergeable", value = "false")
|
||||
@GerritConfig(name = "change.mergeabilityComputationBehavior", value = "NEVER")
|
||||
public void mergeableFailsWhenNotIndexed() throws Exception {
|
||||
TestRepository<Repo> repo = createProject("repo");
|
||||
RevCommit commit1 = repo.parseBody(repo.commit().add("file1", "contents1").create());
|
||||
|
||||
@@ -1001,7 +1001,6 @@
|
||||
this.ListChangesOption.MESSAGES,
|
||||
this.ListChangesOption.SUBMITTABLE,
|
||||
this.ListChangesOption.WEB_LINKS,
|
||||
this.ListChangesOption.SKIP_MERGEABLE,
|
||||
this.ListChangesOption.SKIP_DIFFSTAT,
|
||||
];
|
||||
return this.getConfig(false).then(config => {
|
||||
@@ -1024,7 +1023,6 @@
|
||||
const optionsHex = this.listChangesOptionsToHex(
|
||||
this.ListChangesOption.ALL_COMMITS,
|
||||
this.ListChangesOption.ALL_REVISIONS,
|
||||
this.ListChangesOption.SKIP_MERGEABLE,
|
||||
this.ListChangesOption.SKIP_DIFFSTAT
|
||||
);
|
||||
return this._getChangeDetail(changeNum, optionsHex, opt_errFn,
|
||||
|
||||
Reference in New Issue
Block a user