Add experiment flag to disable change etags
Etags help to avoid unnecessary recomputation of responses, e.g. for
changes we compute an etag, and when the client presents this etag and
the change hasn’t been updated, we do not need to recompute the change
info, but we can return '304 Not Modified'.
Etags work well when they are cheap to compute, but that’s not the case
for our change etags. To give one example for what makes the change etag
computation slow, we include the known groups of the current user to
ensure that the etag changes when the group memberships of the current
user change, because that may impact the permitted labels that are
returned with change info, and getting the known groups is expensive if
there are group backends that need to retrieve them from third-party
systems (actually the same issue exists for the reviewers as well since
permitted labels are also returned for them, but for them we do not
include known groups).
Etags need to be computed when a response is returned (to include the
etag into the response) and when the client presents an etag (to check
whether the etag has changed).
Skipping the etag computation means that we can safe the time for
computing the etag, in exchange for this we need to recompute the change
info even if the change hasn't been updated (when the client presented
an etag and the etag hadn't changed):
1. Change is requested without presenting an etag:
* latency before: change computation + etag computation (to return it
with the response)
* latency after: change computation
2. Change is requested with etag, but the etag doesn't match:
* latency before: etag computation (to compare against the presented
etag) + change computation + etag computation (to return it with the
response)
* latency after: change computation
3. Change is requested with etag and the etag matches:
* latency before: etag computation (to compare against the presented
etag)
* latency after: change computation
Skipping the etag computation is a performance gain for the cases 1. and
2. For case 3. the change computation is likely similar expensive as the
etag computation, so that we probably don't lose (much) performance
here.
Since we are not 100% sure about the effect, we protect skipping change
etags with an experiment flag, so that we can measure the impact safely
before removing the change etag computation for good.
Bug: Google b/336400432
Release-Notes: skip
Change-Id: Ib1ca473180d930c3cea0ade3b528ece34f295b5a
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/server/change/ChangeResource.java b/java/com/google/gerrit/server/change/ChangeResource.java
index 8300541..99a0b50 100644
--- a/java/com/google/gerrit/server/change/ChangeResource.java
+++ b/java/com/google/gerrit/server/change/ChangeResource.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.change;
+import static com.google.gerrit.server.experiments.ExperimentFeaturesConstants.DISABLE_CHANGE_ETAGS;
import static com.google.gerrit.server.project.ProjectCache.illegalState;
import static java.nio.charset.StandardCharsets.UTF_8;
@@ -36,6 +37,7 @@
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.experiments.ExperimentFeatures;
import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.TraceContext;
import com.google.gerrit.server.logging.TraceContext.TraceTimer;
@@ -72,6 +74,7 @@
private static final String ZERO_ID_STRING = ObjectId.zeroId().name();
+ private final ExperimentFeatures experimentFeatures;
private final AccountCache accountCache;
private final ApprovalsUtil approvalUtil;
private final PatchSetUtil patchSetUtil;
@@ -84,6 +87,7 @@
@AssistedInject
ChangeResource(
+ ExperimentFeatures experimentFeatures,
AccountCache accountCache,
ApprovalsUtil approvalUtil,
PatchSetUtil patchSetUtil,
@@ -94,6 +98,7 @@
ChangeData.Factory changeDataFactory,
@Assisted ChangeNotes notes,
@Assisted CurrentUser user) {
+ this.experimentFeatures = experimentFeatures;
this.accountCache = accountCache;
this.approvalUtil = approvalUtil;
this.patchSetUtil = patchSetUtil;
@@ -107,6 +112,7 @@
@AssistedInject
ChangeResource(
+ ExperimentFeatures experimentFeatures,
AccountCache accountCache,
ApprovalsUtil approvalUtil,
PatchSetUtil patchSetUtil,
@@ -116,6 +122,7 @@
PluginSetContext<ChangeETagComputation> changeETagComputation,
@Assisted ChangeData changeData,
@Assisted CurrentUser user) {
+ this.experimentFeatures = experimentFeatures;
this.accountCache = accountCache;
this.approvalUtil = approvalUtil;
this.patchSetUtil = patchSetUtil;
@@ -231,7 +238,12 @@
}
@Override
+ @Nullable
public String getETag() {
+ if (experimentFeatures.isFeatureEnabled(DISABLE_CHANGE_ETAGS)) {
+ return null;
+ }
+
try (TraceTimer ignored =
TraceContext.newTimer(
"Compute change ETag",
diff --git a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
index cd91745..49b5d2a6 100644
--- a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
+++ b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
@@ -60,4 +60,7 @@
/** Whether we allow fix suggestions in HumanComments. */
public static final String ALLOW_FIX_SUGGESTIONS_IN_COMMENTS =
"GerritBackendFeature__allow_fix_suggestions_in_comments";
+
+ /** Whether etags should be disabled for change resources. */
+ public static final String DISABLE_CHANGE_ETAGS = "GerritBackendFeature__disable_change_etags";
}
diff --git a/javatests/com/google/gerrit/acceptance/api/config/ListExperimentsIT.java b/javatests/com/google/gerrit/acceptance/api/config/ListExperimentsIT.java
index fe3cb00..cef9ddd 100644
--- a/javatests/com/google/gerrit/acceptance/api/config/ListExperimentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/config/ListExperimentsIT.java
@@ -52,6 +52,7 @@
.GERRIT_BACKEND_FEATURE_ALWAYS_REJECT_IMPLICIT_MERGES_ON_MERGE,
ExperimentFeaturesConstants.GERRIT_BACKEND_FEATURE_ATTACH_NONCE_TO_DOCUMENTATION,
ExperimentFeaturesConstants.GERRIT_BACKEND_FEATURE_CHECK_IMPLICIT_MERGES_ON_MERGE,
+ ExperimentFeaturesConstants.DISABLE_CHANGE_ETAGS,
ExperimentFeaturesConstants.GERRIT_BACKEND_FEATURE_REJECT_IMPLICIT_MERGES_ON_MERGE)
.inOrder();
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeEtagIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeEtagIT.java
new file mode 100644
index 0000000..6df8c86
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeEtagIT.java
@@ -0,0 +1,56 @@
+// Copyright (C) 2024 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.acceptance.rest.change;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.net.HttpHeaders;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.RestResponse;
+import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
+import com.google.inject.Inject;
+import org.junit.Test;
+
+public class ChangeEtagIT extends AbstractDaemonTest {
+ @Inject private ChangeOperations changeOperations;
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = ExperimentFeaturesConstants.DISABLE_CHANGE_ETAGS)
+ public void changeEtagsDisabled() throws Exception {
+ Change.Id changeId = changeOperations.newChange().create();
+
+ RestResponse response = adminRestSession.get(String.format("/changes/%s", changeId));
+ assertThat(response.getHeader(HttpHeaders.ETAG)).isNull();
+
+ response = adminRestSession.get(String.format("/changes/%s/detail", changeId));
+ assertThat(response.getHeader(HttpHeaders.ETAG)).isNull();
+ }
+
+ @Test
+ public void changeEtagsEnabled() throws Exception {
+ Change.Id changeId = changeOperations.newChange().create();
+
+ RestResponse response = adminRestSession.get(String.format("/changes/%s", changeId));
+ assertThat(response.getHeader(HttpHeaders.ETAG)).isNotNull();
+
+ response = adminRestSession.get(String.format("/changes/%s/detail", changeId));
+ assertThat(response.getHeader(HttpHeaders.ETAG)).isNotNull();
+ }
+}
