From 22ca9734e71d7bb39b55ff3c30e27f228637536f Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 4 Jul 2016 14:01:42 +0200 Subject: [PATCH] Remove suggest.fullTextSearch option This is no longer needed since the reviewer suggestion is now based on the account index which supports full text search as well. Change-Id: I14ab15f143a4a77b3d1bdae125e3c34007355033 Signed-off-by: Edwin Kempin --- Documentation/config-gerrit.txt | 18 -- .../rest/change/SuggestReviewersIT.java | 22 --- .../google/gerrit/server/ReviewersUtil.java | 33 +--- .../change/ReviewerSuggestionCache.java | 187 ------------------ .../server/change/SuggestReviewers.java | 15 -- 5 files changed, 2 insertions(+), 273 deletions(-) delete mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerSuggestionCache.java diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 85bbbead11..1b522b4664 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -3636,12 +3636,6 @@ 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. -+ -By default `false`. - [[suggest.from]]suggest.from:: + The number of characters that a user must have typed before suggestions @@ -3649,18 +3643,6 @@ 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. -+ -By default 100. - -[[suggest.fullTextSearchRefresh]]suggest.fullTextSearchRefresh:: -+ -Refresh interval for the in-memory account search index. -+ -By default 1 hour. - [[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 29886194cf..419c2d92c5 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 @@ -168,17 +168,7 @@ public class SuggestReviewersIT extends AbstractDaemonTest { } @Test - @GerritConfig(name = "suggest.fullTextSearch", value = "true") public void suggestReviewersFullTextSearch() throws Exception { - testSuggestReviewersFullTextSearch(); - } - - @Test - public void suggestReviewersFullTextSearchWithAccountIndex() throws Exception { - testSuggestReviewersFullTextSearch(); - } - - private void testSuggestReviewersFullTextSearch() throws Exception { String changeId = createChange().getChangeId(); List reviewers; @@ -229,18 +219,6 @@ public class SuggestReviewersIT extends AbstractDaemonTest { assertThat(reviewers.get(0).account.email).isEqualTo(user4.email); } - @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, name("user"), 2); - assertThat(reviewers).hasSize(2); - } - @Test public void suggestReviewersWithoutLimitOptionSpecified() throws Exception { String changeId = createChange().getChangeId(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java index 7b1e963e1e..e7d7d8004c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java @@ -41,7 +41,6 @@ import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.account.GroupMembers; import com.google.gerrit.server.account.AccountDirectory.FillOptions; import com.google.gerrit.server.change.PostReviewers; -import com.google.gerrit.server.change.ReviewerSuggestionCache; import com.google.gerrit.server.change.SuggestReviewers; import com.google.gerrit.server.index.account.AccountIndex; import com.google.gerrit.server.index.account.AccountIndexCollection; @@ -61,7 +60,6 @@ import java.util.Collection; import java.util.Collections; import java.util.EnumSet; import java.util.HashMap; -import java.util.Iterator; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -88,7 +86,6 @@ public class ReviewersUtil { private final AccountIndexCollection indexes; private final AccountQueryBuilder queryBuilder; private final AccountQueryProcessor queryProcessor; - private final ReviewerSuggestionCache reviewerSuggestionCache; private final AccountControl accountControl; private final Provider dbProvider; private final GroupBackend groupBackend; @@ -101,7 +98,6 @@ public class ReviewersUtil { AccountIndexCollection indexes, AccountQueryBuilder queryBuilder, AccountQueryProcessor queryProcessor, - ReviewerSuggestionCache reviewerSuggestionCache, AccountControl.Factory accountControlFactory, Provider dbProvider, GroupBackend groupBackend, @@ -114,7 +110,6 @@ public class ReviewersUtil { this.indexes = indexes; this.queryBuilder = queryBuilder; this.queryProcessor = queryProcessor; - this.reviewerSuggestionCache = reviewerSuggestionCache; this.accountControl = accountControlFactory.get(); this.dbProvider = dbProvider; this.groupBackend = groupBackend; @@ -133,7 +128,6 @@ public class ReviewersUtil { String query = suggestReviewers.getQuery(); boolean suggestAccounts = suggestReviewers.getSuggestAccounts(); int suggestFrom = suggestReviewers.getSuggestFrom(); - boolean useFullTextSearch = suggestReviewers.getUseFullTextSearch(); int limit = suggestReviewers.getLimit(); if (Strings.isNullOrEmpty(query)) { @@ -144,13 +138,8 @@ public class ReviewersUtil { return Collections.emptyList(); } - Collection suggestedAccounts; - if (useFullTextSearch) { - suggestedAccounts = - suggestAccountFullTextSearch(suggestReviewers, visibilityControl); - } else { - suggestedAccounts = suggestAccounts(suggestReviewers, visibilityControl); - } + Collection suggestedAccounts = + suggestAccounts(suggestReviewers, visibilityControl); List reviewer = new ArrayList<>(); for (AccountInfo a : suggestedAccounts) { @@ -178,24 +167,6 @@ public class ReviewersUtil { return reviewer.subList(0, limit); } - private List suggestAccountFullTextSearch( - SuggestReviewers suggestReviewers, VisibilityControl visibilityControl) - throws IOException, OrmException { - List results = reviewerSuggestionCache.search( - suggestReviewers.getQuery(), suggestReviewers.getFullTextMaxMatches()); - - Iterator it = results.iterator(); - while (it.hasNext()) { - Account.Id accountId = new Account.Id(it.next()._accountId); - if (!(visibilityControl.isVisibleTo(accountId) - && accountControl.canSee(accountId))) { - it.remove(); - } - } - - return results; - } - private Collection suggestAccounts(SuggestReviewers suggestReviewers, VisibilityControl visibilityControl) throws OrmException { 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 deleted file mode 100644 index 20078cca1b..0000000000 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerSuggestionCache.java +++ /dev/null @@ -1,187 +0,0 @@ -// 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 static com.google.common.base.Preconditions.checkNotNull; - -import com.google.common.base.Splitter; -import com.google.common.cache.CacheBuilder; -import com.google.common.cache.CacheLoader; -import com.google.common.cache.LoadingCache; -import com.google.gerrit.extensions.common.AccountInfo; -import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.server.AccountExternalIdAccess; -import com.google.gerrit.reviewdb.server.ReviewDb; -import com.google.gerrit.server.account.AccountState; -import com.google.gerrit.server.config.ConfigUtil; -import com.google.gerrit.server.config.GerritServerConfig; -import com.google.gwtorm.server.OrmException; -import com.google.inject.Inject; -import com.google.inject.Provider; -import com.google.inject.Singleton; - -import org.apache.lucene.analysis.standard.StandardAnalyzer; -import org.apache.lucene.analysis.util.CharArraySet; -import org.apache.lucene.document.Document; -import org.apache.lucene.document.Field.Store; -import org.apache.lucene.document.IntField; -import org.apache.lucene.document.StringField; -import org.apache.lucene.document.TextField; -import org.apache.lucene.index.DirectoryReader; -import org.apache.lucene.index.IndexWriter; -import org.apache.lucene.index.IndexWriterConfig; -import org.apache.lucene.index.IndexWriterConfig.OpenMode; -import org.apache.lucene.index.IndexableField; -import org.apache.lucene.index.Term; -import org.apache.lucene.search.BooleanClause.Occur; -import org.apache.lucene.search.BooleanQuery; -import org.apache.lucene.search.IndexSearcher; -import org.apache.lucene.search.PrefixQuery; -import org.apache.lucene.search.ScoreDoc; -import org.apache.lucene.search.TopDocs; -import org.apache.lucene.store.RAMDirectory; -import org.eclipse.jgit.lib.Config; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import java.io.IOException; -import java.util.Collections; -import java.util.LinkedList; -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 {@code 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 static final String ID = "id"; - private static final String NAME = "name"; - private static final String EMAIL = "email"; - private static final String USERNAME = "username"; - private static final String[] ALL = {ID, NAME, EMAIL, USERNAME}; - - private final LoadingCache cache; - private final Provider db; - - @Inject - ReviewerSuggestionCache(Provider db, - @GerritServerConfig Config cfg) { - this.db = db; - long expiration = ConfigUtil.getTimeUnit(cfg, - "suggest", null, "fullTextSearchRefresh", - TimeUnit.HOURS.toMillis(1), - TimeUnit.MILLISECONDS); - this.cache = - CacheBuilder.newBuilder().maximumSize(1) - .refreshAfterWrite(expiration, TimeUnit.MILLISECONDS) - .build(new CacheLoader() { - @Override - public IndexSearcher load(Boolean key) throws Exception { - return index(); - } - }); - } - - public List search(String query, int n) throws IOException { - IndexSearcher searcher = get(); - if (searcher == null) { - return Collections.emptyList(); - } - - List segments = Splitter.on(' ').omitEmptyStrings().splitToList( - query.toLowerCase()); - BooleanQuery.Builder q = new BooleanQuery.Builder(); - for (String field : ALL) { - BooleanQuery.Builder and = new BooleanQuery.Builder(); - for (String s : segments) { - and.add(new PrefixQuery(new Term(field, s)), Occur.MUST); - } - q.add(and.build(), Occur.SHOULD); - } - - TopDocs results = searcher.search(q.build(), n); - ScoreDoc[] hits = results.scoreDocs; - - List result = new LinkedList<>(); - - for (ScoreDoc h : hits) { - Document doc = searcher.doc(h.doc); - - IndexableField idField = checkNotNull(doc.getField(ID)); - AccountInfo info = new AccountInfo(idField.numericValue().intValue()); - info.name = doc.get(NAME); - info.email = doc.get(EMAIL); - info.username = doc.get(USERNAME); - result.add(info); - } - - return result; - } - - private IndexSearcher get() { - try { - return cache.get(true); - } catch (ExecutionException e) { - log.warn("Cannot fetch reviewers from cache", e); - return null; - } - } - - private IndexSearcher index() throws IOException, OrmException { - RAMDirectory idx = new RAMDirectory(); - IndexWriterConfig config = new IndexWriterConfig( - new StandardAnalyzer(CharArraySet.EMPTY_SET)); - config.setOpenMode(OpenMode.CREATE); - - try (IndexWriter writer = new IndexWriter(idx, config)) { - for (Account a : db.get().accounts().all()) { - if (a.isActive()) { - addAccount(writer, a); - } - } - } - - return new IndexSearcher(DirectoryReader.open(idx)); - } - - private void addAccount(IndexWriter writer, Account a) - throws IOException, OrmException { - Document doc = new Document(); - doc.add(new IntField(ID, a.getId().get(), Store.YES)); - if (a.getFullName() != null) { - doc.add(new TextField(NAME, a.getFullName(), Store.YES)); - } - if (a.getPreferredEmail() != null) { - doc.add(new TextField(EMAIL, a.getPreferredEmail(), Store.YES)); - doc.add(new StringField(EMAIL, a.getPreferredEmail().toLowerCase(), - Store.YES)); - } - AccountExternalIdAccess extIdAccess = db.get().accountExternalIds(); - String username = AccountState.getUserName( - extIdAccess.byAccount(a.getId()).toList()); - if (username != null) { - doc.add(new StringField(USERNAME, username, Store.YES)); - } - writer.addDocument(doc); - } -} 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 3b610336e2..f63e0114eb 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 @@ -27,7 +27,6 @@ import org.kohsuke.args4j.Option; public class SuggestReviewers { private static final int DEFAULT_MAX_SUGGESTED = 10; - private static final int DEFAULT_MAX_MATCHES = 100; protected final Provider dbProvider; protected final IdentifiedUser.GenericFactory identifiedUserFactory; @@ -38,8 +37,6 @@ public class SuggestReviewers { private final int maxAllowed; protected int limit; protected String query; - private boolean useFullTextSearch; - private final int fullTextMaxMatches; protected final int maxSuggestedReviewers; @Option(name = "--limit", aliases = {"-n"}, metaVar = "CNT", @@ -68,14 +65,6 @@ public class SuggestReviewers { return suggestFrom; } - public boolean getUseFullTextSearch() { - return useFullTextSearch; - } - - public int getFullTextMaxMatches() { - return fullTextMaxMatches; - } - public int getLimit() { return limit; } @@ -96,15 +85,11 @@ public class SuggestReviewers { this.maxSuggestedReviewers = cfg.getInt("suggest", "maxSuggestedReviewers", DEFAULT_MAX_SUGGESTED); this.limit = this.maxSuggestedReviewers; - 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); }