Merge changes from topic 'notedb-reviewers'

* changes:
  Include reviewers when rebuilding NoteDb
  Extract a type for sets of reviewers, and store timestamp

gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java

@@ -14,8 +14,6 @@
 
 package com.google.gerrit.server;
 
-import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
 import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -23,13 +21,10 @@ import com.google.common.base.Function;
 import com.google.common.base.Predicate;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
-import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.LinkedHashMultimap;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Ordering;
-import com.google.common.collect.SetMultimap;
 import com.google.common.collect.Sets;
 import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.common.data.LabelTypes;
@@ -45,7 +40,6 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.notedb.NotesMigration;
-import com.google.gerrit.server.notedb.ReviewerStateInternal;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.util.LabelVote;
 import com.google.gwtorm.server.OrmException;
@@ -115,15 +109,14 @@ public class ApprovalsUtil {
    *
    * @param db review database.
    * @param notes change notes.
-   * @return multimap of reviewers keyed by state, where each account appears
-   *     exactly once in {@link SetMultimap#values()}, and
-   *     {@link ReviewerStateInternal#REMOVED} is not present.
+   * @return reviewers for the change.
    * @throws OrmException if reviewers for the change could not be read.
    */
-  public ImmutableSetMultimap<ReviewerStateInternal, Account.Id> getReviewers(
-      ReviewDb db, ChangeNotes notes) throws OrmException {
+  public ReviewerSet getReviewers(ReviewDb db, ChangeNotes notes)
+      throws OrmException {
     if (!migration.readChanges()) {
-      return getReviewers(db.patchSetApprovals().byChange(notes.getChangeId()));
+      return ReviewerSet.fromApprovals(
+          db.patchSetApprovals().byChange(notes.getChangeId()));
     }
     return notes.load().getReviewers();
   }
@@ -133,44 +126,18 @@ public class ApprovalsUtil {
    *
    * @param allApprovals all approvals to consider; must all belong to the same
    *     change.
-   * @return multimap of reviewers keyed by state, where each account appears
-   *     exactly once in {@link SetMultimap#values()}, and
-   *     {@link ReviewerStateInternal#REMOVED} is not present.
+   * @return reviewers for the change.
+   * @throws OrmException if reviewers for the change could not be read.
    */
-  public ImmutableSetMultimap<ReviewerStateInternal, Account.Id> getReviewers(
-      ChangeNotes notes, Iterable<PatchSetApproval> allApprovals)
+  public ReviewerSet getReviewers(ChangeNotes notes,
+      Iterable<PatchSetApproval> allApprovals)
       throws OrmException {
     if (!migration.readChanges()) {
-      return getReviewers(allApprovals);
+      return ReviewerSet.fromApprovals(allApprovals);
     }
     return notes.load().getReviewers();
   }
 
-  private static ImmutableSetMultimap<ReviewerStateInternal, Account.Id> getReviewers(
-      Iterable<PatchSetApproval> allApprovals) {
-    PatchSetApproval first = null;
-    SetMultimap<ReviewerStateInternal, Account.Id> reviewers =
-        LinkedHashMultimap.create();
-    for (PatchSetApproval psa : allApprovals) {
-      if (first == null) {
-        first = psa;
-      } else {
-        checkArgument(
-            first.getKey().getParentKey().getParentKey().equals(
-              psa.getKey().getParentKey().getParentKey()),
-            "multiple change IDs: %s, %s", first.getKey(), psa.getKey());
-      }
-      Account.Id id = psa.getAccountId();
-      if (psa.getValue() != 0) {
-        reviewers.put(REVIEWER, id);
-        reviewers.remove(CC, id);
-      } else if (!reviewers.containsEntry(REVIEWER, id)) {
-        reviewers.put(CC, id);
-      }
-    }
-    return ImmutableSetMultimap.copyOf(reviewers);
-  }
-
   public List<PatchSetApproval> addReviewers(ReviewDb db,
       ChangeUpdate update, LabelTypes labelTypes, Change change, PatchSet ps,
       PatchSetInfo info, Iterable<Account.Id> wantReviewers,
@@ -185,7 +152,7 @@ public class ApprovalsUtil {
       Iterable<Account.Id> wantReviewers) throws OrmException {
     PatchSet.Id psId = change.currentPatchSetId();
     return addReviewers(db, update, labelTypes, change, psId, false, null, null,
-        wantReviewers, getReviewers(db, notes).values());
+        wantReviewers, getReviewers(db, notes).all());
   }
 
   private List<PatchSetApproval> addReviewers(ReviewDb db, ChangeUpdate update,

gerrit-server/src/main/java/com/google/gerrit/server/ReviewerSet.java

@@ -0,0 +1,115 @@
+// Copyright (C) 2016 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;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
+import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
+
+import com.google.common.collect.HashBasedTable;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableTable;
+import com.google.common.collect.Table;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.server.notedb.ReviewerStateInternal;
+
+import java.sql.Timestamp;
+
+/**
+ * Set of reviewers on a change.
+ * <p>
+ * A given account may appear in multiple states and at different timestamps. No
+ * reviewers with state {@link ReviewerStateInternal#REMOVED} are ever exposed
+ * by this interface.
+ */
+public class ReviewerSet {
+  private static final ReviewerSet EMPTY = new ReviewerSet(
+      ImmutableTable.<ReviewerStateInternal, Account.Id, Timestamp>of());
+
+  public static ReviewerSet fromApprovals(
+      Iterable<PatchSetApproval> approvals) {
+    PatchSetApproval first = null;
+    Table<ReviewerStateInternal, Account.Id, Timestamp> reviewers =
+        HashBasedTable.create();
+    for (PatchSetApproval psa : approvals) {
+      if (first == null) {
+        first = psa;
+      } else {
+        checkArgument(
+            first.getKey().getParentKey().getParentKey().equals(
+              psa.getKey().getParentKey().getParentKey()),
+            "multiple change IDs: %s, %s", first.getKey(), psa.getKey());
+      }
+      Account.Id id = psa.getAccountId();
+      if (psa.getValue() != 0) {
+        reviewers.put(REVIEWER, id, psa.getGranted());
+        reviewers.remove(CC, id);
+      } else if (!reviewers.contains(REVIEWER, id)) {
+        reviewers.put(CC, id, psa.getGranted());
+      }
+    }
+    return new ReviewerSet(reviewers);
+  }
+
+  public static ReviewerSet fromTable(
+      Table<ReviewerStateInternal, Account.Id, Timestamp> table) {
+    return new ReviewerSet(table);
+  }
+
+  public static ReviewerSet empty() {
+    return EMPTY;
+  }
+
+  private final ImmutableTable<ReviewerStateInternal, Account.Id, Timestamp>
+      table;
+  private ImmutableSet<Account.Id> accounts;
+
+  private ReviewerSet(Table<ReviewerStateInternal, Account.Id, Timestamp> table) {
+    this.table = ImmutableTable.copyOf(table);
+  }
+
+  public ImmutableSet<Account.Id> all() {
+    if (accounts == null) {
+      // Idempotent and immutable, don't bother locking.
+      accounts = ImmutableSet.copyOf(table.columnKeySet());
+    }
+    return accounts;
+  }
+
+  public ImmutableSet<Account.Id> byState(ReviewerStateInternal state) {
+    return table.row(state).keySet();
+  }
+
+  public ImmutableTable<ReviewerStateInternal, Account.Id, Timestamp>
+      asTable() {
+    return table;
+  }
+
+  @Override
+  public boolean equals(Object o) {
+    return (o instanceof ReviewerSet) && table.equals(((ReviewerSet) o).table);
+  }
+
+  @Override
+  public int hashCode() {
+    return table.hashCode();
+  }
+
+  @Override
+  public String toString() {
+    return getClass().getSimpleName() + table;
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java

@@ -442,10 +442,10 @@ public class ChangeJson {
       out.removableReviewers = removableReviewers(ctl, out.labels.values());
 
       out.reviewers = new HashMap<>();
-      for (Map.Entry<ReviewerStateInternal, Collection<Account.Id>> e
-          : cd.reviewers().asMap().entrySet()) {
+      for (Map.Entry<ReviewerStateInternal, Map<Account.Id, Timestamp>> e
+          : cd.reviewers().asTable().rowMap().entrySet()) {
         out.reviewers.put(e.getKey().asReviewerState(),
-            toAccountInfo(e.getValue()));
+            toAccountInfo(e.getValue().keySet()));
       }
     }
 
@@ -617,7 +617,7 @@ public class ChangeJson {
     //  - They are an explicit reviewer.
     //  - They ever voted on this change.
     Set<Account.Id> allUsers = new HashSet<>();
-    allUsers.addAll(cd.reviewers().values());
+    allUsers.addAll(cd.reviewers().all());
     for (PatchSetApproval psa : cd.approvals().values()) {
       allUsers.add(psa.getAccountId());
     }

gerrit-server/src/main/java/com/google/gerrit/server/change/ListReviewers.java

@@ -51,7 +51,7 @@ class ListReviewers implements RestReadView<ChangeResource> {
     Map<Account.Id, ReviewerResource> reviewers = new LinkedHashMap<>();
     ReviewDb db = dbProvider.get();
     for (Account.Id accountId
-        : approvalsUtil.getReviewers(db, rsrc.getNotes()).values()) {
+        : approvalsUtil.getReviewers(db, rsrc.getNotes()).all()) {
       if (!reviewers.containsKey(accountId)) {
         reviewers.put(accountId, resourceFactory.create(rsrc, accountId));
       }

gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java

@@ -19,10 +19,8 @@ import static com.google.common.base.Preconditions.checkState;
 import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
 import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
 
-import com.google.common.collect.SetMultimap;
 import com.google.gerrit.common.ChangeHooks;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
-import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.PatchSet;
@@ -33,6 +31,7 @@ import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.ChangeMessagesUtil;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.events.CommitReceivedEvent;
 import com.google.gerrit.server.git.BanCommit;
 import com.google.gerrit.server.git.BatchUpdate;
@@ -43,7 +42,6 @@ import com.google.gerrit.server.git.validators.CommitValidationException;
 import com.google.gerrit.server.git.validators.CommitValidators;
 import com.google.gerrit.server.mail.ReplacePatchSetSender;
 import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.notedb.ReviewerStateInternal;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.RefControl;
@@ -106,7 +104,7 @@ public class PatchSetInserter extends BatchUpdate.Op {
   private PatchSet patchSet;
   private PatchSetInfo patchSetInfo;
   private ChangeMessage changeMessage;
-  private SetMultimap<ReviewerStateInternal, Account.Id> oldReviewers;
+  private ReviewerSet oldReviewers;
 
   @AssistedInject
   public PatchSetInserter(ChangeHooks hooks,
@@ -264,8 +262,8 @@ public class PatchSetInserter extends BatchUpdate.Op {
         cm.setFrom(ctx.getUser().getAccountId());
         cm.setPatchSet(patchSet, patchSetInfo);
         cm.setChangeMessage(changeMessage);
-        cm.addReviewers(oldReviewers.get(REVIEWER));
-        cm.addExtraCC(oldReviewers.get(CC));
+        cm.addReviewers(oldReviewers.byState(REVIEWER));
+        cm.addExtraCC(oldReviewers.byState(CC));
         cm.send();
       } catch (Exception err) {
         log.error("Cannot send email for new patch set on change "

gerrit-server/src/main/java/com/google/gerrit/server/change/PublishDraftPatchSet.java

@@ -207,7 +207,7 @@ public class PublishDraftPatchSet implements RestModifyView<RevisionResource, In
         throws OrmException, IOException {
       LabelTypes labelTypes = ctx.getControl().getLabelTypes();
       Collection<Account.Id> oldReviewers = approvalsUtil.getReviewers(
-          ctx.getDb(), ctx.getNotes()).values();
+          ctx.getDb(), ctx.getNotes()).all();
       RevCommit commit = ctx.getRevWalk().parseCommit(
           ObjectId.fromString(patchSet.getRevision().get()));
       patchSetInfo = patchSetInfoFactory.get(ctx.getRevWalk(), commit, psId);

gerrit-server/src/main/java/com/google/gerrit/server/change/Reviewers.java

@@ -83,6 +83,6 @@ public class Reviewers implements
   private Collection<Account.Id> fetchAccountIds(ChangeResource rsrc)
       throws OrmException {
     return approvalsUtil.getReviewers(
-        dbProvider.get(), rsrc.getNotes()).values();
+        dbProvider.get(), rsrc.getNotes()).all();
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java

@@ -203,7 +203,7 @@ public class EventFactory {
   public void addAllReviewers(ReviewDb db, ChangeAttribute a, ChangeNotes notes)
       throws OrmException {
     Collection<Account.Id> reviewers =
-        approvalsUtil.getReviewers(db, notes).values();
+        approvalsUtil.getReviewers(db, notes).all();
     if (!reviewers.isEmpty()) {
       a.allReviewers = Lists.newArrayListWithCapacity(reviewers.size());
       for (Account.Id id : reviewers) {

gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java

@@ -111,7 +111,7 @@ public abstract class ChangeEmail extends NotificationEmail {
     appendText(velocifyFile("ChangeFooter.vm"));
     try {
       TreeSet<String> names = new TreeSet<>();
-      for (Account.Id who : changeData.reviewers().values()) {
+      for (Account.Id who : changeData.reviewers().all()) {
         names.add(getNameEmailFor(who));
       }
       for (String name : names) {
@@ -337,7 +337,7 @@ public abstract class ChangeEmail extends NotificationEmail {
     }
 
     try {
-      for (Account.Id id : changeData.reviewers().values()) {
+      for (Account.Id id : changeData.reviewers().all()) {
         add(RecipientType.CC, id);
       }
     } catch (OrmException err) {
@@ -353,7 +353,7 @@ public abstract class ChangeEmail extends NotificationEmail {
     }
 
     try {
-      for (Account.Id id : changeData.reviewers().get(REVIEWER)) {
+      for (Account.Id id : changeData.reviewers().byState(REVIEWER)) {
         add(RecipientType.CC, id);
       }
     } catch (OrmException err) {

gerrit-server/src/main/java/com/google/gerrit/server/mail/MailUtil.java

@@ -17,12 +17,11 @@ package com.google.gerrit.server.mail;
 import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
 import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
 
-import com.google.common.collect.Multimap;
 import com.google.gerrit.common.FooterConstants;
 import com.google.gerrit.common.errors.NoSuchAccountException;
 import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.account.AccountResolver;
-import com.google.gerrit.server.notedb.ReviewerStateInternal;
 import com.google.gwtorm.server.OrmException;
 
 import org.eclipse.jgit.revwalk.FooterKey;
@@ -58,10 +57,10 @@ public class MailUtil {
   }
 
   public static MailRecipients getRecipientsFromReviewers(
-      Multimap<ReviewerStateInternal, Account.Id> reviewers) {
+      ReviewerSet reviewers) {
     MailRecipients recipients = new MailRecipients();
-    recipients.reviewers.addAll(reviewers.get(REVIEWER));
-    recipients.cc.addAll(reviewers.get(CC));
+    recipients.reviewers.addAll(reviewers.byState(REVIEWER));
+    recipients.cc.addAll(reviewers.byState(CC));
     return recipients;
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java

@@ -40,6 +40,7 @@ import com.google.common.collect.Maps;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.Ordering;
 import com.google.common.collect.Sets;
+import com.google.common.collect.Table;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
@@ -50,6 +51,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.PatchLineCommentsUtil;
+import com.google.gerrit.server.ReviewerSet;
 import com.google.gwtorm.client.Column;
 import com.google.gwtorm.server.OrmException;
 
@@ -59,6 +61,7 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Comparator;
+import java.util.HashMap;
 import java.util.Iterator;
 import java.util.LinkedList;
 import java.util.List;
@@ -84,12 +87,15 @@ public class ChangeBundle {
       throws OrmException {
     db.changes().beginTransaction(id);
     try {
+      List<PatchSetApproval> approvals =
+          db.patchSetApprovals().byChange(id).toList();
       return new ChangeBundle(
           db.changes().get(id),
           db.changeMessages().byChange(id),
           db.patchSets().byChange(id),
-          db.patchSetApprovals().byChange(id),
+          approvals,
           db.patchComments().byChange(id),
+          ReviewerSet.fromApprovals(approvals),
           Source.REVIEW_DB);
     } finally {
       db.rollback();
@@ -106,6 +112,7 @@ public class ChangeBundle {
         Iterables.concat(
             plcUtil.draftByChange(null, notes),
             plcUtil.publishedByChange(null, notes)),
+        notes.getReviewers(),
         Source.NOTE_DB);
   }
 
@@ -156,7 +163,6 @@ public class ChangeBundle {
     return CHANGE_MESSAGE_ORDER.immutableSortedCopy(in);
   }
 
-
   private static TreeMap<PatchSet.Id, PatchSet> patchSetMap(Iterable<PatchSet> in) {
     TreeMap<PatchSet.Id, PatchSet> out = new TreeMap<>(
         new Comparator<PatchSet.Id>() {
@@ -256,6 +262,7 @@ public class ChangeBundle {
       patchSetApprovals;
   private final ImmutableMap<PatchLineComment.Key, PatchLineComment>
       patchLineComments;
+  private final ReviewerSet reviewers;
   private final Source source;
 
   public ChangeBundle(
@@ -264,6 +271,7 @@ public class ChangeBundle {
       Iterable<PatchSet> patchSets,
       Iterable<PatchSetApproval> patchSetApprovals,
       Iterable<PatchLineComment> patchLineComments,
+      ReviewerSet reviewers,
       Source source) {
     this.change = checkNotNull(change);
     this.changeMessages = changeMessageList(changeMessages);
@@ -272,6 +280,7 @@ public class ChangeBundle {
         ImmutableMap.copyOf(patchSetApprovalMap(patchSetApprovals));
     this.patchLineComments =
         ImmutableMap.copyOf(patchLineCommentMap(patchLineComments));
+    this.reviewers = checkNotNull(reviewers);
     this.source = checkNotNull(source);
 
     for (ChangeMessage m : this.changeMessages) {
@@ -309,6 +318,10 @@ public class ChangeBundle {
     return patchLineComments.values();
   }
 
+  public ReviewerSet getReviewers() {
+    return reviewers;
+  }
+
   public Source getSource() {
     return source;
   }
@@ -319,6 +332,7 @@ public class ChangeBundle {
     diffChangeMessages(diffs, this, o);
     diffPatchSets(diffs, this, o);
     diffPatchSetApprovals(diffs, this, o);
+    diffReviewers(diffs, this, o);
     diffPatchLineComments(diffs, this, o);
     return ImmutableList.copyOf(diffs);
   }
@@ -661,6 +675,39 @@ public class ChangeBundle {
     }
   }
 
+  @AutoValue
+  static abstract class ReviewerKey {
+    private static Map<ReviewerKey, Timestamp> toMap(ReviewerSet reviewers) {
+      Map<ReviewerKey, Timestamp> result = new HashMap<>();
+      for (Table.Cell<ReviewerStateInternal, Account.Id, Timestamp> c :
+          reviewers.asTable().cellSet()) {
+        result.put(new AutoValue_ChangeBundle_ReviewerKey(
+            c.getRowKey(), c.getColumnKey()), c.getValue());
+      }
+      return result;
+    }
+
+    abstract ReviewerStateInternal state();
+    abstract Account.Id account();
+
+    @Override
+    public String toString() {
+      return state() + "," + account();
+    }
+  }
+
+  private static void diffReviewers(List<String> diffs,
+      ChangeBundle bundleA, ChangeBundle bundleB) {
+    Map<ReviewerKey, Timestamp> as = ReviewerKey.toMap(bundleA.reviewers);
+    Map<ReviewerKey, Timestamp> bs = ReviewerKey.toMap(bundleB.reviewers);
+    for (ReviewerKey k : diffKeySets(diffs, as, bs)) {
+      Timestamp a = as.get(k);
+      Timestamp b = bs.get(k);
+      String desc = describe(k);
+      diffTimestamps(diffs, desc, bundleA, a, bundleB, b, "timestamp");
+    }
+  }
+
   private static void diffPatchLineComments(List<String> diffs,
       ChangeBundle bundleA, ChangeBundle bundleB) {
     Map<PatchLineComment.Key, PatchLineComment> as =
@@ -825,9 +872,14 @@ public class ChangeBundle {
   private static String keyClass(Object obj) {
     Class<?> clazz = obj.getClass();
     String name = clazz.getSimpleName();
-    checkArgument(name.equals("Key") || name.equals("Id"),
+    checkArgument(name.endsWith("Key") || name.endsWith("Id"),
         "not an Id/Key class: %s", name);
-    return clazz.getEnclosingClass().getSimpleName() + "." + name;
+    if (name.equals("Key") || name.equals("Id")) {
+      return clazz.getEnclosingClass().getSimpleName() + "." + name;
+    } else if (name.startsWith("AutoValue_")) {
+      return name.substring(name.lastIndexOf('_') + 1);
+    }
+    return name;
   }
 
   @Override

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java

@@ -28,7 +28,6 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Iterables;
@@ -36,6 +35,7 @@ import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.Multimaps;
 import com.google.common.collect.Ordering;
+import com.google.common.collect.Tables;
 import com.google.common.util.concurrent.AsyncFunction;
 import com.google.common.util.concurrent.CheckedFuture;
 import com.google.common.util.concurrent.Futures;
@@ -55,6 +55,7 @@ import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.reviewdb.server.ReviewDbUtil;
+import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.git.RefCache;
 import com.google.gerrit.server.git.RepoRefCache;
 import com.google.gerrit.server.project.NoSuchChangeException;
@@ -381,7 +382,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
 
   private ImmutableSortedMap<PatchSet.Id, PatchSet> patchSets;
   private ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals;
-  private ImmutableSetMultimap<ReviewerStateInternal, Account.Id> reviewers;
+  private ReviewerSet reviewers;
   private ImmutableList<Account.Id> allPastReviewers;
   private ImmutableList<SubmitRecord> submitRecords;
   private ImmutableList<ChangeMessage> allChangeMessages;
@@ -420,7 +421,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     return approvals;
   }
 
-  public ImmutableSetMultimap<ReviewerStateInternal, Account.Id> getReviewers() {
+  public ReviewerSet getReviewers() {
     return reviewers;
   }
 
@@ -583,13 +584,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     } else {
       hashtags = ImmutableSet.of();
     }
-    ImmutableSetMultimap.Builder<ReviewerStateInternal, Account.Id> reviewers =
-        ImmutableSetMultimap.builder();
-    for (Map.Entry<Account.Id, ReviewerStateInternal> e
-        : parser.reviewers.entrySet()) {
-      reviewers.put(e.getValue(), e.getKey());
-    }
-    this.reviewers = reviewers.build();
+    this.reviewers = ReviewerSet.fromTable(Tables.transpose(parser.reviewers));
     this.allPastReviewers = ImmutableList.copyOf(parser.allPastReviewers);
 
     submitRecords = ImmutableList.copyOf(parser.submitRecords);
@@ -598,7 +593,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
   @Override
   protected void loadDefaults() {
     approvals = ImmutableListMultimap.of();
-    reviewers = ImmutableSetMultimap.of();
+    reviewers = ReviewerSet.empty();
     submitRecords = ImmutableList.of();
     allChangeMessages = ImmutableList.of();
     changeMessagesByPatchSet = ImmutableListMultimap.of();

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java

@@ -36,6 +36,7 @@ import com.google.common.base.Optional;
 import com.google.common.base.Splitter;
 import com.google.common.base.Supplier;
 import com.google.common.collect.ArrayListMultimap;
+import com.google.common.collect.HashBasedTable;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableSet;
@@ -94,7 +95,7 @@ class ChangeNotesParser {
   private static final RevId PARTIAL_PATCH_SET =
       new RevId("INVALID PARTIAL PATCH SET");
 
-  final Map<Account.Id, ReviewerStateInternal> reviewers;
+  final Table<Account.Id, ReviewerStateInternal, Timestamp> reviewers;
   final List<Account.Id> allPastReviewers;
   final List<SubmitRecord> submitRecords;
   final Multimap<RevId, PatchLineComment> comments;
@@ -134,7 +135,7 @@ class ChangeNotesParser {
     this.noteUtil = noteUtil;
     this.metrics = metrics;
     approvals = new HashMap<>();
-    reviewers = new LinkedHashMap<>();
+    reviewers = HashBasedTable.create();
     allPastReviewers = new ArrayList<>();
     submitRecords = Lists.newArrayListWithExpectedSize(1);
     allChangeMessages = new ArrayList<>();
@@ -156,7 +157,7 @@ class ChangeNotesParser {
         parse(commit);
       }
       parseNotes();
-      allPastReviewers.addAll(reviewers.keySet());
+      allPastReviewers.addAll(reviewers.rowKeySet());
       pruneReviewers();
       updatePatchSetStates();
       checkMandatoryFooters();
@@ -262,7 +263,7 @@ class ChangeNotesParser {
 
     for (ReviewerStateInternal state : ReviewerStateInternal.values()) {
       for (String line : commit.getFooterLineValues(state.getFooterKey())) {
-        parseReviewer(state, line);
+        parseReviewer(ts, state, line);
       }
       // Don't update timestamp when a reviewer was added, matching RevewDb
       // behavior.
@@ -698,27 +699,27 @@ class ChangeNotesParser {
     return noteUtil.parseIdent(commit.getAuthorIdent(), id);
   }
 
-  private void parseReviewer(ReviewerStateInternal state, String line)
-      throws ConfigInvalidException {
+  private void parseReviewer(Timestamp ts, ReviewerStateInternal state,
+      String line) throws ConfigInvalidException {
     PersonIdent ident = RawParseUtils.parsePersonIdent(line);
     if (ident == null) {
       throw invalidFooter(state.getFooterKey(), line);
     }
     Account.Id accountId = noteUtil.parseIdent(ident, id);
-    if (!reviewers.containsKey(accountId)) {
-      reviewers.put(accountId, state);
+    if (!reviewers.containsRow(accountId)) {
+      reviewers.put(accountId, state, ts);
     }
   }
 
   private void pruneReviewers() {
-    Iterator<Map.Entry<Account.Id, ReviewerStateInternal>> rit =
-        reviewers.entrySet().iterator();
+    Iterator<Table.Cell<Account.Id, ReviewerStateInternal, Timestamp>> rit =
+        reviewers.cellSet().iterator();
     while (rit.hasNext()) {
-      Map.Entry<Account.Id, ReviewerStateInternal> e = rit.next();
-      if (e.getValue() == ReviewerStateInternal.REMOVED) {
+      Table.Cell<Account.Id, ReviewerStateInternal, Timestamp> e = rit.next();
+      if (e.getColumnKey() == ReviewerStateInternal.REMOVED) {
         rit.remove();
         for (Table<Account.Id, ?, ?> curr : approvals.values()) {
-          curr.rowKeySet().remove(e.getKey());
+          curr.rowKeySet().remove(e.getRowKey());
         }
       }
     }

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java

@@ -36,6 +36,7 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.Ordering;
 import com.google.common.collect.Sets;
+import com.google.common.collect.Table;
 import com.google.common.primitives.Ints;
 import com.google.gerrit.common.FormatUtil;
 import com.google.gerrit.common.Nullable;
@@ -277,6 +278,11 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
       }
     }
 
+    for (Table.Cell<ReviewerStateInternal, Account.Id, Timestamp> r :
+        bundle.getReviewers().asTable().cellSet()) {
+      events.add(new ReviewerEvent(r, change.getCreatedOn()));
+    }
+
     Change noteDbChange = new Change(null, null, null, null, null);
     for (ChangeMessage msg : bundle.getChangeMessages()) {
       if (msg.getPatchSetId() == null || psIds.contains(msg.getPatchSetId())) {
@@ -714,6 +720,33 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
     }
   }
 
+  private static class ReviewerEvent extends Event {
+    private Table.Cell<ReviewerStateInternal, Account.Id, Timestamp> reviewer;
+
+    ReviewerEvent(
+        Table.Cell<ReviewerStateInternal, Account.Id, Timestamp> reviewer,
+        Timestamp changeCreatedOn) {
+      super(
+          // Reviewers aren't generally associated with a particular patch set
+          // (although as an implementation detail they were in ReviewDb). Just
+          // use the latest patch set at the time of the event.
+          null,
+          reviewer.getColumnKey(), reviewer.getValue(), changeCreatedOn, null);
+      this.reviewer = reviewer;
+    }
+
+    @Override
+    boolean uniquePerUpdate() {
+      return false;
+    }
+
+    @Override
+    void apply(ChangeUpdate update) throws IOException, OrmException {
+      checkUpdate(update);
+      update.putReviewer(reviewer.getColumnKey(), reviewer.getRowKey());
+    }
+  }
+
   private static class PatchSetEvent extends Event {
     private final Change change;
     private final PatchSet ps;

gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java

@@ -340,7 +340,7 @@ public class ChangeControl {
   public boolean isReviewer(ReviewDb db, @Nullable ChangeData cd)
       throws OrmException {
     if (getUser().isIdentifiedUser()) {
-      Collection<Account.Id> results = changeData(db, cd).reviewers().values();
+      Collection<Account.Id> results = changeData(db, cd).reviewers().all();
       return results.contains(getUser().getAccountId());
     }
     return false;

gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java

@@ -30,7 +30,6 @@ import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Multimap;
-import com.google.common.collect.SetMultimap;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.SubmitRecord;
 import com.google.gerrit.common.data.SubmitTypeRecord;
@@ -50,13 +49,13 @@ import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.PatchLineCommentsUtil;
 import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.StarredChangesUtil;
 import com.google.gerrit.server.change.MergeabilityCache;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.MergeUtil;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.NotesMigration;
-import com.google.gerrit.server.notedb.ReviewerStateInternal;
 import com.google.gerrit.server.patch.PatchList;
 import com.google.gerrit.server.patch.PatchListCache;
 import com.google.gerrit.server.patch.PatchListEntry;
@@ -905,8 +904,7 @@ public void setPatchSets(Collection<PatchSet> patchSets) {
     return Optional.absent();
   }
 
-  public SetMultimap<ReviewerStateInternal, Account.Id> reviewers()
-      throws OrmException {
+  public ReviewerSet reviewers() throws OrmException {
     return approvalsUtil.getReviewers(notes(), approvals().values());
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerPredicate.java

@@ -40,7 +40,7 @@ class ReviewerPredicate extends IndexPredicate<ChangeData> {
         object.change().getStatus() == Change.Status.DRAFT) {
       return false;
     }
-    for (Account.Id accountId : object.reviewers().values()) {
+    for (Account.Id accountId : object.reviewers().all()) {
       if (id.equals(accountId)) {
         return true;
       }

gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerinPredicate.java

@@ -37,7 +37,7 @@ class ReviewerinPredicate extends OperatorPredicate<ChangeData> {
 
   @Override
   public boolean match(final ChangeData object) throws OrmException {
-    for (Account.Id accountId : object.reviewers().values()) {
+    for (Account.Id accountId : object.reviewers().all()) {
       IdentifiedUser reviewer = userFactory.create(accountId);
       if (reviewer.getEffectiveGroups().contains(uuid)) {
         return true;