diff --git a/CHANGELOG.md b/CHANGELOG.md index 15b8f97..5b18b2f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,20 @@ # Changelog All notable changes to this project will be documented in this file. +## 0.4 - 2015-01-13 +### Added +- ReviewAssistant is now configurable. +- Maximum reviewers, configuration parameter. +- Auto add reviewer, configuration parameter. +- If +2 user is required, configuration parameter. +- Modifier for total review time, configuration parameter. +- Load balancing, configuration parameter. +- Age and limit in the +2 query, configuration parameter. + +### Fixed +- Query only looks for changes with +2 code review. +- ReviewAssistant will now try to fill with reviewers until maxReviewers is reached. + ## 0.3 - 2015-01-05 ### Added - Automatic adding of reviewer with merge rights. diff --git a/pom.xml b/pom.xml index 88f9522..5c87f2f 100644 --- a/pom.xml +++ b/pom.xml @@ -6,7 +6,7 @@ com.github.nexception.reviewassistant reviewassistant jar - 0.3 + 0.4 reviewassistant diff --git a/src/main/java/com/github/nexception/reviewassistant/AdviceCache.java b/src/main/java/com/github/nexception/reviewassistant/AdviceCache.java new file mode 100644 index 0000000..4ad2573 --- /dev/null +++ b/src/main/java/com/github/nexception/reviewassistant/AdviceCache.java @@ -0,0 +1,25 @@ +package com.github.nexception.reviewassistant; + +import com.github.nexception.reviewassistant.models.Calculation; +import com.google.gerrit.server.change.RevisionResource; + +/** + * The AdviceCache interface is used to store and fetch calculations. + */ +public interface AdviceCache { + + /** + * Caches the provided calculation. + * + * @param calculation the calculation object to be cached + */ + public void storeCalculation(Calculation calculation); + + /** + * Returns the calculation object with the matching RevisionResource. + * + * @param resource the RevisionResource to fetch calculation from cache + * @return a Calculation object if one is found, null otherwise + */ + public Calculation fetchCalculation(RevisionResource resource); +} diff --git a/src/main/java/com/github/nexception/reviewassistant/AdviceCacheImpl.java b/src/main/java/com/github/nexception/reviewassistant/AdviceCacheImpl.java new file mode 100644 index 0000000..f66699e --- /dev/null +++ b/src/main/java/com/github/nexception/reviewassistant/AdviceCacheImpl.java @@ -0,0 +1,101 @@ +package com.github.nexception.reviewassistant; + +import com.github.nexception.reviewassistant.models.Calculation; +import com.google.gerrit.extensions.annotations.PluginData; +import com.google.gerrit.extensions.api.GerritApi; +import com.google.gerrit.extensions.api.changes.ChangeApi; +import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.server.change.RevisionResource; +import com.google.gerrit.server.config.PluginConfigFactory; +import com.google.gerrit.server.project.NoSuchProjectException; +import com.google.gson.Gson; +import com.google.inject.Inject; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.BufferedReader; +import java.io.BufferedWriter; +import java.io.File; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.nio.charset.Charset; +import java.nio.file.Files; + +/** + * This implementation of AdviceCache writes to the plugin's data directory ({gerrit url}/plugins/ReviewAssistant). + * The structure follows that of git's object directory, which means that the first two letters of the + * commit's SHA-1 is used as name for the sub directory, and the rest of the SHA-1 is used as file name. + */ +public class AdviceCacheImpl implements AdviceCache { + + private static final Logger log = LoggerFactory.getLogger(AdviceCacheImpl.class); + private File dir; + private GerritApi gApi; + private PluginConfigFactory cfg; + + @Inject AdviceCacheImpl(@PluginData File dir, GerritApi gApi, PluginConfigFactory cfg) { + this.dir = dir; + this.gApi = gApi; + this.cfg = cfg; + } + + @Override public void storeCalculation(Calculation calculation) { + File file = new File(dir, + calculation.commitId.substring(0, 2) + File.separator + calculation.commitId + .substring(2)); + log.debug("Writing calculation to {}", file); + file.getParentFile().mkdirs(); + try (BufferedWriter writer = Files + .newBufferedWriter(file.toPath(), Charset.forName("UTF-8"))) { + Gson gson = new Gson(); + String s = gson.toJson(calculation); + writer.write(s, 0, s.length()); + log.debug("Cached calculation in file {}", file); + } catch (FileNotFoundException e) { + log.error("Could not find file {}", file); + log.error(e.toString()); + } catch (IOException e) { + log.error("Could not write to file {}", file); + log.error(e.toString()); + } + } + + @Override public Calculation fetchCalculation(RevisionResource resource) { + File file = new File(dir, + resource.getPatchSet().getRevision().get().substring(0, 2) + File.separator + resource + .getPatchSet().getRevision().get().substring(2)); + Calculation calculation = null; + log.debug("Loading calculation from {}", file); + try (BufferedReader reader = Files + .newBufferedReader(file.toPath(), Charset.forName("UTF-8"))) { + Gson gson = new Gson(); + calculation = gson.fromJson(reader.readLine(), Calculation.class); + log.info("Returning Calculation {}", calculation.toString()); + } catch (IOException e) { + log.error("Could not read calculation file for {}", + resource.getPatchSet().getRevision().get()); + log.error(e.toString()); + } + + if (calculation == null || calculation.totalReviewTime == 0) { + log.debug("Corrupt or missing calculation. Will recalculate for {}", + resource.getPatchSet().getRevision().get()); + try { + ChangeApi cApi = gApi.changes().id(resource.getChange().getChangeId()); + ChangeInfo info = cApi.get(); + double reviewTimeModifier = + cfg.getProjectPluginConfigWithInheritance(resource.getChange().getProject(), + "reviewassistant").getInt("time", "reviewTimeModifier", 100); + calculation = ReviewAssistant.calculate(info, reviewTimeModifier / 100); + storeCalculation(calculation); + } catch (RestApiException e) { + log.error("Could not get ChangeInfo for change {}", + resource.getChange().getChangeId()); + } catch (NoSuchProjectException e) { + log.error(e.getMessage(), e); + } + } + return calculation; + } +} diff --git a/src/main/java/com/github/nexception/reviewassistant/ChangeEventListener.java b/src/main/java/com/github/nexception/reviewassistant/ChangeEventListener.java index 39d337d..a518d76 100644 --- a/src/main/java/com/github/nexception/reviewassistant/ChangeEventListener.java +++ b/src/main/java/com/github/nexception/reviewassistant/ChangeEventListener.java @@ -8,10 +8,12 @@ import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.PluginUser; +import com.google.gerrit.server.config.PluginConfigFactory; import com.google.gerrit.server.events.ChangeEvent; import com.google.gerrit.server.events.PatchSetCreatedEvent; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.WorkQueue; +import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.util.RequestContext; import com.google.gerrit.server.util.ThreadLocalRequestContext; import com.google.gwtorm.server.OrmException; @@ -19,15 +21,13 @@ import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.ProvisionException; -import org.eclipse.jgit.errors.IncorrectObjectTypeException; -import org.eclipse.jgit.errors.MissingObjectException; -import org.eclipse.jgit.errors.RepositoryNotFoundException; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; import org.slf4j.Logger; import org.slf4j.LoggerFactory; + import java.io.IOException; /** @@ -37,20 +37,20 @@ class ChangeEventListener implements ChangeListener { private static final Logger log = LoggerFactory.getLogger(ChangeEventListener.class); private final ReviewAssistant.Factory reviewAssistantFactory; + private final ThreadLocalRequestContext tl; + private final PluginUser pluginUser; + private final IdentifiedUser.GenericFactory identifiedUserFactory; + private final PluginConfigFactory cfg; private WorkQueue workQueue; private GitRepositoryManager repoManager; private SchemaFactory schemaFactory; - private final ThreadLocalRequestContext tl; private ReviewDb db; - private final PluginUser pluginUser; - private final IdentifiedUser.GenericFactory identifiedUserFactory; - @Inject - ChangeEventListener(final ReviewAssistant.Factory reviewAssistantFactory, - final WorkQueue workQueue, final GitRepositoryManager repoManager, - final SchemaFactory schemaFactory, - final ThreadLocalRequestContext tl, final PluginUser pluginUser, - final IdentifiedUser.GenericFactory identifiedUserFactory) { + @Inject ChangeEventListener(final ReviewAssistant.Factory reviewAssistantFactory, + final WorkQueue workQueue, final GitRepositoryManager repoManager, + final SchemaFactory schemaFactory, final ThreadLocalRequestContext tl, + final PluginUser pluginUser, final IdentifiedUser.GenericFactory identifiedUserFactory, + final PluginConfigFactory cfg) { this.workQueue = workQueue; this.reviewAssistantFactory = reviewAssistantFactory; this.repoManager = repoManager; @@ -58,109 +58,115 @@ class ChangeEventListener implements ChangeListener { this.tl = tl; this.pluginUser = pluginUser; this.identifiedUserFactory = identifiedUserFactory; + this.cfg = cfg; } - @Override - public void onChangeEvent(ChangeEvent changeEvent) { + @Override public void onChangeEvent(ChangeEvent changeEvent) { if (!(changeEvent instanceof PatchSetCreatedEvent)) { return; } PatchSetCreatedEvent event = (PatchSetCreatedEvent) changeEvent; - log.info("Received new commit: " + event.patchSet.revision); + log.debug("Received new commit: {}", event.patchSet.revision); Project.NameKey projectName = new Project.NameKey(event.change.project); - Repository repo; + + boolean autoAddReviewers = true; try { - repo = repoManager.openRepository(projectName); - } catch (RepositoryNotFoundException e) { - log.error(e.getMessage(), e); - return; - } catch (IOException e) { - log.error(e.getMessage(), e); - return; + log.debug("Checking if autoAddReviewers is enabled"); + autoAddReviewers = + cfg.getProjectPluginConfigWithInheritance(projectName, "reviewassistant") + .getBoolean("reviewers", "autoAddReviewers", true); + } catch (NoSuchProjectException e) { + log.error("Could not find project {}", projectName); } + log.debug( + autoAddReviewers ? "autoAddReviewers is enabled" : "autoAddReviewers is disabled"); + if (autoAddReviewers) { + Repository repo; + try { + repo = repoManager.openRepository(projectName); + } catch (IOException e) { + log.error("Could not open repository for {}", projectName); + return; + } - final ReviewDb reviewDb; - final RevWalk walk = new RevWalk(repo); + final ReviewDb reviewDb; + final RevWalk walk = new RevWalk(repo); - try { - reviewDb = schemaFactory.open(); try { - Change.Id changeId = new Change.Id(Integer.parseInt(event.change.number)); - PatchSet.Id psId = new PatchSet.Id(changeId, Integer.parseInt(event.patchSet.number)); - PatchSet ps = reviewDb.patchSets().get(psId); - if (ps == null) { - log.warn("Could not find patch set " + psId.get()); - return; - } - // psId.getParentKey = changeID - final Change change = reviewDb.changes().get(psId.getParentKey()); - if (change == null) { - log.warn("Could not find change " + psId.getParentKey()); - return; - } + reviewDb = schemaFactory.open(); + try { + Change.Id changeId = new Change.Id(Integer.parseInt(event.change.number)); + PatchSet.Id psId = + new PatchSet.Id(changeId, Integer.parseInt(event.patchSet.number)); + PatchSet ps = reviewDb.patchSets().get(psId); + if (ps == null) { + log.warn("Could not find patch set {}", psId.get()); + return; + } + // psId.getParentKey = changeID + final Change change = reviewDb.changes().get(psId.getParentKey()); + if (change == null) { + log.warn("Could not find change {}", psId.getParentKey()); + return; + } - RevCommit commit = walk.parseCommit(ObjectId.fromString(event.patchSet.revision)); + RevCommit commit = + walk.parseCommit(ObjectId.fromString(event.patchSet.revision)); - //TODO: Make the create method take only project name, change and patchset. - //TODO: (The rest should be moved into ReviewAssistant) - final Runnable task = reviewAssistantFactory.create(commit, change, ps, repo, projectName); - workQueue.getDefaultQueue().submit(new Runnable() { - @Override - public void run() { - RequestContext old = tl.setContext(new RequestContext() { + final Runnable task = + reviewAssistantFactory.create(commit, change, ps, repo, projectName); + workQueue.getDefaultQueue().submit(new Runnable() { + @Override public void run() { + RequestContext old = tl.setContext(new RequestContext() { - @Override - public CurrentUser getCurrentUser() { - if(!ReviewAssistant.realUser) { - return pluginUser; - } else { - return identifiedUserFactory.create(change.getOwner()); + @Override public CurrentUser getCurrentUser() { + if (!ReviewAssistant.realUser) { + return pluginUser; + } else { + return identifiedUserFactory.create(change.getOwner()); + } } - } - @Override - public Provider getReviewDbProvider() { - return new Provider() { - @Override - public ReviewDb get() { - if (db == null) { - try { - db = schemaFactory.open(); - } catch (OrmException e) { - throw new ProvisionException("Cannot open ReviewDb", e); + @Override public Provider getReviewDbProvider() { + return new Provider() { + @Override public ReviewDb get() { + if (db == null) { + try { + db = schemaFactory.open(); + } catch (OrmException e) { + throw new ProvisionException( + "Cannot open ReviewDb", e); + } } + return db; } - return db; - } - }; - } - }); - try { - task.run(); - } finally { - tl.setContext(old); - if (db != null) { - db.close(); - db = null; + }; + } + }); + try { + task.run(); + } finally { + tl.setContext(old); + if (db != null) { + db.close(); + db = null; + } } } - } - }); - } catch (IncorrectObjectTypeException e) { - log.error(e.getMessage(), e); - } catch (MissingObjectException e) { - log.error(e.getMessage(), e); - } catch (IOException e) { - log.error(e.getMessage(), e); + }); + } catch (IOException e) { + log.error("Could not get commit for revision {}: {}", event.patchSet.revision, + e); + } finally { + reviewDb.close(); + } + } catch (OrmException e) { + log.error("Could not open review database: {}", e); } finally { - reviewDb.close(); + walk.release(); + repo.close(); } - } catch (OrmException e) { - log.error(e.getMessage(), e); - } finally { - walk.release(); - repo.close(); } } } diff --git a/src/main/java/com/github/nexception/reviewassistant/HttpModule.java b/src/main/java/com/github/nexception/reviewassistant/HttpModule.java index c269d04..f5769d4 100644 --- a/src/main/java/com/github/nexception/reviewassistant/HttpModule.java +++ b/src/main/java/com/github/nexception/reviewassistant/HttpModule.java @@ -5,9 +5,9 @@ import com.google.gerrit.extensions.webui.WebUiPlugin; import com.google.gerrit.httpd.plugins.HttpPluginModule; -public class HttpModule extends HttpPluginModule{ - @Override - protected void configureServlets() { - DynamicSet.bind(binder(), WebUiPlugin.class).toInstance(new JavaScriptPlugin("reviewassistant.js")); +public class HttpModule extends HttpPluginModule { + @Override protected void configureServlets() { + DynamicSet.bind(binder(), WebUiPlugin.class) + .toInstance(new JavaScriptPlugin("reviewassistant.js")); } } diff --git a/src/main/java/com/github/nexception/reviewassistant/Module.java b/src/main/java/com/github/nexception/reviewassistant/Module.java index 2fab761..442ef9d 100644 --- a/src/main/java/com/github/nexception/reviewassistant/Module.java +++ b/src/main/java/com/github/nexception/reviewassistant/Module.java @@ -1,24 +1,22 @@ package com.github.nexception.reviewassistant; -import static com.google.gerrit.server.change.RevisionResource.REVISION_KIND; - import com.github.nexception.reviewassistant.rest.GetAdvice; import com.google.gerrit.common.ChangeListener; import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.extensions.restapi.RestApiModule; import com.google.gerrit.server.config.FactoryModule; +import static com.google.gerrit.server.change.RevisionResource.REVISION_KIND; + public class Module extends FactoryModule { - @Override - protected void configure() { + @Override protected void configure() { DynamicSet.bind(binder(), ChangeListener.class).to(ChangeEventListener.class); - bind(Storage.class).to(SimpleStorage.class); + bind(AdviceCache.class).to(AdviceCacheImpl.class); factory(ReviewAssistant.Factory.class); install(new RestApiModule() { - @Override - protected void configure() { + @Override protected void configure() { get(REVISION_KIND, "advice").to(GetAdvice.class); } }); diff --git a/src/main/java/com/github/nexception/reviewassistant/ReviewAssistant.java b/src/main/java/com/github/nexception/reviewassistant/ReviewAssistant.java index 2a84ebb..9ff1264 100644 --- a/src/main/java/com/github/nexception/reviewassistant/ReviewAssistant.java +++ b/src/main/java/com/github/nexception/reviewassistant/ReviewAssistant.java @@ -36,7 +36,6 @@ import java.util.Collections; import java.util.Comparator; import java.util.HashMap; -import java.util.HashSet; import java.util.Iterator; import java.util.LinkedList; import java.util.List; @@ -44,12 +43,17 @@ import java.util.Map.Entry; import java.util.Set; +enum AddReason {PLUS_TWO, EXPERIENCE} + + /** * A class for calculating recommended review time and * recommended reviewers. */ public class ReviewAssistant implements Runnable { + private static final Logger log = LoggerFactory.getLogger(ReviewAssistant.class); + public static boolean realUser; private final AccountByEmailCache emailCache; private final AccountCache accountCache; private final Change change; @@ -57,12 +61,13 @@ public class ReviewAssistant implements Runnable { private final PatchSet ps; private final Repository repo; private final RevCommit commit; - private final PluginConfigFactory cfg; - private static final Logger log = LoggerFactory.getLogger(ReviewAssistant.class); private final Project.NameKey projectName; private final GerritApi gApi; - public static boolean realUser = false; - + private final int maxReviewers; + private final boolean loadBalancing; + private final int plusTwoAge; + private final int plusTwoLimit; + private final boolean plusTwoRequired; public interface Factory { ReviewAssistant create(RevCommit commit, Change change, PatchSet ps, Repository repo, @@ -71,8 +76,7 @@ ReviewAssistant create(RevCommit commit, Change change, PatchSet ps, Repository @Inject public ReviewAssistant(final PatchListCache patchListCache, final AccountCache accountCache, - final GerritApi gApi, final AccountByEmailCache emailCache, - final PluginConfigFactory cfg, + final GerritApi gApi, final AccountByEmailCache emailCache, final PluginConfigFactory cfg, @Assisted final RevCommit commit, @Assisted final Change change, @Assisted final PatchSet ps, @Assisted final Repository repo, @Assisted final Project.NameKey projectName) { @@ -83,9 +87,42 @@ public ReviewAssistant(final PatchListCache patchListCache, final AccountCache a this.repo = repo; this.accountCache = accountCache; this.emailCache = emailCache; - this.cfg = cfg; this.projectName = projectName; this.gApi = gApi; + int tmpMaxReviewers; + boolean tmpLoadBalancing; + int tmpPlusTwoAge; + int tmpPlusTwoLimit; + boolean tmpPlusTwoRequired; + try { + tmpMaxReviewers = + cfg.getProjectPluginConfigWithInheritance(projectName, "reviewassistant") + .getInt("reviewers", "maxReviewers", 3); + tmpLoadBalancing = + cfg.getProjectPluginConfigWithInheritance(projectName, "reviewassistant") + .getBoolean("reviewers", "enableLoadBalancing", false); + tmpPlusTwoAge = + cfg.getProjectPluginConfigWithInheritance(projectName, "reviewassistant") + .getInt("reviewers", "plusTwoAge", 8); + tmpPlusTwoLimit = + cfg.getProjectPluginConfigWithInheritance(projectName, "reviewassistant") + .getInt("reviewers", "plusTwoLimit", 10); + tmpPlusTwoRequired = + cfg.getProjectPluginConfigWithInheritance(projectName, "reviewassistant") + .getBoolean("reviewers", "plusTwoRequired", true); + } catch (NoSuchProjectException e) { + log.error(e.getMessage(), e); + tmpMaxReviewers = 3; + tmpLoadBalancing = false; + tmpPlusTwoAge = 8; + tmpPlusTwoLimit = 10; + tmpPlusTwoRequired = true; + } + this.maxReviewers = tmpMaxReviewers; + this.loadBalancing = tmpLoadBalancing; + this.plusTwoAge = tmpPlusTwoAge; + this.plusTwoLimit = tmpPlusTwoLimit; + this.plusTwoRequired = tmpPlusTwoRequired; } /** @@ -95,14 +132,14 @@ public ReviewAssistant(final PatchListCache patchListCache, final AccountCache a * @param info the data for a patch set * @return the Calculation object for a review */ - public static Calculation calculate(ChangeInfo info) { - log.info("Received event: " + info.currentRevision); //Commit-ID + public static Calculation calculate(ChangeInfo info, double reviewTimeModifier) { + log.debug("Received event: {}", info.currentRevision); Calculation calculation = new Calculation(); calculation.commitId = info.currentRevision; - calculation.totalReviewTime = calculateReviewTime(info); - calculation.hours = calculateReviewTime(info) / 60; - calculation.minutes = calculateReviewTime(info) % 60; - calculation.sessions = calculateReviewSessions(calculateReviewTime(info)); + calculation.totalReviewTime = calculateReviewTime(info, reviewTimeModifier); + calculation.hours = calculation.totalReviewTime / 60; + calculation.minutes = calculation.totalReviewTime % 60; + calculation.sessions = calculateReviewSessions(calculation.totalReviewTime); return calculation; } @@ -116,9 +153,10 @@ public static Calculation calculate(ChangeInfo info) { * @param info the data for a patch set * @return the total amount of time recommended for a review */ - private static int calculateReviewTime(ChangeInfo info) { + private static int calculateReviewTime(ChangeInfo info, double reviewTimeModifier) { + log.debug("reviewTimeModifier: {}", reviewTimeModifier); int lines = info.insertions + Math.abs(info.deletions); - int minutes = (int) Math.ceil(lines / 5); + int minutes = (int) Math.ceil(lines * reviewTimeModifier / 5); minutes = (int) Math.ceil(minutes / 5.0); minutes = minutes * 5; if (minutes < 5) { @@ -150,28 +188,33 @@ private static int calculateReviewSessions(int minutes) { private List> getApprovalAccounts() { Map reviewersApproved = new HashMap<>(); try { - //TODO: Use config. parameter for age and limit - List infoList = - gApi.changes().query("status:merged -owner:" + change.getOwner().get() + - " -age:8weeks limit:10 label:Code-Review=2 project:" + + List infoList = gApi.changes().query( + "status:merged -age:" + plusTwoAge + "weeks limit:" + plusTwoLimit + + " -label:Code-Review=2," + change.getOwner().get() + + " label:Code-Review=2 project:" + projectName.toString()) - .withOptions(ListChangesOption.LABELS, ListChangesOption.DETAILED_ACCOUNTS) - .get(); + .withOptions(ListChangesOption.LABELS, ListChangesOption.DETAILED_ACCOUNTS).get(); for (ChangeInfo info : infoList) { - Account account = - accountCache.getByUsername(info.labels.get("Code-Review").approved.username) - .getAccount(); - if (reviewersApproved.containsKey(account)) { - reviewersApproved.put(account, reviewersApproved.get(account) + 1); - } else { - reviewersApproved.put(account, 1); + //TODO Check if this is good enough + try { + Account account = + accountCache.getByUsername(info.labels.get("Code-Review").approved.username) + .getAccount(); + if (reviewersApproved.containsKey(account)) { + reviewersApproved.put(account, reviewersApproved.get(account) + 1); + } else { + reviewersApproved.put(account, 1); + } + } catch (NullPointerException e) { + log.error("No username for this account found in cache {}", e); } + } } catch (RestApiException e) { log.error(e.getMessage(), e); } - log.info("getApprovalAccounts found " + reviewersApproved.size() + " reviewers"); + log.debug("getApprovalAccounts found {} reviewers", reviewersApproved.size()); try { List> approvalAccounts = @@ -185,8 +228,7 @@ public int compare(Entry o1, Entry o2) { } catch (Exception e) { log.error(e.getMessage(), e); } - //TODO: Return empty list - return null; + return new ArrayList<>(); } /** @@ -230,13 +272,12 @@ private List> getReviewers(List edits, BlameResult emailCache.get(commit.getAuthorIdent().getEmailAddress()); for (Account.Id id : idSet) { Account account = accountCache.get(id).getAccount(); - // Check if account is active and not owner of change if (account.isActive() && !change.getOwner().equals(account.getId())) { Integer count = blameData.get(account); if (count == null) { count = 1; } else { - count = count.intValue() + 1; + count = count + 1; } blameData.put(account, count); } @@ -246,42 +287,43 @@ private List> getReviewers(List edits, BlameResult } catch (Exception e) { log.error(e.getMessage(), e); } - try { - //TODO: Move to top, global variable - int maxReviewers = - cfg.getProjectPluginConfigWithInheritance(projectName, "reviewassistant") - .getInt("reviewers", "maxReviewers", 3); - log.info("maxReviewers set to " + maxReviewers); - List> topReviewers = - Ordering.from(new Comparator>() { - @Override - public int compare(Entry itemOne, - Entry itemTwo) { - return itemOne.getValue() - itemTwo.getValue(); - } - }).greatestOf(blameData.entrySet(), maxReviewers * 2); - //TODO Check if maxReviewers * 2 is sufficient - log.info("getReviewers found " + topReviewers.size() + " reviewers"); - return topReviewers; - } catch (NoSuchProjectException e) { - log.error(e.getMessage(), e); - return null; - } + + List> topReviewers = + Ordering.from(new Comparator>() { + @Override public int compare(Entry itemOne, + Entry itemTwo) { + return itemOne.getValue() - itemTwo.getValue(); + } + }).greatestOf(blameData.entrySet(), maxReviewers * 2); + //TODO Check if maxReviewers * 2 is sufficient + log.debug("getReviewers found {} reviewers", topReviewers.size()); + return topReviewers; } /** * Adds reviewers to the change. * * @param change the change for which reviewers should be added - * @param set set of reviewers + * @param map map of reviewers and their reasons for being added */ - private void addReviewers(Change change, Set set) { + private void addReviewers(Change change, Map map) { try { ChangeApi cApi = gApi.changes().id(change.getId().get()); - for (Account account : set) { - cApi.addReviewer(account.getId().toString()); - log.info("{} was added to change {}", account.getPreferredEmail(), - change.getChangeId()); + for (Entry entry : map.entrySet()) { + cApi.addReviewer(entry.getKey().getId().toString()); + String reason; + switch (entry.getValue()) { + case PLUS_TWO: + reason = "+2"; + break; + case EXPERIENCE: + reason = "experience"; + break; + default: + reason = "unknown reason"; + } + log.info("{} was added to change {} ({})", entry.getKey().getPreferredEmail(), + change.getChangeId(), reason); } } catch (RestApiException e) { log.error(e.getMessage(), e); @@ -312,15 +354,16 @@ public int compare(Entry o1, Entry o2) { }); } catch (RestApiException e) { log.error(e.getMessage(), e); - } catch (Exception e) { - log.error(e.getMessage(), e); } } return modifiableList; } - @Override - public void run() { + @Override public void run() { + log.info( + "CONFIG: maxReviewers: " + maxReviewers + ", enableLoadBalancing: " + loadBalancing + + ", plusTwoAge: " + plusTwoAge + ", plusTwoLimit: " + plusTwoLimit + + ", plusTwoRequired: " + plusTwoRequired); PatchList patchList; try { patchList = patchListCache.get(change, ps); @@ -333,54 +376,89 @@ public void run() { log.error("No merge/initial"); return; } - //TODO Use config. parameter - boolean LOAD_BALANCING = true; - - List> mergeCandidates = getApprovalAccounts(); + List> mergeCandidates; + if (plusTwoRequired) { + mergeCandidates = getApprovalAccounts(); + } else { + //TODO Fugly + mergeCandidates = new ArrayList<>(); + } List> blameCandidates = new LinkedList<>(); for (PatchListEntry entry : patchList.getPatches()) { /* * Only git blame at the moment. If other methods are used in the future, * other change types might be required. */ - if (entry.getChangeType() == ChangeType.MODIFIED || - entry.getChangeType() == ChangeType.DELETED) { + if (entry.getChangeType() == ChangeType.MODIFIED + || entry.getChangeType() == ChangeType.DELETED) { BlameResult blameResult = calculateBlame(commit.getParent(0), entry); if (blameResult != null) { List edits = entry.getEdits(); blameCandidates.addAll(getReviewers(edits, blameResult)); - for (int i = 0; i < blameCandidates.size(); i++) { - //TODO Log added candidates instead - log.info("Candidate " + (i + 1) + ": " + blameCandidates.get(i).getKey() - .getPreferredEmail() + - ", blame score: " + blameCandidates.get(i).getValue()); - } } else { log.error("calculateBlame returned null for commit {}", commit); } } } - if (LOAD_BALANCING) { - mergeCandidates = sortByOpenChanges(mergeCandidates); + + if (loadBalancing) { blameCandidates = sortByOpenChanges(blameCandidates); + if (!mergeCandidates.isEmpty()) { + mergeCandidates = sortByOpenChanges(mergeCandidates); + } + } + + for (Entry e : mergeCandidates) { + log.debug("Merge candidate: {}, score: {}", e.getKey().getPreferredEmail(), + e.getValue()); } - log.info("Best candidate with merge rights: " + mergeCandidates.get(0).getKey() - .getPreferredEmail()); + for (Entry e : blameCandidates) { + log.debug("Blame candidate: {}, score: {}", e.getKey().getPreferredEmail(), + e.getValue()); + } - Set finalSet = new HashSet<>(); - Iterator> itr = blameCandidates.iterator(); - finalSet.add(mergeCandidates.get(0).getKey()); - //TODO Change "3" to config. parameter - while (finalSet.size() < 3 && itr.hasNext()) { - finalSet.add(itr.next().getKey()); + Map finalMap = new HashMap<>(); + if (blameCandidates.size() < maxReviewers) { + Iterator> mergeItr = mergeCandidates.iterator(); + for (Entry e : blameCandidates) { + finalMap.put(e.getKey(), AddReason.EXPERIENCE); + log.debug("Added {} ({})", e.getKey().getPreferredEmail(), AddReason.EXPERIENCE); + } + boolean plusTwoAdded = false; + while (finalMap.size() < maxReviewers && mergeItr.hasNext()) { + Account account = mergeItr.next().getKey(); + if (!finalMap.containsKey(account)) { + finalMap.put(account, AddReason.PLUS_TWO); + log.debug("Added {} ({})", account.getPreferredEmail(), AddReason.PLUS_TWO); + plusTwoAdded = true; + } + } + if (!plusTwoAdded && plusTwoRequired) { + finalMap.put(mergeCandidates.get(0).getKey(), AddReason.PLUS_TWO); + log.debug("Changed reason for {} to {}", + mergeCandidates.get(0).getKey().getPreferredEmail(), AddReason.PLUS_TWO); + } + } else { + Iterator> blameItr = blameCandidates.iterator(); + if (!mergeCandidates.isEmpty()) { + finalMap.put(mergeCandidates.get(0).getKey(), AddReason.PLUS_TWO); + log.debug("Added {} ({})", mergeCandidates.get(0).getKey().getPreferredEmail(), + AddReason.PLUS_TWO); + } + while (finalMap.size() < maxReviewers && blameItr.hasNext()) { + Account account = blameItr.next().getKey(); + if (!finalMap.containsKey(account)) { + finalMap.put(account, AddReason.EXPERIENCE); + log.debug("Added {} ({})", account.getPreferredEmail(), AddReason.EXPERIENCE); + } + } } //TODO Move into addReviewers? realUser = true; - addReviewers(change, finalSet); + addReviewers(change, finalMap); realUser = false; - } } diff --git a/src/main/java/com/github/nexception/reviewassistant/SimpleStorage.java b/src/main/java/com/github/nexception/reviewassistant/SimpleStorage.java deleted file mode 100644 index af67c5e..0000000 --- a/src/main/java/com/github/nexception/reviewassistant/SimpleStorage.java +++ /dev/null @@ -1,71 +0,0 @@ -package com.github.nexception.reviewassistant; - -import com.github.nexception.reviewassistant.models.Calculation; -import com.google.gerrit.extensions.annotations.PluginData; -import com.google.gson.Gson; -import com.google.inject.Inject; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import java.io.BufferedReader; -import java.io.BufferedWriter; -import java.io.File; -import java.io.FileNotFoundException; -import java.io.IOException; -import java.nio.charset.Charset; -import java.nio.file.Files; - -/** - * This implementation of Storage writes to the plugin's data directory ({gerrit url}/plugins/ReviewAssistant). - * The structure follows that of git's object directory, which means that the first two letters of the - * commit's SHA-1 is used as name for the sub directory, and the rest of the SHA-1 is used as file name. - */ -public class SimpleStorage implements Storage { - - private static final Logger log = LoggerFactory.getLogger(SimpleStorage.class); - private File dir; - - @Inject - SimpleStorage(@PluginData File dir) { - this.dir = dir; - } - - @Override - public void storeCalculation(Calculation calculation) { - File file = new File(dir, calculation.commitId.substring(0, 2) + File.separator + calculation.commitId.substring(2)); - log.debug("Writing calculation to " + file); - file.getParentFile().mkdirs(); - try (BufferedWriter writer = Files.newBufferedWriter(file.toPath(), Charset.forName("UTF-8"))) { - Gson gson = new Gson(); - String s = gson.toJson(calculation); - writer.write(s, 0, s.length()); - log.debug("Stored calculation in file " + file); - } catch (FileNotFoundException e) { - log.error("Could not find file " + file); - log.error(e.toString()); - } catch (IOException e) { - log.error("Could not write to file " + file); - log.error(e.toString()); - } - } - - @Override - public Calculation fetchCalculation(String commitId) { - File file = new File(dir, commitId.substring(0, 2) + File.separator + commitId.substring(2)); - log.debug("Loading calculation from " + file); - try (BufferedReader reader = Files.newBufferedReader(file.toPath(), Charset.forName("UTF-8"))) { - Gson gson = new Gson(); - Calculation calculation = gson.fromJson(reader.readLine(), Calculation.class); - log.info("Returning Calculation " + calculation.toString()); - return calculation; - } catch (IOException e) { - log.error("Could not read calculation file for " + commitId); - log.error(e.toString()); - } - - /** - * If no calculation is found, maybe one should be triggered? - */ - return null; - } -} diff --git a/src/main/java/com/github/nexception/reviewassistant/Storage.java b/src/main/java/com/github/nexception/reviewassistant/Storage.java deleted file mode 100644 index 6700091..0000000 --- a/src/main/java/com/github/nexception/reviewassistant/Storage.java +++ /dev/null @@ -1,22 +0,0 @@ -package com.github.nexception.reviewassistant; - -import com.github.nexception.reviewassistant.models.Calculation; - -/** - * The Storage interface is used to store and fetch calculations. - */ -public interface Storage { - - /** - * Stores the provided calculation. - * @param calculation the calculation object to be stored - */ - public void storeCalculation(Calculation calculation); - - /** - * Returns the calculation object with the matching commit id. - * @param commitId the commit id to look fetch - * @return a Calculation object if one is found, null otherwise - */ - public Calculation fetchCalculation(String commitId); -} diff --git a/src/main/java/com/github/nexception/reviewassistant/rest/GetAdvice.java b/src/main/java/com/github/nexception/reviewassistant/rest/GetAdvice.java index 98ae339..321c84b 100644 --- a/src/main/java/com/github/nexception/reviewassistant/rest/GetAdvice.java +++ b/src/main/java/com/github/nexception/reviewassistant/rest/GetAdvice.java @@ -1,73 +1,55 @@ package com.github.nexception.reviewassistant.rest; -import com.github.nexception.reviewassistant.ReviewAssistant; -import com.github.nexception.reviewassistant.Storage; +import com.github.nexception.reviewassistant.AdviceCache; import com.github.nexception.reviewassistant.models.Calculation; -import com.google.gerrit.extensions.api.GerritApi; -import com.google.gerrit.extensions.api.changes.ChangeApi; -import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.ResourceConflictException; -import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestReadView; import com.google.gerrit.server.change.RevisionResource; import com.google.inject.Inject; import com.google.inject.Singleton; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; /** * This rest view fetches a calculation and returns it. It is used by the front-end to * present the review suggestions to the users. */ -@Singleton -public class GetAdvice implements RestReadView { +@Singleton public class GetAdvice implements RestReadView { - private Storage storage; - private GerritApi gApi; - private ChangeApi cApi; - private static final Logger log = LoggerFactory.getLogger(GetAdvice.class); + private AdviceCache adviceCache; - @Inject - public GetAdvice(GerritApi gApi, Storage storage) { - this.storage = storage; - this.gApi = gApi; + @Inject public GetAdvice(AdviceCache adviceCache) { + this.adviceCache = adviceCache; } - @Override - public Object apply(RevisionResource resource) throws AuthException, BadRequestException, ResourceConflictException { - Calculation calculation = storage.fetchCalculation(resource.getPatchSet().getRevision().get()); - String advice = "
ReviewAssistant"; - advice += "
Reviewers should spend "; - if (calculation == null || calculation.totalReviewTime == 0) { - try { - cApi = gApi.changes().id(resource.getChange().getChangeId()); - ChangeInfo info = cApi.get(); - calculation = ReviewAssistant.calculate(info); - storage.storeCalculation(calculation); - } catch (RestApiException e) { - log.error(e.getMessage(), e); + @Override public Object apply(RevisionResource resource) + throws AuthException, BadRequestException, ResourceConflictException { + Calculation calculation = adviceCache.fetchCalculation(resource); + String advice = + "
ReviewAssistant"; + if (calculation != null) { + advice += "
Reviewers should spend "; + if (calculation.hours == 1) { + advice += calculation.hours + " hour"; + } else if (calculation.hours > 1) { + advice += calculation.hours + " hours"; } - } - if (calculation.hours == 1) { - advice += calculation.hours + " hour"; - } else if (calculation.hours > 1) { - advice += calculation.hours + " hours"; - } - if (calculation.hours > 0 && calculation.minutes > 0) { - advice += " and "; - } - if (calculation.minutes > 0) { - advice += calculation.minutes + " minutes"; - } - advice += " reviewing this change.
"; - if (calculation.hours >= 1) { - advice += "
This should be split up in " + calculation.sessions + + if (calculation.hours > 0 && calculation.minutes > 0) { + advice += " and "; + } + if (calculation.minutes > 0) { + advice += calculation.minutes + " minutes"; + } + advice += " reviewing this change.
"; + if (calculation.hours >= 1) { + advice += "
This should be split up in " + calculation.sessions + " to " + (calculation.sessions + 1) + " sessions.
"; + } + } else { + advice += "
Could not get advice for this change.
"; } advice += "
"; return advice; } -} \ No newline at end of file +} diff --git a/src/main/resources/Documentation/about.md b/src/main/resources/Documentation/about.md index f65dc7c..307364a 100644 --- a/src/main/resources/Documentation/about.md +++ b/src/main/resources/Documentation/about.md @@ -4,4 +4,5 @@ The review assistant plugin gives advice on how to perform code reviews in an ef - Time estimations: Reviewers get advice on how much time that should be put into reviewing the current patch. - Review sessions: Reviewers get advice on how to partition the review time. -- Auto add reviewers. Reviewers are auto added based on git blame. \ No newline at end of file +- Auto add reviewers based on git blame. +- Auto add one reviewer that has +2'd a change in the project previously. diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md index cbd4481..c3774c5 100644 --- a/src/main/resources/Documentation/config.md +++ b/src/main/resources/Documentation/config.md @@ -11,9 +11,59 @@ Other projects can then override the configuration in their own ``` [reviewers] maxReviewers = 3 + enableLoadBalancing = false + plusTwoAge = 8 + plusTwoLimit = 10 + plusTwoRequired = true + autoAddReviewers = true + + [time] + reviewTimeModifier = 100 + ``` reviewers.maxReviewers -: The maximum number of reviewers that should automatically be added to a change. +: The maximum number of reviewers that should automatically be added to a change. By default 3. + +reviewers.enableLoadBalancing +: If loadbalancing is enabled, reviewers' other reviews are taken into account, and those with + fewer other reviews are favored. + + By default false. + + The query used for finding suitable +2 accounts is + + status:merged -age:weeks limit: -label:Code-Review=2, + label:Code-Review=2 project: + + This query has a potential to slow down performance, but care has been taken to choose sensible defaults. + The query returns a list of changes, whenever one of the conditions age or limit is fulfilled. The + account that +2'd the change is then considered as a candidate to review the new change. + +reviewers.plusTwoAge +: How far back (in weeks) in history to look. + + By default 8. + +reviewers.plusTwoLimit +: How many changes to take. + + By default 10. + +reviewers.plusTwoRequired +: Whether a user with merge rights is required to be added as a reviewer. + + By default true. + +reviewers.autoAddReviewer +: If reviewers should be added to a change. If disabled, only review advice is given. + + By default true. + +time.reviewTimeModifier +: Modifier for total review time in percentage. Also affects sessions indirectly. + Value of 50 will cut the review time in half. + + By default 100.