opendev/gerrit
Code Issues Proposed changes

Retry requests in RestApiServlet, no more retrying for Java API

Browse Source 
Instead of implementing retrying of REST requests in base classes of
REST endpoints, implement retrying of requests once in RestApiServlet.

This has the following advantages:
- We now automatically retry all modifying REST requests (not only
  change related modifying REST requests)
- The implementations of the REST endpoints get easier (no longer need
  to extend a Retrying* base class, RetryHelper no longer needs to be
  injected).
- It's no longer possible to forget to enable retrying for a REST
  endpoint (e.g. by just implementing the interface without extending a
  Retrying* base class)
- Follow-up changes can easily add retrying for other types of requests
  (e.g. read requests or create/delete on collection). Before this
  change we would have needed to add another Retrying* base class and
  adapt all relevant REST endpoints to extend that base class.
- In case of auto-retry-with-trace we do not need a complicated way to
  return the trace ID from the REST endpoint implementation back to
  RestApiServlet (means we can simplify the existing code in a
  follow-up change).
- In RestApiServlet we have more information about the REST endpoint
  available and may include this information into traces on auto-retry
  (e.g. URL of the invoked REST endpoint).

One important change in behavior with this change is that we are no
longer retrying calls that are made through the Java extension API. This
is actually good, because it prevents amplifying of retries when this
API is used by plugins. Examples:

- A plugin implements a new REST endpoint and uses the Java extension
  API to implement it. With the old behavior we would have retrying on
  the REST endpoint and retrying on each call to the Java extension API.
  However we would prefer to have retrying only on one level.

- A plugin implements an extension point that is invoked from a REST
  endpoint and uses the Java extension API to implement it. With the old
  behavior we would have retrying on the REST endpoint and retrying on
  each call to the Java extension API. However we would prefer to have
  retrying only on one level.

In addition it's also better for our tests (that mostly use the Java
extension API) that retrying is not done, since the retry logic may
obfuscate flaky behavior.

