From 8103d8109aca2da4a616cfcda9f4dc2662c4b027 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 15 Jan 2021 14:02:24 +0100 Subject: [PATCH] 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 Change-Id: Ia3db97037755fc364af4d1cb8fef94bb1e69fb5a --- .../server/api/changes/RevisionApiImpl.java | 7 +- .../restapi/change/GetRevisionActions.java | 47 +------ .../acceptance/api/revision/RevisionIT.java | 28 ---- .../acceptance/rest/change/ActionsIT.java | 129 ------------------ 4 files changed, 4 insertions(+), 207 deletions(-) diff --git a/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java b/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java index 36d48033b8..573f2f580a 100644 --- a/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java +++ b/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(); diff --git a/java/com/google/gerrit/server/restapi/change/GetRevisionActions.java b/java/com/google/gerrit/server/restapi/change/GetRevisionActions.java index c4da3b61a7..527129cc23 100644 --- a/java/com/google/gerrit/server/restapi/change/GetRevisionActions.java +++ b/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 { +public class GetRevisionActions implements RestReadView { private final ActionJson delegate; - private final Config config; - private final Provider mergeSuperSet; - private final ChangeResource.Factory changeResourceFactory; @Inject - GetRevisionActions( - ActionJson delegate, - Provider mergeSuperSet, - ChangeResource.Factory changeResourceFactory, - @GerritServerConfig Config config) { + GetRevisionActions(ActionJson delegate) { this.delegate = delegate; - this.mergeSuperSet = mergeSuperSet; - this.changeResourceFactory = changeResourceFactory; - this.config = config; } @Override public Response> 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(); - } } diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java index e4c7f9c3e9..3700421b98 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java +++ b/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 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"); diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java index c6a2819fe4..e35f758cff 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java +++ b/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();