GetRevisionActions: Remove ETag

The computation of the ETag takes as long as doing the real computation.
This defeats the purpose of using ETags, hence rather drop it. This cuts
the execution time by half for cases where the ETag didn't match
(because instead of computing the ETag and the actual value, we now only
need to compute the actual value). For cases where the ETag matched the
execution time should roughly stay the same (instead of the ETag we now
compute the actual value, but this should take the same amount of time).

Keep the etag() method on the RevisionApi interface to not break
implementors of this interface.

Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ia3db97037755fc364af4d1cb8fef94bb1e69fb5a

java/com/google/gerrit/server/api/changes/RevisionApiImpl.java

@@ -106,7 +106,7 @@ import java.util.Set;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevWalk;
 
-class RevisionApiImpl implements RevisionApi {
+class RevisionApiImpl extends RevisionApi.NotImplemented {
   interface Factory {
     RevisionApiImpl create(RevisionResource r);
   }
@@ -686,11 +686,6 @@ class RevisionApiImpl implements RevisionApi {
     }
   }
 
-  @Override
-  public String etag() throws RestApiException {
-    return revisionActions.getETag(revision);
-  }
-
   @Override
   public BinaryResult getArchive(ArchiveFormat format) throws RestApiException {
     GetArchive getArchive = getArchiveProvider.get();

java/com/google/gerrit/server/restapi/change/GetRevisionActions.java

@@ -14,67 +14,26 @@
 
 package com.google.gerrit.server.restapi.change;
 
-import com.google.common.hash.Hasher;
-import com.google.common.hash.Hashing;
-import com.google.gerrit.exceptions.StorageException;
 import com.google.gerrit.extensions.common.ActionInfo;
-import com.google.gerrit.extensions.restapi.ETagView;
 import com.google.gerrit.extensions.restapi.Response;
-import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.gerrit.server.change.ActionJson;
-import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.change.RevisionResource;
-import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.submit.ChangeSet;
-import com.google.gerrit.server.submit.MergeSuperSet;
 import com.google.inject.Inject;
-import com.google.inject.Provider;
 import com.google.inject.Singleton;
-import java.io.IOException;
 import java.util.Map;
-import org.eclipse.jgit.lib.Config;
 
 @Singleton
-public class GetRevisionActions implements ETagView<RevisionResource> {
+public class GetRevisionActions implements RestReadView<RevisionResource> {
   private final ActionJson delegate;
-  private final Config config;
-  private final Provider<MergeSuperSet> mergeSuperSet;
-  private final ChangeResource.Factory changeResourceFactory;
 
   @Inject
-  GetRevisionActions(
+  GetRevisionActions(ActionJson delegate) {
-      ActionJson delegate,
-      Provider<MergeSuperSet> mergeSuperSet,
-      ChangeResource.Factory changeResourceFactory,
-      @GerritServerConfig Config config) {
     this.delegate = delegate;
-    this.mergeSuperSet = mergeSuperSet;
-    this.changeResourceFactory = changeResourceFactory;
-    this.config = config;
   }
 
   @Override
   public Response<Map<String, ActionInfo>> apply(RevisionResource rsrc) {
     return Response.withMustRevalidate(delegate.format(rsrc));
   }
-
-  @Override
-  public String getETag(RevisionResource rsrc) {
-    Hasher h = Hashing.murmur3_128().newHasher();
-    CurrentUser user = rsrc.getUser();
-    try {
-      rsrc.getChangeResource().prepareETag(h, user);
-      h.putBoolean(MergeSuperSet.wholeTopicEnabled(config));
-      ChangeSet cs = mergeSuperSet.get().completeChangeSet(rsrc.getChange(), user);
-      for (ChangeData cd : cs.changes()) {
-        changeResourceFactory.create(cd.notes(), user).prepareETag(h, user);
-      }
-      h.putBoolean(cs.furtherHiddenChanges());
-    } catch (IOException | PermissionBackendException e) {
-      throw new StorageException(e);
-    }
-    return h.hash().toString();
-  }
 }

javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java

@@ -87,15 +87,12 @@ import com.google.gerrit.extensions.events.ChangeIndexedListener;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.BinaryResult;
-import com.google.gerrit.extensions.restapi.ETagView;
 import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.extensions.webui.PatchSetWebLink;
-import com.google.gerrit.server.change.RevisionResource;
 import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.restapi.change.GetRevisionActions;
 import com.google.gerrit.testing.FakeEmailSender;
 import com.google.inject.Inject;
 import java.io.ByteArrayOutputStream;
@@ -121,7 +118,6 @@ import org.eclipse.jgit.transport.RefSpec;
 import org.junit.Test;
 
 public class RevisionIT extends AbstractDaemonTest {
-  @Inject private GetRevisionActions getRevisionActions;
   @Inject private ProjectOperations projectOperations;
   @Inject private RequestScopeOperations requestScopeOperations;
   @Inject private ExtensionRegistry extensionRegistry;
@@ -1825,23 +1821,6 @@ public class RevisionIT extends AbstractDaemonTest {
     assertThat(current(r).actions().keySet()).containsExactly("cherrypick");
   }
 
-  @Test
-  public void actionsETag() throws Exception {
-    PushOneCommit.Result r1 = createChange();
-    PushOneCommit.Result r2 = createChange();
-
-    String oldETag = checkETag(getRevisionActions, r2, null);
-    current(r2).review(ReviewInput.approve());
-    oldETag = checkETag(getRevisionActions, r2, oldETag);
-
-    // Dependent change is included in ETag.
-    current(r1).review(ReviewInput.approve());
-    oldETag = checkETag(getRevisionActions, r2, oldETag);
-
-    current(r2).submit();
-    checkETag(getRevisionActions, r2, oldETag);
-  }
-
   @Test
   public void deleteVoteOnNonCurrentPatchSet() throws Exception {
     PushOneCommit.Result r = createChange(); // patch set 1
@@ -2013,13 +1992,6 @@ public class RevisionIT extends AbstractDaemonTest {
     return gApi.changes().id(r.getChangeId()).current();
   }
 
-  private String checkETag(ETagView<RevisionResource> view, PushOneCommit.Result r, String oldETag)
-      throws Exception {
-    String eTag = view.getETag(parseRevisionResource(r));
-    assertThat(eTag).isNotEqualTo(oldETag);
-    return eTag;
-  }
-
   private PushOneCommit.Result createCherryPickableMerge(
       String parent1FileName, String parent2FileName) throws Exception {
     RevCommit initialCommit = getHead(repo(), "HEAD");

javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java

@@ -20,26 +20,20 @@ import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_ACTI
 import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVISION;
 import static com.google.gerrit.truth.MapSubject.assertThatMap;
 
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.ExtensionRegistry;
 import com.google.gerrit.acceptance.ExtensionRegistry.Registration;
 import com.google.gerrit.acceptance.PushOneCommit;
-import com.google.gerrit.acceptance.TestProjectInput;
-import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
 import com.google.gerrit.entities.Change;
 import com.google.gerrit.entities.PatchSet;
-import com.google.gerrit.exceptions.StorageException;
 import com.google.gerrit.extensions.api.changes.ActionVisitor;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
 import com.google.gerrit.extensions.client.ListChangesOption;
-import com.google.gerrit.extensions.client.SubmitType;
 import com.google.gerrit.extensions.common.ActionInfo;
 import com.google.gerrit.extensions.common.ChangeInfo;
 import com.google.gerrit.extensions.common.RevisionInfo;
 import com.google.gerrit.server.change.RevisionJson;
-import com.google.gerrit.server.change.testing.TestChangeETagComputation;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.testing.ConfigSuite;
 import com.google.inject.Inject;
@@ -56,7 +50,6 @@ public class ActionsIT extends AbstractDaemonTest {
     return submitWholeTopicEnabledConfig();
   }
 
-  @Inject private RequestScopeOperations requestScopeOperations;
   @Inject private RevisionJson.Factory revisionJsonFactory;
   @Inject private ExtensionRegistry extensionRegistry;
 
@@ -68,10 +61,6 @@ public class ActionsIT extends AbstractDaemonTest {
     return gApi.changes().id(id).get().actions;
   }
 
-  protected String getETag(String id) throws Exception {
-    return gApi.changes().id(id).current().etag();
-  }
-
   @Test
   public void changeActionOneMergedChangeHasOnlyNormalRevert() throws Exception {
     String changeId = createChangeWithTopic().getChangeId();
@@ -137,124 +126,6 @@ public class ActionsIT extends AbstractDaemonTest {
     }
   }
 
-  @Test
-  public void revisionActionsETag() throws Exception {
-    String parent = createChange().getChangeId();
-    String change = createChangeWithTopic().getChangeId();
-    approve(change);
-    String etag1 = getETag(change);
-
-    approve(parent);
-    String etag2 = getETag(change);
-
-    String changeWithSameTopic = createChangeWithTopic().getChangeId();
-    String etag3 = getETag(change);
-
-    approve(changeWithSameTopic);
-    String etag4 = getETag(change);
-
-    if (isSubmitWholeTopicEnabled()) {
-      assertThat(ImmutableList.of(etag1, etag2, etag3, etag4)).containsNoDuplicates();
-    } else {
-      assertThat(etag2).isNotEqualTo(etag1);
-      assertThat(etag3).isEqualTo(etag2);
-      assertThat(etag4).isEqualTo(etag2);
-    }
-  }
-
-  @Test
-  public void revisionActionsAnonymousETag() throws Exception {
-    String parent = createChange().getChangeId();
-    String change = createChangeWithTopic().getChangeId();
-    approve(change);
-
-    requestScopeOperations.setApiUserAnonymous();
-    String etag1 = getETag(change);
-
-    requestScopeOperations.setApiUser(admin.id());
-    approve(parent);
-
-    requestScopeOperations.setApiUserAnonymous();
-    String etag2 = getETag(change);
-
-    requestScopeOperations.setApiUser(admin.id());
-    String changeWithSameTopic = createChangeWithTopic().getChangeId();
-
-    requestScopeOperations.setApiUserAnonymous();
-    String etag3 = getETag(change);
-
-    requestScopeOperations.setApiUser(admin.id());
-    approve(changeWithSameTopic);
-
-    requestScopeOperations.setApiUserAnonymous();
-    String etag4 = getETag(change);
-
-    if (isSubmitWholeTopicEnabled()) {
-      assertThat(ImmutableList.of(etag1, etag2, etag3, etag4)).containsNoDuplicates();
-    } else {
-      assertThat(etag2).isNotEqualTo(etag1);
-      assertThat(etag3).isEqualTo(etag2);
-      assertThat(etag4).isEqualTo(etag2);
-    }
-  }
-
-  @Test
-  @TestProjectInput(submitType = SubmitType.CHERRY_PICK)
-  public void revisionActionsAnonymousETagCherryPickStrategy() throws Exception {
-    String parent = createChange().getChangeId();
-    String change = createChange().getChangeId();
-    approve(change);
-
-    requestScopeOperations.setApiUserAnonymous();
-    String etag1 = getETag(change);
-
-    requestScopeOperations.setApiUser(admin.id());
-    approve(parent);
-
-    requestScopeOperations.setApiUserAnonymous();
-    String etag2 = getETag(change);
-    assertThat(etag2).isEqualTo(etag1);
-  }
-
-  @Test
-  public void pluginCanContributeToETagComputation() throws Exception {
-    String change = createChange().getChangeId();
-    String oldETag = getETag(change);
-
-    try (Registration registration =
-        extensionRegistry.newRegistration().add(TestChangeETagComputation.withETag("foo"))) {
-      assertThat(getETag(change)).isNotEqualTo(oldETag);
-    }
-
-    assertThat(getETag(change)).isEqualTo(oldETag);
-  }
-
-  @Test
-  public void returningNullFromETagComputationDoesNotBreakGerrit() throws Exception {
-    String change = createChange().getChangeId();
-    String oldETag = getETag(change);
-
-    try (Registration registration =
-        extensionRegistry.newRegistration().add(TestChangeETagComputation.withETag(null))) {
-      assertThat(getETag(change)).isEqualTo(oldETag);
-    }
-  }
-
-  @Test
-  public void throwingExceptionFromETagComputationDoesNotBreakGerrit() throws Exception {
-    String change = createChange().getChangeId();
-    String oldETag = getETag(change);
-
-    try (Registration registration =
-        extensionRegistry
-            .newRegistration()
-            .add(
-                TestChangeETagComputation.withException(
-                    new StorageException("exception during test")))) {
-      assertThat(getETag(change)).isEqualTo(oldETag);
-    }
-  }
-
   @Test
   public void revisionActionsTwoChangesInTopic_conflicting() throws Exception {
     String changeId = createChangeWithTopic().getChangeId();