From db5b37e106d13b9ec3b2725b04f6cd9783f8c359 Mon Sep 17 00:00:00 2001 From: Marco Miller Date: Tue, 27 Mar 2018 17:44:18 -0400 Subject: [PATCH] Fix init with elasticsearch and companion reindex AbstractElasticIndex: use the real (raw) index name in markReady (setReady, towards index config), rather than the one formatted with prefix and version suffix. Indeed, setReady assumes no prefix in index config and already appends the version -independently of the raw index name. Concatenating the prefix with index names (alongside versions) is already assumed internally within gerrit when interacting with its Elasticsearch indices. Such a dynamic, on-the-fly behaviour is based on: 1. the prefix -for this site instance, configured in gerrit.config, and 2. index names with version suffixes, configured in gerrit_index.config. VersionManager classes: align the Elasticsearch implementation with the one for Lucene. Namely, make the 'dirtiness' of Elastic versions consistent with Lucene's. Meaning, align the existence nature of an index between the two implementations: have an Elasticsearch index exist only if part of the Elasticsearch server indices, even if listed as a known schema version. The lucene implementation does this through the existence of a corresponding index directory, within the site. Before the aforementioned co-fixes, reindexing a site just initialized with elasticsearch failed, by means of extra indices generated within gerrit_index.config: 1. some with previous schema versions and ready = false set for them, 2. and some others with wrongfully prefixed index names, which also had their version suffix doubled; e.g., [index "gerrit_changes_0039_0039"], which reads [index "changes_0039"] and appears only once after this fix (also expectedly set as ready = true). The above sample exposes the fact that such a 'gerrit_' prefix does not match the default 'gerrit' (no trailing underscore). A follow-up change could consider changing that default to a more readable gerrit_ prefix. The current Elasticsearch implementation still requires a reindex after a site init, as per the ui console message already shown -telling the init user about such a necessity. So, a reindex is required in order to generate gerrit_index.config after an init, and this very change is to properly generate that file, for the site to work with Elasticsearch. Bug: Issue 8553 Change-Id: I33d0f02fcca1bacb9d66c57f3d0bc3fa3b233197 --- .../elasticsearch/AbstractElasticIndex.java | 6 ++++-- .../elasticsearch/ElasticAccountIndex.java | 2 +- .../elasticsearch/ElasticChangeIndex.java | 2 +- .../elasticsearch/ElasticGroupIndex.java | 2 +- .../elasticsearch/ElasticVersionManager.java | 20 +++++++------------ .../gerrit/lucene/LuceneVersionManager.java | 16 --------------- .../server/index/AbstractVersionManager.java | 10 +++++++--- 7 files changed, 21 insertions(+), 37 deletions(-) diff --git a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java index c813d2b582..a60c552c25 100644 --- a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java +++ b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java @@ -62,6 +62,7 @@ abstract class AbstractElasticIndex implements Index { private final Schema schema; private final FillArgs fillArgs; private final SitePaths sitePaths; + private final String indexNameRaw; protected final String indexName; protected final JestHttpClient client; @@ -82,10 +83,11 @@ abstract class AbstractElasticIndex implements Index { this.queryBuilder = new ElasticQueryBuilder(); this.indexName = String.format( - "%s%s%04d", + "%s%s_%04d", Strings.nullToEmpty(cfg.getString("elasticsearch", null, "prefix")), indexName, schema.getVersion()); + this.indexNameRaw = indexName; this.client = clientBuilder.build(); } @@ -101,7 +103,7 @@ abstract class AbstractElasticIndex implements Index { @Override public void markReady(boolean ready) throws IOException { - IndexUtils.setReady(sitePaths, indexName, schema.getVersion(), ready); + IndexUtils.setReady(sitePaths, indexNameRaw, schema.getVersion(), ready); } @Override diff --git a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticAccountIndex.java b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticAccountIndex.java index 2a46c40954..f0766bca04 100644 --- a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticAccountIndex.java +++ b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticAccountIndex.java @@ -84,7 +84,7 @@ public class ElasticAccountIndex extends AbstractElasticIndex schema) { // No parts of FillArgs are currently required, just use null. - super(cfg, null, sitePaths, schema, clientBuilder, ACCOUNTS_PREFIX); + super(cfg, null, sitePaths, schema, clientBuilder, ACCOUNTS); this.accountCache = accountCache; this.mapping = new AccountMapping(schema); } diff --git a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java index 15185a1e92..9692358e96 100644 --- a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java +++ b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java @@ -109,7 +109,7 @@ class ElasticChangeIndex extends AbstractElasticIndex SitePaths sitePaths, JestClientBuilder clientBuilder, @Assisted Schema schema) { - super(cfg, fillArgs, sitePaths, schema, clientBuilder, CHANGES_PREFIX); + super(cfg, fillArgs, sitePaths, schema, clientBuilder, CHANGES); this.db = db; this.changeDataFactory = changeDataFactory; mapping = new ChangeMapping(schema); diff --git a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticGroupIndex.java b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticGroupIndex.java index e2c34f2bb3..01aa0aa25f 100644 --- a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticGroupIndex.java +++ b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticGroupIndex.java @@ -81,7 +81,7 @@ public class ElasticGroupIndex extends AbstractElasticIndex schema) { // No parts of FillArgs are currently required, just use null. - super(cfg, null, sitePaths, schema, clientBuilder, GROUPS_PREFIX); + super(cfg, null, sitePaths, schema, clientBuilder, GROUPS); this.groupCache = groupCache; this.mapping = new GroupMapping(schema); } diff --git a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticVersionManager.java b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticVersionManager.java index 74a6b69c0d..91c1becb74 100644 --- a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticVersionManager.java +++ b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticVersionManager.java @@ -51,20 +51,10 @@ public class ElasticVersionManager extends AbstractVersionManager implements Lif prefix = MoreObjects.firstNonNull(cfg.getString("index", null, "prefix"), "gerrit"); } - @Override - protected boolean isDirty(Collection> inUse, Version v) { - return !inUse.contains(v); - } - @Override protected > TreeMap> scanVersions( IndexDefinition def, GerritIndexStatus cfg) { TreeMap> versions = new TreeMap<>(); - for (Schema schema : def.getSchemas().values()) { - int v = schema.getVersion(); - versions.put(v, new Version<>(schema, v, cfg.getReady(def.getName(), v))); - } - try { for (String version : versionDiscovery.discover(prefix, def.getName())) { Integer v = Ints.tryParse(version); @@ -72,13 +62,17 @@ public class ElasticVersionManager extends AbstractVersionManager implements Lif log.warn("Unrecognized version in index {}: {}", def.getName(), version); continue; } - if (!versions.containsKey(v)) { - versions.put(v, new Version(null, v, cfg.getReady(def.getName(), v))); - } + versions.put(v, new Version(null, v, true, cfg.getReady(def.getName(), v))); } } catch (IOException e) { log.error("Error scanning index: " + def.getName(), e); } + + for (Schema schema : def.getSchemas().values()) { + int v = schema.getVersion(); + boolean exists = versions.containsKey(v); + versions.put(v, new Version<>(schema, v, exists, cfg.getReady(def.getName(), v))); + } return versions; } } diff --git a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneVersionManager.java b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneVersionManager.java index 04d7d3552d..441a4b77b9 100644 --- a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneVersionManager.java +++ b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneVersionManager.java @@ -39,15 +39,6 @@ import org.slf4j.LoggerFactory; public class LuceneVersionManager extends AbstractVersionManager implements LifecycleListener { private static final Logger log = LoggerFactory.getLogger(LuceneVersionManager.class); - private static class Version extends AbstractVersionManager.Version { - private final boolean exists; - - private Version(Schema schema, int version, boolean exists, boolean ready) { - super(schema, version, ready); - this.exists = exists; - } - } - static Path getDir(SitePaths sitePaths, String prefix, Schema schema) { return sitePaths.index_dir.resolve(String.format("%s%04d", prefix, schema.getVersion())); } @@ -60,13 +51,6 @@ public class LuceneVersionManager extends AbstractVersionManager implements Life super(cfg, sitePaths, defs); } - @Override - protected boolean isDirty( - Collection> inUse, - com.google.gerrit.server.index.AbstractVersionManager.Version v) { - return !inUse.contains(v) && ((Version) v).exists; - } - @Override protected > TreeMap> scanVersions( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/AbstractVersionManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/AbstractVersionManager.java index 33cca1e160..a0273ce220 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/AbstractVersionManager.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/AbstractVersionManager.java @@ -35,12 +35,14 @@ public abstract class AbstractVersionManager implements LifecycleListener { public static class Version { public final Schema schema; public final int version; + public final boolean exists; public final boolean ready; - public Version(Schema schema, int version, boolean ready) { + public Version(Schema schema, int version, boolean exists, boolean ready) { checkArgument(schema == null || schema.getVersion() == version); this.schema = schema; this.version = version; + this.exists = exists; this.ready = ready; } } @@ -179,11 +181,13 @@ public abstract class AbstractVersionManager implements LifecycleListener { } } - protected abstract boolean isDirty(Collection> inUse, Version v); - protected abstract > TreeMap> scanVersions( IndexDefinition def, GerritIndexStatus cfg); + private boolean isDirty(Collection> inUse, Version v) { + return !inUse.contains(v) && v.exists; + } + private boolean isLatestIndexVersion(String name, OnlineReindexer reindexer) { int readVersion = defs.get(name).getIndexCollection().getSearchIndex().getSchema().getVersion(); return reindexer == null || reindexer.getVersion() == readVersion;