Factor out common code from RepoSequence

IntBlob is a simple value class encapsulating the results of reading an
integer blob from a given object ID in a repo. It includes factory
methods for reading/writing blobs to/from the repo, in an interface that
provides everything RepoSequence needs.

In the write case, RepoSequence needs two methods:
  * A tryStore method which returns a completed RefUpdate, which may or
    may not have succeeded. This return value is essentially an
    encapsulation of the RefUpdate.Result and the ref name; the latter
    is necessary for producing detailed error messages.
  * A checked store method which throws an exception if the result of
    tryStore was not success. This in turn delegates to a new
    RefUpdateUtil#checkResult method paralleling the existing
    checkResults.

Change-Id: Ibbe99972697cb4cb837673f19d7a6fac8b90fce0

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

@@ -0,0 +1,126 @@
+// Copyright (C) 2018 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;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.base.CharMatcher;
+import com.google.common.primitives.Ints;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
+import com.google.gerrit.server.update.RefUpdateUtil;
+import com.google.gwtorm.server.OrmException;
+import java.io.IOException;
+import java.util.Optional;
+import org.eclipse.jgit.errors.IncorrectObjectTypeException;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.lib.ObjectLoader;
+import org.eclipse.jgit.lib.ObjectReader;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.RefUpdate;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevWalk;
+
+@AutoValue
+public abstract class IntBlob {
+  public static Optional<IntBlob> parse(Repository repo, String refName)
+      throws IOException, OrmException {
+    try (ObjectReader or = repo.newObjectReader()) {
+      return parse(repo, refName, or);
+    }
+  }
+
+  public static Optional<IntBlob> parse(Repository repo, String refName, RevWalk rw)
+      throws IOException, OrmException {
+    return parse(repo, refName, rw.getObjectReader());
+  }
+
+  private static Optional<IntBlob> parse(Repository repo, String refName, ObjectReader or)
+      throws IOException, OrmException {
+    Ref ref = repo.exactRef(refName);
+    if (ref == null) {
+      return Optional.empty();
+    }
+    ObjectId id = ref.getObjectId();
+    ObjectLoader ol = or.open(id, OBJ_BLOB);
+    if (ol.getType() != OBJ_BLOB) {
+      // In theory this should be thrown by open but not all implementations may do it properly
+      // (certainly InMemoryRepository doesn't).
+      throw new IncorrectObjectTypeException(id, OBJ_BLOB);
+    }
+    String str = CharMatcher.whitespace().trimFrom(new String(ol.getCachedBytes(), UTF_8));
+    Integer value = Ints.tryParse(str);
+    if (value == null) {
+      throw new OrmException("invalid value in " + refName + " blob at " + id.name());
+    }
+    return Optional.of(IntBlob.create(id, value));
+  }
+
+  public static RefUpdate tryStore(
+      Repository repo,
+      RevWalk rw,
+      Project.NameKey projectName,
+      String refName,
+      @Nullable ObjectId oldId,
+      int val,
+      GitReferenceUpdated gitRefUpdated)
+      throws IOException {
+    ObjectId newId;
+    try (ObjectInserter ins = repo.newObjectInserter()) {
+      newId = ins.insert(OBJ_BLOB, Integer.toString(val).getBytes(UTF_8));
+      ins.flush();
+    }
+    RefUpdate ru = repo.updateRef(refName);
+    if (oldId != null) {
+      ru.setExpectedOldObjectId(oldId);
+    }
+    ru.disableRefLog();
+    ru.setNewObjectId(newId);
+    ru.setForceUpdate(true); // Required for non-commitish updates.
+    RefUpdate.Result result = ru.update(rw);
+    if (refUpdated(result)) {
+      gitRefUpdated.fire(projectName, ru, null);
+    }
+    return ru;
+  }
+
+  public static void store(
+      Repository repo,
+      RevWalk rw,
+      Project.NameKey projectName,
+      String refName,
+      @Nullable ObjectId oldId,
+      int val,
+      GitReferenceUpdated gitRefUpdated)
+      throws IOException {
+    RefUpdateUtil.checkResult(tryStore(repo, rw, projectName, refName, oldId, val, gitRefUpdated));
+  }
+
+  private static boolean refUpdated(RefUpdate.Result result) {
+    return result == RefUpdate.Result.NEW || result == RefUpdate.Result.FORCED;
+  }
+
+  private static IntBlob create(ObjectId id, int value) {
+    return new AutoValue_IntBlob(id, value);
+  }
+
+  public abstract ObjectId id();
+
+  public abstract int value();
+}

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

