Remove eTag from RevisionResource, subject to experiment
In https://gerrit-review.googlesource.com/c/gerrit/+/293978, eTag was
removed from GetRevisionActions, but not RevisionResource. This resulted
in the UI to show stale 'submit' action on the change in the same
submission chain. The UI now uses RevisionResource eTag, that does not include
MergeSuperSet (all related changes).
In this change:
- Return non-cacheable result from GetRevisionActions, so it does not
return RevisionResource eTag.
- Add an experiment flag to remove revision eTag. The computation of eTag
can be expensive, we can run experiments to see if it can be dropped
permanently.
Change-Id: I65a3277d3cb0a116a56f6dcaed205c7f808443c3
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 9b86a4f..269d1c4 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -30,6 +30,7 @@
import static com.google.common.net.HttpHeaders.CONTENT_TYPE;
import static com.google.common.net.HttpHeaders.ORIGIN;
import static com.google.common.net.HttpHeaders.VARY;
+import static com.google.gerrit.server.experiments.ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_REMOVE_REVISION_ETAG;
import static java.math.RoundingMode.CEILING;
import static java.nio.charset.StandardCharsets.ISO_8859_1;
import static java.nio.charset.StandardCharsets.UTF_8;
@@ -110,7 +111,9 @@
import com.google.gerrit.server.audit.ExtendedHttpAuditEvent;
import com.google.gerrit.server.cache.PerThreadCache;
import com.google.gerrit.server.change.ChangeFinder;
+import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.experiments.ExperimentFeatures;
import com.google.gerrit.server.group.GroupAuditService;
import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.PerformanceLogContext;
@@ -252,6 +255,7 @@
final PluginSetContext<ExceptionHook> exceptionHooks;
final Injector injector;
final DynamicMap<DynamicOptions.DynamicBean> dynamicBeans;
+ final ExperimentFeatures experimentFeatures;
@Inject
Globals(
@@ -269,7 +273,8 @@
RetryHelper retryHelper,
PluginSetContext<ExceptionHook> exceptionHooks,
Injector injector,
- DynamicMap<DynamicOptions.DynamicBean> dynamicBeans) {
+ DynamicMap<DynamicOptions.DynamicBean> dynamicBeans,
+ ExperimentFeatures experimentFeatures) {
this.currentUser = currentUser;
this.webSession = webSession;
this.paramParser = paramParser;
@@ -286,6 +291,7 @@
allowOrigin = makeAllowOrigin(config);
this.injector = injector;
this.dynamicBeans = dynamicBeans;
+ this.experimentFeatures = experimentFeatures;
}
private static Pattern makeAllowOrigin(Config cfg) {
@@ -775,6 +781,11 @@
TraceContext.newTimer(
"RestApiServlet#getEtagWithRetry:resource",
Metadata.builder().restViewName(rsrc.getClass().getSimpleName()).build())) {
+ if (rsrc instanceof RevisionResource
+ && globals.experimentFeatures.isFeatureEnabled(
+ GERRIT_BACKEND_REQUEST_FEATURE_REMOVE_REVISION_ETAG)) {
+ return null;
+ }
return invokeRestEndpointWithRetry(
req,
traceContext,
@@ -1056,7 +1067,7 @@
if (rsrc instanceof RestResource.HasETag) {
String have = req.getHeader(HttpHeaders.IF_NONE_MATCH);
- if (have != null) {
+ if (!Strings.isNullOrEmpty(have)) {
String eTag = getEtagWithRetry(req, traceContext, (RestResource.HasETag) rsrc);
return have.equals(eTag);
}
@@ -1134,7 +1145,9 @@
res.setHeader(HttpHeaders.ETAG, eTag);
} else if (rsrc instanceof RestResource.HasETag) {
String eTag = getEtagWithRetry(req, traceContext, (RestResource.HasETag) rsrc);
- res.setHeader(HttpHeaders.ETAG, eTag);
+ if (!Strings.isNullOrEmpty(eTag)) {
+ res.setHeader(HttpHeaders.ETAG, eTag);
+ }
}
if (rsrc instanceof RestResource.HasLastModified) {
res.setDateHeader(
diff --git a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
index af49438..0f85578 100644
--- a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
+++ b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
@@ -22,6 +22,9 @@
/** Features that are known experiments and can be referenced in the code. */
public static String UI_FEATURE_PATCHSET_COMMENTS = "UiFeature__patchset_comments";
+ public static String GERRIT_BACKEND_REQUEST_FEATURE_REMOVE_REVISION_ETAG =
+ "GerritBackendRequestFeature__remove_revision_etag";
+
/** Features, enabled by default in the current release. */
public static final ImmutableSet<String> DEFAULT_ENABLED_FEATURES =
ImmutableSet.of(UI_FEATURE_PATCHSET_COMMENTS);
diff --git a/java/com/google/gerrit/server/restapi/change/GetRevisionActions.java b/java/com/google/gerrit/server/restapi/change/GetRevisionActions.java
index 527129c..f3c0fb8 100644
--- a/java/com/google/gerrit/server/restapi/change/GetRevisionActions.java
+++ b/java/com/google/gerrit/server/restapi/change/GetRevisionActions.java
@@ -34,6 +34,6 @@
@Override
public Response<Map<String, ActionInfo>> apply(RevisionResource rsrc) {
- return Response.withMustRevalidate(delegate.format(rsrc));
+ return Response.ok(delegate.format(rsrc));
}
}
