Remove etags for ListFiles REST endpoint
Change Ia92fbad2a and change Ic54db5dc9 remove etags for change and
revision resources because the etag computation is too expensive to
bring any latency benefits. The same is true for the ListFiles etag,
i.e. it includes the expensive change and revision etag computation.
An etag is only returned if the REST endpoint returns a response with
cache control, see RestApiServlet#configureCaching which calls
RestApiServlet#addResourceStateHeaders that sets the etag in the
response. For ListFiles this is only the case if a concrete revision
number is specified, but not for the magic "current" revision.
Etags are mostly relevant for UI's that do auto-refreshing via polling.
PolyGerrit uses the ListFiles REST endpoint (with a concrete revision
number) but fetches the file list only once when the change is loaded
and then doesn't do any polling for it, i.e. it never makes use of the
etag that is returned by ListFiles. For other callers (mostly service
users) it's unlikely that they do polling with etags, especially since
the etag support for this REST endpoint is not even documented [1].
Hence having ListFiles return etags likely doesn't have any benefits,
but calculating them can add up to 1.2s to ListFiles requests, just for
including the etag into the response.
[1] https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#list-files
Release-Notes: Removed etags for ListFiles REST endpoint
Bug: Google b/336400432
Change-Id: Ib49748bfbdde6c69cf215a95c8edbdc33f9db4a8
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/acceptance/ExtensionRegistry.java b/java/com/google/gerrit/acceptance/ExtensionRegistry.java
index d2051d5..a35e427 100644
--- a/java/com/google/gerrit/acceptance/ExtensionRegistry.java
+++ b/java/com/google/gerrit/acceptance/ExtensionRegistry.java
@@ -45,7 +45,6 @@
import com.google.gerrit.extensions.webui.ResolveConflictsWebLink;
import com.google.gerrit.server.ExceptionHook;
import com.google.gerrit.server.account.GroupBackend;
-import com.google.gerrit.server.change.ChangeETagComputation;
import com.google.gerrit.server.change.FilterIncludedIn;
import com.google.gerrit.server.change.ReviewerSuggestion;
import com.google.gerrit.server.config.ProjectConfigEntry;
@@ -81,7 +80,6 @@
private final DynamicSet<SubmitRule> submitRules;
private final DynamicSet<SubmitRequirement> submitRequirements;
private final DynamicSet<ChangeMessageModifier> changeMessageModifiers;
- private final DynamicSet<ChangeETagComputation> changeETagComputations;
private final DynamicSet<ActionVisitor> actionVisitors;
private final DynamicMap<DownloadScheme> downloadSchemes;
private final DynamicSet<RefOperationValidationListener> refOperationValidationListeners;
@@ -128,7 +126,6 @@
DynamicSet<SubmitRule> submitRules,
DynamicSet<SubmitRequirement> submitRequirements,
DynamicSet<ChangeMessageModifier> changeMessageModifiers,
- DynamicSet<ChangeETagComputation> changeETagComputations,
DynamicSet<ActionVisitor> actionVisitors,
DynamicMap<DownloadScheme> downloadSchemes,
DynamicSet<RefOperationValidationListener> refOperationValidationListeners,
@@ -170,7 +167,6 @@
this.submitRules = submitRules;
this.submitRequirements = submitRequirements;
this.changeMessageModifiers = changeMessageModifiers;
- this.changeETagComputations = changeETagComputations;
this.actionVisitors = actionVisitors;
this.downloadSchemes = downloadSchemes;
this.refOperationValidationListeners = refOperationValidationListeners;
@@ -286,11 +282,6 @@
}
@CanIgnoreReturnValue
- public Registration add(ChangeETagComputation changeETagComputation) {
- return add(changeETagComputations, changeETagComputation);
- }
-
- @CanIgnoreReturnValue
public Registration add(ActionVisitor actionVisitor) {
return add(actionVisitors, actionVisitor);
}
diff --git a/java/com/google/gerrit/server/change/ChangeETagComputation.java b/java/com/google/gerrit/server/change/ChangeETagComputation.java
deleted file mode 100644
index e96d5ff..0000000
--- a/java/com/google/gerrit/server/change/ChangeETagComputation.java
+++ /dev/null
@@ -1,64 +0,0 @@
-// Copyright (C) 2019 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.change;
-
-import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.Project;
-import com.google.gerrit.extensions.annotations.ExtensionPoint;
-
-/**
- * Allows plugins to contribute a value to the change ETag computation.
- *
- * <p>Plugins can affect the result of the get change / get change details REST endpoints by:
- *
- * <ul>
- * <li>providing plugin defined attributes to {@link
- * com.google.gerrit.extensions.common.ChangeInfo#plugins} (see {@link
- * ChangePluginDefinedInfoFactory})
- * <li>implementing a {@link com.google.gerrit.server.rules.SubmitRule} which affects the
- * computation of {@link com.google.gerrit.extensions.common.ChangeInfo#submittable}
- * </ul>
- *
- * <p>If the plugin defined part of {@link com.google.gerrit.extensions.common.ChangeInfo} depends
- * on plugin specific data, callers that use the change ETags to avoid unneeded recomputations of
- * ChangeInfos may see outdated plugin attributes and/or outdated submittable information, because a
- * ChangeInfo is only reloaded if the change ETag changes.
- *
- * <p>By implementating this interface plugins can contribute to the change ETag computation and
- * thus ensure that the ETag changes when the plugin data was changed. This way it is ensured that
- * callers do not see outdated ChangeInfos.
- *
- * @see ChangeResource#prepareETag(com.google.common.hash.Hasher,
- * com.google.gerrit.server.CurrentUser)
- */
-@ExtensionPoint
-public interface ChangeETagComputation {
- /**
- * Computes an ETag of plugin-specific data for the given change.
- *
- * <p><strong>Note:</strong> Change ETags are computed very frequently and the computation must be
- * cheap. Take good care to not perform any expensive computations when implementing this.
- *
- * <p>If an error is encountered during the ETag computation the plugin can indicate this by
- * throwing any RuntimeException. In this case no value will be included in the change ETag
- * computation. This means if the error is transient, the ETag will differ when the computation
- * succeeds on a follow-up run.
- *
- * @param projectName the name of the project that contains the change
- * @param changeId ID of the change for which the ETag should be computed
- * @return the ETag
- */
- String getETag(Project.NameKey projectName, Change.Id changeId);
-}
diff --git a/java/com/google/gerrit/server/change/ChangeResource.java b/java/com/google/gerrit/server/change/ChangeResource.java
index f1c82ed..a7fa6f4 100644
--- a/java/com/google/gerrit/server/change/ChangeResource.java
+++ b/java/com/google/gerrit/server/change/ChangeResource.java
@@ -14,48 +14,20 @@
package com.google.gerrit.server.change;
-import static com.google.gerrit.server.project.ProjectCache.illegalState;
-import static java.nio.charset.StandardCharsets.UTF_8;
-
-import com.google.common.base.MoreObjects;
-import com.google.common.hash.Hasher;
-import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
-import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
-import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.restapi.RestResource;
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.PatchSetUtil;
-import com.google.gerrit.server.account.AccountCache;
-import com.google.gerrit.server.account.AccountState;
-import com.google.gerrit.server.approval.ApprovalsUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackend;
-import com.google.gerrit.server.plugincontext.PluginSetContext;
-import com.google.gerrit.server.project.ProjectCache;
-import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.TypeLiteral;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
-import java.util.HashSet;
-import java.util.Optional;
-import java.util.Set;
-import org.eclipse.jgit.lib.ObjectId;
public class ChangeResource implements RestResource {
- /**
- * JSON format version number for ETag computations.
- *
- * <p>Should be bumped on any JSON format change (new fields, etc.) so that otherwise unmodified
- * changes get new ETags.
- */
- public static final int JSON_FORMAT_VERSION = 1;
-
public static final TypeLiteral<RestView<ChangeResource>> CHANGE_KIND = new TypeLiteral<>() {};
public interface Factory {
@@ -64,54 +36,27 @@
ChangeResource create(ChangeData changeData, CurrentUser user);
}
- private static final String ZERO_ID_STRING = ObjectId.zeroId().name();
-
- private final AccountCache accountCache;
- private final ApprovalsUtil approvalUtil;
- private final PatchSetUtil patchSetUtil;
private final PermissionBackend permissionBackend;
- private final ProjectCache projectCache;
- private final PluginSetContext<ChangeETagComputation> changeETagComputation;
private final ChangeData changeData;
private final CurrentUser user;
@AssistedInject
ChangeResource(
- AccountCache accountCache,
- ApprovalsUtil approvalUtil,
- PatchSetUtil patchSetUtil,
PermissionBackend permissionBackend,
- ProjectCache projectCache,
- PluginSetContext<ChangeETagComputation> changeETagComputation,
ChangeData.Factory changeDataFactory,
@Assisted ChangeNotes notes,
@Assisted CurrentUser user) {
- this.accountCache = accountCache;
- this.approvalUtil = approvalUtil;
- this.patchSetUtil = patchSetUtil;
this.permissionBackend = permissionBackend;
- this.projectCache = projectCache;
- this.changeETagComputation = changeETagComputation;
this.changeData = changeDataFactory.create(notes);
this.user = user;
}
@AssistedInject
ChangeResource(
- AccountCache accountCache,
- ApprovalsUtil approvalUtil,
- PatchSetUtil patchSetUtil,
PermissionBackend permissionBackend,
- ProjectCache projectCache,
- PluginSetContext<ChangeETagComputation> changeETagComputation,
@Assisted ChangeData changeData,
@Assisted CurrentUser user) {
- this.accountCache = accountCache;
- this.approvalUtil = approvalUtil;
- this.patchSetUtil = patchSetUtil;
this.permissionBackend = permissionBackend;
- this.projectCache = projectCache;
- this.changeETagComputation = changeETagComputation;
this.changeData = changeData;
this.user = user;
}
@@ -153,88 +98,4 @@
public Change.Id getVirtualId() {
return getChangeData().virtualId();
}
-
- // This includes all information relevant for ETag computation
- // unrelated to the UI.
- public void prepareETag(Hasher h, CurrentUser user) {
- h.putInt(JSON_FORMAT_VERSION)
- .putLong(getChange().getLastUpdatedOn().toEpochMilli())
- .putInt(user.isIdentifiedUser() ? user.getAccountId().get() : 0);
-
- if (user.isIdentifiedUser()) {
- for (AccountGroup.UUID uuid : user.getEffectiveGroups().getKnownGroups()) {
- h.putBytes(uuid.get().getBytes(UTF_8));
- }
- }
-
- byte[] buf = new byte[20];
- Set<Account.Id> accounts = new HashSet<>();
- accounts.add(getChange().getOwner());
- try {
- patchSetUtil.byChange(getNotes()).stream().map(PatchSet::uploader).forEach(accounts::add);
-
- // It's intentional to include the states for *all* reviewers into the ETag computation.
- // We need the states of all current reviewers and CCs because they are part of ChangeInfo.
- // Including removed reviewers is a cheap way of making sure that the states of accounts that
- // posted a message on the change are included. Loading all change messages to find the exact
- // set of accounts that posted a message is too expensive. However everyone who posts a
- // message is automatically added as reviewer. Hence if we include removed reviewers we can
- // be sure that we have all accounts that posted messages on the change.
- accounts.addAll(approvalUtil.getReviewers(getNotes()).all());
- } catch (StorageException e) {
- // This ETag will be invalidated if it loads next time.
- }
-
- for (Account.Id accountId : accounts) {
- Optional<AccountState> accountState = accountCache.get(accountId);
- if (accountState.isPresent()) {
- hashAccount(h, accountState.get(), buf);
- } else {
- h.putInt(accountId.get());
- }
- }
-
- ObjectId noteId;
- try {
- noteId = getNotes().loadRevision();
- } catch (StorageException e) {
- noteId = null; // This ETag will be invalidated if it loads next time.
- }
- hashObjectId(h, noteId, buf);
- // TODO(dborowitz): Include more NoteDb and other related refs, e.g. drafts
- // and edits.
-
- Iterable<ProjectState> projectStateTree =
- projectCache.get(getProject()).orElseThrow(illegalState(getProject())).tree();
- for (ProjectState p : projectStateTree) {
- hashObjectId(h, p.getConfig().getRevision().orElse(null), buf);
- }
-
- changeETagComputation.runEach(
- c -> {
- String pluginETag = c.getETag(changeData.project(), changeData.getId());
- if (pluginETag != null) {
- h.putString(pluginETag, UTF_8);
- }
- });
- }
-
- private void hashObjectId(Hasher h, @Nullable ObjectId id, byte[] buf) {
- MoreObjects.firstNonNull(id, ObjectId.zeroId()).copyRawTo(buf, 0);
- h.putBytes(buf);
- }
-
- private void hashAccount(Hasher h, AccountState accountState, byte[] buf) {
- h.putInt(accountState.account().id().get());
- // Based on the code, it seems the metaId should never be null in this place and so the
- // uniqueTag.
- // However, the null-check for metaId has been existed here for some time already - for safety
- // the same check is applied to uniqueTag.
- h.putString(
- MoreObjects.firstNonNull(
- accountState.account().uniqueTag(),
- MoreObjects.firstNonNull(accountState.account().metaId(), ZERO_ID_STRING)),
- UTF_8);
- accountState.externalIds().stream().forEach(e -> hashObjectId(h, e.blobId(), buf));
- }
}
diff --git a/java/com/google/gerrit/server/change/RevisionResource.java b/java/com/google/gerrit/server/change/RevisionResource.java
index 64bcc8c..a09cb1f 100644
--- a/java/com/google/gerrit/server/change/RevisionResource.java
+++ b/java/com/google/gerrit/server/change/RevisionResource.java
@@ -14,7 +14,6 @@
package com.google.gerrit.server.change;
-import com.google.common.hash.Hasher;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
@@ -85,12 +84,6 @@
return ps;
}
- public void prepareETag(Hasher h, CurrentUser user) {
- // Conservative estimate: refresh the revision if its parent change has changed, so we don't
- // have to check whether a given modification affected this revision specifically.
- changeResource.prepareETag(h, user);
- }
-
public Account.Id getAccountId() {
return getUser().getAccountId();
}
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index a38a3fc..2b26956 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -114,7 +114,6 @@
import com.google.gerrit.server.cache.CacheRemovalListener;
import com.google.gerrit.server.change.AbandonOp;
import com.google.gerrit.server.change.AccountPatchReviewStore;
-import com.google.gerrit.server.change.ChangeETagComputation;
import com.google.gerrit.server.change.ChangeFinder;
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.change.ChangeKindCacheImpl;
@@ -458,7 +457,6 @@
DynamicSet.setOf(binder(), PerformanceLogger.class);
DynamicSet.setOf(binder(), RequestListener.class);
DynamicSet.bind(binder(), RequestListener.class).to(TraceRequestListener.class);
- DynamicSet.setOf(binder(), ChangeETagComputation.class);
DynamicSet.setOf(binder(), ExceptionHook.class);
DynamicSet.bind(binder(), ExceptionHook.class).to(ExceptionHookImpl.class);
DynamicSet.setOf(binder(), MailSoyTemplateProvider.class);
diff --git a/java/com/google/gerrit/server/restapi/change/Files.java b/java/com/google/gerrit/server/restapi/change/Files.java
index 96e5645..07e4372 100644
--- a/java/com/google/gerrit/server/restapi/change/Files.java
+++ b/java/com/google/gerrit/server/restapi/change/Files.java
@@ -17,8 +17,6 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.flogger.FluentLogger;
-import com.google.common.hash.Hasher;
-import com.google.common.hash.Hashing;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
@@ -31,10 +29,10 @@
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.CacheControl;
import com.google.gerrit.extensions.restapi.ChildCollection;
-import com.google.gerrit.extensions.restapi.ETagView;
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.PatchSetUtil;
@@ -47,7 +45,6 @@
import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.DiffOperations;
import com.google.gerrit.server.patch.DiffOptions;
-import com.google.gerrit.server.patch.PatchListKey;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -100,7 +97,7 @@
return new FileResource(rev, id.get());
}
- public static final class ListFiles implements ETagView<RevisionResource> {
+ public static final class ListFiles implements RestReadView<RevisionResource> {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@Option(name = "--base", metaVar = "revision-id")
@@ -353,16 +350,6 @@
return this;
}
- @Override
- public String getETag(RevisionResource resource) {
- Hasher h = Hashing.murmur3_128().newHasher();
- resource.prepareETag(h, resource.getUser());
- // File list comes from the PatchListCache, so any change to the key or value should
- // invalidate ETag.
- h.putLong(PatchListKey.serialVersionUID);
- return h.hash().toString();
- }
-
@Nullable
private ObjectId getOldId(Map<String, FileDiffOutput> fileDiffList) {
return fileDiffList.isEmpty()
