Yi EungJun 2014-06-18
PullRequest: Fix painful waiting for merging
Reason: When Yobi merges a pullrequest, it also gets authors related to
the changes by the pullrequest to send them notification. But it is very
very slow because jgit-blame is about 8x slower than git. It takes
hundreds of milliseconds for each changed file.

Solution:
* Get related authors just in time (just before sending notification).
* Give up getting related authors from changes if the changes include
  too many files. The limit is defined as
  GitRepository.BLAME_FILE_LIMIT.

And remove PullRequest.relatedAuthors field because this patch removes
the need.
@e3b7295f9860fe19e52460d5144c9198f1940c43
app/models/NotificationEvent.java
--- app/models/NotificationEvent.java
+++ app/models/NotificationEvent.java
@@ -29,6 +29,8 @@
 import org.apache.commons.collections.CollectionUtils;
 import org.apache.commons.collections.Predicate;
 import org.apache.commons.lang3.StringUtils;
+import org.eclipse.jgit.api.errors.GitAPIException;
+import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.joda.time.DateTime;
 import org.tmatesoft.svn.core.SVNException;
@@ -36,18 +38,19 @@
 import play.db.ebean.Model;
 import play.i18n.Messages;
 import play.libs.Akka;
-import playRepository.Commit;
-import playRepository.GitCommit;
-import playRepository.GitConflicts;
-import playRepository.RepositoryService;
+import playRepository.*;
 import scala.concurrent.duration.Duration;
 import utils.EventConstants;
 import utils.RouteUtil;
 
+import javax.naming.LimitExceededException;
 import javax.persistence.*;
 import javax.servlet.ServletException;
 import java.io.IOException;
-import java.util.*;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
 import java.util.concurrent.TimeUnit;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -741,7 +744,25 @@
 
     private static Set<User> getReceiversWithRelatedAuthors(User sender, PullRequest pullRequest) {
         Set<User> receivers = getDefaultReceivers(pullRequest);
-        receivers.addAll(pullRequest.relatedAuthors);
+        String failureMessage =
+                "Failed to get authors related to the pullrequest " + pullRequest;
+        try {
+            Repository clonedRepository = GitRepository.buildMergingRepository(pullRequest);
+            receivers.addAll(GitRepository.getRelatedAuthors(
+                    clonedRepository,
+                    pullRequest.mergedCommitIdFrom,
+                    pullRequest.mergedCommitIdTo));
+        } catch (LimitExceededException e) {
+            for (ProjectUser member : pullRequest.toProject.members()) {
+                receivers.add(member.user);
+            }
+            play.Logger.info(failureMessage
+                    + ": Get all project members instead", e);
+        } catch (GitAPIException e) {
+            play.Logger.warn(failureMessage, e);
+        } catch (IOException e) {
+            play.Logger.warn(failureMessage, e);
+        }
         receivers.remove(sender);
         return receivers;
     }
app/models/PullRequest.java
--- app/models/PullRequest.java
+++ app/models/PullRequest.java
@@ -155,14 +155,6 @@
     )
     public List<User> reviewers = new ArrayList<>();
 
-    @ManyToMany(cascade = CascadeType.ALL)
-    @JoinTable(
-        name = "pull_request_related_authors",
-        joinColumns = @JoinColumn(name = "pull_request_id"),
-        inverseJoinColumns = @JoinColumn(name = "user_id")
-    )
-    public Set<User> relatedAuthors = new HashSet<>();
-
     @OneToMany(mappedBy = "pullRequest")
     public List<CommentThread> commentThreads = new ArrayList<>();
 
@@ -402,9 +394,6 @@
                     String mergedCommitIdTo = mergeCommit.getId().getName();
                     pullRequest.mergedCommitIdFrom = mergedCommitIdFrom;
                     pullRequest.mergedCommitIdTo = mergedCommitIdTo;
-
-                    pullRequest.relatedAuthors = GitRepository.getRelatedAuthors(cloneRepository,
-                            mergedCommitIdFrom, mergedCommitIdTo);
 
                     GitRepository.push(cloneRepository, GitRepository.getGitDirectoryURL(pullRequest.toProject), mergeBranchName, srcToBranchName);
 
@@ -701,8 +690,6 @@
                     String mergedCommitIdTo = mergeResult.getNewHead().getName();
                     pullRequest.mergedCommitIdFrom = mergedCommitIdFrom;
                     pullRequest.mergedCommitIdTo = mergedCommitIdTo;
