Skip to content

Commit

Permalink
Merge "Guard ChangeCleanupRunner.run with a lock"
Browse files Browse the repository at this point in the history
  • Loading branch information
msohn authored and Gerrit Code Review committed Dec 9, 2024
2 parents 7b5e410 + 91a2122 commit e511344
Showing 1 changed file with 17 additions and 0 deletions.
17 changes: 17 additions & 0 deletions java/com/google/gerrit/server/change/ChangeCleanupRunner.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,15 @@
import com.google.gerrit.lifecycle.LifecycleModule;
import com.google.gerrit.server.config.ChangeCleanupConfig;
import com.google.gerrit.server.git.WorkQueue;
import com.google.gerrit.server.project.LockManager;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.ManualRequestContext;
import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.util.concurrent.locks.Lock;

/** Runnable to enable scheduling change cleanups to run periodically */
public class ChangeCleanupRunner implements Runnable {
Expand Down Expand Up @@ -73,6 +75,7 @@ public void stop() {
private final OneOffRequestContext oneOffRequestContext;
private final AbandonUtil abandonUtil;
private final RetryHelper retryHelper;
private final LockManager lockManager;
private final long abandonAfterMillis;
private final boolean abandonIfMergeable;
@Nullable private final String message;
Expand All @@ -82,12 +85,14 @@ public void stop() {
OneOffRequestContext oneOffRequestContext,
AbandonUtil abandonUtil,
RetryHelper retryHelper,
LockManager lockManager,
@Assisted long abandonAfterMillis,
@Assisted boolean abandonIfMergeable,
@Assisted @Nullable String message) {
this.oneOffRequestContext = oneOffRequestContext;
this.abandonUtil = abandonUtil;
this.retryHelper = retryHelper;
this.lockManager = lockManager;
this.abandonAfterMillis = abandonAfterMillis;
this.abandonIfMergeable = abandonIfMergeable;
this.message = message;
Expand All @@ -98,17 +103,27 @@ public void stop() {
OneOffRequestContext oneOffRequestContext,
AbandonUtil abandonUtil,
RetryHelper retryHelper,
LockManager lockManager,
ChangeCleanupConfig cfg) {
this.oneOffRequestContext = oneOffRequestContext;
this.abandonUtil = abandonUtil;
this.retryHelper = retryHelper;
this.lockManager = lockManager;
this.abandonAfterMillis = cfg.getAbandonAfter();
this.abandonIfMergeable = cfg.getAbandonIfMergeable();
this.message = cfg.getAbandonMessage();
}

@Override
public void run() {
Lock lock = lockManager.getLock("change-cleanup");
if (!lock.tryLock()) {
logger.atInfo().log(
"Couldn't acquire change-cleanup lock. Assuming another server is running"
+ " change-cleanup");
return;
}

logger.atInfo().log("Running change cleanups.");
try (ManualRequestContext ctx = oneOffRequestContext.open()) {
// abandonInactiveOpenChanges skips failures instead of throwing, so retrying will never
Expand All @@ -127,6 +142,8 @@ public void run() {
.call();
} catch (RestApiException | UpdateException e) {
logger.atSevere().withCause(e).log("Failed to cleanup changes.");
} finally {
lock.unlock();
}
}

Expand Down

0 comments on commit e511344

Please sign in to comment.