Replace reindexAfterRefUpdate setting with indexMergeable
Gerrit can index a boolean field called 'mergeable' that determines if a change can be merged into the target ref. This bit can change whenever the target ref advances. Gerrit therefore has logic to reindex all open changes when the target ref advances. Depending on the number of open changes, the frequency of updates of the target ref and the size of the repo, this can be a very expensive operation. For large installations, 'reindexAfterRefUpdate' was added as a setting back in 2015 (I88ae7f4ad) to turn off automatic reindexing. This setting however, leads to inconsistent behavior: Gerrit stops updating documents when the target ref advances, so the 'mergeable' bit in the indexed document can be stale. For large repos, it is most likely stale. Users can still query for 'is:mergeable' though and Gerrit happily serves that stale bit in any query response. It is worth noting, that all of this does not affect the UI as that sends a separate, asynchronous request to compute mergeablitly when needed and does not rely on the index. This commit cleans this behavior up by replacing reindexAfterRefUpdate with indexMergeable. After this commit, there are two modes of operation: 1) Gerrit indexes 'mergable' and keeps it up to date when the target ref advances. Gerrit allows queries for 'is:mergeable'. 2) Gerrit does not index 'mergeable' at all. Gerrit does not allow queries for 'is:mergeable'. This way, users always get a consistent and correct result. Change-Id: I053af1b99616920db7f0dda8f8ec770e8683df5c
This commit is contained in:
parent
01f2af22af
commit
d03143571d
@ -2766,6 +2766,22 @@ 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
|
||||
@ -2813,19 +2829,6 @@ are the same.
|
||||
+
|
||||
Defaults to 1024.
|
||||
|
||||
[[index.reindexAfterRefUpdate]]index.reindexAfterRefUpdate::
|
||||
+
|
||||
Whether to reindex all affected open changes after a ref is updated. This
|
||||
includes reindexing all open changes to recompute the "mergeable" bit every time
|
||||
the destination branch moves, as well as reindexing changes to take into account
|
||||
new project configuration (e.g. label definitions).
|
||||
+
|
||||
Leaving this enabled may result in fresher results, but may cause performance
|
||||
problems if there are lots of open changes on a project whose branches advance
|
||||
frequently.
|
||||
+
|
||||
Defaults to true.
|
||||
|
||||
[[index.autoReindexIfStale]]index.autoReindexIfStale::
|
||||
+
|
||||
Whether to automatically check if a document became stale in the index
|
||||
|
@ -315,6 +315,11 @@ When link:config-gerrit.html#change.api.excludeMergeableInChangeInfo[
|
||||
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.
|
||||
--
|
||||
|
@ -563,8 +563,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", null, "reindexAfterRefUpdate") == null) {
|
||||
cfg.setBoolean("index", null, "reindexAfterRefUpdate", false);
|
||||
if (cfg.getString("index", "change", "indexMergeable") == null) {
|
||||
cfg.setBoolean("index", "change", "indexMergeable", false);
|
||||
}
|
||||
}
|
||||
|
||||
|
@ -14,6 +14,7 @@
|
||||
|
||||
package com.google.gerrit.elasticsearch;
|
||||
|
||||
import com.google.common.collect.ImmutableSet;
|
||||
import com.google.gerrit.elasticsearch.ElasticMapping.MappingProperties;
|
||||
import com.google.gerrit.elasticsearch.bulk.BulkRequest;
|
||||
import com.google.gerrit.elasticsearch.bulk.IndexRequest;
|
||||
@ -74,7 +75,7 @@ public class ElasticAccountIndex extends AbstractElasticIndex<Account.Id, Accoun
|
||||
public void replace(AccountState as) {
|
||||
BulkRequest bulk =
|
||||
new IndexRequest(getId(as), indexName, type, client.adapter())
|
||||
.add(new UpdateRequest<>(schema, as));
|
||||
.add(new UpdateRequest<>(schema, as, ImmutableSet.of()));
|
||||
|
||||
String uri = getURI(type, BULK);
|
||||
Response response = postRequest(uri, bulk, getRefreshParam());
|
||||
|
@ -22,6 +22,7 @@ import static java.util.Objects.requireNonNull;
|
||||
import com.google.common.collect.FluentIterable;
|
||||
import com.google.common.collect.ImmutableListMultimap;
|
||||
import com.google.common.collect.ImmutableMap;
|
||||
import com.google.common.collect.ImmutableSet;
|
||||
import com.google.common.collect.Iterables;
|
||||
import com.google.common.collect.ListMultimap;
|
||||
import com.google.common.collect.Lists;
|
||||
@ -48,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.config.GerritServerConfig;
|
||||
import com.google.gerrit.server.config.SitePaths;
|
||||
import com.google.gerrit.server.index.IndexUtils;
|
||||
import com.google.gerrit.server.index.change.ChangeField;
|
||||
@ -65,6 +67,7 @@ import java.util.List;
|
||||
import java.util.Optional;
|
||||
import java.util.Set;
|
||||
import org.apache.http.HttpStatus;
|
||||
import org.eclipse.jgit.lib.Config;
|
||||
import org.elasticsearch.client.Response;
|
||||
|
||||
/** Secondary index implementation using Elasticsearch. */
|
||||
@ -91,6 +94,7 @@ class ElasticChangeIndex extends AbstractElasticIndex<Change.Id, ChangeData>
|
||||
private final ChangeData.Factory changeDataFactory;
|
||||
private final Schema<ChangeData> schema;
|
||||
private final FieldDef<ChangeData, ?> idField;
|
||||
private final ImmutableSet<String> skipFields;
|
||||
|
||||
@Inject
|
||||
ElasticChangeIndex(
|
||||
@ -98,6 +102,7 @@ class ElasticChangeIndex extends AbstractElasticIndex<Change.Id, ChangeData>
|
||||
ChangeData.Factory changeDataFactory,
|
||||
SitePaths sitePaths,
|
||||
ElasticRestClientProvider clientBuilder,
|
||||
@GerritServerConfig Config gerritConfig,
|
||||
@Assisted Schema<ChangeData> schema) {
|
||||
super(cfg, sitePaths, schema, clientBuilder, CHANGES);
|
||||
this.changeDataFactory = changeDataFactory;
|
||||
@ -105,6 +110,10 @@ class ElasticChangeIndex extends AbstractElasticIndex<Change.Id, ChangeData>
|
||||
this.mapping = new ChangeMapping(schema, client.adapter());
|
||||
this.idField =
|
||||
this.schema.useLegacyNumericFields() ? ChangeField.LEGACY_ID : ChangeField.LEGACY_ID_STR;
|
||||
this.skipFields =
|
||||
gerritConfig.getBoolean("index", "change", "indexMergeable", true)
|
||||
? ImmutableSet.of()
|
||||
: ImmutableSet.of(ChangeField.MERGEABLE.getName());
|
||||
}
|
||||
|
||||
@Override
|
||||
@ -123,7 +132,7 @@ class ElasticChangeIndex extends AbstractElasticIndex<Change.Id, ChangeData>
|
||||
ElasticQueryAdapter adapter = client.adapter();
|
||||
BulkRequest bulk =
|
||||
new IndexRequest(getId(cd), indexName, adapter.getType(insertIndex), adapter)
|
||||
.add(new UpdateRequest<>(schema, cd));
|
||||
.add(new UpdateRequest<>(schema, cd, skipFields));
|
||||
if (adapter.deleteToReplace()) {
|
||||
bulk.add(new DeleteRequest(cd.getId().toString(), indexName, deleteIndex, adapter));
|
||||
}
|
||||
@ -263,7 +272,7 @@ class ElasticChangeIndex extends AbstractElasticIndex<Change.Id, ChangeData>
|
||||
|
||||
// Mergeable.
|
||||
JsonElement mergeableElement = source.get(ChangeField.MERGEABLE.getName());
|
||||
if (mergeableElement != null) {
|
||||
if (mergeableElement != null && !skipFields.contains(ChangeField.MERGEABLE.getName())) {
|
||||
String mergeable = mergeableElement.getAsString();
|
||||
if ("1".equals(mergeable)) {
|
||||
cd.setMergeable(true);
|
||||
|
@ -14,6 +14,7 @@
|
||||
|
||||
package com.google.gerrit.elasticsearch;
|
||||
|
||||
import com.google.common.collect.ImmutableSet;
|
||||
import com.google.gerrit.elasticsearch.ElasticMapping.MappingProperties;
|
||||
import com.google.gerrit.elasticsearch.bulk.BulkRequest;
|
||||
import com.google.gerrit.elasticsearch.bulk.IndexRequest;
|
||||
@ -74,7 +75,7 @@ public class ElasticGroupIndex extends AbstractElasticIndex<AccountGroup.UUID, I
|
||||
public void replace(InternalGroup group) {
|
||||
BulkRequest bulk =
|
||||
new IndexRequest(getId(group), indexName, type, client.adapter())
|
||||
.add(new UpdateRequest<>(schema, group));
|
||||
.add(new UpdateRequest<>(schema, group, ImmutableSet.of()));
|
||||
|
||||
String uri = getURI(type, BULK);
|
||||
Response response = postRequest(uri, bulk, getRefreshParam());
|
||||
|
@ -14,6 +14,7 @@
|
||||
|
||||
package com.google.gerrit.elasticsearch;
|
||||
|
||||
import com.google.common.collect.ImmutableSet;
|
||||
import com.google.gerrit.elasticsearch.ElasticMapping.MappingProperties;
|
||||
import com.google.gerrit.elasticsearch.bulk.BulkRequest;
|
||||
import com.google.gerrit.elasticsearch.bulk.IndexRequest;
|
||||
@ -74,7 +75,7 @@ public class ElasticProjectIndex extends AbstractElasticIndex<Project.NameKey, P
|
||||
public void replace(ProjectData projectState) {
|
||||
BulkRequest bulk =
|
||||
new IndexRequest(projectState.getProject().getName(), indexName, type, client.adapter())
|
||||
.add(new UpdateRequest<>(schema, projectState));
|
||||
.add(new UpdateRequest<>(schema, projectState, ImmutableSet.of()));
|
||||
|
||||
String uri = getURI(type, BULK);
|
||||
Response response = postRequest(uri, bulk, getRefreshParam());
|
||||
|
@ -16,6 +16,7 @@ package com.google.gerrit.elasticsearch.bulk;
|
||||
|
||||
import static java.util.stream.Collectors.toList;
|
||||
|
||||
import com.google.common.collect.ImmutableSet;
|
||||
import com.google.common.collect.Iterables;
|
||||
import com.google.common.collect.Streams;
|
||||
import com.google.gerrit.elasticsearch.builders.XContentBuilder;
|
||||
@ -27,17 +28,19 @@ public class UpdateRequest<V> extends BulkRequest {
|
||||
|
||||
private final Schema<V> schema;
|
||||
private final V v;
|
||||
private final ImmutableSet<String> skipFields;
|
||||
|
||||
public UpdateRequest(Schema<V> schema, V v) {
|
||||
public UpdateRequest(Schema<V> schema, V v, ImmutableSet<String> skipFields) {
|
||||
this.schema = schema;
|
||||
this.v = v;
|
||||
this.skipFields = skipFields;
|
||||
}
|
||||
|
||||
@Override
|
||||
protected String getRequest() {
|
||||
try (XContentBuilder closeable = new XContentBuilder()) {
|
||||
XContentBuilder builder = closeable.startObject();
|
||||
for (Values<V> values : schema.buildFields(v)) {
|
||||
for (Values<V> values : schema.buildFields(v, skipFields)) {
|
||||
String name = values.getField().getName();
|
||||
if (values.getField().isRepeatable()) {
|
||||
builder.field(name, Streams.stream(values.getValues()).collect(toList()));
|
||||
|
@ -15,11 +15,12 @@
|
||||
package com.google.gerrit.index;
|
||||
|
||||
import static com.google.common.base.Preconditions.checkState;
|
||||
import static com.google.common.collect.ImmutableList.toImmutableList;
|
||||
|
||||
import com.google.common.base.MoreObjects;
|
||||
import com.google.common.collect.FluentIterable;
|
||||
import com.google.common.collect.ImmutableList;
|
||||
import com.google.common.collect.ImmutableMap;
|
||||
import com.google.common.collect.ImmutableSet;
|
||||
import com.google.common.flogger.FluentLogger;
|
||||
import java.util.ArrayList;
|
||||
import java.util.Arrays;
|
||||
@ -181,12 +182,17 @@ public class Schema<T> {
|
||||
* <p>Null values are omitted, as are fields which cause errors, which are logged.
|
||||
*
|
||||
* @param obj input object.
|
||||
* @param skipFields set of field names to skip when indexing the document
|
||||
* @return all non-null field values from the object.
|
||||
*/
|
||||
public final Iterable<Values<T>> buildFields(T obj) {
|
||||
return FluentIterable.from(fields.values())
|
||||
.transform(
|
||||
public final Iterable<Values<T>> buildFields(T obj, ImmutableSet<String> skipFields) {
|
||||
return fields.values().stream()
|
||||
.map(
|
||||
f -> {
|
||||
if (skipFields.contains(f.getName())) {
|
||||
return null;
|
||||
}
|
||||
|
||||
Object v;
|
||||
try {
|
||||
v = f.get(obj);
|
||||
@ -203,7 +209,8 @@ public class Schema<T> {
|
||||
return new Values<>(f, Collections.singleton(v));
|
||||
}
|
||||
})
|
||||
.filter(Objects::nonNull);
|
||||
.filter(Objects::nonNull)
|
||||
.collect(toImmutableList());
|
||||
}
|
||||
|
||||
@Override
|
||||
|
@ -21,6 +21,7 @@ import static java.util.concurrent.TimeUnit.MILLISECONDS;
|
||||
import com.google.common.base.Joiner;
|
||||
import com.google.common.collect.ArrayListMultimap;
|
||||
import com.google.common.collect.ImmutableList;
|
||||
import com.google.common.collect.ImmutableSet;
|
||||
import com.google.common.collect.ListMultimap;
|
||||
import com.google.common.collect.Sets;
|
||||
import com.google.common.flogger.FluentLogger;
|
||||
@ -98,6 +99,7 @@ public abstract class AbstractLuceneIndex<K, V> implements Index<K, V> {
|
||||
private final SitePaths sitePaths;
|
||||
private final Directory dir;
|
||||
private final String name;
|
||||
private final ImmutableSet<String> skipFields;
|
||||
private final ListeningExecutorService writerThread;
|
||||
private final IndexWriter writer;
|
||||
private final ReferenceManager<IndexSearcher> searcherManager;
|
||||
@ -110,6 +112,7 @@ public abstract class AbstractLuceneIndex<K, V> implements Index<K, V> {
|
||||
SitePaths sitePaths,
|
||||
Directory dir,
|
||||
String name,
|
||||
ImmutableSet<String> skipFields,
|
||||
String subIndex,
|
||||
GerritIndexWriterConfig writerConfig,
|
||||
SearcherFactory searcherFactory)
|
||||
@ -118,6 +121,7 @@ public abstract class AbstractLuceneIndex<K, V> implements Index<K, V> {
|
||||
this.sitePaths = sitePaths;
|
||||
this.dir = dir;
|
||||
this.name = name;
|
||||
this.skipFields = skipFields;
|
||||
String index = Joiner.on('_').skipNulls().join(name, subIndex);
|
||||
long commitPeriod = writerConfig.getCommitWithinMs();
|
||||
|
||||
@ -311,7 +315,7 @@ public abstract class AbstractLuceneIndex<K, V> implements Index<K, V> {
|
||||
|
||||
Document toDocument(V obj) {
|
||||
Document result = new Document();
|
||||
for (Values<V> vs : schema.buildFields(obj)) {
|
||||
for (Values<V> vs : schema.buildFields(obj, skipFields)) {
|
||||
if (vs.getValues() != null) {
|
||||
add(result, vs);
|
||||
}
|
||||
|
@ -20,6 +20,7 @@ import static com.google.gerrit.lucene.LuceneChangeIndex.ID_SORT_FIELD;
|
||||
import static com.google.gerrit.lucene.LuceneChangeIndex.UPDATED_SORT_FIELD;
|
||||
import static com.google.gerrit.server.index.change.ChangeSchemaDefinitions.NAME;
|
||||
|
||||
import com.google.common.collect.ImmutableSet;
|
||||
import com.google.gerrit.entities.Change;
|
||||
import com.google.gerrit.index.FieldDef;
|
||||
import com.google.gerrit.index.QueryOptions;
|
||||
@ -48,6 +49,7 @@ public class ChangeSubIndex extends AbstractLuceneIndex<Change.Id, ChangeData>
|
||||
Schema<ChangeData> schema,
|
||||
SitePaths sitePaths,
|
||||
Path path,
|
||||
ImmutableSet<String> skipFields,
|
||||
GerritIndexWriterConfig writerConfig,
|
||||
SearcherFactory searcherFactory)
|
||||
throws IOException {
|
||||
@ -56,6 +58,7 @@ public class ChangeSubIndex extends AbstractLuceneIndex<Change.Id, ChangeData>
|
||||
sitePaths,
|
||||
FSDirectory.open(path),
|
||||
path.getFileName().toString(),
|
||||
skipFields,
|
||||
writerConfig,
|
||||
searcherFactory);
|
||||
}
|
||||
@ -65,10 +68,11 @@ public class ChangeSubIndex extends AbstractLuceneIndex<Change.Id, ChangeData>
|
||||
SitePaths sitePaths,
|
||||
Directory dir,
|
||||
String subIndex,
|
||||
ImmutableSet<String> skipFields,
|
||||
GerritIndexWriterConfig writerConfig,
|
||||
SearcherFactory searcherFactory)
|
||||
throws IOException {
|
||||
super(schema, sitePaths, dir, NAME, subIndex, writerConfig, searcherFactory);
|
||||
super(schema, sitePaths, dir, NAME, skipFields, subIndex, writerConfig, searcherFactory);
|
||||
}
|
||||
|
||||
@Override
|
||||
|
@ -20,6 +20,7 @@ import static com.google.gerrit.server.index.account.AccountField.ID;
|
||||
import static com.google.gerrit.server.index.account.AccountField.ID_STR;
|
||||
import static com.google.gerrit.server.index.account.AccountField.PREFERRED_EMAIL_EXACT;
|
||||
|
||||
import com.google.common.collect.ImmutableSet;
|
||||
import com.google.gerrit.entities.Account;
|
||||
import com.google.gerrit.exceptions.StorageException;
|
||||
import com.google.gerrit.index.FieldDef;
|
||||
@ -100,6 +101,7 @@ public class LuceneAccountIndex extends AbstractLuceneIndex<Account.Id, AccountS
|
||||
sitePaths,
|
||||
dir(schema, cfg, sitePaths),
|
||||
ACCOUNTS,
|
||||
ImmutableSet.of(),
|
||||
null,
|
||||
new GerritIndexWriterConfig(cfg, ACCOUNTS),
|
||||
new SearcherFactory());
|
||||
|
@ -29,6 +29,7 @@ import com.google.common.base.Throwables;
|
||||
import com.google.common.collect.Collections2;
|
||||
import com.google.common.collect.FluentIterable;
|
||||
import com.google.common.collect.ImmutableList;
|
||||
import com.google.common.collect.ImmutableSet;
|
||||
import com.google.common.collect.Iterables;
|
||||
import com.google.common.collect.ListMultimap;
|
||||
import com.google.common.collect.MultimapBuilder;
|
||||
@ -167,6 +168,7 @@ public class LuceneChangeIndex implements ChangeIndex {
|
||||
private final String idSortFieldName;
|
||||
private final IdTerm idTerm;
|
||||
private final ChangeIdExtractor extractor;
|
||||
private final ImmutableSet<String> skipFields;
|
||||
|
||||
@Inject
|
||||
LuceneChangeIndex(
|
||||
@ -179,6 +181,10 @@ public class LuceneChangeIndex implements ChangeIndex {
|
||||
this.executor = executor;
|
||||
this.changeDataFactory = changeDataFactory;
|
||||
this.schema = schema;
|
||||
this.skipFields =
|
||||
cfg.getBoolean("index", "change", "indexMergeable", true)
|
||||
? ImmutableSet.of()
|
||||
: ImmutableSet.of(ChangeField.MERGEABLE.getName());
|
||||
|
||||
GerritIndexWriterConfig openConfig = new GerritIndexWriterConfig(cfg, "changes_open");
|
||||
GerritIndexWriterConfig closedConfig = new GerritIndexWriterConfig(cfg, "changes_closed");
|
||||
@ -189,18 +195,40 @@ public class LuceneChangeIndex implements ChangeIndex {
|
||||
if (LuceneIndexModule.isInMemoryTest(cfg)) {
|
||||
openIndex =
|
||||
new ChangeSubIndex(
|
||||
schema, sitePaths, new RAMDirectory(), "ramOpen", openConfig, searcherFactory);
|
||||
schema,
|
||||
sitePaths,
|
||||
new RAMDirectory(),
|
||||
"ramOpen",
|
||||
skipFields,
|
||||
openConfig,
|
||||
searcherFactory);
|
||||
closedIndex =
|
||||
new ChangeSubIndex(
|
||||
schema, sitePaths, new RAMDirectory(), "ramClosed", closedConfig, searcherFactory);
|
||||
schema,
|
||||
sitePaths,
|
||||
new RAMDirectory(),
|
||||
"ramClosed",
|
||||
skipFields,
|
||||
closedConfig,
|
||||
searcherFactory);
|
||||
} else {
|
||||
Path dir = LuceneVersionManager.getDir(sitePaths, CHANGES, schema);
|
||||
openIndex =
|
||||
new ChangeSubIndex(
|
||||
schema, sitePaths, dir.resolve(CHANGES_OPEN), openConfig, searcherFactory);
|
||||
schema,
|
||||
sitePaths,
|
||||
dir.resolve(CHANGES_OPEN),
|
||||
skipFields,
|
||||
openConfig,
|
||||
searcherFactory);
|
||||
closedIndex =
|
||||
new ChangeSubIndex(
|
||||
schema, sitePaths, dir.resolve(CHANGES_CLOSED), closedConfig, searcherFactory);
|
||||
schema,
|
||||
sitePaths,
|
||||
dir.resolve(CHANGES_CLOSED),
|
||||
skipFields,
|
||||
closedConfig,
|
||||
searcherFactory);
|
||||
}
|
||||
|
||||
idField = this.schema.useLegacyNumericFields() ? LEGACY_ID : LEGACY_ID_STR;
|
||||
@ -565,7 +593,7 @@ public class LuceneChangeIndex implements ChangeIndex {
|
||||
|
||||
private void decodeMergeable(ListMultimap<String, IndexableField> doc, ChangeData cd) {
|
||||
IndexableField f = Iterables.getFirst(doc.get(MERGEABLE_FIELD), null);
|
||||
if (f != null) {
|
||||
if (f != null && !skipFields.contains(MERGEABLE_FIELD)) {
|
||||
String mergeable = f.stringValue();
|
||||
if ("1".equals(mergeable)) {
|
||||
cd.setMergeable(true);
|
||||
|
@ -17,6 +17,7 @@ package com.google.gerrit.lucene;
|
||||
import static com.google.common.collect.Iterables.getOnlyElement;
|
||||
import static com.google.gerrit.server.index.group.GroupField.UUID;
|
||||
|
||||
import com.google.common.collect.ImmutableSet;
|
||||
import com.google.gerrit.entities.AccountGroup;
|
||||
import com.google.gerrit.exceptions.StorageException;
|
||||
import com.google.gerrit.index.FieldDef;
|
||||
@ -90,6 +91,7 @@ public class LuceneGroupIndex extends AbstractLuceneIndex<AccountGroup.UUID, Int
|
||||
sitePaths,
|
||||
dir(schema, cfg, sitePaths),
|
||||
GROUPS,
|
||||
ImmutableSet.of(),
|
||||
null,
|
||||
new GerritIndexWriterConfig(cfg, GROUPS),
|
||||
new SearcherFactory());
|
||||
|
@ -17,6 +17,7 @@ package com.google.gerrit.lucene;
|
||||
import static com.google.common.collect.Iterables.getOnlyElement;
|
||||
import static com.google.gerrit.index.project.ProjectField.NAME;
|
||||
|
||||
import com.google.common.collect.ImmutableSet;
|
||||
import com.google.gerrit.entities.Project;
|
||||
import com.google.gerrit.exceptions.StorageException;
|
||||
import com.google.gerrit.index.FieldDef;
|
||||
@ -90,6 +91,7 @@ public class LuceneProjectIndex extends AbstractLuceneIndex<Project.NameKey, Pro
|
||||
sitePaths,
|
||||
dir(schema, cfg, sitePaths),
|
||||
PROJECTS,
|
||||
ImmutableSet.of(),
|
||||
null,
|
||||
new GerritIndexWriterConfig(cfg, PROJECTS),
|
||||
new SearcherFactory());
|
||||
|
@ -76,7 +76,7 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
|
||||
this.accountCache = accountCache;
|
||||
this.indexer = indexer;
|
||||
this.executor = executor;
|
||||
this.enabled = cfg.getBoolean("index", null, "reindexAfterRefUpdate", true);
|
||||
this.enabled = cfg.getBoolean("index", "change", "indexMergeable", true);
|
||||
}
|
||||
|
||||
@Override
|
||||
|
@ -63,6 +63,7 @@ import com.google.gerrit.server.account.VersionedAccountQueries;
|
||||
import com.google.gerrit.server.change.ChangeTriplet;
|
||||
import com.google.gerrit.server.config.AllProjectsName;
|
||||
import com.google.gerrit.server.config.AllUsersName;
|
||||
import com.google.gerrit.server.config.GerritServerConfig;
|
||||
import com.google.gerrit.server.config.OperatorAliasConfig;
|
||||
import com.google.gerrit.server.git.GitRepositoryManager;
|
||||
import com.google.gerrit.server.index.change.ChangeField;
|
||||
@ -94,6 +95,7 @@ import java.util.function.Function;
|
||||
import java.util.regex.Pattern;
|
||||
import org.eclipse.jgit.errors.ConfigInvalidException;
|
||||
import org.eclipse.jgit.errors.RepositoryNotFoundException;
|
||||
import org.eclipse.jgit.lib.Config;
|
||||
import org.eclipse.jgit.lib.Repository;
|
||||
|
||||
/** Parses a query string meant to be applied to change objects. */
|
||||
@ -221,6 +223,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
|
||||
final GroupMembers groupMembers;
|
||||
final Provider<AnonymousUser> anonymousUserProvider;
|
||||
final OperatorAliasConfig operatorAliasConfig;
|
||||
final boolean indexMergeable;
|
||||
|
||||
private final Provider<CurrentUser> self;
|
||||
|
||||
@ -253,7 +256,8 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
|
||||
AccountCache accountCache,
|
||||
GroupMembers groupMembers,
|
||||
Provider<AnonymousUser> anonymousUserProvider,
|
||||
OperatorAliasConfig operatorAliasConfig) {
|
||||
OperatorAliasConfig operatorAliasConfig,
|
||||
@GerritServerConfig Config gerritConfig) {
|
||||
this(
|
||||
queryProvider,
|
||||
rewriter,
|
||||
@ -281,7 +285,8 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
|
||||
accountCache,
|
||||
groupMembers,
|
||||
anonymousUserProvider,
|
||||
operatorAliasConfig);
|
||||
operatorAliasConfig,
|
||||
gerritConfig.getBoolean("index", "change", "indexMergeable", true));
|
||||
}
|
||||
|
||||
private Arguments(
|
||||
@ -311,7 +316,8 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
|
||||
AccountCache accountCache,
|
||||
GroupMembers groupMembers,
|
||||
Provider<AnonymousUser> anonymousUserProvider,
|
||||
OperatorAliasConfig operatorAliasConfig) {
|
||||
OperatorAliasConfig operatorAliasConfig,
|
||||
boolean indexMergeable) {
|
||||
this.queryProvider = queryProvider;
|
||||
this.rewriter = rewriter;
|
||||
this.opFactories = opFactories;
|
||||
@ -339,6 +345,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
|
||||
this.groupMembers = groupMembers;
|
||||
this.anonymousUserProvider = anonymousUserProvider;
|
||||
this.operatorAliasConfig = operatorAliasConfig;
|
||||
this.indexMergeable = indexMergeable;
|
||||
}
|
||||
|
||||
Arguments asUser(CurrentUser otherUser) {
|
||||
@ -369,7 +376,8 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
|
||||
accountCache,
|
||||
groupMembers,
|
||||
anonymousUserProvider,
|
||||
operatorAliasConfig);
|
||||
operatorAliasConfig,
|
||||
indexMergeable);
|
||||
}
|
||||
|
||||
Arguments asUser(Account.Id otherId) {
|
||||
@ -570,6 +578,9 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
|
||||
}
|
||||
|
||||
if ("mergeable".equalsIgnoreCase(value)) {
|
||||
if (!args.indexMergeable) {
|
||||
throw new QueryParseException("server does not support 'mergeable'. check configs");
|
||||
}
|
||||
return new BooleanPredicate(ChangeField.MERGEABLE);
|
||||
}
|
||||
|
||||
|
@ -23,7 +23,9 @@ public class IndexConfig {
|
||||
|
||||
public static Config createFromExistingConfig(Config cfg) {
|
||||
cfg.setInt("index", null, "maxPages", 10);
|
||||
cfg.setBoolean("index", null, "reindexAfterRefUpdate", false);
|
||||
// To avoid this flakiness indexMergeable is switched off for the tests as it incurs background
|
||||
// reindex calls.
|
||||
cfg.setBoolean("index", "change", "indexMergeable", false);
|
||||
cfg.setString("trackingid", "query-bug", "footer", "Bug:");
|
||||
cfg.setString("trackingid", "query-bug", "match", "QUERY\\d{2,8}");
|
||||
cfg.setString("trackingid", "query-bug", "system", "querytests");
|
||||
|
@ -4393,6 +4393,20 @@ public class ChangeIT extends AbstractDaemonTest {
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
@GerritConfig(name = "index.change.indexMergeable", value = "true")
|
||||
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")
|
||||
public void changeQueryDoesNotReturnMergeableWhenGerritDoesNotIndexMergeable() throws Exception {
|
||||
String changeId = createChange().getChangeId();
|
||||
assertThat(gApi.changes().query(changeId).get().get(0).mergeable).isNull();
|
||||
}
|
||||
|
||||
private PushOneCommit.Result createWorkInProgressChange() throws Exception {
|
||||
return pushTo("refs/for/master%wip");
|
||||
}
|
||||
|
@ -46,6 +46,7 @@ import com.google.gerrit.acceptance.PushOneCommit;
|
||||
import com.google.gerrit.acceptance.RestResponse;
|
||||
import com.google.gerrit.acceptance.TestAccount;
|
||||
import com.google.gerrit.acceptance.TestProjectInput;
|
||||
import com.google.gerrit.acceptance.config.GerritConfig;
|
||||
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
|
||||
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
|
||||
import com.google.gerrit.common.data.Permission;
|
||||
@ -1062,6 +1063,7 @@ public class RevisionIT extends AbstractDaemonTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
@GerritConfig(name = "index.change.indexMergeable", value = "true")
|
||||
public void mergeable() throws Exception {
|
||||
ObjectId initial = repo().exactRef(HEAD).getLeaf().getObjectId();
|
||||
|
||||
|
@ -1205,7 +1205,7 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
@GerritConfig(name = "index.reindexAfterRefUpdate", value = "true")
|
||||
@GerritConfig(name = "index.change.indexMergeable", value = "true")
|
||||
public void submitSchedulesOpenChangesOfSameBranchForReindexing() throws Throwable {
|
||||
// Create a merged change.
|
||||
PushOneCommit push =
|
||||
|
@ -22,6 +22,7 @@ import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS
|
||||
import com.google.gerrit.acceptance.AbstractDaemonTest;
|
||||
import com.google.gerrit.acceptance.PushOneCommit;
|
||||
import com.google.gerrit.acceptance.TestAccount;
|
||||
import com.google.gerrit.acceptance.config.GerritConfig;
|
||||
import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
|
||||
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
|
||||
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
|
||||
@ -58,6 +59,7 @@ public class IndexChangeIT extends AbstractDaemonTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
@GerritConfig(name = "index.change.indexMergeable", value = "true")
|
||||
public void indexChangeAfterOwnerLosesVisibility() throws Exception {
|
||||
// Create a test group with 2 users as members
|
||||
TestAccount user2 = accountCreator.user2();
|
||||
|
@ -18,6 +18,7 @@ import com.google.gerrit.index.query.OperatorPredicate;
|
||||
import com.google.gerrit.index.query.Predicate;
|
||||
import com.google.gerrit.server.query.change.ChangeData;
|
||||
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
|
||||
import org.eclipse.jgit.lib.Config;
|
||||
import org.junit.Ignore;
|
||||
|
||||
@Ignore
|
||||
@ -26,8 +27,34 @@ public class FakeQueryBuilder extends ChangeQueryBuilder {
|
||||
super(
|
||||
new ChangeQueryBuilder.Definition<>(FakeQueryBuilder.class),
|
||||
new ChangeQueryBuilder.Arguments(
|
||||
null, null, null, null, null, null, null, null, null, null, null, null, null, null,
|
||||
null, null, null, null, indexes, null, null, null, null, null, null, null, null));
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
indexes,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
new Config()));
|
||||
}
|
||||
|
||||
@Operator
|
||||
|
@ -30,6 +30,7 @@ import static java.util.concurrent.TimeUnit.MILLISECONDS;
|
||||
import static java.util.concurrent.TimeUnit.MINUTES;
|
||||
import static java.util.concurrent.TimeUnit.SECONDS;
|
||||
import static java.util.stream.Collectors.toList;
|
||||
import static org.hamcrest.MatcherAssert.assertThat;
|
||||
import static org.junit.Assert.fail;
|
||||
|
||||
import com.google.common.base.MoreObjects;
|
||||
@ -41,6 +42,7 @@ import com.google.common.collect.Iterables;
|
||||
import com.google.common.collect.Lists;
|
||||
import com.google.common.collect.Streams;
|
||||
import com.google.common.truth.ThrowableSubject;
|
||||
import com.google.gerrit.acceptance.config.GerritConfig;
|
||||
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
|
||||
import com.google.gerrit.common.Nullable;
|
||||
import com.google.gerrit.common.data.LabelType;
|
||||
@ -79,6 +81,7 @@ import com.google.gerrit.index.FieldDef;
|
||||
import com.google.gerrit.index.IndexConfig;
|
||||
import com.google.gerrit.index.Schema;
|
||||
import com.google.gerrit.index.query.Predicate;
|
||||
import com.google.gerrit.index.query.QueryParseException;
|
||||
import com.google.gerrit.lifecycle.LifecycleManager;
|
||||
import com.google.gerrit.server.AnonymousUser;
|
||||
import com.google.gerrit.server.CurrentUser;
|
||||
@ -2023,6 +2026,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
|
||||
}
|
||||
|
||||
@Test
|
||||
@GerritConfig(name = "index.change.indexMergeable", value = "true")
|
||||
public void mergeable() throws Exception {
|
||||
TestRepository<Repo> repo = createProject("repo");
|
||||
RevCommit commit1 = repo.parseBody(repo.commit().add("file1", "contents1").create());
|
||||
@ -2040,7 +2044,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 reindexAfterRefUpdate is switched off for the
|
||||
// was not updated yet. To avoid this flakiness indexMergeable is switched off for the
|
||||
// tests and we index change2 synchronously here.
|
||||
gApi.changes().id(change2.getChangeId()).index();
|
||||
|
||||
@ -3079,6 +3083,20 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
@GerritConfig(name = "index.change.indexMergeable", value = "false")
|
||||
public void mergeableFailsWhenNotIndexed() throws Exception {
|
||||
TestRepository<Repo> repo = createProject("repo");
|
||||
RevCommit commit1 = repo.parseBody(repo.commit().add("file1", "contents1").create());
|
||||
insert(repo, newChangeForCommit(repo, commit1));
|
||||
|
||||
Throwable thrown = assertThrows(Throwable.class, () -> assertQuery("status:open is:mergeable"));
|
||||
assertThat(thrown.getCause()).isInstanceOf(QueryParseException.class);
|
||||
assertThat(thrown)
|
||||
.hasMessageThat()
|
||||
.contains("server does not support 'mergeable'. check configs");
|
||||
}
|
||||
|
||||
protected ChangeInserter newChange(TestRepository<Repo> repo) throws Exception {
|
||||
return newChange(repo, null, null, null, null, false);
|
||||
}
|
||||
|
@ -16,12 +16,14 @@ java_library(
|
||||
"//prolog:gerrit-prolog-common",
|
||||
],
|
||||
deps = [
|
||||
"//java/com/google/gerrit/acceptance/config",
|
||||
"//java/com/google/gerrit/acceptance/testsuite/project",
|
||||
"//java/com/google/gerrit/common:annotations",
|
||||
"//java/com/google/gerrit/common:server",
|
||||
"//java/com/google/gerrit/entities",
|
||||
"//java/com/google/gerrit/extensions:api",
|
||||
"//java/com/google/gerrit/index",
|
||||
"//java/com/google/gerrit/index:query_exception",
|
||||
"//java/com/google/gerrit/lifecycle",
|
||||
"//java/com/google/gerrit/server",
|
||||
"//java/com/google/gerrit/server/project/testing:project-test-util",
|
||||
|
Loading…
Reference in New Issue
Block a user