-                    pullRequest.relatedAuthors = GitRepository.getRelatedAuthors(clonedRepository,
-                            mergedCommitIdFrom, mergedCommitIdTo);
                 }
             }
         });
app/playRepository/GitRepository.java
--- app/playRepository/GitRepository.java
+++ app/playRepository/GitRepository.java
@@ -20,7 +20,6 @@
  */
 package playRepository;
 
-import controllers.ProjectApp;
 import controllers.UserApp;
 import controllers.routes;
 import models.Project;
@@ -38,7 +37,6 @@
 import org.codehaus.jackson.node.ObjectNode;
 import org.eclipse.jgit.api.*;
 import org.eclipse.jgit.api.errors.GitAPIException;
-import org.eclipse.jgit.api.errors.NoHeadException;
 import org.eclipse.jgit.blame.BlameResult;
 import org.eclipse.jgit.diff.*;
 import org.eclipse.jgit.diff.Edit.Type;
@@ -65,6 +63,7 @@
 import utils.FileUtil;
 import utils.GravatarUtil;
 
+import javax.naming.LimitExceededException;
 import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.IOException;
@@ -81,6 +80,7 @@
     public static final int DIFF_LINE_LIMIT = 3 * FileDiff.LINE_LIMIT;
     public static final int DIFF_FILE_LIMIT = 2000;
     public static final int COMMIT_HISTORY_LIMIT = 1000 * 1000;
+    public static final int BLAME_FILE_LIMIT = 10;
 
     /**
      * The base directory of Git repository
@@ -998,7 +998,7 @@
      * @throws GitAPIException
      */
     public static Set<User> getRelatedAuthors(Repository repository, String revA, String revB)
-            throws IOException, GitAPIException {
+            throws IOException, GitAPIException, LimitExceededException {
         Set<User> authors = new HashSet<>();
         RevWalk revWalk = null;
 
@@ -1008,6 +1008,14 @@
             RevCommit commitB = revWalk.parseCommit(repository.resolve(revB));
             List<DiffEntry> diffs = getDiffEntries(repository, commitA, commitB);
 
+            if (diffs.size() > BLAME_FILE_LIMIT) {
+                String msg = String.format("Reject to get related authors " +
+                        "from changes because of performance issue: The " +
+                        "changes include %n files and it exceeds our limit " +
+                        "of '%n' files.", diffs.size(), BLAME_FILE_LIMIT);
+                throw new LimitExceededException(msg);
+            }
+
             for (DiffEntry diff : diffs) {
                 if (isTypeMatching(diff.getChangeType(), MODIFY, DELETE)) {
                     authors.addAll(getAuthorsFromDiffEntry(repository, diff, commitA));
 
conf/evolutions/default/81.sql (added)
+++ conf/evolutions/default/81.sql
@@ -0,0 +1,12 @@
+# --- !Ups
+alter table pull_request_related_authors drop constraint if exists fk_pull_request_related_authors_1;
+alter table pull_request_related_authors drop constraint if exists fk_pull_request_related_authors_2;
+drop table if exists pull_request_related_authors;
+
+# --- !Downs
+create table pull_request_related_authors (
+    pull_request_id BIGINT NOT NULL,
+    user_id INT NOT NULL
+);
+alter table pull_request_related_authors add constraint fk_pull_request_related_authors_1 foreign key (pull_request_id) references pull_request (id) on delete restrict on update restrict;
+alter table pull_request_related_authors add constraint fk_pull_request_related_authors_2 foreign key (user_id) references n4user (id) on delete restrict on update restrict;
test/playRepository/GitRepositoryTest.java
--- test/playRepository/GitRepositoryTest.java
+++ test/playRepository/GitRepositoryTest.java
@@ -45,6 +45,7 @@
 import play.test.FakeApplication;
 import play.test.Helpers;
 
+import javax.naming.LimitExceededException;
 import java.io.BufferedWriter;
 import java.io.File;
 import java.io.FileWriter;
@@ -595,7 +596,7 @@
     }
 
     @Test
-    public void getRelatedAuthors() throws IOException, GitAPIException {
+    public void getRelatedAuthors() throws IOException, GitAPIException, LimitExceededException {
         FakeApplication app = support.Helpers.makeTestApplication();
         Helpers.start(app);
 
Add a comment
List