diff options
author | Adithya Chakilam <achakila@codeaurora.org> | 2021-11-18 22:39:36 +0000 |
---|---|---|
committer | Gerrit Code Review <noreply-gerritcodereview@google.com> | 2021-11-18 22:39:36 +0000 |
commit | cf1429912692d755dcb20c119b36f8ffd7004945 (patch) | |
tree | 053592a4d42649052bb680bed8d3b77f935fbd8c | |
parent | 387af5311f5935eb1d5acb24379550ee04ed70ff (diff) | |
parent | 43cce8b744fd5540be987647d5510cfe2d8ba8b3 (diff) |
Merge "Avoid lucene index deletes during offline reindexing" into stable-3.2
19 files changed, 183 insertions, 13 deletions
diff --git a/java/com/google/gerrit/acceptance/DisabledAccountIndex.java b/java/com/google/gerrit/acceptance/DisabledAccountIndex.java index 271d15c176..7bd0c73013 100644 --- a/java/com/google/gerrit/acceptance/DisabledAccountIndex.java +++ b/java/com/google/gerrit/acceptance/DisabledAccountIndex.java @@ -45,6 +45,11 @@ public class DisabledAccountIndex implements AccountIndex { } @Override + public void insert(AccountState obj) { + throw new UnsupportedOperationException("AccountIndex is disabled"); + } + + @Override public void replace(AccountState obj) { throw new UnsupportedOperationException("AccountIndex is disabled"); } diff --git a/java/com/google/gerrit/acceptance/DisabledChangeIndex.java b/java/com/google/gerrit/acceptance/DisabledChangeIndex.java index 34f72f5ccf..7671ad4068 100644 --- a/java/com/google/gerrit/acceptance/DisabledChangeIndex.java +++ b/java/com/google/gerrit/acceptance/DisabledChangeIndex.java @@ -52,6 +52,11 @@ public class DisabledChangeIndex implements ChangeIndex { } @Override + public void insert(ChangeData obj) { + throw new UnsupportedOperationException("ChangeIndex is disabled"); + } + + @Override public void replace(ChangeData obj) { throw new UnsupportedOperationException("ChangeIndex is disabled"); } diff --git a/java/com/google/gerrit/acceptance/DisabledProjectIndex.java b/java/com/google/gerrit/acceptance/DisabledProjectIndex.java index ed119ff3bc..2e3dd906b6 100644 --- a/java/com/google/gerrit/acceptance/DisabledProjectIndex.java +++ b/java/com/google/gerrit/acceptance/DisabledProjectIndex.java @@ -50,6 +50,11 @@ public class DisabledProjectIndex implements ProjectIndex { } @Override + public void insert(ProjectData obj) { + throw new UnsupportedOperationException("ProjectIndex is disabled"); + } + + @Override public void replace(ProjectData obj) { throw new UnsupportedOperationException("ProjectIndex is disabled"); } diff --git a/java/com/google/gerrit/acceptance/ReadOnlyChangeIndex.java b/java/com/google/gerrit/acceptance/ReadOnlyChangeIndex.java index e943519d19..f7a0669739 100644 --- a/java/com/google/gerrit/acceptance/ReadOnlyChangeIndex.java +++ b/java/com/google/gerrit/acceptance/ReadOnlyChangeIndex.java @@ -45,6 +45,11 @@ class ReadOnlyChangeIndex implements ChangeIndex { } @Override + public void insert(ChangeData obj) { + // do nothing + } + + @Override public void replace(ChangeData obj) { // do nothing } diff --git a/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java b/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java index e56f470300..d53490b8f6 100644 --- a/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java +++ b/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java @@ -175,6 +175,12 @@ abstract class AbstractElasticIndex<K, V> implements Index<K, V> { } @Override + public void insert(V obj) { + // TODO: Implement real insert() if it helps performance + replace(obj); + } + + @Override public void deleteAll() { // Delete the index, if it exists. String endpoint = indexName + client.adapter().indicesExistParams(); diff --git a/java/com/google/gerrit/index/Index.java b/java/com/google/gerrit/index/Index.java index 44f8b4236e..e662bc8477 100644 --- a/java/com/google/gerrit/index/Index.java +++ b/java/com/google/gerrit/index/Index.java @@ -40,6 +40,16 @@ public interface Index<K, V> { void close(); /** + * Insert a document into the index. + * + * <p>Results may not be immediately visible to searchers, but should be visible within a + * reasonable amount of time. + * + * @param obj document object + */ + void insert(V obj); + + /** * Update a document in the index. * * <p>Semantically equivalent to deleting the document and reinserting it with new field values. A diff --git a/java/com/google/gerrit/lucene/ChangeSubIndex.java b/java/com/google/gerrit/lucene/ChangeSubIndex.java index 5c24b9a009..360331a7bd 100644 --- a/java/com/google/gerrit/lucene/ChangeSubIndex.java +++ b/java/com/google/gerrit/lucene/ChangeSubIndex.java @@ -89,6 +89,11 @@ public class ChangeSubIndex extends AbstractLuceneIndex<Change.Id, ChangeData> } @Override + public void insert(ChangeData obj) { + throw new UnsupportedOperationException("don't use ChangeSubIndex directly"); + } + + @Override public void replace(ChangeData obj) { throw new UnsupportedOperationException("don't use ChangeSubIndex directly"); } diff --git a/java/com/google/gerrit/lucene/LuceneAccountIndex.java b/java/com/google/gerrit/lucene/LuceneAccountIndex.java index 87b7ccef6b..c4a524060d 100644 --- a/java/com/google/gerrit/lucene/LuceneAccountIndex.java +++ b/java/com/google/gerrit/lucene/LuceneAccountIndex.java @@ -144,6 +144,15 @@ public class LuceneAccountIndex extends AbstractLuceneIndex<Account.Id, AccountS } @Override + public void insert(AccountState as) { + try { + insert(toDocument(as)).get(); + } catch (ExecutionException | InterruptedException e) { + throw new StorageException(e); + } + } + + @Override public void delete(Account.Id key) { try { delete(idTerm(getSchema().useLegacyNumericFields(), key)).get(); diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java index bb97936bad..78f4624a91 100644 --- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java +++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java @@ -287,6 +287,23 @@ public class LuceneChangeIndex implements ChangeIndex { } @Override + public void insert(ChangeData cd) { + Term id = LuceneChangeIndex.idTerm(idTerm, idField, cd); + // toDocument is essentially static and doesn't depend on the specific + // sub-index, so just pick one. + Document doc = openIndex.toDocument(cd); + try { + if (cd.change().isNew()) { + openIndex.insert(doc).get(); + } else { + closedIndex.insert(doc).get(); + } + } catch (ExecutionException | InterruptedException e) { + throw new StorageException(e); + } + } + + @Override public void delete(Change.Id changeId) { Term id = LuceneChangeIndex.idTerm(idTerm, idField, changeId); try { diff --git a/java/com/google/gerrit/lucene/LuceneGroupIndex.java b/java/com/google/gerrit/lucene/LuceneGroupIndex.java index c1b44c7829..9f8b62e741 100644 --- a/java/com/google/gerrit/lucene/LuceneGroupIndex.java +++ b/java/com/google/gerrit/lucene/LuceneGroupIndex.java @@ -125,6 +125,15 @@ public class LuceneGroupIndex extends AbstractLuceneIndex<AccountGroup.UUID, Int } @Override + public void insert(InternalGroup group) { + try { + insert(toDocument(group)).get(); + } catch (ExecutionException | InterruptedException e) { + throw new StorageException(e); + } + } + + @Override public void delete(AccountGroup.UUID key) { try { delete(idTerm(key)).get(); diff --git a/java/com/google/gerrit/lucene/LuceneProjectIndex.java b/java/com/google/gerrit/lucene/LuceneProjectIndex.java index 7e7aecb923..11707be379 100644 --- a/java/com/google/gerrit/lucene/LuceneProjectIndex.java +++ b/java/com/google/gerrit/lucene/LuceneProjectIndex.java @@ -125,6 +125,15 @@ public class LuceneProjectIndex extends AbstractLuceneIndex<Project.NameKey, Pro } @Override + public void insert(ProjectData projectState) { + try { + insert(toDocument(projectState)).get(); + } catch (ExecutionException | InterruptedException e) { + throw new StorageException(e); + } + } + + @Override public void delete(Project.NameKey nameKey) { try { delete(idTerm(nameKey)).get(); diff --git a/java/com/google/gerrit/pgm/Reindex.java b/java/com/google/gerrit/pgm/Reindex.java index 8eaf3b3094..e8be0e6af5 100644 --- a/java/com/google/gerrit/pgm/Reindex.java +++ b/java/com/google/gerrit/pgm/Reindex.java @@ -38,12 +38,15 @@ import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.index.IndexModule; import com.google.gerrit.server.index.change.ChangeSchemaDefinitions; import com.google.gerrit.server.index.options.AutoFlush; +import com.google.gerrit.server.index.options.IsFirstInsertForEntry; import com.google.gerrit.server.plugins.PluginGuiceEnvironment; import com.google.gerrit.server.util.ReplicaUtil; +import com.google.inject.AbstractModule; import com.google.inject.Inject; import com.google.inject.Injector; import com.google.inject.Key; import com.google.inject.Module; +import com.google.inject.multibindings.OptionalBinder; import java.io.StringWriter; import java.io.Writer; import java.util.ArrayList; @@ -172,6 +175,16 @@ public class Reindex extends SiteProgram { throw new IllegalStateException("unsupported index.type = " + indexType); } modules.add(indexModule); + modules.add( + new AbstractModule() { + @Override + protected void configure() { + super.configure(); + OptionalBinder.newOptionalBinder(binder(), IsFirstInsertForEntry.class) + .setBinding() + .toInstance(IsFirstInsertForEntry.YES); + } + }); modules.add(new BatchProgramModule()); modules.add( new FactoryModule() { diff --git a/java/com/google/gerrit/server/index/IndexModule.java b/java/com/google/gerrit/server/index/IndexModule.java index 17665c0903..8b04c8dede 100644 --- a/java/com/google/gerrit/server/index/IndexModule.java +++ b/java/com/google/gerrit/server/index/IndexModule.java @@ -51,6 +51,7 @@ import com.google.gerrit.server.index.group.GroupIndexRewriter; import com.google.gerrit.server.index.group.GroupIndexer; import com.google.gerrit.server.index.group.GroupIndexerImpl; import com.google.gerrit.server.index.group.GroupSchemaDefinitions; +import com.google.gerrit.server.index.options.IsFirstInsertForEntry; import com.google.gerrit.server.index.project.ProjectIndexDefinition; import com.google.gerrit.server.index.project.ProjectIndexerImpl; import com.google.inject.Inject; @@ -59,6 +60,7 @@ import com.google.inject.Key; import com.google.inject.Provides; import com.google.inject.ProvisionException; import com.google.inject.Singleton; +import com.google.inject.multibindings.OptionalBinder; import java.util.Collection; import java.util.Set; import java.util.concurrent.TimeUnit; @@ -143,6 +145,9 @@ public class IndexModule extends LifecycleModule { } DynamicSet.setOf(binder(), OnlineUpgradeListener.class); + OptionalBinder.newOptionalBinder(binder(), IsFirstInsertForEntry.class) + .setDefault() + .toInstance(IsFirstInsertForEntry.NO); } @Provides diff --git a/java/com/google/gerrit/server/index/account/AllAccountsIndexer.java b/java/com/google/gerrit/server/index/account/AllAccountsIndexer.java index 63889b798d..ec27db0f1f 100644 --- a/java/com/google/gerrit/server/index/account/AllAccountsIndexer.java +++ b/java/com/google/gerrit/server/index/account/AllAccountsIndexer.java @@ -27,6 +27,7 @@ import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.AccountState; import com.google.gerrit.server.account.Accounts; import com.google.gerrit.server.index.IndexExecutor; +import com.google.gerrit.server.index.options.IsFirstInsertForEntry; import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; @@ -50,15 +51,18 @@ public class AllAccountsIndexer extends SiteIndexer<Account.Id, AccountState, Ac private final ListeningExecutorService executor; private final Accounts accounts; private final AccountCache accountCache; + private final IsFirstInsertForEntry isFirstInsertForEntry; @Inject AllAccountsIndexer( @IndexExecutor(BATCH) ListeningExecutorService executor, Accounts accounts, - AccountCache accountCache) { + AccountCache accountCache, + IsFirstInsertForEntry isFirstInsertForEntry) { this.executor = executor; this.accounts = accounts; this.accountCache = accountCache; + this.isFirstInsertForEntry = isFirstInsertForEntry; } @Override @@ -92,7 +96,11 @@ public class AllAccountsIndexer extends SiteIndexer<Account.Id, AccountState, Ac try { Optional<AccountState> a = accountCache.get(id); if (a.isPresent()) { - index.replace(a.get()); + if (isFirstInsertForEntry.equals(isFirstInsertForEntry.YES)) { + index.insert(a.get()); + } else { + index.replace(a.get()); + } } else { index.delete(id); } diff --git a/java/com/google/gerrit/server/index/change/ChangeIndexer.java b/java/com/google/gerrit/server/index/change/ChangeIndexer.java index e0f6bec8e1..a088af02e0 100644 --- a/java/com/google/gerrit/server/index/change/ChangeIndexer.java +++ b/java/com/google/gerrit/server/index/change/ChangeIndexer.java @@ -30,6 +30,7 @@ import com.google.gerrit.index.Index; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.index.IndexExecutor; import com.google.gerrit.server.index.StalenessCheckResult; +import com.google.gerrit.server.index.options.IsFirstInsertForEntry; import com.google.gerrit.server.logging.Metadata; import com.google.gerrit.server.logging.TraceContext; import com.google.gerrit.server.logging.TraceContext.TraceTimer; @@ -78,6 +79,7 @@ public class ChangeIndexer { private final PluginSetContext<ChangeIndexedListener> indexedListeners; private final StalenessChecker stalenessChecker; private final boolean autoReindexIfStale; + private final IsFirstInsertForEntry isFirstInsertForEntry; private final Set<IndexTask> queuedIndexTasks = Collections.newSetFromMap(new ConcurrentHashMap<>()); @@ -94,7 +96,8 @@ public class ChangeIndexer { StalenessChecker stalenessChecker, @IndexExecutor(BATCH) ListeningExecutorService batchExecutor, @Assisted ListeningExecutorService executor, - @Assisted ChangeIndex index) { + @Assisted ChangeIndex index, + IsFirstInsertForEntry isFirstInsertForEntry) { this.executor = executor; this.changeDataFactory = changeDataFactory; this.notesFactory = notesFactory; @@ -105,6 +108,7 @@ public class ChangeIndexer { this.autoReindexIfStale = autoReindexIfStale(cfg); this.index = index; this.indexes = null; + this.isFirstInsertForEntry = isFirstInsertForEntry; } @AssistedInject @@ -117,7 +121,8 @@ public class ChangeIndexer { StalenessChecker stalenessChecker, @IndexExecutor(BATCH) ListeningExecutorService batchExecutor, @Assisted ListeningExecutorService executor, - @Assisted ChangeIndexCollection indexes) { + @Assisted ChangeIndexCollection indexes, + IsFirstInsertForEntry isFirstInsertForEntry) { this.executor = executor; this.changeDataFactory = changeDataFactory; this.notesFactory = notesFactory; @@ -128,6 +133,7 @@ public class ChangeIndexer { this.autoReindexIfStale = autoReindexIfStale(cfg); this.index = null; this.indexes = indexes; + this.isFirstInsertForEntry = isFirstInsertForEntry; } private static boolean autoReindexIfStale(Config cfg) { @@ -198,21 +204,25 @@ public class ChangeIndexer { } private void indexImpl(ChangeData cd) { - logger.atFine().log("Replace change %d in index.", cd.getId().get()); + logger.atFine().log("Reindex change %d in index.", cd.getId().get()); for (Index<?, ChangeData> i : getWriteIndexes()) { try (TraceTimer traceTimer = TraceContext.newTimer( - "Replacing change in index", + "Reindexing change in index", Metadata.builder() .changeId(cd.getId().get()) .patchSetId(cd.currentPatchSet().number()) .indexVersion(i.getSchema().getVersion()) .build())) { - i.replace(cd); + if (isFirstInsertForEntry.equals(isFirstInsertForEntry.YES)) { + i.insert(cd); + } else { + i.replace(cd); + } } catch (RuntimeException e) { throw new StorageException( String.format( - "Failed to replace change %d in index version %d (current patch set = %d)", + "Failed to reindex change %d in index version %d (current patch set = %d)", cd.getId().get(), i.getSchema().getVersion(), cd.currentPatchSet().number()), e); } diff --git a/java/com/google/gerrit/server/index/group/AllGroupsIndexer.java b/java/com/google/gerrit/server/index/group/AllGroupsIndexer.java index 51c7730640..84d6a9d77c 100644 --- a/java/com/google/gerrit/server/index/group/AllGroupsIndexer.java +++ b/java/com/google/gerrit/server/index/group/AllGroupsIndexer.java @@ -30,6 +30,7 @@ import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.group.db.Groups; import com.google.gerrit.server.group.db.GroupsNoteDbConsistencyChecker; import com.google.gerrit.server.index.IndexExecutor; +import com.google.gerrit.server.index.options.IsFirstInsertForEntry; import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; @@ -54,15 +55,18 @@ public class AllGroupsIndexer extends SiteIndexer<AccountGroup.UUID, InternalGro private final ListeningExecutorService executor; private final GroupCache groupCache; private final Groups groups; + private final IsFirstInsertForEntry isFirstInsertForEntry; @Inject AllGroupsIndexer( @IndexExecutor(BATCH) ListeningExecutorService executor, GroupCache groupCache, - Groups groups) { + Groups groups, + IsFirstInsertForEntry isFirstInsertForEntry) { this.executor = executor; this.groupCache = groupCache; this.groups = groups; + this.isFirstInsertForEntry = isFirstInsertForEntry; } @Override @@ -97,7 +101,11 @@ public class AllGroupsIndexer extends SiteIndexer<AccountGroup.UUID, InternalGro groupCache.evict(uuid); Optional<InternalGroup> internalGroup = groupCache.get(uuid); if (internalGroup.isPresent()) { - index.replace(internalGroup.get()); + if (isFirstInsertForEntry.equals(isFirstInsertForEntry.YES)) { + index.insert(internalGroup.get()); + } else { + index.replace(internalGroup.get()); + } } else { index.delete(uuid); diff --git a/java/com/google/gerrit/server/index/options/IsFirstInsertForEntry.java b/java/com/google/gerrit/server/index/options/IsFirstInsertForEntry.java new file mode 100644 index 0000000000..f943309e07 --- /dev/null +++ b/java/com/google/gerrit/server/index/options/IsFirstInsertForEntry.java @@ -0,0 +1,26 @@ +// Copyright (C) 2021 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.index.options; + +/** + * This enum should be injected and checked to decide on which operation ({@link + * com.google.gerrit.index.Index#replace(Object) replace()} or {@link + * com.google.gerrit.index.Index#insert(Object) insert()}) should be performed on a specific {@link + * com.google.gerrit.index.Index index}. + */ +public enum IsFirstInsertForEntry { + YES, + NO +} diff --git a/java/com/google/gerrit/server/index/project/AllProjectsIndexer.java b/java/com/google/gerrit/server/index/project/AllProjectsIndexer.java index 0e4b688d6c..86c7e94230 100644 --- a/java/com/google/gerrit/server/index/project/AllProjectsIndexer.java +++ b/java/com/google/gerrit/server/index/project/AllProjectsIndexer.java @@ -27,6 +27,7 @@ import com.google.gerrit.index.SiteIndexer; import com.google.gerrit.index.project.ProjectData; import com.google.gerrit.index.project.ProjectIndex; import com.google.gerrit.server.index.IndexExecutor; +import com.google.gerrit.server.index.options.IsFirstInsertForEntry; import com.google.gerrit.server.project.ProjectCache; import com.google.inject.Inject; import com.google.inject.Singleton; @@ -48,12 +49,16 @@ public class AllProjectsIndexer extends SiteIndexer<Project.NameKey, ProjectData private final ListeningExecutorService executor; private final ProjectCache projectCache; + private final IsFirstInsertForEntry isFirstInsertForEntry; @Inject AllProjectsIndexer( - @IndexExecutor(BATCH) ListeningExecutorService executor, ProjectCache projectCache) { + @IndexExecutor(BATCH) ListeningExecutorService executor, + ProjectCache projectCache, + IsFirstInsertForEntry isFirstInsertForEntry) { this.executor = executor; this.projectCache = projectCache; + this.isFirstInsertForEntry = isFirstInsertForEntry; } @Override @@ -79,8 +84,13 @@ public class AllProjectsIndexer extends SiteIndexer<Project.NameKey, ProjectData () -> { try { projectCache.evict(name); - index.replace( - projectCache.get(name).orElseThrow(illegalState(name)).toProjectData()); + ProjectData projectData = + projectCache.get(name).orElseThrow(illegalState(name)).toProjectData(); + if (isFirstInsertForEntry.equals(isFirstInsertForEntry.YES)) { + index.insert(projectData); + } else { + index.replace(projectData); + } verboseWriter.println("Reindexed " + desc); done.incrementAndGet(); } catch (Exception e) { diff --git a/javatests/com/google/gerrit/server/index/change/FakeChangeIndex.java b/javatests/com/google/gerrit/server/index/change/FakeChangeIndex.java index a23ccaba9a..fc56a3c00f 100644 --- a/javatests/com/google/gerrit/server/index/change/FakeChangeIndex.java +++ b/javatests/com/google/gerrit/server/index/change/FakeChangeIndex.java @@ -74,6 +74,11 @@ public class FakeChangeIndex implements ChangeIndex { } @Override + public void insert(ChangeData obj) { + throw new UnsupportedOperationException(); + } + + @Override public void replace(ChangeData cd) { throw new UnsupportedOperationException(); } |