RetryingRestModifyView and RetryingRestCollectionModifyView are now no
longer needed and are removed. There was one small complication with
removing RetryingRestModifyView. NoteDbUtil.guessRestApiHandler() was
using this class as a marker to find the REST endpoint class from a
stacktrace if the call was done through the Java API and not via REST.
Instead of looking for RetryingRestModifyView in the stacktrace, we now
watch out for a "com.google.gerrit.server.api.*ApiImpl" class. This may
look a little fragile, but API classes are not supposed to change and if
they do and this breaks we have a test that catches this
(ReflogIT#guessRestApiInReflog). For reference here are sample
stacktraces from before [1] and after this change [2].

[1]
com.google.gerrit.server.notedb.NoteDbUtil.guessRestApiHandler(NoteDbUtil.java:74)
  at com.google.gerrit.server.notedb.NoteDbUpdateManager.execute(NoteDbUpdateManager.java:340)
  at com.google.gerrit.server.notedb.NoteDbUpdateManager.execute(NoteDbUpdateManager.java:305)
  at com.google.gerrit.server.update.BatchUpdate$ChangesHandle.execute(BatchUpdate.java:508)
  at com.google.gerrit.server.update.BatchUpdate.execute(BatchUpdate.java:141)
  at com.google.gerrit.server.update.BatchUpdate.execute(BatchUpdate.java:357)
  at com.google.gerrit.server.update.BatchUpdate.execute(BatchUpdate.java:361)
  at com.google.gerrit.server.restapi.change.PutTopic.applyImpl(PutTopic.java:78)
  at com.google.gerrit.server.restapi.change.PutTopic.applyImpl(PutTopic.java:1)
  at com.google.gerrit.server.update.RetryingRestModifyView.lambda$1(RetryingRestModifyView.java:43)
  at com.google.gerrit.server.update.RetryHelper.lambda$1(RetryHelper.java:274)
  at com.github.rholder.retry.AttemptTimeLimiters$NoAttemptTimeLimit.call(AttemptTimeLimiters.java:78)
  at com.github.rholder.retry.Retryer.call(Retryer.java:160)
  at com.google.gerrit.server.update.RetryHelper.executeWithTimeoutCount(RetryHelper.java:375)
  at com.google.gerrit.server.update.RetryHelper.executeWithAttemptAndTimeoutCount(RetryHelper.java:353)
  at com.google.gerrit.server.update.RetryHelper.execute(RetryHelper.java:257)
  at com.google.gerrit.server.update.RetryHelper.execute(RetryHelper.java:272)
  at com.google.gerrit.server.update.RetryingRestModifyView.apply(RetryingRestModifyView.java:43)
  at com.google.gerrit.server.api.changes.ChangeApiImpl.topic(ChangeApiImpl.java:430)
  at com.google.gerrit.acceptance.server.project.ReflogIT.guessRestApiInReflog(ReflogIT.java:60)
  ...

[2]
com.google.gerrit.server.notedb.NoteDbUtil.guessRestApiHandler(NoteDbUtil.java:72)
  at com.google.gerrit.server.notedb.NoteDbUpdateManager.execute(NoteDbUpdateManager.java:340)
  at com.google.gerrit.server.notedb.NoteDbUpdateManager.execute(NoteDbUpdateManager.java:305)
  at com.google.gerrit.server.update.BatchUpdate$ChangesHandle.execute(BatchUpdate.java:508)
  at com.google.gerrit.server.update.BatchUpdate.execute(BatchUpdate.java:141)
  at com.google.gerrit.server.update.BatchUpdate.execute(BatchUpdate.java:357)
  at com.google.gerrit.server.update.BatchUpdate.execute(BatchUpdate.java:361)
  at com.google.gerrit.server.restapi.change.PutTopic.apply(PutTopic.java:77)
  at com.google.gerrit.server.api.changes.ChangeApiImpl.topic(ChangeApiImpl.java:430)
  at com.google.gerrit.acceptance.server.project.ReflogIT.guessRestApiInReflog(ReflogIT.java:60)
  ...

Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ief5ee42aea9aad575e6927ee54e0b770fd14dc88
This commit is contained in:
Edwin Kempin
2019-10-22 13:38:43 +02:00
committed by David Pursehouse
parent 880a438c90
commit cec334a06f
40 changed files with 207 additions and 404 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
java/com/google/gerrit/server/restapi/change/Rebase.java
View File

@@ -47,8 +47,6 @@ import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.RetryingRestModifyView;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.Inject;
@@ -63,7 +61,7 @@ import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
@Singleton
public class Rebase extends RetryingRestModifyView<RevisionResource, RebaseInput, ChangeInfo>
public class Rebase
implements RestModifyView<RevisionResource, RebaseInput>, UiAction<RevisionResource> {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -81,7 +79,6 @@ public class Rebase extends RetryingRestModifyView<RevisionResource, RebaseInput
@Inject
public Rebase(
RetryHelper retryHelper,
BatchUpdate.Factory updateFactory,
GitRepositoryManager repoManager,
RebaseChangeOp.Factory rebaseFactory,
@@ -90,7 +87,6 @@ public class Rebase extends RetryingRestModifyView<RevisionResource, RebaseInput
PermissionBackend permissionBackend,
ProjectCache projectCache,
PatchSetUtil patchSetUtil) {
super(retryHelper);
this.updateFactory = updateFactory;
this.repoManager = repoManager;
this.rebaseFactory = rebaseFactory;
@@ -102,7 +98,7 @@ public class Rebase extends RetryingRestModifyView<RevisionResource, RebaseInput
}
@Override
protected Response<ChangeInfo> applyImpl(RevisionResource rsrc, RebaseInput input)
public Response<ChangeInfo> apply(RevisionResource rsrc, RebaseInput input)
throws UpdateException, RestApiException, IOException, PermissionBackendException {
// Not allowed to rebase if the current patch set is locked.
patchSetUtil.checkPatchSetNotLocked(rsrc.getNotes());