opendev/gerrit
Code Issues Proposed changes

ChangeRebuilder: Fuse StatusChangeEvent into ChangeMessageEvent

Browse Source 
Previously, parsing a ChangeMessage might have resulted in a
StatusChangeEvent as well as a ChangeMessageEvent. Depending on
ordering, the ChangeMessageEvent might have come first and thus gotten
grouped together into a different event group. This was happening for
ChangeIT#restore.

Avoid this possibility by setting the status from ChangeMessageEvent
directly, so the status update is guaranteed to be grouped with the
message that triggered it. If we had other ways of parsing status
updates, this would be less flexible, but really we don't. (And if we
did I might want to keep them fused for this reason as well.)

Change-Id: Ibfa5348c1c6e62b393923454507b4c57d25e6b95
This commit is contained in:
Dave Borowitz
2017-02-06 17:08:48 -05:00
committed by Edwin Kempin
parent 3192366781
commit 694ea78ba9
3 changed files with 51 additions and 132 deletions
Download Patch File Download Diff File Expand all files Collapse all files

51
gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeMessageEvent.java
View File

@@ -15,24 +15,38 @@
package com.google.gerrit.server.notedb.rebuild;
import com.google.common.base.MoreObjects.ToStringHelper;
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gwtorm.server.OrmException;
import java.sql.Timestamp;
import java.util.Map;
import java.util.Optional;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
class ChangeMessageEvent extends Event {
private static final ImmutableMap<Change.Status, Pattern> STATUS_PATTERNS =
ImmutableMap.of(
Change.Status.ABANDONED, Pattern.compile("^Abandoned(\n.*)*$"),
Change.Status.MERGED,
Pattern.compile(
"^Change has been successfully" + " (merged|cherry-picked|rebased|pushed).*$"),
Change.Status.NEW, Pattern.compile("^Restored(\n.*)*$"));
private static final Pattern TOPIC_SET_REGEXP = Pattern.compile("^Topic set to (.+)$");
private static final Pattern TOPIC_CHANGED_REGEXP =
Pattern.compile("^Topic changed from (.+) to (.+)$");
private static final Pattern TOPIC_REMOVED_REGEXP = Pattern.compile("^Topic (.+) removed$");
private final ChangeMessage message;
private final Change change;
private final Change noteDbChange;
private final Optional<Change.Status> status;
private final ChangeMessage message;
ChangeMessageEvent(ChangeMessage message, Change noteDbChange, Timestamp changeCreatedOn) {
ChangeMessageEvent(
Change change, Change noteDbChange, ChangeMessage message, Timestamp changeCreatedOn) {
super(
message.getPatchSetId(),
message.getAuthor(),
@@ -40,8 +54,10 @@ class ChangeMessageEvent extends Event {
message.getWrittenOn(),
changeCreatedOn,
message.getTag());
this.message = message;
this.change = change;
this.noteDbChange = noteDbChange;
this.message = message;
this.status = parseStatus(message);
}
@Override
@@ -49,16 +65,45 @@ class ChangeMessageEvent extends Event {
return true;
}
@Override
protected boolean isSubmit() {
return status.isPresent() && status.get() == Change.Status.MERGED;
}
@Override
protected boolean canHaveTag() {
return true;
}
@SuppressWarnings("deprecation")
@Override
void apply(ChangeUpdate update) throws OrmException {
checkUpdate(update);
update.setChangeMessage(message.getMessage());
setTopic(update);
if (status.isPresent()) {
Change.Status s = status.get();
update.fixStatus(s);
noteDbChange.setStatus(s);
if (s == Change.Status.MERGED) {
update.setSubmissionId(change.getSubmissionId());
noteDbChange.setSubmissionId(change.getSubmissionId());
}
}
}
private static Optional<Change.Status> parseStatus(ChangeMessage message) {
String msg = message.getMessage();
if (msg == null) {
return Optional.empty();
}
for (Map.Entry<Change.Status, Pattern> e : STATUS_PATTERNS.entrySet()) {
if (e.getValue().matcher(msg).matches()) {
return Optional.of(e.getKey());
}
}
return Optional.empty();
}
private void setTopic(ChangeUpdate update) {

18
gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java
View File

@@ -336,17 +336,15 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
Change noteDbChange = new Change(null, null, null, null, null);
for (ChangeMessage msg : bundle.getChangeMessages()) {
List<Event> msgEvents = parseChangeMessage(msg, change, noteDbChange);
Event msgEvent = new ChangeMessageEvent(change, noteDbChange, msg, change.getCreatedOn());
if (msg.getPatchSetId() != null) {
PatchSetEvent pse = patchSetEvents.get(msg.getPatchSetId());
if (pse == null) {
continue; // Ignore events for missing patch sets.
}
for (Event e : msgEvents) {
e.addDep(pse);
}
msgEvent.addDep(pse);
}
events.addAll(msgEvents);
events.add(msgEvent);
}
sortAndFillEvents(change, noteDbChange, bundle.getPatchSets(), events, minPsNum);
@@ -374,16 +372,6 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
}
}
private List<Event> parseChangeMessage(ChangeMessage msg, Change change, Change noteDbChange) {
List<Event> events = new ArrayList<>(2);
events.add(new ChangeMessageEvent(msg, noteDbChange, change.getCreatedOn()));
Optional<StatusChangeEvent> sce = StatusChangeEvent.parseFromMessage(msg, change, noteDbChange);
if (sce.isPresent()) {
events.add(sce.get());
}
return events;
}
private static Integer getMinPatchSetNum(ChangeBundle bundle) {
Integer minPsNum = null;
for (PatchSet ps : bundle.getPatchSets()) {

114
gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/StatusChangeEvent.java
View File

@@ -1,114 +0,0 @@
// 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.notedb.rebuild;
import com.google.common.base.MoreObjects.ToStringHelper;
import com.google.common.collect.ImmutableMap;
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;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gwtorm.server.OrmException;
import java.sql.Timestamp;
import java.util.Map;
import java.util.Optional;
import java.util.regex.Pattern;
class StatusChangeEvent extends Event {
private static final ImmutableMap<Change.Status, Pattern> PATTERNS =
ImmutableMap.of(
Change.Status.ABANDONED, Pattern.compile("^Abandoned(\n.*)*$"),
Change.Status.MERGED,
Pattern.compile(
"^Change has been successfully" + " (merged|cherry-picked|rebased|pushed).*$"),
Change.Status.NEW, Pattern.compile("^Restored(\n.*)*$"));
static Optional<StatusChangeEvent> parseFromMessage(
ChangeMessage message, Change change, Change noteDbChange) {
String msg = message.getMessage();
if (msg == null) {
return Optional.empty();
}
for (Map.Entry<Change.Status, Pattern> e : PATTERNS.entrySet()) {
if (e.getValue().matcher(msg).matches()) {
return Optional.of(new StatusChangeEvent(message, change, noteDbChange, e.getKey()));
}
}
return Optional.empty();
}
private final Change.Status status;
private final Change change;
private final Change noteDbChange;
private StatusChangeEvent(
ChangeMessage message, Change change, Change noteDbChange, Change.Status status) {
this(
message.getPatchSetId(),
message.getAuthor(),
message.getWrittenOn(),
change,
noteDbChange,
message.getTag(),
status);
}
private StatusChangeEvent(
PatchSet.Id psId,
Account.Id author,
Timestamp when,
Change change,
Change noteDbChange,
String tag,
Change.Status status) {
super(psId, author, author, when, change.getCreatedOn(), tag);
this.change = change;
this.noteDbChange = noteDbChange;
this.status = status;
}
@Override
boolean uniquePerUpdate() {
return true;
}
@SuppressWarnings("deprecation")
@Override
void apply(ChangeUpdate update) throws OrmException {
checkUpdate(update);
update.fixStatus(status);
noteDbChange.setStatus(status);
if (status == Change.Status.MERGED) {
update.setSubmissionId(change.getSubmissionId());
noteDbChange.setSubmissionId(change.getSubmissionId());
}
}
@Override
protected boolean isSubmit() {
return status == Change.Status.MERGED;
}
@Override
protected boolean canHaveTag() {
return isSubmit();
}
@Override
protected void addToString(ToStringHelper helper) {
helper.add("status", status);
}
}