@@ -27,33 +27,27 @@ import com.github.rholder.retry.RetryerBuilder;
 import com.github.rholder.retry.StopStrategies;
 import com.github.rholder.retry.WaitStrategies;
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.CharMatcher;
-import com.google.common.base.Predicates;
 import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
-import com.google.common.primitives.Ints;
 import com.google.common.util.concurrent.Runnables;
-import com.google.gerrit.common.Nullable;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.update.RefUpdateUtil;
 import com.google.gwtorm.server.OrmException;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Optional;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
-import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
-import org.eclipse.jgit.lib.ObjectLoader;
-import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.RefUpdate;
-import org.eclipse.jgit.lib.RefUpdate.Result;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.transport.ReceiveCommand;
@@ -75,9 +69,9 @@ public class RepoSequence {
   }
 
   @VisibleForTesting
-  static RetryerBuilder<RefUpdate.Result> retryerBuilder() {
+  static RetryerBuilder<RefUpdate> retryerBuilder() {
-    return RetryerBuilder.<RefUpdate.Result>newBuilder()
+    return RetryerBuilder.<RefUpdate>newBuilder()
-        .retryIfResult(Predicates.equalTo(RefUpdate.Result.LOCK_FAILURE))
+        .retryIfResult(ru -> ru != null && RefUpdate.Result.LOCK_FAILURE.equals(ru.getResult()))
         .withWaitStrategy(
             WaitStrategies.join(
                 WaitStrategies.exponentialWait(5, TimeUnit.SECONDS),
@@ -85,7 +79,7 @@ public class RepoSequence {
         .withStopStrategy(StopStrategies.stopAfterDelay(30, TimeUnit.SECONDS));
   }
 
-  private static final Retryer<RefUpdate.Result> RETRYER = retryerBuilder().build();
+  private static final Retryer<RefUpdate> RETRYER = retryerBuilder().build();
 
   private final GitRepositoryManager repoManager;
   private final GitReferenceUpdated gitRefUpdated;
@@ -95,7 +89,7 @@ public class RepoSequence {
   private final int floor;
   private final int batchSize;
   private final Runnable afterReadRef;
-  private final Retryer<RefUpdate.Result> retryer;
+  private final Retryer<RefUpdate> retryer;
 
   // Protects all non-final fields.
   private final Lock counterLock;
@@ -153,7 +147,7 @@ public class RepoSequence {
       Seed seed,
       int batchSize,
       Runnable afterReadRef,
-      Retryer<RefUpdate.Result> retryer) {
+      Retryer<RefUpdate> retryer) {
     this(repoManager, gitRefUpdated, projectName, name, seed, batchSize, afterReadRef, retryer, 0);
   }
 
@@ -165,7 +159,7 @@ public class RepoSequence {
       Seed seed,
       int batchSize,
       Runnable afterReadRef,
-      Retryer<RefUpdate.Result> retryer,
+      Retryer<RefUpdate> retryer,
       int floor) {
     this.repoManager = requireNonNull(repoManager, "repoManager");
     this.gitRefUpdated = requireNonNull(gitRefUpdated, "gitRefUpdated");
@@ -234,7 +228,7 @@ public class RepoSequence {
     try {
       try (Repository repo = repoManager.openRepository(projectName);
           RevWalk rw = new RevWalk(repo)) {
-        checkResult(store(repo, rw, null, val));
+        IntBlob.store(repo, rw, projectName, refName, null, val, gitRefUpdated);
         counter = limit;
       } catch (IOException e) {
         throw new OrmException(e);
@@ -250,7 +244,11 @@ public class RepoSequence {
       try (Repository repo = repoManager.openRepository(projectName);
           RevWalk rw = new RevWalk(repo)) {
         TryIncreaseTo attempt = new TryIncreaseTo(repo, rw, val);
-        checkResult(retryer.call(attempt));
+        RefUpdate ru = retryer.call(attempt);
+        // Null update is a sentinel meaning nothing to do.
+        if (ru != null) {
+          RefUpdateUtil.checkResult(ru);
+        }
         counter = limit;
       } catch (ExecutionException | RetryException e) {
         if (e.getCause() != null) {
@@ -269,7 +267,7 @@ public class RepoSequence {
     try (Repository repo = repoManager.openRepository(projectName);
         RevWalk rw = new RevWalk(repo)) {
       TryAcquire attempt = new TryAcquire(repo, rw, count);
-      checkResult(retryer.call(attempt));
+      RefUpdateUtil.checkResult(retryer.call(attempt));
       counter = attempt.next;
       limit = counter + count;
       acquireCount++;
@@ -283,17 +281,7 @@ public class RepoSequence {
     }
   }
 
-  private void checkResult(RefUpdate.Result result) throws OrmException {
+  private class TryAcquire implements Callable<RefUpdate> {
-    if (!refUpdated(result) && result != Result.NO_CHANGE) {
-      throw new OrmException("failed to update " + refName + ": " + result);
-    }
-  }
-
-  private boolean refUpdated(RefUpdate.Result result) {
-    return result == RefUpdate.Result.NEW || result == RefUpdate.Result.FORCED;
-  }
-
-  private class TryAcquire implements Callable<RefUpdate.Result> {
     private final Repository repo;
     private final RevWalk rw;
     private final int count;
@@ -307,23 +295,23 @@ public class RepoSequence {
     }
 
     @Override
-    public RefUpdate.Result call() throws Exception {
+    public RefUpdate call() throws Exception {
-      Ref ref = repo.exactRef(refName);
+      Optional<IntBlob> blob = IntBlob.parse(repo, refName, rw);
       afterReadRef.run();
       ObjectId oldId;
-      if (ref == null) {
+      if (!blob.isPresent()) {
         oldId = ObjectId.zeroId();
         next = seed.get();
       } else {
-        oldId = ref.getObjectId();
+        oldId = blob.get().id();
-        next = parse(rw, oldId);
+        next = blob.get().value();
       }
       next = Math.max(floor, next);
-      return store(repo, rw, oldId, next + count);
+      return IntBlob.tryStore(repo, rw, projectName, refName, oldId, next + count, gitRefUpdated);
     }
   }
 
-  private class TryIncreaseTo implements Callable<RefUpdate.Result> {
+  private class TryIncreaseTo implements Callable<RefUpdate> {
     private final Repository repo;
     private final RevWalk rw;
     private final int value;
@@ -335,60 +323,26 @@ public class RepoSequence {
     }
 
     @Override
-    public RefUpdate.Result call() throws Exception {
+    public RefUpdate call() throws Exception {
-      Ref ref = repo.exactRef(refName);
+      Optional<IntBlob> blob = IntBlob.parse(repo, refName, rw);
       afterReadRef.run();
       ObjectId oldId;
-      if (ref == null) {
+      if (!blob.isPresent()) {
         oldId = ObjectId.zeroId();
       } else {
-        oldId = ref.getObjectId();
+        oldId = blob.get().id();
-        int next = parse(rw, oldId);
+        int next = blob.get().value();
         if (next >= value) {
-          // a concurrent write updated the ref already to this or a higher value
+          // A concurrent write updated the ref already to this or a higher value; return null as a
-          return RefUpdate.Result.NO_CHANGE;
+          // sentinel meaning nothing to do. Returning RefUpdate doesn't give us the flexibility to
+          // return any other kind of sentinel, since it's a fairly thick object.
+          return null;
         }
       }
-      return store(repo, rw, oldId, value);
+      return IntBlob.tryStore(repo, rw, projectName, refName, oldId, value, gitRefUpdated);
     }
   }
 
-  private int parse(RevWalk rw, ObjectId id) throws IOException, OrmException {
-    ObjectLoader ol = rw.getObjectReader().open(id, OBJ_BLOB);
-    if (ol.getType() != OBJ_BLOB) {
-      // In theory this should be thrown by open but not all implementations
-      // may do it properly (certainly InMemoryRepository doesn't).
-      throw new IncorrectObjectTypeException(id, OBJ_BLOB);
-    }
-    String str = CharMatcher.whitespace().trimFrom(new String(ol.getCachedBytes(), UTF_8));
-    Integer val = Ints.tryParse(str);
-    if (val == null) {
-      throw new OrmException("invalid value in " + refName + " blob at " + id.name());
-    }
-    return val;
-  }
-
-  private RefUpdate.Result store(Repository repo, RevWalk rw, @Nullable ObjectId oldId, int val)
-      throws IOException {
-    ObjectId newId;
-    try (ObjectInserter ins = repo.newObjectInserter()) {
-      newId = ins.insert(OBJ_BLOB, Integer.toString(val).getBytes(UTF_8));
-      ins.flush();
-    }
-    RefUpdate ru = repo.updateRef(refName);
-    if (oldId != null) {
-      ru.setExpectedOldObjectId(oldId);
-    }
-    ru.disableRefLog();
-    ru.setNewObjectId(newId);
-    ru.setForceUpdate(true); // Required for non-commitish updates.
-    RefUpdate.Result result = ru.update(rw);
-    if (refUpdated(result)) {
-      gitRefUpdated.fire(projectName, ru, null);
-    }
-    return result;
-  }
-
   public static ReceiveCommand storeNew(ObjectInserter ins, String name, int val)
       throws IOException {
     ObjectId newId = ins.insert(OBJ_BLOB, Integer.toString(val).getBytes(UTF_8));

java/com/google/gerrit/server/update/RefUpdateUtil.java

@@ -104,6 +104,32 @@ public class RefUpdateUtil {
     }
   }
 
+  /**
+   * Check results of a single ref update, throwing an exception if there was a failure.
+   *
+   * @param ru ref update; must already have been executed.
+   * @throws IllegalArgumentException if the result was {@code NOT_ATTEMPTED}.
+   * @throws LockFailureException if the result was {@code LOCK_FAILURE}.
+   * @throws IOException if the result failed for another reason.
+   */
+  public static void checkResult(RefUpdate ru) throws IOException {
+    RefUpdate.Result result = ru.getResult();
+    switch (result) {
+      case NOT_ATTEMPTED:
+        throw new IllegalArgumentException("Not attempted: " + ru.getName());
+      case NEW:
+      case FORCED:
+      case NO_CHANGE:
+      case FAST_FORWARD:
+      case RENAMED:
+        return;
+      case LOCK_FAILURE:
+        throw new LockFailureException("Failed to update " + ru.getName() + ": " + result, ru);
+      default:
+        throw new IOException("Failed to update " + ru.getName() + ": " + ru.getResult());
+    }
+  }
+
   /**
    * Delete a single ref, throwing a checked exception on failure.
    *

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

@@ -46,7 +46,7 @@ import org.junit.rules.ExpectedException;
 
 public class RepoSequenceTest {
   // Don't sleep in tests.
-  private static final Retryer<RefUpdate.Result> RETRYER =
+  private static final Retryer<RefUpdate> RETRYER =
       RepoSequence.retryerBuilder().withBlockStrategy(t -> {}).build();
 
   @Rule public ExpectedException exception = ExpectedException.none();
@@ -200,11 +200,11 @@ public class RepoSequenceTest {
             1,
             10,
             () -> writeBlob("id", Integer.toString(bgCounter.getAndAdd(1000))),
-            RetryerBuilder.<RefUpdate.Result>newBuilder()
+            RetryerBuilder.<RefUpdate>newBuilder()
                 .withStopStrategy(StopStrategies.stopAfterAttempt(3))
                 .build());
     exception.expect(OrmException.class);
-    exception.expectMessage("failed to update refs/sequences/id: LOCK_FAILURE");
+    exception.expectMessage("Failed to update refs/sequences/id: LOCK_FAILURE");
     s.next();
   }
 
@@ -335,11 +335,11 @@ public class RepoSequenceTest {
             1,
             10,
             () -> writeBlob("id", Integer.toString(bgCounter.getAndAdd(1000))),
-            RetryerBuilder.<RefUpdate.Result>newBuilder()
+            RetryerBuilder.<RefUpdate>newBuilder()
                 .withStopStrategy(StopStrategies.stopAfterAttempt(3))
                 .build());
     exception.expect(OrmException.class);
-    exception.expectMessage("failed to update refs/sequences/id: LOCK_FAILURE");
+    exception.expectMessage("Failed to update refs/sequences/id: LOCK_FAILURE");
     s.increaseTo(2);
   }
 
@@ -352,7 +352,7 @@ public class RepoSequenceTest {
       final int start,
       int batchSize,
       Runnable afterReadRef,
-      Retryer<RefUpdate.Result> retryer) {
+      Retryer<RefUpdate> retryer) {
     return new RepoSequence(
         repoManager,
         GitReferenceUpdated.DISABLED,