1682: PullRequestBot::getPeriodicItems takes too long

Reviewed-by: zsong, ihse

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java

@@ -51,8 +51,9 @@ class CheckWorkItem extends PullRequestWorkItem {
     private static final Pattern BACKPORT_ISSUE_TITLE_PATTERN = Pattern.compile("^Backport\\s*(?:(?<prefix>[A-Za-z][A-Za-z0-9]+)-)?(?<id>[0-9]+)\\s*$");
     private static final String ELLIPSIS = "…";
 
-    CheckWorkItem(PullRequestBot bot, String prId, Consumer<RuntimeException> errorHandler, ZonedDateTime prUpdatedAt) {
-        super(bot, prId, errorHandler, prUpdatedAt);
+    CheckWorkItem(PullRequestBot bot, String prId, Consumer<RuntimeException> errorHandler, ZonedDateTime prUpdatedAt,
+            boolean needsReadyCheck) {
+        super(bot, prId, errorHandler, prUpdatedAt, needsReadyCheck);
     }
 
     private String encodeReviewer(HostUser reviewer, CensusInstance censusInstance) {
@@ -268,7 +269,7 @@ public Collection<WorkItem> prRun(Path scratchPath) {
                 log.info("Skipping check of integrated PR");
                 // We still need to make sure any commands get run or are able to finish a
                 // previously interrupted run
-                return List.of(new PullRequestCommandWorkItem(bot, prId, errorHandler, prUpdatedAt));
+                return List.of(new PullRequestCommandWorkItem(bot, prId, errorHandler, prUpdatedAt, false));
             }
 
             var backportHashMatcher = BACKPORT_HASH_TITLE_PATTERN.matcher(pr.title());
@@ -332,7 +333,7 @@ public Collection<WorkItem> prRun(Path scratchPath) {
                     comment.add(text);
                     pr.addComment(String.join("\n", comment));
                     pr.addLabel("backport");
-                    return List.of(new CheckWorkItem(bot, prId, errorHandler, prUpdatedAt));
+                    return List.of(new CheckWorkItem(bot, prId, errorHandler, prUpdatedAt, false));
                 } else {
                     var botUser = pr.repository().forge().currentUser();
                     var text = "<!-- backport error -->\n" +
@@ -372,14 +373,14 @@ public Collection<WorkItem> prRun(Path scratchPath) {
                 var comment = pr.addComment(text);
                 pr.addLabel("backport");
                 logLatency("Time from PR updated to backport comment posted ", comment.createdAt(), log);
-                return List.of(new CheckWorkItem(bot, prId, errorHandler, prUpdatedAt));
+                return List.of(new CheckWorkItem(bot, prId, errorHandler, prUpdatedAt, false));
             }
 
             // If the title needs updating, we run the check again
             if (updateTitle()) {
                 var updatedPr = bot.repo().pullRequest(prId);
                 logLatency("Time from PR updated to title corrected ", updatedPr.updatedAt(), log);
-                return List.of(new CheckWorkItem(bot, prId, errorHandler, prUpdatedAt));
+                return List.of(new CheckWorkItem(bot, prId, errorHandler, prUpdatedAt, false));
             }
 
             try {
@@ -412,7 +413,7 @@ public Collection<WorkItem> prRun(Path scratchPath) {
             log.log(Level.INFO, "Time from labels added to /integrate posted " + latency, latency);
         }
 
-        return List.of(new PullRequestCommandWorkItem(bot, prId, errorHandler, prUpdatedAt));
+        return List.of(new PullRequestCommandWorkItem(bot, prId, errorHandler, prUpdatedAt, false));
     }
 
     /**

bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelerWorkItem.java

@@ -39,7 +39,7 @@ public class LabelerWorkItem extends PullRequestWorkItem {
 
     LabelerWorkItem(PullRequestBot bot, String prId, Consumer<RuntimeException> errorHandler,
             ZonedDateTime prUpdatedAt) {
-        super(bot, prId, errorHandler, prUpdatedAt);
+        super(bot, prId, errorHandler, prUpdatedAt, false);
     }
 
     @Override

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBot.java

@@ -112,36 +112,6 @@ static PullRequestBotBuilder newBuilder() {
         return new PullRequestBotBuilder();
     }
 
-    private boolean isReady(PullRequest pr) {
-        var labels = new HashSet<>(pr.labelNames());
-        for (var readyLabel : readyLabels) {
-            if (!labels.contains(readyLabel)) {
-                log.fine("PR is not yet ready - missing label '" + readyLabel + "'");
-                return false;
-            }
-        }
-
-        var comments = pr.comments();
-        for (var readyComment : readyComments.entrySet()) {
-            var commentFound = false;
-            for (var comment : comments) {
-                if (comment.author().username().equals(readyComment.getKey())) {
-                    var matcher = readyComment.getValue().matcher(comment.body());
-                    if (matcher.find()) {
-                        commentFound = true;
-                        break;
-                    }
-                }
-            }
-            if (!commentFound) {
-                log.fine("PR is not yet ready - missing ready comment from '" + readyComment.getKey() +
-                                 "containing '" + readyComment.getValue().pattern() + "'");
-                return false;
-            }
-        }
-        return true;
-    }
-
     void scheduleRecheckAt(PullRequest pr, Instant expiresAt) {
         log.info("Setting check metadata expiration to: " + expiresAt + " for PR #" + pr.id());
         poller.retryPullRequest(pr, expiresAt);
@@ -152,14 +122,11 @@ private List<WorkItem> getWorkItems(List<PullRequest> pullRequests) {
         ret.add(new CommitCommentsWorkItem(this, remoteRepo, excludeCommitCommentsFrom));
 
         for (var pr : pullRequests) {
-            if (!isReady(pr)) {
-                continue;
-            }
             if (pr.state() == Issue.State.OPEN) {
-                ret.add(new CheckWorkItem(this, pr.id(), e -> poller.retryPullRequest(pr), pr.updatedAt()));
+                ret.add(new CheckWorkItem(this, pr.id(), e -> poller.retryPullRequest(pr), pr.updatedAt(), true));
             } else {
                 // Closed PR's do not need to be checked
-                ret.add(new PullRequestCommandWorkItem(this, pr.id(), e -> poller.retryPullRequest(pr), pr.updatedAt()));
+                ret.add(new PullRequestCommandWorkItem(this, pr.id(), e -> poller.retryPullRequest(pr), pr.updatedAt(), true));
             }
         }
 
@@ -212,6 +179,10 @@ Map<String, String> blockingCheckLabels() {
         return blockingCheckLabels;
     }
 
+    public Set<String> readyLabels() {
+        return readyLabels;
+    }
+
     Set<String> twoReviewersLabels() {
         return twoReviewersLabels;
     }
@@ -220,6 +191,10 @@ Set<String> twentyFourHoursLabels() {
         return twentyFourHoursLabels;
     }
 
+    public Map<String, Pattern> readyComments() {
+        return readyComments;
+    }
+
     IssueProject issueProject() {
         return issueProject;
     }

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestCommandWorkItem.java

@@ -46,8 +46,8 @@ public class PullRequestCommandWorkItem extends PullRequestWorkItem {
     public static final String VALID_BOT_COMMAND_MARKER = "<!-- Valid self-command -->";
 
     PullRequestCommandWorkItem(PullRequestBot bot, String prId, Consumer<RuntimeException> errorHandler,
-            ZonedDateTime prUpdatedAt) {
-        super(bot, prId, errorHandler, prUpdatedAt);
+            ZonedDateTime prUpdatedAt, boolean needsReadyCheck) {
+        super(bot, prId, errorHandler, prUpdatedAt, needsReadyCheck);
     }
 
     private static class InvalidBodyCommandHandler implements CommandHandler {
@@ -212,7 +212,7 @@ public Collection<WorkItem> prRun(Path scratchPath) {
         if (!pr.labelNames().contains("integrated") || pr.findIntegratedCommitHash().isEmpty()) {
             processCommand(pr, census, scratchPath.resolve("pr").resolve("command"), command, comments, false);
             // Run another check to reflect potential changes from commands
-            return List.of(new CheckWorkItem(bot, prId, errorHandler, prUpdatedAt));
+            return List.of(new CheckWorkItem(bot, prId, errorHandler, prUpdatedAt, false));
         } else {
             processCommand(pr, census, scratchPath.resolve("pr").resolve("command"), command, comments, true);
             return List.of();

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestWorkItem.java

@@ -26,6 +26,8 @@
 import java.time.Duration;
 import java.time.ZonedDateTime;
 import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import org.openjdk.skara.bot.WorkItem;
@@ -34,9 +36,11 @@
 import java.util.function.Consumer;
 
 abstract class PullRequestWorkItem implements WorkItem {
+    private static final Logger log = Logger.getLogger(PullRequestWorkItem.class.getName());
     final Consumer<RuntimeException> errorHandler;
     final PullRequestBot bot;
     final String prId;
+    private final boolean needsReadyCheck;
     /**
      * The updatedAt timestamp of the PR that triggered this WorkItem at the
      * time it was triggered. Used for tracking reaction latency of the bot
@@ -49,11 +53,12 @@ abstract class PullRequestWorkItem implements WorkItem {
     PullRequest pr;
 
     PullRequestWorkItem(PullRequestBot bot, String prId, Consumer<RuntimeException> errorHandler,
-            ZonedDateTime prUpdatedAt) {
+            ZonedDateTime prUpdatedAt, boolean needsReadyCheck) {
         this.bot = bot;
         this.prId = prId;
         this.errorHandler = errorHandler;
         this.prUpdatedAt = prUpdatedAt;
+        this.needsReadyCheck = needsReadyCheck;
     }
 
     @Override
@@ -67,6 +72,39 @@ public final boolean concurrentWith(WorkItem other) {
         return false;
     }
 
+    private boolean isReady() {
+        if (!needsReadyCheck) {
+            return true;
+        }
+        var labels = new HashSet<>(pr.labelNames());
+        for (var readyLabel : bot.readyLabels()) {
+            if (!labels.contains(readyLabel)) {
+                log.fine("PR is not yet ready - missing label '" + readyLabel + "'");
+                return false;
+            }
+        }
+
+        var comments = pr.comments();
+        for (var readyComment : bot.readyComments().entrySet()) {
+            var commentFound = false;
+            for (var comment : comments) {
+                if (comment.author().username().equals(readyComment.getKey())) {
+                    var matcher = readyComment.getValue().matcher(comment.body());
+                    if (matcher.find()) {
+                        commentFound = true;
+                        break;
+                    }
+                }
+            }
+            if (!commentFound) {
+                log.fine("PR is not yet ready - missing ready comment from '" + readyComment.getKey() +
+                        "containing '" + readyComment.getValue().pattern() + "'");
+                return false;
+            }
+        }
+        return true;
+    }
+
     /**
      * Loads the PR from the remote repo at the start of run to guarantee that all
      * PullRequestWorkItems have a coherent and current view of the PR to avoid
@@ -78,6 +116,10 @@ public final boolean concurrentWith(WorkItem other) {
     @Override
     public final Collection<WorkItem> run(Path scratchPath) {
         pr = bot.repo().pullRequest(prId);
+        // Check if PR is ready to be evaluated at all.
+        if (!isReady()) {
+            return List.of();
+        }
         return prRun(scratchPath);
     }