opendev/gerrit
Code Issues Proposed changes

Split change index into two for open/closed changes

Browse Source 
When preprocessing predicates to send to the index, determine the set
of change statuses that can possibly match that change, and use this
to send the query to the minimal set of indexes needed to satisfy it.
Do not attempt to rewrite the query further by e.g. splitting the
result and pruning subtrees that cannot be satisfied by a particular
index; assume once we hit the index, the extra cost of branches that
cannot contribute to the result is negligible.

When updating a change, delete the change from the index where it
doesn't belong. This adds an indexing step at many more places that
modify changes.

Change-Id: I928d34c883d3f8558e9324db5fa558f7b3b97612
This commit is contained in:
Dave Borowitz 2013-05-24 11:17:47 -07:00
parent 9ffced54a3
commit 45661d83d3
9 changed files with 157 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
gerrit-lucene/src/main/java/com/google/gerrit/lucene/IndexVersionCheck.java
View File

@ -36,7 +36,8 @@ import java.util.Map;
public class IndexVersionCheck implements LifecycleListener {
public static final Map<String, Integer> SCHEMA_VERSIONS = ImmutableMap.of(
"changes", ChangeField.SCHEMA_VERSION);
"changes_open", ChangeField.SCHEMA_VERSION,
"changes_closed", ChangeField.SCHEMA_VERSION);
public static File gerritIndexConfig(SitePaths sitePaths) {
return new File(sitePaths.index_dir, "gerrit_index.config");

10
gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java
View File

@ -21,9 +21,7 @@ import static org.apache.lucene.search.BooleanClause.Occur.MUST_NOT;
import static org.apache.lucene.search.BooleanClause.Occur.SHOULD;
import com.google.common.collect.Lists;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.index.ChangeField;
import com.google.gerrit.server.index.ChangeIndex;
import com.google.gerrit.server.index.FieldDef;
@ -39,8 +37,6 @@ import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeDataSource;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.ResultSet;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import org.apache.lucene.analysis.standard.StandardAnalyzer;
import org.apache.lucene.document.Document;
@ -132,6 +128,12 @@ public class LuceneChangeIndex implements ChangeIndex {
commit();
}
@Override
public void delete(ChangeData cd) throws IOException {
writer.deleteDocuments(intTerm(FIELD_CHANGE, cd.getId().get()));
commit();
}
@Override
public ChangeDataSource getSource(Predicate<ChangeData> p)
throws QueryParseException {

15
gerrit-pgm/src/main/java/com/google/gerrit/pgm/Reindex.java
View File

@ -23,7 +23,6 @@ import com.google.common.base.Stopwatch;
import com.google.common.collect.Lists;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.lifecycle.LifecycleManager;
import com.google.gerrit.lucene.LuceneChangeIndex;
import com.google.gerrit.lucene.LuceneIndexModule;
import com.google.gerrit.pgm.util.SiteProgram;
import com.google.gerrit.reviewdb.client.Change;
@ -31,9 +30,8 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.cache.CacheRemovalListener;
import com.google.gerrit.server.cache.h2.DefaultCacheFactory;
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.index.ChangeIndex;
import com.google.gerrit.server.index.ChangeIndexer;
import com.google.gerrit.server.patch.PatchListCacheImpl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.AbstractModule;
import com.google.inject.Injector;
@ -84,17 +82,14 @@ public class Reindex extends SiteProgram {
ReviewDb db = schema.open();
dbRef.set(db);
LuceneChangeIndex index = (LuceneChangeIndex) sysInjector
.getInstance(ChangeIndex.Manager.class)
.get("changes");
ChangeIndexer indexer = sysInjector.getInstance(ChangeIndexer.class);
Stopwatch sw = new Stopwatch().start();
int i = 0;
for (Change change : db.changes().all()) {
index.insert(new ChangeData(change));
indexer.index(change).get();
i++;
}
index.getWriter().commit();
double elapsed = sw.elapsed(TimeUnit.MILLISECONDS) / 1000d;
System.out.format("Reindexed %d changes in %.02fms", i, elapsed);
writeVersion();
@ -128,7 +123,8 @@ public class Reindex extends SiteProgram {
}
private void deleteAll() throws IOException {
File file = new File(sitePaths.index_dir, "changes");
for (String index : SCHEMA_VERSIONS.keySet()) {
File file = new File(sitePaths.index_dir, index);
if (file.exists()) {
Directory dir = FSDirectory.open(file);
try {
@ -140,6 +136,7 @@ public class Reindex extends SiteProgram {
}
}
}
}
private void writeVersion() throws IOException, ConfigInvalidException {
FileBasedConfig cfg =

16
gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeIndex.java
View File

@ -48,6 +48,11 @@ public interface ChangeIndex {
// Do nothing.
}
@Override
public void delete(ChangeData cd) throws IOException {
// Do nothing.
}
@Override
public ChangeDataSource getSource(Predicate<ChangeData> p)
throws QueryParseException {
@ -76,7 +81,7 @@ public interface ChangeIndex {
/**
* Update a change document in the index.
* <p>
* Semantically equivalent to removing the document and reinserting it with
* Semantically equivalent to deleting the document and reinserting it with
* new field values. Results may not be immediately visible to searchers, but
* should be visible within a reasonable amount of time.
*
@ -87,6 +92,15 @@ public interface ChangeIndex {
*/
public void replace(ChangeData cd) throws IOException;
/**
* Delete a change document from the index.
*
* @param cd change document.
*
* @throws IOException
*/
public void delete(ChangeData cd) throws IOException;
/**
* Convert the given operator predicate into a source searching the index and
* returning only the documents matching that predicate.

14
gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeIndexerImpl.java
View File

@ -37,13 +37,15 @@ public class ChangeIndexerImpl implements ChangeIndexer {
LoggerFactory.getLogger(ChangeIndexerImpl.class);
private final WorkQueue workQueue;
private final ChangeIndex index;
private final ChangeIndex openIndex;
private final ChangeIndex closedIndex;
@Inject
ChangeIndexerImpl(WorkQueue workQueue,
ChangeIndex.Manager indexManager) throws IOException {
this.workQueue = workQueue;
this.index = indexManager.get("changes");
this.openIndex = indexManager.get("changes_open");
this.closedIndex = indexManager.get("changes_closed");
}
@Override
@ -71,7 +73,13 @@ public class ChangeIndexerImpl implements ChangeIndexer {
public void run() {
ChangeData cd = new ChangeData(change);
try {
index.replace(cd);
if (change.getStatus().isOpen()) {
closedIndex.delete(cd);
openIndex.replace(cd);
} else {
openIndex.delete(cd);
closedIndex.replace(cd);
}
} catch (IOException e) {
log.error("Error indexing change", e);
}

10
gerrit-server/src/main/java/com/google/gerrit/server/index/PredicateWrapper.java
View File

@ -14,6 +14,7 @@
package com.google.gerrit.server.index;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.gerrit.server.query.Predicate;
@ -41,10 +42,15 @@ public class PredicateWrapper extends Predicate<ChangeData> implements
private final Predicate<ChangeData> pred;
private final List<ChangeDataSource> sources;
public PredicateWrapper(Predicate<ChangeData> pred, ChangeIndex... indexes)
public PredicateWrapper(Predicate<ChangeData> pred, ChangeIndex index)
throws QueryParseException {
this(pred, ImmutableList.of(index));
}
public PredicateWrapper(Predicate<ChangeData> pred,
Collection<ChangeIndex> indexes) throws QueryParseException {
this.pred = pred;
sources = Lists.newArrayListWithCapacity(indexes.length);
sources = Lists.newArrayListWithCapacity(indexes.size());
for (ChangeIndex index : indexes) {
sources.add(index.getSource(pred));
}

1
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryRewriter.java
View File

@ -30,7 +30,6 @@ import com.google.inject.OutOfScopeException;
import com.google.inject.Provider;
import com.google.inject.name.Named;
import java.io.IOException;
import java.util.Collection;
public class ChangeQueryRewriter extends QueryRewriter<ChangeData> {

67
gerrit-server/src/main/java/com/google/gerrit/server/query/change/IndexRewriteImpl.java
View File

@ -16,6 +16,8 @@ package com.google.gerrit.server.query.change;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.index.ChangeIndex;
import com.google.gerrit.server.index.IndexPredicate;
import com.google.gerrit.server.index.PredicateWrapper;
@ -28,20 +30,41 @@ import com.google.inject.Inject;
import java.io.IOException;
import java.util.BitSet;
import java.util.EnumSet;
import java.util.List;
import java.util.Set;
/** Rewriter that pushes boolean logic into the secondary index. */
public class IndexRewriteImpl implements IndexRewrite {
private final ChangeIndex index;
private static final Set<Change.Status> OPEN_STATUSES;
private static final Set<Change.Status> CLOSED_STATUSES;
static {
EnumSet<Change.Status> open = EnumSet.noneOf(Change.Status.class);
EnumSet<Change.Status> closed = EnumSet.noneOf(Change.Status.class);
for (Change.Status s : Change.Status.values()) {
if (s.isOpen()) {
open.add(s);
} else {
closed.add(s);
}
}
OPEN_STATUSES = Sets.immutableEnumSet(open);
CLOSED_STATUSES = Sets.immutableEnumSet(closed);
}
private final ChangeIndex openIndex;
private final ChangeIndex closedIndex;
@Inject
IndexRewriteImpl(ChangeIndex.Manager indexManager) throws IOException {
this(indexManager.get("changes"));
this(indexManager.get("changes_open"), indexManager.get("changes_closed"));
}
@VisibleForTesting
IndexRewriteImpl(ChangeIndex index) {
this.index = index;
IndexRewriteImpl(ChangeIndex openIndex, ChangeIndex closedIndex) {
this.openIndex = openIndex;
this.closedIndex = closedIndex;
}
@Override
@ -158,9 +181,43 @@ public class IndexRewriteImpl implements IndexRewrite {
}
}
@VisibleForTesting
static EnumSet<Change.Status> getPossibleStatus(Predicate<ChangeData> in) {
if (in instanceof ChangeStatusPredicate) {
return EnumSet.of(((ChangeStatusPredicate) in).getStatus());
} else if (in.getClass() == NotPredicate.class) {
return EnumSet.complementOf(getPossibleStatus(in.getChild(0)));
} else if (in.getClass() == OrPredicate.class) {
EnumSet<Change.Status> s = EnumSet.noneOf(Change.Status.class);
for (int i = 0; i < in.getChildCount(); i++) {
s.addAll(getPossibleStatus(in.getChild(i)));
}
return s;
} else if (in.getClass() == AndPredicate.class) {
EnumSet<Change.Status> s = EnumSet.allOf(Change.Status.class);
for (int i = 0; i < in.getChildCount(); i++) {
s.retainAll(getPossibleStatus(in.getChild(i)));
}
return s;
} else if (in.getChildCount() == 0) {
return EnumSet.allOf(Change.Status.class);
} else {
throw new IllegalStateException(
"Invalid predicate type in change index query: " + in.getClass());
}
}
private PredicateWrapper wrap(Predicate<ChangeData> p) {
try {
return new PredicateWrapper(p, index);
Set<Change.Status> possibleStatus = getPossibleStatus(p);
List<ChangeIndex> indexes = Lists.newArrayListWithCapacity(2);
if (!Sets.intersection(possibleStatus, OPEN_STATUSES).isEmpty()) {
indexes.add(openIndex);
}
if (!Sets.intersection(possibleStatus, CLOSED_STATUSES).isEmpty()) {
indexes.add(closedIndex);
}
return new PredicateWrapper(p, indexes);
} catch (QueryParseException e) {
throw new IllegalStateException(
"Failed to convert " + p + " to index predicate", e);

44
gerrit-server/src/test/java/com/google/gerrit/server/query/change/IndexRewriteTest.java
View File

@ -14,7 +14,14 @@
package com.google.gerrit.server.query.change;
import static com.google.gerrit.reviewdb.client.Change.Status.ABANDONED;
import static com.google.gerrit.reviewdb.client.Change.Status.DRAFT;
import static com.google.gerrit.reviewdb.client.Change.Status.MERGED;
import static com.google.gerrit.reviewdb.client.Change.Status.NEW;
import static com.google.gerrit.reviewdb.client.Change.Status.SUBMITTED;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.index.ChangeIndex;
import com.google.gerrit.server.index.PredicateWrapper;
import com.google.gerrit.server.query.AndPredicate;
@ -27,6 +34,8 @@ import com.google.gwtorm.server.ResultSet;
import junit.framework.TestCase;
import java.io.IOException;
import java.util.EnumSet;
import java.util.Set;
@SuppressWarnings("unchecked")
public class IndexRewriteTest extends TestCase {
@ -41,6 +50,11 @@ public class IndexRewriteTest extends TestCase {
throw new UnsupportedOperationException();
}
@Override
public void delete(ChangeData cd) throws IOException {
throw new UnsupportedOperationException();
}
@Override
public ChangeDataSource getSource(Predicate<ChangeData> p)
throws QueryParseException {
@ -65,20 +79,22 @@ public class IndexRewriteTest extends TestCase {
}
}
private DummyIndex index;
private DummyIndex openIndex;
private DummyIndex closedIndex;
private ChangeQueryBuilder queryBuilder;
private IndexRewrite rewrite;
@Override
public void setUp() throws Exception {
super.setUp();
index = new DummyIndex();
openIndex = new DummyIndex();
closedIndex = new DummyIndex();
queryBuilder = new ChangeQueryBuilder(
new ChangeQueryBuilder.Arguments(null, null, null, null, null, null,
null, null, null, null, null, null),
null);
rewrite = new IndexRewriteImpl(index);
rewrite = new IndexRewriteImpl(openIndex, closedIndex);
}
public void testIndexPredicate() throws Exception {
@ -166,6 +182,22 @@ public class IndexRewriteTest extends TestCase {
out.getChildren());
}
public void testGetPossibleStatus() throws Exception {
assertEquals(EnumSet.allOf(Change.Status.class), status("file:a"));
assertEquals(EnumSet.of(NEW), status("is:new"));
assertEquals(EnumSet.of(SUBMITTED, DRAFT, MERGED, ABANDONED),
status("-is:new"));
assertEquals(EnumSet.of(NEW, MERGED), status("is:new OR is:merged"));
EnumSet<Change.Status> none = EnumSet.noneOf(Change.Status.class);
assertEquals(none, status("is:new is:merged"));
assertEquals(none, status("(is:new is:draft) (is:merged is:submitted)"));
assertEquals(none, status("(is:new is:draft) (is:merged is:submitted)"));
assertEquals(EnumSet.of(MERGED, SUBMITTED),
status("(is:new is:draft) OR (is:merged OR is:submitted)"));
}
private Predicate<ChangeData> parse(String query) throws QueryParseException {
return queryBuilder.parse(query);
}
@ -176,6 +208,10 @@ public class IndexRewriteTest extends TestCase {
private PredicateWrapper wrap(Predicate<ChangeData> p)
throws QueryParseException {
return new PredicateWrapper(p, index);
return new PredicateWrapper(p, openIndex);
}
private Set<Change.Status> status(String query) throws QueryParseException {
return IndexRewriteImpl.getPossibleStatus(parse(query));
}
}