
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
124 lines
4.7 KiB
Java
124 lines
4.7 KiB
Java
// Copyright (C) 2016 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.restapi.change;
|
|
|
|
import com.google.gerrit.entities.Account;
|
|
import com.google.gerrit.entities.Change;
|
|
import com.google.gerrit.entities.ChangeMessage;
|
|
import com.google.gerrit.extensions.common.AccountInfo;
|
|
import com.google.gerrit.extensions.common.Input;
|
|
import com.google.gerrit.extensions.restapi.Response;
|
|
import com.google.gerrit.extensions.restapi.RestApiException;
|
|
import com.google.gerrit.extensions.restapi.RestModifyView;
|
|
import com.google.gerrit.server.ChangeMessagesUtil;
|
|
import com.google.gerrit.server.IdentifiedUser;
|
|
import com.google.gerrit.server.account.AccountLoader;
|
|
import com.google.gerrit.server.account.AccountState;
|
|
import com.google.gerrit.server.change.ChangeResource;
|
|
import com.google.gerrit.server.extensions.events.AssigneeChanged;
|
|
import com.google.gerrit.server.notedb.ChangeUpdate;
|
|
import com.google.gerrit.server.permissions.ChangePermission;
|
|
import com.google.gerrit.server.permissions.PermissionBackendException;
|
|
import com.google.gerrit.server.update.BatchUpdate;
|
|
import com.google.gerrit.server.update.BatchUpdateOp;
|
|
import com.google.gerrit.server.update.ChangeContext;
|
|
import com.google.gerrit.server.update.Context;
|
|
import com.google.gerrit.server.update.UpdateException;
|
|
import com.google.gerrit.server.util.time.TimeUtil;
|
|
import com.google.inject.Inject;
|
|
import com.google.inject.Singleton;
|
|
|
|
@Singleton
|
|
public class DeleteAssignee implements RestModifyView<ChangeResource, Input> {
|
|
private final BatchUpdate.Factory updateFactory;
|
|
private final ChangeMessagesUtil cmUtil;
|
|
private final AssigneeChanged assigneeChanged;
|
|
private final IdentifiedUser.GenericFactory userFactory;
|
|
private final AccountLoader.Factory accountLoaderFactory;
|
|
|
|
@Inject
|
|
DeleteAssignee(
|
|
BatchUpdate.Factory updateFactory,
|
|
ChangeMessagesUtil cmUtil,
|
|
AssigneeChanged assigneeChanged,
|
|
IdentifiedUser.GenericFactory userFactory,
|
|
AccountLoader.Factory accountLoaderFactory) {
|
|
this.updateFactory = updateFactory;
|
|
this.cmUtil = cmUtil;
|
|
this.assigneeChanged = assigneeChanged;
|
|
this.userFactory = userFactory;
|
|
this.accountLoaderFactory = accountLoaderFactory;
|
|
}
|
|
|
|
@Override
|
|
public Response<AccountInfo> apply(ChangeResource rsrc, Input input)
|
|
throws RestApiException, UpdateException, PermissionBackendException {
|
|
rsrc.permissions().check(ChangePermission.EDIT_ASSIGNEE);
|
|
|
|
try (BatchUpdate bu =
|
|
updateFactory.create(rsrc.getProject(), rsrc.getUser(), TimeUtil.nowTs())) {
|
|
Op op = new Op();
|
|
bu.addOp(rsrc.getChange().getId(), op);
|
|
bu.execute();
|
|
Account.Id deletedAssignee = op.getDeletedAssignee();
|
|
return deletedAssignee == null
|
|
? Response.none()
|
|
: Response.ok(accountLoaderFactory.create(true).fillOne(deletedAssignee));
|
|
}
|
|
}
|
|
|
|
private class Op implements BatchUpdateOp {
|
|
private Change change;
|
|
private AccountState deletedAssignee;
|
|
|
|
@Override
|
|
public boolean updateChange(ChangeContext ctx) throws RestApiException {
|
|
change = ctx.getChange();
|
|
ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId());
|
|
Account.Id currentAssigneeId = change.getAssignee();
|
|
if (currentAssigneeId == null) {
|
|
return false;
|
|
}
|
|
IdentifiedUser deletedAssigneeUser = userFactory.create(currentAssigneeId);
|
|
deletedAssignee = deletedAssigneeUser.state();
|
|
// noteDb
|
|
update.removeAssignee();
|
|
// reviewDb
|
|
change.setAssignee(null);
|
|
addMessage(ctx, update, deletedAssigneeUser);
|
|
return true;
|
|
}
|
|
|
|
public Account.Id getDeletedAssignee() {
|
|
return deletedAssignee != null ? deletedAssignee.account().id() : null;
|
|
}
|
|
|
|
private void addMessage(
|
|
ChangeContext ctx, ChangeUpdate update, IdentifiedUser deletedAssignee) {
|
|
ChangeMessage cmsg =
|
|
ChangeMessagesUtil.newMessage(
|
|
ctx,
|
|
"Assignee deleted: " + deletedAssignee.getNameEmail(),
|
|
ChangeMessagesUtil.TAG_DELETE_ASSIGNEE);
|
|
cmUtil.addChangeMessage(update, cmsg);
|
|
}
|
|
|
|
@Override
|
|
public void postUpdate(Context ctx) {
|
|
assigneeChanged.fire(change, ctx.getAccount(), deletedAssignee, ctx.getWhen());
|
|
}
|
|
}
|
|
}
|