opendev/gerrit
Code Issues Proposed changes

Remove mergeable bit from Change

Browse Source 
Upgrade the schema to remove the field, but do not actually migrate
data. This will be backfilled done during the next site reindex.

The reason for this suboptimal upgrade process is that it would be
nontrivial to copy the data in a schema upgrade, since the key for the
cache requires running the entire submit rule evaluator to get the
submit type. The dbInjector used to create the chain of SchemaVersions
has far too few bindings for this. Getting these at schema upgrade
time would require a completely different approach to injecting schema
upgrade steps.

Change-Id: I6fd6de3d06ed92ee4d2dbea43f3e9974e9491542
This commit is contained in:
Dave Borowitz
2014-09-30 16:56:45 -07:00
parent 10129d4af2
commit 1c8767ed14
8 changed files with 37 additions and 78 deletions
Download Patch File Download Diff File Expand all files Collapse all files

30
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java
View File

@@ -432,17 +432,6 @@ public final class Change {
@Column(id = 14, notNull = false)
protected String topic;
/**
* Null if the change has never been tested.
* Empty if it has been tested but against a branch that does
* not exist.
*/
@Column(id = 15, notNull = false)
protected RevId lastSha1MergeTested;
@Column(id = 16)
protected boolean mergeable;
protected Change() {
}
@@ -455,7 +444,6 @@ public final class Change {
owner = ownedBy;
dest = forBranch;
setStatus(Status.NEW);
setLastSha1MergeTested(null);
}
public Change(Change other) {
@@ -472,8 +460,6 @@ public final class Change {
currentPatchSetId = other.currentPatchSetId;
subject = other.subject;
topic = other.topic;
mergeable = other.mergeable;
lastSha1MergeTested = other.lastSha1MergeTested;
}
/** Legacy 32 bit integer identity for a change. */
@@ -564,20 +550,4 @@ public final class Change {
public void setTopic(String topic) {
this.topic = topic;
}
public RevId getLastSha1MergeTested() {
return lastSha1MergeTested;
}
public void setLastSha1MergeTested(RevId lastSha1MergeTested) {
this.lastSha1MergeTested = lastSha1MergeTested;
}
public boolean isMergeable() {
return mergeable;
}
public void setMergeable(boolean mergeable) {
this.mergeable = mergeable;
}
}

31
gerrit-server/src/main/java/com/google/gerrit/server/change/Mergeable.java
View File

@@ -22,8 +22,8 @@ import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.git.BranchOrderSection;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MergeUtil;
@@ -32,7 +32,6 @@ import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.project.SubmitRuleEvaluator;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.AtomicUpdate;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -118,7 +117,6 @@ public class Mergeable implements RestReadView<RevisionResource> {
throw new OrmException("Submit type rule failed: " + rec);
}
result.submitType = rec.type;
result.mergeable = change.isMergeable();
Repository git = gitManager.openRepository(change.getProject());
try {
@@ -136,7 +134,7 @@ public class Mergeable implements RestReadView<RevisionResource> {
cache.getIfPresent(commit, ref, result.submitType, strategy);
if (force || old == null) {
result.mergeable = refresh(change, ps, commit, ref, result.submitType,
result.mergeable = refresh(change, commit, ref, result.submitType,
strategy, git, old);
}
@@ -172,29 +170,14 @@ public class Mergeable implements RestReadView<RevisionResource> {
}
}
private boolean refresh(final Change change, final PatchSet ps,
ObjectId commit, final Ref ref, SubmitType type, String strategy,
Repository git, Boolean old) throws OrmException, IOException {
private boolean refresh(final Change change, ObjectId commit,
final Ref ref, SubmitType type, String strategy, Repository git,
Boolean old) throws OrmException, IOException {
final boolean mergeable =
cache.get(commit, ref, type, strategy, change.getDest(), git, db.get());
db.get().changes().atomicUpdate(
change.getId(),
new AtomicUpdate<Change>() {
@Override
public Change update(Change c) {
if (c.getStatus().isOpen()
&& ps.getId().equals(c.currentPatchSetId())) {
c.setMergeable(mergeable);
ObjectId into = MergeabilityCache.toId(ref);
c.setLastSha1MergeTested(new RevId(
!into.equals(ObjectId.zeroId()) ? into.name() : ""));
return c;
} else {
return null;
}
}
});
if (!Objects.equals(mergeable, old)) {
// TODO(dborowitz): Include cache info in ETag somehow instead.
ChangeUtil.bumpRowVersionNotLastUpdatedOn(change.getId(), db.get());
indexer.index(db.get(), change);
}
return mergeable;

1
gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
View File

@@ -269,7 +269,6 @@ public class PatchSetInserter {
if (change.getStatus() != Change.Status.DRAFT) {
change.setStatus(Change.Status.NEW);
}
change.setLastSha1MergeTested(null);
change.setCurrentPatchSet(patchSetInfoFactory.get(commit,
patchSet.getId()));
ChangeUtil.updated(change);

4
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
View File

@@ -966,10 +966,6 @@ public class MergeOp {
@Override
public Change update(Change c) {
c.setStatus(Change.Status.MERGED);
// It could be possible that the change being merged
// has never had its mergeability tested. So we insure
// merged changes has mergeable field true.
c.setMergeable(true);
if (!merged.equals(c.currentPatchSetId())) {
// Uncool; the patch set changed after we merged it.
// Go back to the patch set that was actually merged.

1
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@@ -2118,7 +2118,6 @@ public class ReceiveCommits {
} else {
change.setStatus(Change.Status.NEW);
}
change.setLastSha1MergeTested(null);
change.setCurrentPatchSet(info);
final List<String> idList = newCommit.getFooterLines(CHANGE_ID);

2
gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java
View File

@@ -32,7 +32,7 @@ import java.util.List;
/** A version of the database schema. */
public abstract class SchemaVersion {
/** The current schema version. */
public static final Class<Schema_99> C = Schema_99.class;
public static final Class<Schema_100> C = Schema_100.class;
public static class Module extends AbstractModule {
@Override

27
gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_100.java Normal file
View File

@@ -0,0 +1,27 @@
// Copyright (C) 2014 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.schema;
import com.google.inject.Inject;
import com.google.inject.Provider;
public class Schema_100 extends SchemaVersion {
@Inject
Schema_100(Provider<Schema_99> prior) {
super(prior);
}
// No database migration; merges are rechecked on reindex.
}

17
gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_59.java
View File

@@ -14,29 +14,14 @@
package com.google.gerrit.server.schema;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.sql.SQLException;
import java.util.List;
public class Schema_59 extends SchemaVersion {
@Inject
Schema_59(Provider<Schema_58> prior) {
super(prior);
}
@Override
protected void migrateData(ReviewDb db, UpdateUI ui) throws OrmException,
SQLException {
List<Change> allChanges = db.changes().all().toList();
for (Change change : allChanges) {
change.setMergeable(true);
change.setLastSha1MergeTested(null);
}
db.changes().update(allChanges);
}
// Don't migrate columns; they are removed in Schema_100.
}