Skip to content
Permalink
Browse files
251: Bypass PR update cache when the cooldown period expires for a br…
…idge candidate

Reviewed-by: ehelin
  • Loading branch information
rwestberg committed Feb 6, 2020
1 parent 72c721a commit 3cbdfd40a43629eec945d29d2cf5b75a9e4241bb
@@ -44,12 +44,14 @@ class ArchiveWorkItem implements WorkItem {
private final PullRequest pr;
private final MailingListBridgeBot bot;
private final Consumer<RuntimeException> exceptionConsumer;
private final Consumer<Instant> retryConsumer;
private final Logger log = Logger.getLogger("org.openjdk.skara.bots.mlbridge");

ArchiveWorkItem(PullRequest pr, MailingListBridgeBot bot, Consumer<RuntimeException> exceptionConsumer) {
ArchiveWorkItem(PullRequest pr, MailingListBridgeBot bot, Consumer<RuntimeException> exceptionConsumer, Consumer<Instant> retryConsumer) {
this.pr = pr;
this.bot = bot;
this.exceptionConsumer = exceptionConsumer;
this.retryConsumer = retryConsumer;
}

@Override
@@ -288,7 +290,8 @@ public void run(Path scratchPath) {
(index, full, inc) -> updateWebrevComment(comments, index, full, inc),
user -> getAuthorAddress(census, user),
user -> getAuthorUserName(census, user),
user -> getAuthorRole(census, user));
user -> getAuthorRole(census, user),
retryConsumer);
if (newMails.isEmpty()) {
return;
}
@@ -0,0 +1,58 @@
/*
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
package org.openjdk.skara.bots.mlbridge;

import org.openjdk.skara.forge.*;

import java.time.*;
import java.util.*;
import java.util.logging.Logger;

public class CooldownQuarantine {
private final Map<String, Instant> quarantineEnd = new HashMap<>();
private final Logger log = Logger.getLogger("org.openjdk.skara.bots.mlbridge");

public synchronized boolean inQuarantine(PullRequest pr) {
var uniqueId = pr.webUrl().toString();

if (!quarantineEnd.containsKey(uniqueId)) {
return false;
}
var end = quarantineEnd.get(uniqueId);
if (end.isBefore(Instant.now())) {
log.info("Released from cooldown quarantine: " + pr.repository().name() + "#" + pr.id());
quarantineEnd.remove(uniqueId);
return false;
}
log.info("Quarantined due to cooldown: " + pr.repository().name() + "#" + pr.id());
return true;
}

public synchronized void updateQuarantineEnd(PullRequest pr, Instant end) {
var uniqueId = pr.webUrl().toString();
var currentEnd = quarantineEnd.getOrDefault(uniqueId, Instant.now());
if (end.isAfter(currentEnd)) {
quarantineEnd.put(uniqueId, end);
}
}
}
@@ -28,7 +28,7 @@

import java.net.URI;
import java.nio.file.Path;
import java.time.Duration;
import java.time.*;
import java.util.*;
import java.util.regex.Pattern;

@@ -52,6 +52,7 @@ public class MailingListBridgeBot implements Bot {
private final PullRequestUpdateCache updateCache;
private final Duration sendInterval;
private final Duration cooldown;
private final CooldownQuarantine cooldownQuarantine;

MailingListBridgeBot(EmailAddress from, HostedRepository repo, HostedRepository archive, String archiveRef,
HostedRepository censusRepo, String censusRef, EmailAddress list,
@@ -78,9 +79,10 @@ public class MailingListBridgeBot implements Bot {
this.sendInterval = sendInterval;
this.cooldown = cooldown;

this.webrevStorage = new WebrevStorage(webrevStorageRepository, webrevStorageRef, webrevStorageBase,
webrevStorageBaseUri, from);
this.updateCache = new PullRequestUpdateCache();
webrevStorage = new WebrevStorage(webrevStorageRepository, webrevStorageRef, webrevStorageBase,
webrevStorageBaseUri, from);
updateCache = new PullRequestUpdateCache();
cooldownQuarantine = new CooldownQuarantine();
}

static MailingListBridgeBotBuilder newBuilder() {
@@ -164,8 +166,10 @@ public List<WorkItem> getPeriodicItems() {
List<WorkItem> ret = new LinkedList<>();

for (var pr : codeRepo.pullRequests()) {
if (updateCache.needsUpdate(pr)) {
ret.add(new ArchiveWorkItem(pr, this, e -> updateCache.invalidate(pr)));
if (!cooldownQuarantine.inQuarantine(pr) || updateCache.needsUpdate(pr)) {
ret.add(new ArchiveWorkItem(pr, this,
e -> updateCache.invalidate(pr),
r -> cooldownQuarantine.updateQuarantineEnd(pr, r)));
}
}

@@ -11,6 +11,7 @@
import java.security.*;
import java.time.*;
import java.util.*;
import java.util.function.Consumer;
import java.util.logging.Logger;
import java.util.stream.*;

@@ -176,7 +177,7 @@ private String getStableMessageId(EmailAddress uniqueMessageId) {
return uniqueMessageId.localPart().split("\\.")[0];
}

List<Email> generateNewEmails(List<Email> sentEmails, Duration cooldown, Repository localRepo, URI issueTracker, String issuePrefix, WebrevStorage.WebrevGenerator webrevGenerator, WebrevNotification webrevNotification, HostUserToEmailAuthor hostUserToEmailAuthor, HostUserToUserName hostUserToUserName, HostUserToRole hostUserToRole) {
List<Email> generateNewEmails(List<Email> sentEmails, Duration cooldown, Repository localRepo, URI issueTracker, String issuePrefix, WebrevStorage.WebrevGenerator webrevGenerator, WebrevNotification webrevNotification, HostUserToEmailAuthor hostUserToEmailAuthor, HostUserToUserName hostUserToUserName, HostUserToRole hostUserToRole, Consumer<Instant> retryConsumer) {
var ret = new ArrayList<Email>();
var allItems = generateArchiveItems(sentEmails, localRepo, issueTracker, issuePrefix, hostUserToEmailAuthor, hostUserToUserName, hostUserToRole, webrevGenerator, webrevNotification);
var sentItemIds = sentItemIds(sentEmails);
@@ -189,8 +190,11 @@ List<Email> generateNewEmails(List<Email> sentEmails, Duration cooldown, Reposit
var lastUpdate = unsentItems.stream()
.map(ArchiveItem::updatedAt)
.max(ZonedDateTime::compareTo).orElseThrow();
var mayUpdate = lastUpdate.plus(cooldown);
if (lastUpdate.plus(cooldown).isAfter(ZonedDateTime.now())) {
log.info("Waiting for new content to settle down - last update was at " + lastUpdate);
log.info("Retry again after " + mayUpdate);
retryConsumer.accept(mayUpdate.toInstant());
return ret;
}

@@ -34,14 +34,17 @@
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.*;
import java.time.Duration;
import java.time.*;
import java.util.*;
import java.util.logging.Logger;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

import static org.junit.jupiter.api.Assertions.*;

class MailingListBridgeBotTests {
private final Logger log = Logger.getLogger("org.openjdk.skara.bots.mlbridge.test");

private Optional<String> archiveContents(Path archive) {
try {
var mbox = Files.find(archive, 50, (path, attrs) -> path.toString().endsWith(".mbox")).findAny();
@@ -1522,4 +1525,83 @@ void cooldown(TestInfo testInfo) throws IOException {
assertTrue(archiveContains(archiveFolder.path(), "Looks good"));
}
}

@Test
void retryAfterCooldown(TestInfo testInfo) throws IOException, InterruptedException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory();
var archiveFolder = new TemporaryDirectory();
var listServer = new TestMailmanServer();
var webrevServer = new TestWebrevServer()) {
var bot = credentials.getHostedRepository();
var author = credentials.getHostedRepository();
var archive = credentials.getHostedRepository();
var listAddress = EmailAddress.parse(listServer.createList("test"));
var censusBuilder = credentials.getCensusBuilder()
.addAuthor(author.forge().currentUser().id());
var from = EmailAddress.from("test", "test@test.mail");
var cooldown = Duration.ofMillis(500);
var mlBotBuilder = MailingListBridgeBot.newBuilder()
.from(from)
.repo(bot)
.ignoredUsers(Set.of(bot.forge().currentUser().userName()))
.archive(archive)
.censusRepo(censusBuilder.build())
.list(listAddress)
.listArchive(listServer.getArchive())
.smtpServer(listServer.getSMTP())
.webrevStorageRepository(archive)
.webrevStorageRef("webrev")
.webrevStorageBase(Path.of("test"))
.webrevStorageBaseUri(webrevServer.uri())
.issueTracker(URIBuilder.base("http://issues.test/browse/").build());

// Populate the projects repository
var reviewFile = Path.of("reviewfile.txt");
var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType(), reviewFile);
var masterHash = localRepo.resolve("master").orElseThrow();
localRepo.push(masterHash, author.url(), "master", true);
localRepo.push(masterHash, archive.url(), "webrev", true);

// Make a change with a corresponding PR
var editHash = CheckableRepository.appendAndCommit(localRepo, "Line 1\nLine 2\nLine 3\nLine 4");
localRepo.push(editHash, author.url(), "edit", true);
var pr = credentials.createPullRequest(archive, "master", "edit", "This is a pull request");
pr.setBody("This is now ready");

var mlBot = mlBotBuilder.cooldown(cooldown).build();
Thread.sleep(cooldown.toMillis());
TestBotRunner.runPeriodicItems(mlBot);
listServer.processIncoming();

// Make a comment and run the check within the cooldown period
int counter;
for (counter = 1; counter < 10; ++counter) {
var start = Instant.now();
pr.addComment("Looks good - " + counter + " -");

// The bot should not bridge the comment due to cooldown
TestBotRunner.runPeriodicItems(mlBot);
var elapsed = Duration.between(start, Instant.now());
if (elapsed.compareTo(cooldown) < 0) {
break;
} else {
log.info("Didn't do the test in time - retrying (elapsed: " + elapsed + " required: " + cooldown + ")");
listServer.processIncoming();
cooldown = cooldown.multipliedBy(2);
mlBot = mlBotBuilder.cooldown(cooldown).build();
}
}
assertThrows(RuntimeException.class, () -> listServer.processIncoming(Duration.ofMillis(1)));

// But after the cooldown period has passed, it should
Thread.sleep(cooldown.toMillis());
TestBotRunner.runPeriodicItems(mlBot);
listServer.processIncoming();

// Check the archive
Repository.materialize(archiveFolder.path(), archive.url(), "master");
assertTrue(archiveContains(archiveFolder.path(), "Looks good - " + counter + " -"));
}
}
}
@@ -29,26 +29,16 @@
import java.util.logging.Logger;

public class PullRequestUpdateCache {
private final Map<HostedRepository, String> repositoryIds = new HashMap<>();
private final Map<String, ZonedDateTime> lastUpdates = new HashMap<>();

private final Logger log = Logger.getLogger("org.openjdk.skara.host");

private String getUniqueId(PullRequest pr) {
var repo = pr.repository();
if (!repositoryIds.containsKey(repo)) {
repositoryIds.put(repo, Integer.toString(repositoryIds.size()));
}
return repositoryIds.get(repo) + ";" + pr.id();
}

public synchronized boolean needsUpdate(PullRequest pr) {
// GitLab CE does not update this field on events such as adding an award
if (pr instanceof GitLabMergeRequest) {
return true;
}

var uniqueId = getUniqueId(pr);
var uniqueId = pr.webUrl().toString();
var update = pr.updatedAt();

if (!lastUpdates.containsKey(uniqueId)) {
@@ -65,7 +55,7 @@ public synchronized boolean needsUpdate(PullRequest pr) {
}

public synchronized void invalidate(PullRequest pr) {
var uniqueId = getUniqueId(pr);
var uniqueId = pr.webUrl().toString();
lastUpdates.remove(uniqueId);
}
}

0 comments on commit 3cbdfd4

Please sign in to comment.