Extract a type for sets of reviewers, and store timestamp

Change-Id: I946b3d89ff9b77a2eecb077b7c76735a8511eab1
2016-05-25 17:21:31 -04:00
parent b2719933ef
commit b01bd893cc
18 changed files with 191 additions and 111 deletions
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java
@@ -298,7 +298,7 @@ public class PushOneCommit {
        throws OrmException, NoSuchChangeException {
      Iterable<Account.Id> actualIds = approvalsUtil
          .getReviewers(db, notesFactory.createChecked(db, c))
-          .values();
+          .all();
      assertThat(actualIds).containsExactlyElementsIn(
          Sets.newHashSet(TestAccount.ids(expectedReviewers)));
    }
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
+++ b/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,
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ReviewerSet.java
+++ b/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;
+  }
+}
--- 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
@@ -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());
    }
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ListReviewers.java
+++ b/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));
      }
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/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 "
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishDraftPatchSet.java
+++ b/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);
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Reviewers.java
+++ b/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();
  }
 }
--- a/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java
+++ b/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) {
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java
+++ b/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) {
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/MailUtil.java
+++ b/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;
  }

--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/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();
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
+++ b/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());
        }
      }
    }
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
+++ b/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;
--- 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
@@ -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());
  }

--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerPredicate.java
+++ b/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;
      }
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerinPredicate.java
+++ b/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;
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -28,7 +28,7 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.ImmutableSetMultimap;
+import com.google.common.collect.ImmutableTable;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Lists;
@@ -46,6 +46,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
 import com.google.gerrit.testutil.TestChanges;
 import com.google.gwtorm.server.OrmException;
@@ -392,10 +393,12 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
    update.commit();

    ChangeNotes notes = newNotes(c);
-    assertThat(notes.getReviewers()).isEqualTo(
-        ImmutableSetMultimap.of(
-          REVIEWER, new Account.Id(1),
-          REVIEWER, new Account.Id(2)));
+    Timestamp ts = new Timestamp(update.getWhen().getTime());
+    assertThat(notes.getReviewers()).isEqualTo(ReviewerSet.fromTable(
+        ImmutableTable.<ReviewerStateInternal, Account.Id, Timestamp>builder()
+            .put(REVIEWER, new Account.Id(1), ts)
+            .put(REVIEWER, new Account.Id(2), ts)
+            .build()));
  }

  @Test
@@ -407,10 +410,12 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
    update.commit();

    ChangeNotes notes = newNotes(c);
-    assertThat(notes.getReviewers()).isEqualTo(
-        ImmutableSetMultimap.of(
-            REVIEWER, new Account.Id(1),
-            CC, new Account.Id(2)));
+    Timestamp ts = new Timestamp(update.getWhen().getTime());
+    assertThat(notes.getReviewers()).isEqualTo(ReviewerSet.fromTable(
+        ImmutableTable.<ReviewerStateInternal, Account.Id, Timestamp>builder()
+            .put(REVIEWER, new Account.Id(1), ts)
+            .put(CC, new Account.Id(2), ts)
+            .build()));
  }

  @Test
@@ -421,16 +426,18 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
    update.commit();

    ChangeNotes notes = newNotes(c);
-    assertThat(notes.getReviewers()).isEqualTo(
-        ImmutableSetMultimap.of(REVIEWER, new Account.Id(2)));
+    Timestamp ts = new Timestamp(update.getWhen().getTime());
+    assertThat(notes.getReviewers()).isEqualTo(ReviewerSet.fromTable(
+        ImmutableTable.of(REVIEWER, new Account.Id(2), ts)));

    update = newUpdate(c, otherUser);
    update.putReviewer(otherUser.getAccount().getId(), CC);
    update.commit();

    notes = newNotes(c);
-    assertThat(notes.getReviewers()).isEqualTo(
-        ImmutableSetMultimap.of(CC, new Account.Id(2)));
+    ts = new Timestamp(update.getWhen().getTime());
+    assertThat(notes.getReviewers()).isEqualTo(ReviewerSet.fromTable(
+        ImmutableTable.of(CC, new Account.Id(2), ts)));
  }

  @Test