diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 63bd69597a..6742cc4297 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -3110,6 +3110,16 @@ otherwise, this value applies to both `suggest.accounts` and New configurations should prefer the boolean value for this field and an enum value for `accounts.visibility`. +[[suggest.maxSuggestedReviewers]]suggest.maxSuggestedReviewers:: ++ +The maximum numbers of reviewers suggested. ++ +By default 10. + +[[suggest.fullTextSearch]]suggest.fullTextSearch:: ++ +If 'true' the reviewer completion suggestions will be based on a full text search. + [[suggest.from]]suggest.from:: + The number of characters that a user must have typed before suggestions @@ -3117,6 +3127,15 @@ are provided. If set to 0, suggestions are always provided. + By default 0. +[[suggest.fullTextSearchMaxMatches]]suggest.fullTextSearchMaxMatches:: ++ +The maximum number of matches evaluated for change access when using full text search. ++ +Making this number too high could have a negative impact on performance. ++ +By default 100. + + [[theme]] === Section theme diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java index 7266c9750a..8a277ca337 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java @@ -161,6 +161,36 @@ public class SuggestReviewersIT extends AbstractDaemonTest { assertEquals("User2", Iterables.getOnlyElement(reviewers).account.name); } + @Test + @GerritConfig(name = "suggest.maxSuggestedReviewers", value = "2") + public void suggestReviewersMaxNbrSuggestions() throws Exception { + String changeId = createChange().getChangeId(); + List reviewers = + suggestReviewers(changeId, "user", 5); + assertEquals(2, reviewers.size()); + } + + @Test + @GerritConfig(name = "suggest.fullTextSearch", value = "true") + public void suggestReviewersFullTextSearch() throws Exception { + String changeId = createChange().getChangeId(); + List reviewers = + suggestReviewers(changeId, "ser", 5); + assertEquals(4, reviewers.size()); + } + + @Test + @GerritConfigs( + {@GerritConfig(name = "suggest.fulltextsearch", value = "true"), + @GerritConfig(name = "suggest.fullTextSearchMaxMatches", value = "2") + }) + public void suggestReviewersFullTextSearchLimitMaxMatches() throws Exception { + String changeId = createChange().getChangeId(); + List reviewers = + suggestReviewers(changeId, "ser", 3); + assertEquals(2, reviewers.size()); + } + private List suggestReviewers(RestSession session, String changeId, String query, int n) throws IOException { return newGson().fromJson( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerSuggestionCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerSuggestionCache.java new file mode 100644 index 0000000000..31e77d39c8 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerSuggestionCache.java @@ -0,0 +1,69 @@ +// Copyright (C) 2014 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.change; + +import com.google.common.cache.CacheBuilder; +import com.google.common.cache.CacheLoader; +import com.google.common.cache.LoadingCache; +import com.google.common.collect.ImmutableList; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gwtorm.server.SchemaFactory; +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.Collections; +import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +/** + * The suggest oracle may be called many times in rapid succession during the course of one operation. + * It would be easy to have a simple Cache> with a short expiration time of 30s. + * Cache only has a single key we're just using Cache for the expiration behavior. + */ +@Singleton +public class ReviewerSuggestionCache { + private static final Logger log = LoggerFactory + .getLogger(ReviewerSuggestionCache.class); + private final LoadingCache> cache; + + @Inject + ReviewerSuggestionCache(final Provider dbProvider) { + this.cache = + CacheBuilder.newBuilder().maximumSize(1) + .expireAfterWrite(30, TimeUnit.SECONDS) + .build(new CacheLoader>() { + @Override + public List load(Boolean key) throws Exception { + return ImmutableList.copyOf(dbProvider.get().accounts().all()); + } + }); + } + + List get() { + try { + return cache.get(true); + } catch (ExecutionException e) { + log.warn("Cannot fetch reviewers from cache", e); + return Collections.emptyList(); + } + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/SuggestReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/SuggestReviewers.java index 832d7ecd95..f0f63eb2a7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/SuggestReviewers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/SuggestReviewers.java @@ -57,7 +57,8 @@ import java.util.Set; public class SuggestReviewers implements RestReadView { private static final String MAX_SUFFIX = "\u9fa5"; - private static final int MAX = 10; + private static final int DEFAULT_MAX_SUGGESTED = 10; + private static final int DEFAULT_MAX_MATCHES = 100; private final AccountInfo.Loader.Factory accountLoaderFactory; private final AccountControl.Factory accountControlFactory; @@ -72,11 +73,17 @@ public class SuggestReviewers implements RestReadView { private final int maxAllowed; private int limit; private String query; + private boolean useFullTextSearch; + private final int fullTextMaxMatches; + private final int maxSuggestedReviewers; + private final ReviewerSuggestionCache reviewerSuggestionCache; @Option(name = "--limit", aliases = {"-n"}, metaVar = "CNT", usage = "maximum number of reviewers to list") public void setLimit(int l) { - this.limit = l <= 0 ? MAX : Math.min(l, MAX); + this.limit = + l <= 0 ? maxSuggestedReviewers : Math.min(l, + maxSuggestedReviewers); } @Option(name = "--query", aliases = {"-q"}, metaVar = "QUERY", @@ -95,7 +102,8 @@ public class SuggestReviewers implements RestReadView { Provider currentUser, Provider dbProvider, @GerritServerConfig Config cfg, - GroupBackend groupBackend) { + GroupBackend groupBackend, + ReviewerSuggestionCache reviewerSuggestionCache) { this.accountLoaderFactory = accountLoaderFactory; this.accountControlFactory = accountControlFactory; this.accountCache = accountCache; @@ -104,12 +112,18 @@ public class SuggestReviewers implements RestReadView { this.identifiedUserFactory = identifiedUserFactory; this.currentUser = currentUser; this.groupBackend = groupBackend; - + this.reviewerSuggestionCache = reviewerSuggestionCache; + this.maxSuggestedReviewers = + cfg.getInt("suggest", "maxSuggestedReviewers", DEFAULT_MAX_SUGGESTED); + this.fullTextMaxMatches = + cfg.getInt("suggest", "fullTextSearchMaxMatches", + DEFAULT_MAX_MATCHES); String suggest = cfg.getString("suggest", null, "accounts"); if ("OFF".equalsIgnoreCase(suggest) || "false".equalsIgnoreCase(suggest)) { this.suggestAccounts = false; } else { + this.useFullTextSearch = cfg.getBoolean("suggest", "fullTextSearch", false); this.suggestAccounts = (av != AccountVisibility.NONE); } @@ -134,7 +148,12 @@ public class SuggestReviewers implements RestReadView { } VisibilityControl visibilityControl = getVisibility(rsrc); - List suggestedAccounts = suggestAccount(visibilityControl); + List suggestedAccounts; + if (useFullTextSearch) { + suggestedAccounts = suggestAccountFullTextSearch(visibilityControl); + } else { + suggestedAccounts = suggestAccount(visibilityControl); + } accountLoaderFactory.create(true).fill(suggestedAccounts); List reviewer = Lists.newArrayList(); @@ -220,6 +239,42 @@ public class SuggestReviewers implements RestReadView { return Lists.newArrayList(r.values()); } + private List suggestAccountFullTextSearch( + VisibilityControl visibilityControl) throws OrmException { + String str = query.toLowerCase(); + LinkedHashMap accountMap = Maps.newLinkedHashMap(); + List fullNameMatches = Lists.newArrayListWithCapacity(fullTextMaxMatches); + List emailMatches = Lists.newArrayListWithCapacity(fullTextMaxMatches); + for (Account a : reviewerSuggestionCache.get()) { + if (a.getFullName() != null + && a.getFullName().toLowerCase().contains(str)) { + fullNameMatches.add(a); + } else if (a.getPreferredEmail() != null + && emailMatches.size() < fullTextMaxMatches + && a.getPreferredEmail().toLowerCase().contains(str)) { + emailMatches.add(a); + } + if (fullNameMatches.size() >= fullTextMaxMatches) { + break; + } + } + for (Account a : fullNameMatches) { + addSuggestion(accountMap, a, new AccountInfo(a.getId()), visibilityControl); + if (accountMap.size() >= limit) { + break; + } + } + if (accountMap.size() < limit) { + for (Account a : emailMatches) { + addSuggestion(accountMap, a, new AccountInfo(a.getId()), visibilityControl); + if (accountMap.size() >= limit) { + break; + } + } + } + return Lists.newArrayList(accountMap.values()); + } + private void addSuggestion(Map map, Account account, AccountInfo info, VisibilityControl visibilityControl) throws OrmException {