Record the number of updates to a NoteDb ref

Eventually we may want to limit the total length of a NoteDb ref's
history, to avoid having to scan and store an unbounded amount of commit
data into ChangeNotes. To do that, we first need to record it.

In most cases, this number will equal getChangeMessages().size(), but
that's partly an artifact of the current APIs, which always accompany a
change with a ChangeMessage. The underlying storage layer doesn't
require it.

Don't bump the ChangeNotesCache version number: old entries will just
return an update count of 0. My current thinking is that populating this
field is not worth flushing the persistent cache. If we change our
minds, we can trivially bump the version number.

Change-Id: I9fbcdfa0583a525e60b75672032f3179e5da3c79

java/com/google/gerrit/server/notedb/ChangeNotes.java

@@ -418,6 +418,10 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     return commentKeys;
   }
 
+  public int getUpdateCount() {
+    return state.updateCount();
+  }
+
   public ImmutableListMultimap<RevId, Comment> getDraftComments(Account.Id author) {
     return getDraftComments(author, null);
   }

java/com/google/gerrit/server/notedb/ChangeNotesCache.java

@@ -112,8 +112,11 @@ public class ChangeNotesCache {
     // Single pointer overhead.
     private static final int P = 8;
 
+    // Single int overhead.
+    private static final int I = 4;
+
     // Single IntKey overhead.
-    private static final int K = O + 4;
+    private static final int K = O + I;
 
     // Single Timestamp overhead.
     private static final int T = O + 8;
@@ -173,7 +176,8 @@ public class ChangeNotesCache {
           + map(state.publishedComments().asMap(), comment())
           + 1 // isPrivate
           + 1 // workInProgress
-          + 1; // reviewStarted
+          + 1 // reviewStarted
+          + I; // updateCount
     }
 
     private static int ptr(Object o, int size) {

java/com/google/gerrit/server/notedb/ChangeNotesParser.java

@@ -166,6 +166,7 @@ class ChangeNotesParser {
   private ReviewerSet pendingReviewers;
   private ReviewerByEmailSet pendingReviewersByEmail;
   private Change.Id revertOf;
+  private int updateCount;
 
   ChangeNotesParser(
       Change.Id changeId,
@@ -264,7 +265,8 @@ class ChangeNotesParser {
         firstNonNull(isPrivate, false),
         firstNonNull(workInProgress, false),
         firstNonNull(hasReviewStarted, true),
-        revertOf);
+        revertOf,
+        updateCount);
   }
 
   private PatchSet.Id buildCurrentPatchSetId() {
@@ -311,6 +313,7 @@ class ChangeNotesParser {
   }
 
   private void parse(ChangeNotesCommit commit) throws ConfigInvalidException {
+    updateCount++;
     Timestamp ts = new Timestamp(commit.getCommitterIdent().getWhen().getTime());
 
     createdOn = ts;

java/com/google/gerrit/server/notedb/ChangeNotesState.java

@@ -121,7 +121,8 @@ public abstract class ChangeNotesState {
       boolean isPrivate,
       boolean workInProgress,
       boolean reviewStarted,
-      @Nullable Change.Id revertOf) {
+      @Nullable Change.Id revertOf,
+      int updateCount) {
     requireNonNull(
         metaId,
         () ->
@@ -165,6 +166,7 @@ public abstract class ChangeNotesState {
         .submitRecords(submitRecords)
         .changeMessages(changeMessages)
         .publishedComments(publishedComments)
+        .updateCount(updateCount)
         .build();
   }
 
@@ -297,6 +299,8 @@ public abstract class ChangeNotesState {
 
   abstract ImmutableListMultimap<RevId, Comment> publishedComments();
 
+  abstract int updateCount();
+
   Change newChange(Project.NameKey project) {
     ChangeColumns c = requireNonNull(columns(), "columns are required");
     Change change =
@@ -363,7 +367,8 @@ public abstract class ChangeNotesState {
           .reviewerUpdates(ImmutableList.of())
           .submitRecords(ImmutableList.of())
           .changeMessages(ImmutableList.of())
-          .publishedComments(ImmutableListMultimap.of());
+          .publishedComments(ImmutableListMultimap.of())
+          .updateCount(0);
     }
 
     abstract Builder metaId(ObjectId metaId);
@@ -398,6 +403,8 @@ public abstract class ChangeNotesState {
 
     abstract Builder publishedComments(ListMultimap<RevId, Comment> publishedComments);
 
+    abstract Builder updateCount(int updateCount);
+
     abstract ChangeNotesState build();
   }
 
@@ -459,6 +466,7 @@ public abstract class ChangeNotesState {
           .changeMessages()
           .forEach(m -> b.addChangeMessage(toByteString(m, ChangeMessageProtoConverter.INSTANCE)));
       object.publishedComments().values().forEach(c -> b.addPublishedComment(GSON.toJson(c)));
+      b.setUpdateCount(object.updateCount());
 
       return Protos.toByteArray(b.build());
     }
@@ -577,7 +585,8 @@ public abstract class ChangeNotesState {
               .publishedComments(
                   proto.getPublishedCommentList().stream()
                       .map(r -> GSON.fromJson(r, Comment.class))
-                      .collect(toImmutableListMultimap(c -> new RevId(c.revId), c -> c)));
+                      .collect(toImmutableListMultimap(c -> new RevId(c.revId), c -> c)))
+              .updateCount(proto.getUpdateCount());
       return b.build();
     }
 

javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java

@@ -703,6 +703,18 @@ public class ChangeNotesStateTest extends GerritBaseTests {
             .build());
   }
 
+  @Test
+  public void serializeUpdateCount() throws Exception {
+    assertRoundTrip(
+        newBuilder().updateCount(234).build(),
+        ChangeNotesStateProto.newBuilder()
+            .setMetaId(SHA_BYTES)
+            .setChangeId(ID.get())
+            .setColumns(colsProto)
+            .setUpdateCount(234)
+            .build());
+  }
+
   @Test
   public void changeNotesStateMethods() throws Exception {
     assertThatSerializedClass(ChangeNotesState.class)
@@ -733,6 +745,7 @@ public class ChangeNotesStateTest extends GerritBaseTests {
                 .put(
                     "publishedComments",
                     new TypeLiteral<ImmutableListMultimap<RevId, Comment>>() {}.getType())
+                .put("updateCount", int.class)
                 .build());
   }
 

javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java

@@ -3021,6 +3021,22 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     update.commit();
   }
 
+  @Test
+  public void updateCount() throws Exception {
+    Change c = newChange();
+    assertThat(newNotes(c).getUpdateCount()).isEqualTo(1);
+
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.putApproval("Code-Review", (short) -1);
+    update.commit();
+    assertThat(newNotes(c).getUpdateCount()).isEqualTo(2);
+
+    update = newUpdate(c, changeOwner);
+    update.putApproval("Code-Review", (short) 1);
+    update.commit();
+    assertThat(newNotes(c).getUpdateCount()).isEqualTo(3);
+  }
+
   private String readNote(ChangeNotes notes, ObjectId noteId) throws Exception {
     ObjectId dataId = notes.revisionNoteMap.noteMap.getNote(noteId).getData();
     return new String(rw.getObjectReader().open(dataId, OBJ_BLOB).getCachedBytes(), UTF_8);

proto/cache.proto

@@ -75,7 +75,7 @@ message ChangeNotesKeyProto {
 // Instead, we just take the tedious yet simple approach of having a "has_foo"
 // field for each nullable field "foo", indicating whether or not foo is null.
 //
-// Next ID: 19
+// Next ID: 20
 message ChangeNotesStateProto {
   // Effectively required, even though the corresponding ChangeNotesState field
   // is optional, since the field is only absent when NoteDb is disabled, in
@@ -183,6 +183,9 @@ message ChangeNotesStateProto {
 
   reserved 17;  // read_only_until
   reserved 18;  // has_read_only_until
+
+  // Number of updates to the change's meta ref.
+  int32 update_count = 19;
 }