Merge "Store PatchSetApprovals instead of labels in the index"

2013-09-12 23:42:11 +00:00
parent ac990d89c8 24b14d5053
commit b82c9bf6f3
8 changed files with 75 additions and 264 deletions
--- a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java
+++ b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java
@@ -25,12 +25,12 @@ import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.ListeningScheduledExecutorService;
 import com.google.gerrit.reviewdb.client.Change;
-import com.google.gerrit.reviewdb.client.SubmitRecord;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.SitePaths;
 import com.google.gerrit.server.index.ChangeField;
 import com.google.gerrit.server.index.ChangeField.ChangeProtoField;
-import com.google.gerrit.server.index.ChangeField.SubmitLabelProtoField;
+import com.google.gerrit.server.index.ChangeField.PatchSetApprovalProtoField;
 import com.google.gerrit.server.index.ChangeIndex;
 import com.google.gerrit.server.index.FieldDef;
 import com.google.gerrit.server.index.FieldDef.FillArgs;
@@ -104,8 +104,7 @@ public class LuceneChangeIndex implements ChangeIndex {
  public static final String CHANGES_CLOSED = "closed";
  private static final String ID_FIELD = ChangeField.LEGACY_ID.getName();
  private static final String CHANGE_FIELD = ChangeField.CHANGE.getName();
-  private static final String SUBMIT_LABEL_FIELD =
-      ChangeField.SUBMIT_RECORD_LABEL.getName();
+  private static final String APPROVAL_FIELD = ChangeField.APPROVAL.getName();

  static interface Factory {
    LuceneChangeIndex create(Schema<ChangeData> schema, String base);
@@ -265,7 +264,7 @@ public class LuceneChangeIndex implements ChangeIndex {

  private static class QuerySource implements ChangeDataSource {
    private static final ImmutableSet<String> FIELDS =
-        ImmutableSet.of(ID_FIELD, CHANGE_FIELD, SUBMIT_LABEL_FIELD);
+        ImmutableSet.of(ID_FIELD, CHANGE_FIELD, APPROVAL_FIELD);

    private final List<SubIndex> indexes;
    private final Query query;
@@ -359,15 +358,15 @@ public class LuceneChangeIndex implements ChangeIndex {
        cb.bytes, cb.offset, cb.length);
    ChangeData cd = new ChangeData(change);

-    BytesRef[] labelsBytes = doc.getBinaryValues(SUBMIT_LABEL_FIELD);
-    if (labelsBytes != null) {
-      List<SubmitRecord.Label> labels =
-          Lists.newArrayListWithCapacity(labelsBytes.length);
-      for (BytesRef lb : labelsBytes) {
-        labels.add(SubmitLabelProtoField.CODEC.decode(
-            lb.bytes, lb.offset, lb.length));
+    BytesRef[] approvalsBytes = doc.getBinaryValues(APPROVAL_FIELD);
+    if (approvalsBytes != null) {
+      List<PatchSetApproval> approvals =
+          Lists.newArrayListWithCapacity(approvalsBytes.length);
+      for (BytesRef ab : approvalsBytes) {
+        approvals.add(PatchSetApprovalProtoField.CODEC.decode(
+            ab.bytes, ab.offset, ab.length));
      }
-      cd.setSubmitRecordLabels(labels);
+      cd.setCurrentApprovals(approvals);
    }
    return cd;
  }
--- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Reindex.java
+++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Reindex.java
@@ -15,13 +15,9 @@
 package com.google.gerrit.pgm;

 import static com.google.gerrit.server.schema.DataSourceProvider.Context.MULTI_USER;
-import static com.google.inject.Scopes.SINGLETON;

 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
-import com.google.gerrit.common.ChangeHooks;
-import com.google.gerrit.common.DisabledChangeHooks;
-import com.google.gerrit.common.errors.EmailException;
 import com.google.gerrit.extensions.events.LifecycleListener;
 import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.lifecycle.LifecycleManager;
@@ -31,28 +27,8 @@ import com.google.gerrit.pgm.util.SiteProgram;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.rules.PrologModule;
-import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.InternalUser;
-import com.google.gerrit.server.PluginUser;
-import com.google.gerrit.server.account.AccountByEmailCacheImpl;
-import com.google.gerrit.server.account.AccountCacheImpl;
-import com.google.gerrit.server.account.CapabilityControl;
-import com.google.gerrit.server.account.EmailExpander;
-import com.google.gerrit.server.account.GroupBackend;
-import com.google.gerrit.server.account.GroupCacheImpl;
-import com.google.gerrit.server.account.GroupControl;
-import com.google.gerrit.server.account.GroupIncludeCacheImpl;
-import com.google.gerrit.server.account.IncludingGroupMembership;
-import com.google.gerrit.server.account.InternalGroupBackend;
-import com.google.gerrit.server.account.UniversalGroupBackend;
 import com.google.gerrit.server.cache.CacheRemovalListener;
 import com.google.gerrit.server.cache.h2.DefaultCacheFactory;
-import com.google.gerrit.server.config.AuthModule;
-import com.google.gerrit.server.config.CanonicalWebUrlModule;
-import com.google.gerrit.server.config.CanonicalWebUrlProvider;
-import com.google.gerrit.server.config.EmailExpanderProvider;
-import com.google.gerrit.server.config.FactoryModule;
 import com.google.gerrit.server.index.ChangeBatchIndexer;
 import com.google.gerrit.server.index.ChangeIndex;
 import com.google.gerrit.server.index.ChangeSchemas;
@@ -60,25 +36,14 @@ import com.google.gerrit.server.index.IndexCollection;
 import com.google.gerrit.server.index.IndexModule;
 import com.google.gerrit.server.index.IndexModule.IndexType;
 import com.google.gerrit.server.index.NoIndexModule;
-import com.google.gerrit.server.mail.Address;
-import com.google.gerrit.server.mail.EmailHeader;
-import com.google.gerrit.server.mail.EmailSender;
-import com.google.gerrit.server.mail.SignedTokenEmailTokenVerifier;
 import com.google.gerrit.server.patch.PatchListCacheImpl;
-import com.google.gerrit.server.plugins.PluginModule;
-import com.google.gerrit.server.project.AccessControlModule;
-import com.google.gerrit.server.project.CommentLinkInfo;
-import com.google.gerrit.server.project.CommentLinkProvider;
-import com.google.gerrit.server.project.ProjectCacheImpl;
-import com.google.gerrit.server.project.ProjectControl;
-import com.google.gerrit.server.project.ProjectState;
-import com.google.gerrit.server.project.SectionSortCache;
-import com.google.gerrit.server.ssh.NoSshKeyCache;
 import com.google.gerrit.solr.SolrIndexModule;
 import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.SchemaFactory;
+import com.google.inject.AbstractModule;
 import com.google.inject.Injector;
 import com.google.inject.Key;
+import com.google.inject.Module;
 import com.google.inject.Provider;
 import com.google.inject.ProvisionException;
 import com.google.inject.TypeLiteral;
@@ -88,10 +53,8 @@ import org.eclipse.jgit.lib.TextProgressMonitor;
 import org.eclipse.jgit.util.io.NullOutputStream;
 import org.kohsuke.args4j.Option;

-import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;

@@ -147,99 +110,33 @@ public class Reindex extends SiteProgram {
  }

  private Injector createSysInjector() {
-    return dbInjector.createChildInjector(new GlobalModule());
-  }
-
-  /**
-   * Subset of GerritGlobalModule including just bindings needed for indexing.
-   *
-   * Does <b>not</b> include thread local request scoping behavior of ReviewDbs;
-   * uses a custom {@link ReviewDbModule} instead.
-   */
-  private class GlobalModule extends FactoryModule {
-    @SuppressWarnings("rawtypes")
-    @Override
-    protected void configure() {
-      install(PatchListCacheImpl.module());
-      install(dbInjector.getInstance(AuthModule.class));
-      install(AccountByEmailCacheImpl.module());
-      install(AccountCacheImpl.module());
-      install(new ReviewDbModule());
-      install(ProjectCacheImpl.module());
-      install(new PluginModule());
-      install(new PrologModule());
-      install(new DefaultCacheFactory.Module());
-      install(NoSshKeyCache.module());
-      install(new SignedTokenEmailTokenVerifier.Module());
-      install(GroupCacheImpl.module());
-      install(new AccessControlModule());
-      install(SectionSortCache.module());
-      install(new CanonicalWebUrlModule() {
-        @Override
-        protected Class<? extends Provider<String>> provider() {
-          return CanonicalWebUrlProvider.class;
-        }
-      });
-
-      switch (IndexModule.getIndexType(dbInjector)) {
-        case LUCENE:
-          install(new LuceneIndexModule(version, threads, outputBase));
-          break;
-        case SOLR:
-          install(new SolrIndexModule(false, threads, outputBase));
-          break;
-        default:
-          install(new NoIndexModule());
-      }
-
-      // No need to support live plugin removal or other live updates of
-      // caches behind our back, so don't worry about cache removal.
-      bind(new TypeLiteral<DynamicSet<CacheRemovalListener>>() {})
-          .toInstance(DynamicSet.<CacheRemovalListener> emptySet());
-
-      // Assume the current user is always the Gerrit internal user; no
-      // indexed data should depend on what user is doing the indexing.
-      bind(CurrentUser.class).to(InternalUser.class);
-
-      bind(ChangeHooks.class).to(DisabledChangeHooks.class);
-      bind(EmailExpander.class).toProvider(EmailExpanderProvider.class).in(
-          SINGLETON);
-      bind(new TypeLiteral<List<CommentLinkInfo>>() {})
-          .toProvider(CommentLinkProvider.class).in(SINGLETON);
-      bind(ProjectControl.GenericFactory.class);
-
-      factory(CapabilityControl.Factory.class);
-      factory(IncludingGroupMembership.Factory.class);
-      factory(InternalUser.Factory.class);
-      factory(PluginUser.Factory.class);
-      factory(ProjectState.Factory.class);
-
-      bind(GroupBackend.class).to(UniversalGroupBackend.class).in(SINGLETON);
-      install(GroupIncludeCacheImpl.module());
-      bind(GroupControl.Factory.class).in(SINGLETON);
-      bind(GroupControl.GenericFactory.class).in(SINGLETON);
-      bind(InternalGroupBackend.class).in(SINGLETON);
-      DynamicSet.setOf(binder(), GroupBackend.class);
-      DynamicSet.bind(binder(), GroupBackend.class).to(InternalGroupBackend.class);
-
-      bind(EmailSender.class).toInstance(new EmailSender() {
-        @Override
-        public boolean isEnabled() {
-          return false;
-        }
-
-        @Override
-        public boolean canEmail(String address) {
-          return false;
-        }
-
-        @Override
-        public void send(Address from, Collection<Address> rcpt,
-            Map<String, EmailHeader> headers, String body)
-            throws EmailException {
-        }
-      });
+    List<Module> modules = Lists.newArrayList();
+    modules.add(PatchListCacheImpl.module());
+    AbstractModule changeIndexModule;
+    switch (IndexModule.getIndexType(dbInjector)) {
+      case LUCENE:
+        changeIndexModule = new LuceneIndexModule(version, threads, outputBase);
+        break;
+      case SOLR:
+        changeIndexModule = new SolrIndexModule(false, threads, outputBase);
+        break;
+      default:
+        changeIndexModule = new NoIndexModule();
    }
+    modules.add(changeIndexModule);
+    modules.add(new ReviewDbModule());
+    modules.add(new AbstractModule() {
+      @SuppressWarnings("rawtypes")
+      @Override
+      protected void configure() {
+        // Plugins are not loaded and we're just running through each change
+        // once, so don't worry about cache removal.
+        bind(new TypeLiteral<DynamicSet<CacheRemovalListener>>() {})
+            .toInstance(DynamicSet.<CacheRemovalListener> emptySet());
+        install(new DefaultCacheFactory.Module());
+      }
+    });
+    return dbInjector.createChildInjector(modules);
  }

  private class ReviewDbModule extends LifecycleModule {
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
@@ -380,10 +380,6 @@ public class ChangeJson {
    if (!standard && !detailed) {
      return null;
    }
-    if (cd.getSubmitRecordLabels() != null && standard && !detailed) {
-      // Use saved labels to avoid recomputing on the fly.
-      return labelsFromSavedState(cd.getSubmitRecordLabels());
-    }

    ChangeControl ctl = control(cd);
    if (ctl == null) {
@@ -398,15 +394,6 @@ public class ChangeJson {
    }
  }

-  private Map<String, LabelInfo> labelsFromSavedState(
-      List<SubmitRecord.Label> savedLabels) {
-    Map<String, LabelInfo> labels = Maps.newLinkedHashMap();
-    for (SubmitRecord.Label r : savedLabels) {
-      labels.put(r.label, newLabelFromSubmitRecord(r, true));
-    }
-    return labels;
-  }
-
  private Map<String, LabelInfo> labelsForOpenChange(ChangeData cd,
      LabelTypes labelTypes, boolean standard, boolean detailed)
      throws OrmException {
@@ -447,34 +434,29 @@ public class ChangeJson {
      for (SubmitRecord.Label r : rec.labels) {
        LabelInfo p = labels.get(r.label);
        if (p == null || p._status.compareTo(r.status) < 0) {
-          labels.put(r.label, newLabelFromSubmitRecord(r, standard));
+          LabelInfo n = new LabelInfo();
+          n._status = r.status;
+          if (standard) {
+            switch (r.status) {
+              case OK:
+                n.approved = accountLoader.get(r.appliedBy);
+                break;
+              case REJECT:
+                n.rejected = accountLoader.get(r.appliedBy);
+                break;
+              default:
+                break;
+            }
+          }
+
+          n.optional = n._status == SubmitRecord.Label.Status.MAY ? true : null;
+          labels.put(r.label, n);
        }
      }
    }
    return labels;
  }

-  private LabelInfo newLabelFromSubmitRecord(SubmitRecord.Label r,
-      boolean standard) {
-    LabelInfo n = new LabelInfo();
-    n._status = r.status;
-    if (standard) {
-      switch (r.status) {
-        case OK:
-          n.approved = accountLoader.get(r.appliedBy);
-          break;
-        case REJECT:
-          n.rejected = accountLoader.get(r.appliedBy);
-          break;
-        default:
-          break;
-      }
-    }
-
-    n.optional = n._status == SubmitRecord.Label.Status.MAY ? true : null;
-    return n;
-  }
-
  private void setLabelScores(LabelType type,
      LabelInfo label, short score, Account.Id accountId)
      throws OrmException {
--- a/gerrit-server/src/main/java/com/google/gerrit/server/config/EmailExpanderProvider.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/EmailExpanderProvider.java
@@ -21,7 +21,7 @@ import com.google.inject.Provider;

 import org.eclipse.jgit.lib.Config;

-public class EmailExpanderProvider implements Provider<EmailExpander> {
+class EmailExpanderProvider implements Provider<EmailExpander> {
  private final EmailExpander expander;

  @Inject
--- a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java
@@ -14,9 +14,7 @@

 package com.google.gerrit.server.index;

-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
-import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
@@ -24,11 +22,8 @@ import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.PatchLineComment;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
-import com.google.gerrit.reviewdb.client.SubmitRecord;
 import com.google.gerrit.reviewdb.client.TrackingId;
 import com.google.gerrit.server.ChangeUtil;
-import com.google.gerrit.server.project.ChangeControl;
-import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.query.change.ChangeQueryBuilder;
 import com.google.gerrit.server.query.change.ChangeStatusPredicate;
@@ -42,7 +37,6 @@ import java.io.IOException;
 import java.sql.Timestamp;
 import java.util.Collection;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;

 /**
@@ -265,63 +259,28 @@ public class ChangeField {
  /** Serialized change object, used for pre-populating results. */
  public static final ChangeProtoField CHANGE = new ChangeProtoField();

-  public static class SubmitLabelProtoField
+  public static class PatchSetApprovalProtoField
      extends FieldDef.Repeatable<ChangeData, byte[]> {
-    public static final ProtobufCodec<SubmitRecord.Label> CODEC =
-        CodecFactory.encoder(SubmitRecord.Label.class);
+    public static final ProtobufCodec<PatchSetApproval> CODEC =
+        CodecFactory.encoder(PatchSetApproval.class);

-    private SubmitLabelProtoField() {
-      super("_label", FieldType.STORED_ONLY, true);
+    private PatchSetApprovalProtoField() {
+      super("_approval", FieldType.STORED_ONLY, true);
    }

    @Override
    public Iterable<byte[]> get(ChangeData input, FillArgs args)
        throws OrmException {
-      // Flatten the highest-valued labels to mimic the results from ChangeJson
-      // with standard labels.
-      Map<String, SubmitRecord.Label> labels = Maps.newLinkedHashMap();
-      for (SubmitRecord rec : getSubmitRecords(input, args)) {
-        if (rec.labels == null) {
-          continue;
-        }
-        for (SubmitRecord.Label r : rec.labels) {
-          SubmitRecord.Label p = labels.get(r.label);
-          if (p == null || p.status.compareTo(r.status) < 0) {
-            labels.put(r.label, r);
-          }
-        }
-      }
-      return toProtos(CODEC, labels.values());
-    }
-
-    private List<SubmitRecord> getSubmitRecords(ChangeData input,
-        FillArgs args) throws OrmException {
-      ChangeControl ctl;
-      try {
-        // Use the ChangeControl for InternalUser. This will give bogus
-        // results for whether or not the change is submittable, but does
-        // not affect label calculation.
-        ctl = args.changeControlFor(input.change(args.db));
-      } catch (NoSuchChangeException e) {
-        throw new OrmException(e);
-      }
-      if (ctl == null) {
-        return ImmutableList.of();
-      }
-      PatchSet ps = input.currentPatchSet(args.db);
-      if (ps == null) {
-        return ImmutableList.of();
-      }
-      return ctl.canSubmit(args.db.get(), ps, input, true, true, true);
+      return toProtos(CODEC, input.currentApprovals(args.db));
    }
  }

  /**
-   * Serialized labels from the submit rule evaluator, used for pre-populating
+   * Serialized approvals for the current patch set, used for pre-populating
   * results.
   */
-  public static final SubmitLabelProtoField SUBMIT_RECORD_LABEL =
-      new SubmitLabelProtoField();
+  public static final PatchSetApprovalProtoField APPROVAL =
+      new PatchSetApprovalProtoField();

  public static String formatLabel(String label, int value) {
    return formatLabel(label, value, null);
--- a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeSchemas.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeSchemas.java
@@ -69,7 +69,7 @@ public class ChangeSchemas {
        ChangeField.COMMIT_MESSAGE,
        ChangeField.COMMENT,
        ChangeField.CHANGE,
-        ChangeField.SUBMIT_RECORD_LABEL);
+        ChangeField.APPROVAL);

  private static Schema<ChangeData> release(FieldDef<ChangeData, ?>... fields) {
    return new Schema<ChangeData>(true, Arrays.asList(fields));
--- a/gerrit-server/src/main/java/com/google/gerrit/server/index/FieldDef.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/FieldDef.java
@@ -14,13 +14,9 @@

 package com.google.gerrit.server.index;

-import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.InternalUser;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.patch.PatchListCache;
-import com.google.gerrit.server.project.ChangeControl;
-import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -64,31 +60,14 @@ public abstract class FieldDef<I, T> {
    final Provider<ReviewDb> db;
    final GitRepositoryManager repoManager;
    final PatchListCache patchListCache;
-    final ChangeControl.GenericFactory changeControlFactory;
-
-    private final InternalUser internalUser;

    @Inject
-    FillArgs(Provider<ReviewDb> db, GitRepositoryManager repoManager,
-        PatchListCache patchListCache,
-        ChangeControl.GenericFactory changeControlFactory,
-        InternalUser internalUser) {
+    FillArgs(Provider<ReviewDb> db,
+        GitRepositoryManager repoManager,
+        PatchListCache patchListCache) {
      this.db = db;
      this.repoManager = repoManager;
      this.patchListCache = patchListCache;
-      this.changeControlFactory = changeControlFactory;
-      this.internalUser = internalUser;
-    }
-
-    /**
-     * Create a change control.
-     *
-     * @param change change object.
-     * @return change control as seen by Gerrit's internal user; index fields
-     *     may not depend on any per-user state.
-     */
-    ChangeControl changeControlFor(Change change) throws NoSuchChangeException {
-      return changeControlFactory.controlFor(change, internalUser);
    }
  }

--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -148,7 +148,6 @@ public class ChangeData {
  private ChangeControl changeControl;
  private List<ChangeMessage> messages;
  private List<SubmitRecord> submitRecords;
-  private List<SubmitRecord.Label> submitRecordLabels;
  private boolean patchesLoaded;

  public ChangeData(final Change.Id id) {
@@ -312,6 +311,10 @@ public class ChangeData {
    return currentApprovals;
  }

+  public void setCurrentApprovals(List<PatchSetApproval> approvals) {
+    currentApprovals = approvals;
+  }
+
  public String commitMessage(GitRepositoryManager repoManager,
      Provider<ReviewDb> db) throws IOException, OrmException {
    if (commitMessage == null) {
@@ -460,12 +463,4 @@ public class ChangeData {
  public List<SubmitRecord> getSubmitRecords() {
    return submitRecords;
  }
-
-  public void setSubmitRecordLabels(List<SubmitRecord.Label> labels) {
-    submitRecordLabels = labels;
-  }
-
-  public List<SubmitRecord.Label> getSubmitRecordLabels() {
-    return submitRecordLabels;
-  }
 }