commit 120bacaada31ce19550bc3cdb6e8c3b1297b9d2e Author: Gal Paikin Date: Wed Sep 30 21:46:45 2020 +0300 Maintain all attention set updates in ChangeNotesState This allows gathering change-based metrics on attention set history, and not only on the current status of the attention set (which will just be empty on the majority of cases on old changes). Change-Id: I6f8ffc72f533c4f706355820151ada74e3203421 diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java index e83d4fd..bd2e80c 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotes.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java @@ -391,6 +391,11 @@ public class ChangeNotes extends AbstractChangeNotes { return state.attentionSet(); } + /** Returns all updates for the attention set. */ + public ImmutableList getAttentionSetUpdates() { + return state.allAttentionSetUpdates(); + } + /** * @return an ImmutableSet of Account.Ids of all users that have been assigned to this change. The * order of the set is the order in which they were assigned. diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java index 7fde297..1650421 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java @@ -168,6 +168,10 @@ public class ChangeNotesCache { + P + list(state.assigneeUpdates(), 4 * O + K + K) + P + + set(state.attentionSet(), 4 * O + K + I + str(15)) + + P + + list(state.allAttentionSetUpdates(), 4 * O + K + I + str(15)) + + P + list(state.submitRecords(), P + list(2, str(4) + P + K) + P) + P + list(state.changeMessages(), changeMessage()) diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java index c92d236..fae29f8 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java @@ -118,6 +118,8 @@ class ChangeNotesParser { private final List reviewerUpdates; /** Holds only the most recent update per user. Older updates are discarded. */ private final Map latestAttentionStatus; + /** Holds all updates to attention set. */ + private final List allAttentionSetUpdates; private final List assigneeUpdates; private final List submitRecords; @@ -175,6 +177,7 @@ class ChangeNotesParser { allPastReviewers = new ArrayList<>(); reviewerUpdates = new ArrayList<>(); latestAttentionStatus = new HashMap<>(); + allAttentionSetUpdates = new ArrayList<>(); assigneeUpdates = new ArrayList<>(); submitRecords = Lists.newArrayListWithExpectedSize(1); allChangeMessages = new ArrayList<>(); @@ -246,6 +249,7 @@ class ChangeNotesParser { allPastReviewers, buildReviewerUpdates(), ImmutableSet.copyOf(latestAttentionStatus.values()), + allAttentionSetUpdates, assigneeUpdates, submitRecords, buildAllMessages(), @@ -589,6 +593,9 @@ class ChangeNotesParser { } // Processing is in reverse chronological order. Keep only the latest update. latestAttentionStatus.putIfAbsent(attentionStatus.get().account(), attentionStatus.get()); + + // Keep all updates as well. + allAttentionSetUpdates.add(attentionStatus.get()); } } diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesState.java b/java/com/google/gerrit/server/notedb/ChangeNotesState.java index 76c4678..98d6e6e 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotesState.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotesState.java @@ -120,6 +120,7 @@ public abstract class ChangeNotesState { List allPastReviewers, List reviewerUpdates, Set attentionSetUpdates, + List allAttentionSetUpdates, List assigneeUpdates, List submitRecords, List changeMessages, @@ -171,6 +172,7 @@ public abstract class ChangeNotesState { .allPastReviewers(allPastReviewers) .reviewerUpdates(reviewerUpdates) .attentionSet(attentionSetUpdates) + .allAttentionSetUpdates(allAttentionSetUpdates) .assigneeUpdates(assigneeUpdates) .submitRecords(submitRecords) .changeMessages(changeMessages) @@ -305,9 +307,12 @@ public abstract class ChangeNotesState { abstract ImmutableList reviewerUpdates(); - /** Returns the most recent update (i.e. current status status) per user. */ + /** Returns the most recent update (i.e. current status) per user. */ abstract ImmutableSet attentionSet(); + /** Returns all attention set updates. */ + abstract ImmutableList allAttentionSetUpdates(); + abstract ImmutableList assigneeUpdates(); abstract ImmutableList submitRecords(); @@ -386,6 +391,7 @@ public abstract class ChangeNotesState { .allPastReviewers(ImmutableList.of()) .reviewerUpdates(ImmutableList.of()) .attentionSet(ImmutableSet.of()) + .allAttentionSetUpdates(ImmutableList.of()) .assigneeUpdates(ImmutableList.of()) .submitRecords(ImmutableList.of()) .changeMessages(ImmutableList.of()) @@ -421,6 +427,8 @@ public abstract class ChangeNotesState { abstract Builder attentionSet(Set attentionSetUpdates); + abstract Builder allAttentionSetUpdates(List attentionSetUpdates); + abstract Builder assigneeUpdates(List assigneeUpdates); abstract Builder submitRecords(List submitRecords); @@ -489,6 +497,9 @@ public abstract class ChangeNotesState { object.allPastReviewers().forEach(a -> b.addPastReviewer(a.get())); object.reviewerUpdates().forEach(u -> b.addReviewerUpdate(toReviewerStatusUpdateProto(u))); object.attentionSet().forEach(u -> b.addAttentionSetUpdate(toAttentionSetUpdateProto(u))); + object + .allAttentionSetUpdates() + .forEach(u -> b.addAllAttentionSetUpdate(toAttentionSetUpdateProto(u))); object.assigneeUpdates().forEach(u -> b.addAssigneeUpdate(toAssigneeStatusUpdateProto(u))); object .submitRecords() @@ -623,6 +634,8 @@ public abstract class ChangeNotesState { proto.getPastReviewerList().stream().map(Account::id).collect(toImmutableList())) .reviewerUpdates(toReviewerStatusUpdateList(proto.getReviewerUpdateList())) .attentionSet(toAttentionSetUpdates(proto.getAttentionSetUpdateList())) + .allAttentionSetUpdates( + toAllAttentionSetUpdates(proto.getAllAttentionSetUpdateList())) .assigneeUpdates(toAssigneeStatusUpdateList(proto.getAssigneeUpdateList())) .submitRecords( proto.getSubmitRecordList().stream() @@ -735,6 +748,20 @@ public abstract class ChangeNotesState { return b.build(); } + private static ImmutableList toAllAttentionSetUpdates( + List protos) { + ImmutableList.Builder b = ImmutableList.builder(); + for (AttentionSetUpdateProto proto : protos) { + b.add( + AttentionSetUpdate.createFromRead( + Instant.ofEpochMilli(proto.getTimestampMillis()), + Account.id(proto.getAccount()), + AttentionSetUpdate.Operation.valueOf(proto.getOperation()), + proto.getReason())); + } + return b.build(); + } + private static ImmutableList toAssigneeStatusUpdateList( List protos) { ImmutableList.Builder b = ImmutableList.builder(); diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java index dd3238f..321e4da 100644 --- a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java +++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java @@ -634,6 +634,39 @@ public class ChangeNotesStateTest { } @Test + public void serializeAllAttentionSetUpdates() throws Exception { + assertRoundTrip( + newBuilder() + .allAttentionSetUpdates( + ImmutableList.of( + AttentionSetUpdate.createFromRead( + Instant.EPOCH.plusSeconds(23), + Account.id(1000), + AttentionSetUpdate.Operation.ADD, + "reason 1"), + AttentionSetUpdate.createFromRead( + Instant.EPOCH.plusSeconds(42), + Account.id(2000), + AttentionSetUpdate.Operation.REMOVE, + "reason 2"))) + .build(), + newProtoBuilder() + .addAllAttentionSetUpdate( + AttentionSetUpdateProto.newBuilder() + .setTimestampMillis(23_000) // epoch millis + .setAccount(1000) + .setOperation("ADD") + .setReason("reason 1")) + .addAllAttentionSetUpdate( + AttentionSetUpdateProto.newBuilder() + .setTimestampMillis(42_000) // epoch millis + .setAccount(2000) + .setOperation("REMOVE") + .setReason("reason 2")) + .build()); + } + + @Test public void serializeAssigneeUpdates() throws Exception { assertRoundTrip( newBuilder() @@ -793,6 +826,9 @@ public class ChangeNotesStateTest { "attentionSet", new TypeLiteral>() {}.getType()) .put( + "allAttentionSetUpdates", + new TypeLiteral>() {}.getType()) + .put( "assigneeUpdates", new TypeLiteral>() {}.getType()) .put("submitRecords", new TypeLiteral>() {}.getType()) diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java index 938fffc..44dd831 100644 --- a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java +++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java @@ -699,6 +699,13 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { } @Test + public void defaultAttentionSetUpdatesIsEmpty() throws Exception { + Change c = newChange(); + ChangeNotes notes = newNotes(c); + assertThat(notes.getAttentionSetUpdates()).isEmpty(); + } + + @Test public void addAttentionStatus() throws Exception { Change c = newChange(); ChangeUpdate update = newUpdate(c, changeOwner); @@ -712,6 +719,19 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { } @Test + public void addAllAttentionUpdates() throws Exception { + Change c = newChange(); + ChangeUpdate update = newUpdate(c, changeOwner); + AttentionSetUpdate attentionSetUpdate = + AttentionSetUpdate.createForWrite(changeOwner.getAccountId(), Operation.ADD, "test"); + update.addToPlannedAttentionSetUpdates(ImmutableSet.of(attentionSetUpdate)); + update.commit(); + + ChangeNotes notes = newNotes(c); + assertThat(notes.getAttentionSetUpdates()).containsExactly(addTimestamp(attentionSetUpdate, c)); + } + + @Test public void filterLatestAttentionStatus() throws Exception { Change c = newChange(); ChangeUpdate update = newUpdate(c, changeOwner); @@ -730,6 +750,28 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { } @Test + public void DoesNotFilterLatestAttentionSetUpdates() throws Exception { + Change c = newChange(); + ChangeUpdate update = newUpdate(c, changeOwner); + AttentionSetUpdate firstAttentionSetUpdate = + AttentionSetUpdate.createForWrite(changeOwner.getAccountId(), Operation.ADD, "test"); + update.addToPlannedAttentionSetUpdates(ImmutableSet.of(firstAttentionSetUpdate)); + update.commit(); + update = newUpdate(c, changeOwner); + firstAttentionSetUpdate = addTimestamp(firstAttentionSetUpdate, c); + + AttentionSetUpdate secondAttentionSetUpdate = + AttentionSetUpdate.createForWrite(changeOwner.getAccountId(), Operation.REMOVE, "test"); + update.addToPlannedAttentionSetUpdates(ImmutableSet.of(secondAttentionSetUpdate)); + update.commit(); + secondAttentionSetUpdate = addTimestamp(secondAttentionSetUpdate, c); + + ChangeNotes notes = newNotes(c); + assertThat(notes.getAttentionSetUpdates()) + .containsExactly(secondAttentionSetUpdate, firstAttentionSetUpdate); + } + + @Test public void addAttentionStatus_rejectTimestamp() throws Exception { Change c = newChange(); ChangeUpdate update = newUpdate(c, changeOwner); diff --git a/proto/cache.proto b/proto/cache.proto index 7924cbd..b6225d9 100644 --- a/proto/cache.proto +++ b/proto/cache.proto @@ -76,7 +76,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: 24 +// Next ID: 25 message ChangeNotesStateProto { // Effectively required, even though the corresponding ChangeNotesState field // is optional, since the field is only absent when NoteDb is disabled, in @@ -218,7 +218,11 @@ message ChangeNotesStateProto { string operation = 3; string reason = 4; } + // Only includes the most recent attention set update for each user. repeated AttentionSetUpdateProto attention_set_update = 23; + + // Includes all attention set updates. + repeated AttentionSetUpdateProto all_attention_set_update = 24; } // Serialized form of com.google.gerrit.server.query.change.ConflictKey