Rename RefPair to RefUpdateSnapshot and clean it up - the old name RefPair was a misnomer, it's not a pair of refs - the new name RefUpdateSnapshot better describes the purpose of this class: it's an immutable snapshot of a JGit RefUpdate as the class javadoc already explained - reduce visibility of this class to package-visibility since it's only used within its enclosing package - add getters for all properties the callers need from a RefUpdateSnapshot Change-Id: I78c9d35c685beffbd3bc3d4ab18bd006d98382a6
diff --git a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/BatchRefUpdateValidator.java b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/BatchRefUpdateValidator.java index 1e45951..cbdb8cd 100644 --- a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/BatchRefUpdateValidator.java +++ b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/BatchRefUpdateValidator.java
@@ -140,22 +140,24 @@ return; } - List<RefPair> refsToUpdate = getRefPairs(commands).collect(Collectors.toList()); - List<RefPair> refsFailures = - refsToUpdate.stream().filter(RefPair::hasFailed).collect(Collectors.toList()); + List<RefUpdateSnapshot> refsToUpdate = + getRefUpdateSnapshots(commands).collect(Collectors.toList()); + List<RefUpdateSnapshot> refsFailures = + refsToUpdate.stream().filter(RefUpdateSnapshot::hasFailed).collect(Collectors.toList()); if (!refsFailures.isEmpty()) { String allFailuresMessage = refsFailures.stream() - .map(refPair -> String.format("Failed to fetch ref %s", refPair.compareRef.getName())) + .map(refSnapshot -> String.format("Failed to fetch ref %s", refSnapshot.getName())) .collect(Collectors.joining(", ")); - Exception firstFailureException = refsFailures.get(0).exception; + Exception firstFailureException = refsFailures.get(0).getException(); logger.atSevere().withCause(firstFailureException).log("%s", allFailuresMessage); throw new IOException(allFailuresMessage, firstFailureException); } try (CloseableSet<AutoCloseable> locks = new CloseableSet<>()) { - final List<RefPair> finalRefsToUpdate = compareAndGetLatestLocalRefs(refsToUpdate, locks); + final List<RefUpdateSnapshot> finalRefsToUpdate = + compareAndGetLatestLocalRefs(refsToUpdate, locks); delegateUpdate.invoke(); boolean sharedDbUpdateSucceeded = false; try { @@ -184,7 +186,7 @@ private void rollback( OneParameterVoidFunction<List<ReceiveCommand>> delegateUpdateRollback, - List<RefPair> refsBeforeUpdate, + List<RefUpdateSnapshot> refsBeforeUpdate, List<ReceiveCommand> receiveCommands) throws IOException { List<ReceiveCommand> rollbackCommands = @@ -192,61 +194,62 @@ .map( refBeforeUpdate -> new ReceiveCommand( - refBeforeUpdate.putValue, - refBeforeUpdate.compareRef.getObjectId(), + refBeforeUpdate.getNewValue(), + refBeforeUpdate.getOldValue(), refBeforeUpdate.getName())) .collect(Collectors.toList()); delegateUpdateRollback.invoke(rollbackCommands); receiveCommands.forEach(command -> command.setResult(ReceiveCommand.Result.LOCK_FAILURE)); } - private void updateSharedRefDb(Stream<ReceiveCommand> commandStream, List<RefPair> refsToUpdate) + private void updateSharedRefDb( + Stream<ReceiveCommand> commandStream, List<RefUpdateSnapshot> refsToUpdate) throws IOException { if (commandStream.anyMatch(cmd -> cmd.getResult() != ReceiveCommand.Result.OK)) { return; } - for (RefPair refPair : refsToUpdate) { - updateSharedDbOrThrowExceptionFor(refPair); + for (RefUpdateSnapshot refUpdateSnapshot : refsToUpdate) { + updateSharedDbOrThrowExceptionFor(refUpdateSnapshot); } } - private Stream<RefPair> getRefPairs(List<ReceiveCommand> receivedCommands) { - return receivedCommands.stream().map(this::getRefPairForCommand); + private Stream<RefUpdateSnapshot> getRefUpdateSnapshots(List<ReceiveCommand> receivedCommands) { + return receivedCommands.stream().map(this::getRefUpdateSnapshotForCommand); } - private RefPair getRefPairForCommand(ReceiveCommand command) { + private RefUpdateSnapshot getRefUpdateSnapshotForCommand(ReceiveCommand command) { try { switch (command.getType()) { case CREATE: - return new RefPair(nullRef(command.getRefName()), getNewRef(command)); + return new RefUpdateSnapshot(nullRef(command.getRefName()), getNewValue(command)); case UPDATE: case UPDATE_NONFASTFORWARD: - return new RefPair(getCurrentRef(command.getRefName()), getNewRef(command)); + return new RefUpdateSnapshot(getCurrentRef(command.getRefName()), getNewValue(command)); case DELETE: - return new RefPair(getCurrentRef(command.getRefName()), ObjectId.zeroId()); + return new RefUpdateSnapshot(getCurrentRef(command.getRefName()), ObjectId.zeroId()); default: - return new RefPair( + return new RefUpdateSnapshot( command.getRef(), new IllegalArgumentException("Unsupported command type " + command.getType())); } } catch (IOException e) { - return new RefPair(command.getRef(), e); + return new RefUpdateSnapshot(command.getRef(), e); } } - private ObjectId getNewRef(ReceiveCommand command) { + private ObjectId getNewValue(ReceiveCommand command) { return command.getNewId(); } - private List<RefPair> compareAndGetLatestLocalRefs( - List<RefPair> refsToUpdate, CloseableSet<AutoCloseable> locks) throws IOException { - List<RefPair> latestRefsToUpdate = new ArrayList<>(); - for (RefPair refPair : refsToUpdate) { - latestRefsToUpdate.add(compareAndGetLatestLocalRef(refPair, locks)); + private List<RefUpdateSnapshot> compareAndGetLatestLocalRefs( + List<RefUpdateSnapshot> refsToUpdate, CloseableSet<AutoCloseable> locks) throws IOException { + List<RefUpdateSnapshot> latestRefsToUpdate = new ArrayList<>(); + for (RefUpdateSnapshot refUpdateSnapshot : refsToUpdate) { + latestRefsToUpdate.add(compareAndGetLatestLocalRef(refUpdateSnapshot, locks)); } return latestRefsToUpdate; }
diff --git a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/RefPair.java b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/RefPair.java deleted file mode 100644 index 4211c0c..0000000 --- a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/RefPair.java +++ /dev/null
@@ -1,76 +0,0 @@ -// Copyright (C) 2020 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.gerritforge.gerrit.globalrefdb.validation; - -import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.Ref; - -/** - * A convenience object encompassing the old (current) and the new (candidate) value of a {@link - * Ref}. This is used to snapshot the current status of a ref update so that validations against the - * global refdb are unaffected by changes on the {@link org.eclipse.jgit.lib.RefDatabase}. - */ -public class RefPair { - public final Ref compareRef; - public final ObjectId putValue; - public final Exception exception; - - /** - * Constructs a {@code RefPair} with the provided old and new ref values. The oldRef value is - * required not to be null, in which case an {@link IllegalArgumentException} is thrown. - * - * @param oldRef the old ref - * @param newRefValue the new (candidate) value for this ref. - */ - RefPair(Ref oldRef, ObjectId newRefValue) { - if (oldRef == null) { - throw new IllegalArgumentException("Required not-null ref in RefPair"); - } - this.compareRef = oldRef; - this.putValue = newRefValue; - this.exception = null; - } - - /** - * Constructs a {@code RefPair} with the current ref and an Exception indicating why the new ref - * value failed being retrieved. - * - * @param newRef - * @param e - */ - RefPair(Ref newRef, Exception e) { - this.compareRef = newRef; - this.exception = e; - this.putValue = ObjectId.zeroId(); - } - - /** - * Getter for the current ref - * - * @return the current ref value - */ - public String getName() { - return compareRef.getName(); - } - - /** - * Whether the new value failed being retrieved - * - * @return true when this refPair has failed, false otherwise. - */ - public boolean hasFailed() { - return exception != null; - } -}
diff --git a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/RefUpdateSnapshot.java b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/RefUpdateSnapshot.java new file mode 100644 index 0000000..2e83be3 --- /dev/null +++ b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/RefUpdateSnapshot.java
@@ -0,0 +1,114 @@ +// Copyright (C) 2020 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.gerritforge.gerrit.globalrefdb.validation; + +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Ref; + +/** + * A convenience object encompassing the old (current) and the new (candidate) value of a {@link + * Ref}. This is used to snapshot the current status of a ref update so that validations against the + * global refdb are unaffected by changes on the underlying {@link + * org.eclipse.jgit.lib.RefDatabase}. + */ +class RefUpdateSnapshot { + private final Ref ref; + private final ObjectId newValue; + private final Exception exception; + + /** + * Constructs a {@code RefUpdateSnapshot} with the provided old and new values. The old value of + * this Ref must not be null, otherwise an {@link IllegalArgumentException} is thrown. + * + * @param ref the ref with its old value + * @param newRefValue the new (candidate) value for this ref. + */ + RefUpdateSnapshot(Ref ref, ObjectId newRefValue) { + if (ref == null) { + throw new IllegalArgumentException("RefUpdateSnapshot cannot be created for null Ref"); + } + this.ref = ref; + this.newValue = newRefValue; + this.exception = null; + } + + /** + * Constructs a {@code RefUpdateSnapshot} with the current ref and an Exception indicating why the + * ref's new value could not be retrieved. + * + * @param ref the ref with its old value + * @param e exception caught when trying to retrieve the ref's new value + */ + RefUpdateSnapshot(Ref ref, Exception e) { + this.ref = ref; + this.newValue = ObjectId.zeroId(); + this.exception = e; + } + + /** + * Get the ref name + * + * @return the ref name + */ + public String getName() { + return ref.getName(); + } + + /** + * Get the ref's old value + * + * @return the ref's old value + */ + public ObjectId getOldValue() { + return ref.getObjectId(); + } + + /** + * Get the ref's new (candidate) value + * + * @return the ref's new (candidate) value + */ + public ObjectId getNewValue() { + return newValue; + } + + /** + * Get the snapshotted ref with its old value + * + * @return the snapshotted ref with its old value + */ + public Ref getRef() { + return ref; + } + + /** + * Get the exception which occurred when retrieving the ref's new value + * + * @return the exception which occurred when retrieving the ref's new value + */ + public Exception getException() { + return exception; + } + + /** + * Whether retrieving the new (candidate) value failed + * + * @return {@code true} when retrieving the ref's new (candidate) value failed, {@code false} + * otherwise. + */ + public boolean hasFailed() { + return exception != null; + } +}
diff --git a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/RefUpdateValidator.java b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/RefUpdateValidator.java index 8823f3d..31d3af1 100644 --- a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/RefUpdateValidator.java +++ b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/RefUpdateValidator.java
@@ -186,16 +186,16 @@ OneParameterFunction<ObjectId, Result> rollbackFunction) throws IOException { try (CloseableSet<AutoCloseable> locks = new CloseableSet<>()) { - RefPair refPairForUpdate = newRefPairFrom(refUpdate); - compareAndGetLatestLocalRef(refPairForUpdate, locks); + RefUpdateSnapshot refUpdateSnapshot = newSnapshot(refUpdate); + compareAndGetLatestLocalRef(refUpdateSnapshot, locks); RefUpdate.Result result = refUpdateFunction.invoke(); if (!isSuccessful(result)) { return result; } try { - updateSharedDbOrThrowExceptionFor(refPairForUpdate); + updateSharedDbOrThrowExceptionFor(refUpdateSnapshot); } catch (Exception e) { - result = rollbackFunction.invoke(refPairForUpdate.compareRef.getObjectId()); + result = rollbackFunction.invoke(refUpdateSnapshot.getOldValue()); if (isSuccessful(result)) { result = RefUpdate.Result.LOCK_FAILURE; } @@ -207,26 +207,26 @@ } catch (OutOfSyncException e) { logger.atWarning().withCause(e).log( "Local node is out of sync with ref-db: %s", e.getMessage()); - return RefUpdate.Result.LOCK_FAILURE; } } - protected void updateSharedDbOrThrowExceptionFor(RefPair refPair) throws IOException { + protected void updateSharedDbOrThrowExceptionFor(RefUpdateSnapshot refSnapshot) + throws IOException { // We are not checking refs that should be ignored final EnforcePolicy refEnforcementPolicy = - refEnforcement.getPolicy(projectName, refPair.getName()); + refEnforcement.getPolicy(projectName, refSnapshot.getName()); if (refEnforcementPolicy == EnforcePolicy.IGNORED) return; boolean succeeded; try { succeeded = sharedRefDb.compareAndPut( - Project.nameKey(projectName), refPair.compareRef, refPair.putValue); + Project.nameKey(projectName), refSnapshot.getRef(), refSnapshot.getNewValue()); } catch (GlobalRefDbSystemError e) { logger.atWarning().withCause(e).log( "Not able to persist the data in global-refdb for project '%s', ref '%s' and value %s, message: %s", - projectName, refPair.getName(), refPair.putValue, e.getMessage()); + projectName, refSnapshot.getName(), refSnapshot.getNewValue(), e.getMessage()); throw e; } @@ -236,17 +236,18 @@ "Not able to persist the data in SharedRef for project '%s' and ref '%s'," + "the cluster is now in Split Brain since the commit has been " + "persisted locally but not in global-refdb the value %s", - projectName, refPair.getName(), refPair.putValue); + projectName, refSnapshot.getName(), refSnapshot.getNewValue()); throw new SharedDbSplitBrainException(errorMessage); } } - protected RefPair compareAndGetLatestLocalRef(RefPair refPair, CloseableSet<AutoCloseable> locks) + protected RefUpdateSnapshot compareAndGetLatestLocalRef( + RefUpdateSnapshot refUpdateSnapshot, CloseableSet<AutoCloseable> locks) throws GlobalRefDbLockException, OutOfSyncException, IOException { - String refName = refPair.getName(); + String refName = refUpdateSnapshot.getName(); EnforcePolicy refEnforcementPolicy = refEnforcement.getPolicy(projectName, refName); if (refEnforcementPolicy == EnforcePolicy.IGNORED) { - return refPair; + return refUpdateSnapshot; } locks.addResourceIfNotExist( @@ -255,30 +256,33 @@ lockWrapperFactory.create( projectName, refName, sharedRefDb.lockRef(Project.nameKey(projectName), refName))); - RefPair latestRefPair = getLatestLocalRef(refPair); - if (sharedRefDb.isUpToDate(Project.nameKey(projectName), latestRefPair.compareRef)) { - return latestRefPair; + RefUpdateSnapshot latestRefUpdateSnapshot = getLatestLocalRef(refUpdateSnapshot); + if (sharedRefDb.isUpToDate(Project.nameKey(projectName), latestRefUpdateSnapshot.getRef())) { + return latestRefUpdateSnapshot; } - if (isNullRef(latestRefPair.compareRef) + if (isNullRef(latestRefUpdateSnapshot.getRef()) || sharedRefDb.exists(Project.nameKey(projectName), refName)) { validationMetrics.incrementSplitBrainPrevention(); softFailBasedOnEnforcement( - new OutOfSyncException(projectName, latestRefPair.compareRef), refEnforcementPolicy); + new OutOfSyncException(projectName, latestRefUpdateSnapshot.getRef()), + refEnforcementPolicy); } - return latestRefPair; + return latestRefUpdateSnapshot; } private boolean isNullRef(Ref ref) { return ref.getObjectId().equals(ObjectId.zeroId()); } - private RefPair getLatestLocalRef(RefPair refPair) throws IOException { - Ref latestRef = refDb.exactRef(refPair.getName()); - return new RefPair( - latestRef == null ? nullRef(refPair.getName()) : latestRef, refPair.putValue); + private RefUpdateSnapshot getLatestLocalRef(RefUpdateSnapshot refUpdateSnapshot) + throws IOException { + Ref latestRef = refDb.exactRef(refUpdateSnapshot.getName()); + return new RefUpdateSnapshot( + latestRef == null ? nullRef(refUpdateSnapshot.getName()) : latestRef, + refUpdateSnapshot.getNewValue()); } private Ref nullRef(String name) { @@ -306,8 +310,8 @@ } } - protected RefPair newRefPairFrom(RefUpdate refUpdate) throws IOException { - return new RefPair(getCurrentRef(refUpdate.getName()), refUpdate.getNewObjectId()); + protected RefUpdateSnapshot newSnapshot(RefUpdate refUpdate) throws IOException { + return new RefUpdateSnapshot(getCurrentRef(refUpdate.getName()), refUpdate.getNewObjectId()); } protected Ref getCurrentRef(String refName) throws IOException {