Skip to content

Commit

Permalink
Optimizing PR search requests (#53)
Browse files Browse the repository at this point in the history
* Using larger page size for open PR searches
* Using PR commit stream instead of list
  • Loading branch information
monitorjbl committed Jan 4, 2018
1 parent c95323a commit 99d3f6d
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 17 deletions.
45 changes: 30 additions & 15 deletions src/main/java/com/monitorjbl/plugins/PullRequestListener.java
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package com.monitorjbl.plugins;

import com.atlassian.bitbucket.build.BuildStatusSetEvent;
import com.atlassian.bitbucket.commit.Commit;
import com.atlassian.bitbucket.event.pull.PullRequestParticipantStatusUpdatedEvent;
import com.atlassian.bitbucket.permission.Permission;
import com.atlassian.bitbucket.pull.PullRequest;
Expand All @@ -12,18 +11,21 @@
import com.atlassian.bitbucket.repository.Repository;
import com.atlassian.bitbucket.user.SecurityService;
import com.atlassian.bitbucket.util.Page;
import com.atlassian.bitbucket.util.PageRequest;
import com.atlassian.bitbucket.util.PageRequestImpl;
import com.atlassian.event.api.EventListener;
import com.monitorjbl.plugins.AsyncProcessor.TaskProcessor;
import com.monitorjbl.plugins.config.Config;
import com.monitorjbl.plugins.config.ConfigDao;

import java.io.Serializable;
import java.util.concurrent.atomic.AtomicBoolean;

public class PullRequestListener {
private static final String PR_APPROVE_BUCKET = "AUTOMERGE_PR_APPROVAL";
private static final String BUILD_APPROVE_BUCKET = "AUTOMERGE_BUILD_APPROVAL";
public static final int MAX_COMMITS = 1048576;

This comment has been minimized.

Copy link
@bturner

bturner Jan 5, 2018

Is this constant still used? Seems like it might be removable now

public static final int SEARCH_PAGE_SIZE = 50;

private final AsyncProcessor asyncProcessor;
private final ConfigDao configDao;
Expand Down Expand Up @@ -69,21 +71,33 @@ void automergePullRequest(PullRequest pr) {
}

PullRequest findPRByCommitId(String commitId) {
int start = 0;
Page<PullRequest> requests = null;
while(requests == null || requests.getSize() > 0) {
requests = prService.search(new PullRequestSearchRequest.Builder()
.state(PullRequestState.OPEN)
.build(), new PageRequestImpl(start, 10));
for(PullRequest pr : requests.getValues()) {
Page<Commit> commits = prService.getCommits(pr.getToRef().getRepository().getId(), pr.getId(), new PageRequestImpl(0, MAX_COMMITS));
for(Commit c : commits.getValues()) {
if(c.getId().equals(commitId)) {
return pr;
}
}
PullRequestSearchRequest request = new PullRequestSearchRequest.Builder()
.state(PullRequestState.OPEN)
.withProperties(false)
.build();
PageRequest nextPage = new PageRequestImpl(0, SEARCH_PAGE_SIZE);

This comment has been minimized.

Copy link
@bturner

bturner Jan 5, 2018

Instead of new PageRequestImpl, I'd use PageUtils.newRequest /nit

do {
Page<PullRequest> page = prService.search(request, nextPage);
PullRequest pr = searchForPR(page, commitId);
if(pr != null) {
return pr;
} else {
nextPage = page.getNextPageRequest();
}
} while(nextPage != null);
return null;
}

private PullRequest searchForPR(Page<PullRequest> requests, String commitId) {
for(PullRequest pr : requests.getValues()) {
AtomicBoolean found = new AtomicBoolean(false);

This comment has been minimized.

Copy link
@bturner

bturner Jan 5, 2018

I wouldn't use an AtomicBoolean. There's no concurrency here, so the synchronization aspects of that are pure overhead. I'd use org.apache.commons.lang3.mutable.MutableBoolean instead.

Something like this:

private PullRequest searchForPR(Page<PullRequest> requests, String commitId) {
    for (PullRequest pr : requests.getValues()) {
        MutableBoolean found = new MutableBoolean();
        prService.streamCommits(pr.getToRef().getRepository().getId(), pr.getId(), commit -> {
            if (commit.getId().equals(commitId)) {
                found.setTrue();
                return false;
            }
            return true;
        });

        if (found.isTrue()) {
            return pr;
        }
    }

    return null;
}
prService.streamCommits(pr.getToRef().getRepository().getId(), pr.getId(), commit -> {
found.set(commit.getId().equals(commitId));
return !found.get();
});
if(found.get()) {
return pr;
}
start += 10;
}
return null;
}
Expand Down Expand Up @@ -130,4 +144,5 @@ public ApprovalTask(Long pullRequestId, Integer repositoryId, String commitId) {
this.commitId = commitId;
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,18 @@
import com.atlassian.bitbucket.pull.PullRequest;
import com.atlassian.bitbucket.pull.PullRequestMergeRequest;
import com.atlassian.bitbucket.pull.PullRequestMergeability;
import com.atlassian.bitbucket.pull.PullRequestParticipant;
import com.atlassian.bitbucket.pull.PullRequestRef;
import com.atlassian.bitbucket.pull.PullRequestService;
import com.atlassian.bitbucket.repository.Repository;
import com.atlassian.bitbucket.user.ApplicationUser;
import com.atlassian.bitbucket.user.EscalatedSecurityContext;
import com.atlassian.bitbucket.user.SecurityService;
import com.atlassian.bitbucket.util.Operation;
import com.google.common.collect.Lists;
import com.monitorjbl.plugins.config.Config;
import com.monitorjbl.plugins.config.ConfigDao;
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;
import org.mockito.InjectMocks;
import org.mockito.Mock;
Expand All @@ -35,7 +36,6 @@
import static org.mockito.Mockito.when;

@SuppressWarnings("unchecked")
@Ignore
public class PullRequestListenerTest {
@Mock
private ConfigDao configDao;
Expand Down Expand Up @@ -66,16 +66,25 @@ public class PullRequestListenerTest {
PullRequestRef fromRef;
@Mock
PullRequestMergeability mergeability;
@Mock
PullRequestParticipant author;
@Mock
ApplicationUser authorUser;
@Mock
EscalatedSecurityContext securityContext;

@Before
public void setUp() throws Exception {
MockitoAnnotations.initMocks(this);
when(securityService.withPermission(any(Permission.class), anyString())).thenReturn(new MockSecurityContext());
when(openedEvent.getPullRequest()).thenReturn(pr);
when(authorUser.getSlug()).thenReturn("someguy");
when(author.getUser()).thenReturn(authorUser);
when(pr.getToRef()).thenReturn(toRef);
when(pr.getFromRef()).thenReturn(fromRef);
when(pr.getId()).thenReturn(10L);
when(pr.getVersion()).thenReturn(10384);
when(pr.getAuthor()).thenReturn(author);
when(toRef.getRepository()).thenReturn(toRepo);
when(toRef.getId()).thenReturn(RegexUtils.REFS_PREFIX + "master");
when(fromRef.getRepository()).thenReturn(fromRepo);
Expand Down Expand Up @@ -118,6 +127,11 @@ public void testAutomerge_canMerge() throws Exception {
.requiredReviewers(newArrayList("user1"))
.requiredReviews(1)
.build());
when(securityService.impersonating(any(), any())).thenReturn(securityContext);
when(securityContext.call(any())).then(inv -> {
((Operation) inv.getArguments()[0]).perform();
return null;
});
sut.automergePullRequest(pr);
verify(prService, times(1)).merge(any(PullRequestMergeRequest.class));
}
Expand Down

0 comments on commit 99d3f6d

Please sign in to comment.