diff options
author | Adithya Chakilam <achakila@codeaurora.org> | 2021-11-10 15:15:19 -0800 |
---|---|---|
committer | Adithya Chakilam <achakila@codeaurora.org> | 2021-11-18 13:34:33 -0800 |
commit | 43cce8b744fd5540be987647d5510cfe2d8ba8b3 (patch) | |
tree | c1e294a9ca67b8e1907efbe63e3f88a30b1ee4f9 | |
parent | e297d8448253ba56dde391a44803a5e1fbf1e3f6 (diff) |
Avoid lucene index deletes during offline reindexing
Since offline reindexing starts off by deleting the index, it's better
to call IndexWriter.addDocument() while adding documents to the new
index instead of IndexWriter.updateDocument(). This avoids deletes
from the index during lucene flushes.
On a test site with ~160K changes and empty caches this change reduced
the runtime of reindex command from ~32 minutes to ~30 minutes. With
populated caches there was a reduction from ~4 minutes to ~3 minutes.
Change-Id: I140c88125bb533f3486bf9a224a49dc3c8ce8897
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(); } |