opendev/gerrit
Code Issues Proposed changes

Maintain all attention set updates in ChangeNotesState

Browse Source 
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
This commit is contained in:
Gal Paikin
2020-09-30 21:46:45 +03:00
parent 0b568f3dba
commit 120bacaada
7 changed files with 127 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
java/com/google/gerrit/server/notedb/ChangeNotes.java
View File

@@ -391,6 +391,11 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
return state.attentionSet();
}
/** Returns all updates for the attention set. */
public ImmutableList<AttentionSetUpdate> 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.

4
java/com/google/gerrit/server/notedb/ChangeNotesCache.java
View File

@@ -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())

7
java/com/google/gerrit/server/notedb/ChangeNotesParser.java
View File

@@ -118,6 +118,8 @@ class ChangeNotesParser {
private final List<ReviewerStatusUpdate> reviewerUpdates;
/** Holds only the most recent update per user. Older updates are discarded. */
private final Map<Account.Id, AttentionSetUpdate> latestAttentionStatus;
/** Holds all updates to attention set. */
private final List<AttentionSetUpdate> allAttentionSetUpdates;
private final List<AssigneeStatusUpdate> assigneeUpdates;
private final List<SubmitRecord> 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());
}
}

29
java/com/google/gerrit/server/notedb/ChangeNotesState.java
View File

@@ -120,6 +120,7 @@ public abstract class ChangeNotesState {
List<Account.Id> allPastReviewers,
List<ReviewerStatusUpdate> reviewerUpdates,
Set<AttentionSetUpdate> attentionSetUpdates,
List<AttentionSetUpdate> allAttentionSetUpdates,
List<AssigneeStatusUpdate> assigneeUpdates,
List<SubmitRecord> submitRecords,
List<ChangeMessage> 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<ReviewerStatusUpdate> 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<AttentionSetUpdate> attentionSet();
/** Returns all attention set updates. */
abstract ImmutableList<AttentionSetUpdate> allAttentionSetUpdates();
abstract ImmutableList<AssigneeStatusUpdate> assigneeUpdates();
abstract ImmutableList<SubmitRecord> 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<AttentionSetUpdate> attentionSetUpdates);
abstract Builder allAttentionSetUpdates(List<AttentionSetUpdate> attentionSetUpdates);
abstract Builder assigneeUpdates(List<AssigneeStatusUpdate> assigneeUpdates);
abstract Builder submitRecords(List<SubmitRecord> 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<AttentionSetUpdate> toAllAttentionSetUpdates(
List<AttentionSetUpdateProto> protos) {
ImmutableList.Builder<AttentionSetUpdate> 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<AssigneeStatusUpdate> toAssigneeStatusUpdateList(
List<AssigneeStatusUpdateProto> protos) {
ImmutableList.Builder<AssigneeStatusUpdate> b = ImmutableList.builder();

36
javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
View File

@@ -633,6 +633,39 @@ public class ChangeNotesStateTest {
.build());
}
@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(
@@ -792,6 +825,9 @@ public class ChangeNotesStateTest {
.put(
"attentionSet",
new TypeLiteral<ImmutableSet<AttentionSetUpdate>>() {}.getType())
.put(
"allAttentionSetUpdates",
new TypeLiteral<ImmutableList<AttentionSetUpdate>>() {}.getType())
.put(
"assigneeUpdates",
new TypeLiteral<ImmutableList<AssigneeStatusUpdate>>() {}.getType())

42
javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
View File

@@ -698,6 +698,13 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
assertThat(notes.getAttentionSet()).isEmpty();
}
@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();
@@ -711,6 +718,19 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
assertThat(notes.getAttentionSet()).containsExactly(addTimestamp(attentionSetUpdate, c));
}
@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();
@@ -729,6 +749,28 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
assertThat(notes.getAttentionSet()).containsExactly(addTimestamp(attentionSetUpdate, c));
}
@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();

6
proto/cache.proto
View File

@@ -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