jdaugherty commented on code in PR #16208:
URL: https://github.com/apache/grails-core/pull/16208#discussion_r4062416285
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -952,7 +1183,7 @@ protected void initializeIndices(final PersistentEntity
entity) {
}
Document indexDef = new Document(compoundIndex);
createOrUpdateIndex(entity, collection, indexDef,
indexAttributes,
- "compound index with definition [" + indexDef +
"]");
+ "compound index with definition [" + indexDef +
"]", summary, existingIndexes);
Review Comment:
Anchored on the changed line; the mutation is a few lines up at
`compoundIndex.remove(INDEX_ATTRIBUTES)`, which is unchanged — but
`buildIndexesAsync` changes what it costs.
`getCompoundIndices()` returns the live `List<Map>` off `MongoCollection`,
and the mapped form is one instance shared by every datastore built on the same
`MongoMappingContext` — this one and each per-connection child. Removing the
key consumes the declaration: the first build to reach it gets the attributes,
every later build sees a compound index with none.
That was already true of a second synchronous build, but it was hard to
reach. Now the children's builds run concurrently on their own executors. For
`compoundIndex name: 1, age: -1, indexAttributes: [unique: true]` with two
connections configured, one connection gets the unique index and the other
silently gets a non-unique one — or an IndexOptionsConflict, which this PR now
counts into `summary.failures`. Two threads calling `remove` on the same
`HashMap` also leaves the enclosing `for (Map compoundIndex :
mappedForm.getCompoundIndices())` open to `ConcurrentModificationException`.
Please read the attributes without mutating the mapping:
```java
Map<?, ?> declaration = new LinkedHashMap<>(compoundIndex);
Object o = declaration.remove(INDEX_ATTRIBUTES);
Map indexAttributes = (o instanceof Map) ? (Map) o : null;
Document indexDef = new Document(declaration);
```
A test with two connections whose entity declares a compound index with
`indexAttributes` would pin it.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -1033,23 +1285,26 @@ private void createOrUpdateIndex(PersistentEntity
entity,
* different options. A TTL difference is the common, safe case (e.g. a
configurable retention
* changed between restarts) and is updated in place via {@code collMod};
anything else needs an
* explicit {@code recreateOnConflict:true} to authorise the
drop-and-recreate.
+ *
+ * @return {@code true} if the index ended up in the declared state,
{@code false} if the conflict
+ * could not be resolved and the existing index was left as it was
*/
- private void reconcileIndexConflict(PersistentEntity entity,
+ private boolean reconcileIndexConflict(PersistentEntity entity,
com.mongodb.client.MongoCollection<Document> collection,
+ ExistingIndexes existingIndexes,
Document keys, IndexOptions desired,
Long expireAfterSeconds,
boolean recreateOnConflict, String
descriptor, MongoCommandException original) {
- Document existing;
- try {
- existing = findIndexByKeyPattern(collection, keys);
- } catch (RuntimeException listError) {
+ List<Document> indexes = existingIndexes.get();
Review Comment:
This replaces a fresh `findIndexByKeyPattern(collection, keys)` — which
issued `listIndexes` at the moment of the conflict — with the snapshot taken
before this entity's first `createIndex`. A conflict caused by something that
appeared in between is invisible to that snapshot.
Two instances starting together, or two connections whose builds this PR now
deliberately runs concurrently: A lists the indexes, B creates `name_1` with
`unique: true`, A's `createIndex` gets IndexOptionsConflict (85). A's snapshot
has no `name_1`, so `indexes` has nothing to match, and this logs a failure
where a fresh listing would have found the index and taken the TTL-update or
`recreateOnConflict` path.
Reusing the snapshot for the `contains()` classification is reasonable; the
reconcile path needs current state. Re-listing on conflict (and refreshing the
cached list with the result) keeps both.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -574,17 +598,188 @@ public ConnectionSources<MongoClient,
MongoConnectionSourceSettings> getConnecti
}
/**
- * Builds the MongoDB index for this datastore
+ * Builds the MongoDB index for this datastore.
+ *
+ * <p>Each index is created by a command that the server answers only once
the index has been built,
+ * so with the default settings this blocks whoever creates the datastore
— in an application, the
+ * startup thread — for as long as MongoDB takes to build every declared
index. Enabling
+ * {@code grails.mongodb.buildIndexesAsync} hands the work to a background
thread and returns
+ * immediately instead.
*/
public void buildIndex() {
- for (PersistentEntity entity :
this.mappingContext.getPersistentEntities()) {
- // Only create Mongo templates for entities that are mapped with
Mongo
- if (!entity.isExternal()) {
- if (entity.isMultiTenant() && multiTenancyMode ==
MultiTenancySettings.MultiTenancyMode.SCHEMA) continue;
+ if (!buildIndexes) {
+ LOG.info("Index creation is disabled by [{} = false]. The indexes
declared by the domain classes " +
+ "will not be created or reconciled; the indexes already
present on the server are left untouched.",
+ MongoSettings.SETTING_BUILD_INDEXES);
+ return;
+ }
+ if (indexBuildExecutor == null) {
+ buildDeclaredIndexes();
+ return;
+ }
+ LOG.info("Building the indexes declared by the domain classes on a
background thread ([{} = true]). " +
+ "Startup does not wait for them, so a query issued before its
index exists is served without it.",
+ MongoSettings.SETTING_BUILD_INDEXES_ASYNC);
+ indexBuildExecutor.execute(() -> {
+ try {
+ buildDeclaredIndexes();
+ }
+ catch (Throwable e) {
+ // Nothing is waiting on this thread, so an error that would
have failed startup has to be
+ // reported here or it is lost entirely.
+ if (indexBuildExecutor.isShutdown() ||
Thread.currentThread().isInterrupted()) {
+ // toString rather than the message: an interrupted driver
call can arrive wrapped in
+ // an exception that carries no message of its own.
+ LOG.debug("The background index build was abandoned
because the datastore is shutting down: {}",
+ e.toString(), e);
+ }
+ else {
+ LOG.error("The background index build failed: {}. The
application is running without the " +
+ "indexes that were not created.", e.getMessage(),
e);
+ }
+ }
+ });
+ }
+
+ /**
+ * Creates and reconciles the indexes declared by every entity mapped to
this datastore, and reports
+ * what that cost. MongoDB answers each {@code createIndex} only once the
index exists, so the elapsed
+ * time is the time the caller — startup, or the background build thread —
actually spent waiting.
+ */
+ private void buildDeclaredIndexes() {
+ long startedAt = System.nanoTime();
+ IndexBuildSummary summary = new IndexBuildSummary();
+ IndexBuildSummary previousSummary = indexBuildSummary.get();
+ indexBuildSummary.set(summary);
+ try {
+ for (PersistentEntity entity :
this.mappingContext.getPersistentEntities()) {
+ // Only create Mongo templates for entities that are mapped
with Mongo
+ if (!entity.isExternal()) {
+ if (entity.isMultiTenant() && multiTenancyMode ==
MultiTenancySettings.MultiTenancyMode.SCHEMA) continue;
- initializeIndices(entity);
+ summary.entities++;
+ initializeIndices(entity);
+ }
}
}
+ finally {
+ if (previousSummary == null) {
+ indexBuildSummary.remove();
+ }
+ else {
+ indexBuildSummary.set(previousSummary);
+ }
+ }
+ long elapsedMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() -
startedAt);
+ if (summary.applied() == 0 && summary.failures == 0) {
+ LOG.debug("No indexes are declared by the {} domain class(es)
mapped to database [{}]",
+ summary.entities, defaultDatabase);
+ return;
+ }
+ String outcome = summary.classified ?
+ summary.created + " created, " + summary.alreadyPresent + "
already present" :
+ summary.applied() + " index declaration(s) applied";
+ if (summary.failures == 0) {
+ LOG.info("Index build for database [{}] finished in {}ms: {}, from
{} domain class(es)",
+ defaultDatabase, elapsedMillis, outcome, summary.entities);
+ }
+ else {
+ LOG.warn("Index build for database [{}] finished in {}ms: {}, {}
failed, from {} domain class(es). " +
+ "The failures are reported above.",
+ defaultDatabase, elapsedMillis, outcome, summary.failures,
summary.entities);
+ }
+ }
+
+ /**
+ * The indexes a collection already had when the build reached it, listed
once on first use and then
+ * reused. {@code createIndex} is idempotent and answers the same way
whether or not it had to build
+ * anything — the driver hands back only the index name, discarding the
{@code numIndexesBefore} /
+ * {@code numIndexesAfter} the server reports — so what was there
beforehand is what distinguishes an
+ * index this build created from one it merely confirmed.
+ *
+ * <p>Listed lazily so that an entity declaring no indexes costs no round
trip, and reused by the
+ * conflict path, which would otherwise list them again. Successful
changes are recorded so later
+ * declarations on the same keys see the current name and TTL, and are not
counted as new indexes.
+ */
+ private static final class ExistingIndexes {
+
+ private final com.mongodb.client.MongoCollection<Document> collection;
+
+ private final IndexBuildSummary summary;
+
+ private List<Document> indexes;
+
+ private boolean listed;
+
+ private ExistingIndexes(com.mongodb.client.MongoCollection<Document>
collection, IndexBuildSummary summary) {
+ this.collection = collection;
+ this.summary = summary;
+ }
+
+ /**
+ * @return the known current indexes, or {@code null} if they could
not be listed
+ */
+ private List<Document> get() {
+ if (!listed) {
+ listed = true;
+ try {
+ indexes = collection.listIndexes().into(new ArrayList<>());
+ } catch (RuntimeException e) {
+ // Not fatal: the build can still create indexes, it just
cannot report which of them
+ // were new. Losing the breakdown is not worth failing a
startup over.
+ LOG.debug("Could not list the existing indexes of
collection [{}]: {}",
+ collection.getNamespace().getCollectionName(),
e.getMessage(), e);
+ summary.classified = false;
+ }
+ }
+ return indexes;
+ }
+
+ private void record(Document keys, String name, Long
expireAfterSeconds) {
+ if (indexes == null) {
+ return;
+ }
+ Document existing = findIndexByKeyPattern(indexes, keys);
+ if (existing != null) {
+ indexes.remove(existing);
+ }
+ Document index = new Document("key", new
Document(keys)).append("name", name);
+ if (expireAfterSeconds != null) {
+ index.append(INDEX_EXPIRE_AFTER_SECONDS, expireAfterSeconds);
+ }
+ indexes.add(index);
+ }
+
+ private boolean contains(Document keys) {
Review Comment:
`contains()` delegates to `findIndexByKeyPattern`, which deliberately
matches *any* existing text index when the desired keys are a text index:
```java
if (desiredIsText && isTextIndex((Document) key)) {
```
That is right for the reconciliation caller — MongoDB allows one text index
per collection, so any text index is the one that conflicts. It is wrong here,
where the only question is whether *this* index already existed.
Collection has a text index on `title`; a new declaration adds a text index
on `body` with `recreateOnConflict: true`. `present` computes as `true`, the
build then drops `title`'s index and creates `body`'s, and the summary reports
`0 created, 1 already present`. Per the new guide text, the operator reads that
as "a restart that changed no mappings" costing milliseconds — from a build
that rebuilt a text index from scratch.
`contains()` should compare strictly with `sameKeyPattern` and leave the
text-index wildcard to `reconcileIndexConflict`.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -1241,6 +1500,14 @@ private boolean ownsClient() {
@PreDestroy
public void close() {
MongoClient current = this.mongo;
+ shutDownIndexBuild();
Review Comment:
`close()` shuts the index build down here; `stop()` — the CRaC path just
above — does not. It closes the client and returns.
With `buildIndexesAsync` and a checkpoint taken shortly after startup,
`stop()` runs `this.mongo.close()` while `gorm-mongo-index-build-DEFAULT` is
inside `createIndex`. The driver throws `IllegalStateException: state should
be: open`, and in the new catch block in `buildIndex()` neither
`indexBuildExecutor.isShutdown()` nor the interrupt flag is set — so a routine
checkpoint logs "The background index build failed. The application is running
without the indexes that were not created."
It is also true after the restore: `start()` only rebuilds the client, so
the indexes that had not been created yet never are.
`stop()` should call `shutDownIndexBuild()` before closing the client, and
`start()` should re-run the build (or the datastore should record that the
build was cut short). Worth a test over the `stop()`/`start()` pair with an
async build in flight.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -1282,7 +1579,12 @@ public void close() {
* @return The {@link ConnectionSources}
*/
protected static ConnectionSources<MongoClient,
MongoConnectionSourceSettings> createDefaultConnectionSources(MongoClient
mongoClient, PropertyResolver configuration, MongoMappingContext
mappingContext, boolean closeable) {
- MongoConnectionSourceSettings settings = new
MongoConnectionSourceSettings();
+ // Bound from the configuration rather than left at the defaults: the
client is supplied here, but
+ // the settings that describe how the datastore behaves (multiTenancy,
stateless, transactional,
+ // buildIndexes, engine, flush mode) still come from grails.mongodb
with grails.gorm fallbacks,
+ // exactly as they do when GORM creates the client itself. The
connection details in them are
+ // unused - this client is already connected.
+ MongoConnectionSourceSettings settings =
buildConnectionSourceSettings(configuration);
settings.setDatabaseName(mappingContext.getDefaultDatabaseName());
Review Comment:
Switching this from `new MongoConnectionSourceSettings()` to
`buildConnectionSourceSettings(configuration)` makes the `setDatabaseName` on
this line ineffective whenever the configuration carries a connection string.
`AbstractMongoConnectionSourceSettings.getDatabase()` prefers the connection
string's database:
```groovy
if (connectionString != null) { return connectionString.database ?:
databaseName }
```
On this path `connectionString` was previously always null, so the mapping
context's database always won. Now, with `grails.mongodb.url =
mongodb://host/analytics` in the configuration, the constructor's
`this.defaultDatabase = settings.getDatabase()` resolves to `analytics` and
`registerEntity` maps every entity into it — even though the caller passed a
`MongoMappingContext` built for a different database, and even though the
client being used is the supplied one.
The comment above states the intent: the connection details "are unused -
this client is already connected". The database name is one of those details,
and it is no longer unused. Clearing the bound connection string on this path,
or setting the database where `getDatabase()` will actually read it, would keep
the documented behaviour. Worth a test on the supplied-client constructor with
a URL naming a different database than the mapping context.
##########
grails-data-mongodb/docs/src/docs/asciidoc/gettingStarted/advancedConfig.adoc:
##########
@@ -36,6 +49,8 @@ grails {
}
----
+NOTE: These settings are read by name, so write them exactly as they are
documented. Unlike Spring Boot's own configuration properties they are not
relaxed-bound, and a kebab-case spelling such as `database-name` is not
recognised — it is ignored, leaving the default in place.
Review Comment:
This NOTE does not hold for applications running under Spring Boot, which is
how these settings are normally read.
`MongoDbGormAutoConfiguration` passes the context's
`ConfigurableEnvironment` straight into `new MongoDatastore(mongo, environment,
...)`. Boot attaches a `ConfigurationPropertySourcesPropertySource` to that
environment, and that is what answers the `getProperty`/`containsProperty`
calls the settings builder makes — so `grails.mongodb.database-name` in
`application.yml` does resolve. A reader who follows this NOTE will believe a
kebab-case setting is inert while it is in fact taking effect, which is the
more dangerous direction for the error to run.
The accompanying spec asserts the claim against
`DatastoreUtils.createPropertyResolver(map)`, a bare `MapPropertySource` with
no relaxed-binding source attached, so it cannot detect the difference: it pins
the resolver's behaviour rather than the application's.
Either scope the NOTE to the non-Boot path (a `PropertyResolver` constructed
directly) and say that relaxed binding does apply under Boot, or drop it. If
the intent really is that kebab-case never works, the test needs to go through
a `ConfigurableEnvironment` with Boot's property source attached, since that is
the configuration applications actually run.
##########
grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/BuildIndexesBackgroundFailureSpec.groovy:
##########
@@ -0,0 +1,170 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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
+ *
+ * https://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 org.grails.datastore.gorm.mongo
+
+import java.util.concurrent.CountDownLatch
+import java.util.concurrent.TimeUnit
+
+import ch.qos.logback.classic.Level
+import ch.qos.logback.classic.Logger
+import ch.qos.logback.classic.spi.ILoggingEvent
+import ch.qos.logback.core.read.ListAppender
+import com.mongodb.MongoException
+import com.mongodb.client.MongoClient
+import com.mongodb.client.MongoClients
+import grails.gorm.annotation.Entity
+import org.slf4j.LoggerFactory
+import spock.lang.Shared
+import spock.util.concurrent.PollingConditions
+
+import org.apache.grails.testing.mongo.AutoStartedMongoSpec
+import org.grails.datastore.mapping.core.DatastoreUtils
+import org.grails.datastore.mapping.mongo.MongoDatastore
+import org.grails.datastore.mapping.mongo.config.MongoSettings
+
+/**
+ * Nothing waits on the background index build, so what it does when it goes
wrong is only visible in the
+ * log. A build that fails has to say so loudly, because it can no longer fail
startup; a build abandoned
+ * because the application is shutting down has to stay quiet, because nothing
went wrong.
+ */
+class BuildIndexesBackgroundFailureSpec extends AutoStartedMongoSpec {
+
+ @Shared
+ MongoClient realClient
+
+ @Shared
+ Logger datastoreLogger
+
+ @Shared
+ ListAppender<ILoggingEvent> logged = new ListAppender<>()
+
+ @Shared
+ Level previousLevel
+
+ @Override
+ boolean shouldInitializeDatastore() {
+ false
+ }
+
+ void setupSpec() {
+ realClient =
MongoClients.create(dbContainer.getReplicaSetUrl('backgroundFailureDb'))
+ datastoreLogger =
LoggerFactory.getLogger('org.grails.datastore.mapping') as Logger
+ previousLevel = datastoreLogger.level
+ datastoreLogger.level = Level.DEBUG
+ logged.start()
+ datastoreLogger.addAppender(logged)
+ }
+
+ void cleanupSpec() {
+ datastoreLogger?.detachAppender(logged)
+ datastoreLogger?.level = previousLevel
+ realClient?.close()
+ }
+
+ private MongoDatastore asyncDatastoreOn(MongoClient client, String
database, Class... classes) {
+ new MongoDatastore(client, DatastoreUtils.createPropertyResolver([
+ 'grails.mongodb.databaseName' : database,
+ (MongoSettings.SETTING_BUILD_INDEXES_ASYNC): true
+ ]), classes)
+ }
+
+ void "test a background build that fails reports the failure instead of
losing it"() {
+ given:
+ def conditions = new PollingConditions(timeout: 30)
+ MongoClient broken = FailingMongoClient.wrap(realClient,
'getCollection') {
+ throw new MongoException('the connection went away mid-build')
+ }
+
+ when: "the datastore is created, which does not wait for the build"
+ def datastore = asyncDatastoreOn(broken, 'backgroundFailureDb',
FailedBackgroundThing)
+
+ then: "startup was not held up by, and did not fail because of, the
broken build"
+ datastore.isBuildIndexesAsync()
+
+ and: "the failure is reported at error level, since nothing else would
surface it"
+ conditions.eventually {
+ assert logged.list.any {
Review Comment:
`ListAppender.list` is a plain `ArrayList`. `AppenderBase.doAppend`
synchronizes the append, but nothing synchronizes iteration — and this iterates
on the test thread precisely while `gorm-mongo-index-build-*` daemons are
logging into the same appender, which is the point of the feature.
So `logged.list.any { ... }` inside `conditions.eventually` can throw
`ConcurrentModificationException` rather than passing or failing. Same shape in
`backgroundBuildFailures()` here, in `eventsForThisDatabase()` in
BuildIndexesFailureSummarySpec, BuildIndexesSummaryLogSpec and
BuildIndexesUnreadableIndexListSpec, and in BuildIndexesPerConnectionSpec.
Copying first is not enough on its own, since the copy iterates too.
`doAppend` synchronizes on the appender, so `synchronized (logged) { new
ArrayList<>(logged.list) }` gives a safe snapshot; a small `ListAppender`
subclass backed by a `CopyOnWriteArrayList` would do as well. Worth applying
across all of these — the daemons can outlive the datastore that started them,
and these run with `maxParallelForks > 1`.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -574,17 +598,188 @@ public ConnectionSources<MongoClient,
MongoConnectionSourceSettings> getConnecti
}
/**
- * Builds the MongoDB index for this datastore
+ * Builds the MongoDB index for this datastore.
+ *
+ * <p>Each index is created by a command that the server answers only once
the index has been built,
+ * so with the default settings this blocks whoever creates the datastore
— in an application, the
+ * startup thread — for as long as MongoDB takes to build every declared
index. Enabling
+ * {@code grails.mongodb.buildIndexesAsync} hands the work to a background
thread and returns
+ * immediately instead.
*/
public void buildIndex() {
- for (PersistentEntity entity :
this.mappingContext.getPersistentEntities()) {
- // Only create Mongo templates for entities that are mapped with
Mongo
- if (!entity.isExternal()) {
- if (entity.isMultiTenant() && multiTenancyMode ==
MultiTenancySettings.MultiTenancyMode.SCHEMA) continue;
+ if (!buildIndexes) {
+ LOG.info("Index creation is disabled by [{} = false]. The indexes
declared by the domain classes " +
+ "will not be created or reconciled; the indexes already
present on the server are left untouched.",
+ MongoSettings.SETTING_BUILD_INDEXES);
+ return;
+ }
+ if (indexBuildExecutor == null) {
+ buildDeclaredIndexes();
+ return;
+ }
+ LOG.info("Building the indexes declared by the domain classes on a
background thread ([{} = true]). " +
+ "Startup does not wait for them, so a query issued before its
index exists is served without it.",
+ MongoSettings.SETTING_BUILD_INDEXES_ASYNC);
+ indexBuildExecutor.execute(() -> {
+ try {
+ buildDeclaredIndexes();
+ }
+ catch (Throwable e) {
+ // Nothing is waiting on this thread, so an error that would
have failed startup has to be
+ // reported here or it is lost entirely.
+ if (indexBuildExecutor.isShutdown() ||
Thread.currentThread().isInterrupted()) {
+ // toString rather than the message: an interrupted driver
call can arrive wrapped in
+ // an exception that carries no message of its own.
+ LOG.debug("The background index build was abandoned
because the datastore is shutting down: {}",
+ e.toString(), e);
+ }
+ else {
+ LOG.error("The background index build failed: {}. The
application is running without the " +
+ "indexes that were not created.", e.getMessage(),
e);
+ }
+ }
+ });
+ }
+
+ /**
+ * Creates and reconciles the indexes declared by every entity mapped to
this datastore, and reports
+ * what that cost. MongoDB answers each {@code createIndex} only once the
index exists, so the elapsed
+ * time is the time the caller — startup, or the background build thread —
actually spent waiting.
+ */
+ private void buildDeclaredIndexes() {
+ long startedAt = System.nanoTime();
+ IndexBuildSummary summary = new IndexBuildSummary();
+ IndexBuildSummary previousSummary = indexBuildSummary.get();
+ indexBuildSummary.set(summary);
+ try {
+ for (PersistentEntity entity :
this.mappingContext.getPersistentEntities()) {
+ // Only create Mongo templates for entities that are mapped
with Mongo
+ if (!entity.isExternal()) {
+ if (entity.isMultiTenant() && multiTenancyMode ==
MultiTenancySettings.MultiTenancyMode.SCHEMA) continue;
- initializeIndices(entity);
+ summary.entities++;
+ initializeIndices(entity);
+ }
}
}
+ finally {
+ if (previousSummary == null) {
+ indexBuildSummary.remove();
+ }
+ else {
+ indexBuildSummary.set(previousSummary);
+ }
+ }
+ long elapsedMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() -
startedAt);
Review Comment:
The summary block sits after the try/finally, so it is skipped whenever the
loop exits by exception.
`createOrUpdateIndex` catches only `MongoCommandException`. A
`MongoSocketException`, a `MongoTimeoutException`, or the driver's
`IllegalStateException` on a closed client, thrown while indexing entity 3 of
30, propagates out of `initializeIndices` and out of the `try` above; the
`finally` restores the ThreadLocal and everything from this line down is
skipped. The remaining 27 entities are never indexed, and nothing reports how
far the build got.
That is the case the summary matters most for, and the guide leans on it —
`queryIndexes.adoc` says of the async build that "this line is also the only
signal that the build has finished". In async mode the operator is left with
the single "The background index build failed" line from the executor and no
idea what was applied before it.
Moving the elapsed-time and summary logging into the `finally` (or wrapping
the loop so the summary is always emitted, then rethrowing) would fix it. A
test with a client that throws a non-`MongoCommandException` partway through
the entities would pin it.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -574,17 +598,188 @@ public ConnectionSources<MongoClient,
MongoConnectionSourceSettings> getConnecti
}
/**
- * Builds the MongoDB index for this datastore
+ * Builds the MongoDB index for this datastore.
+ *
+ * <p>Each index is created by a command that the server answers only once
the index has been built,
+ * so with the default settings this blocks whoever creates the datastore
— in an application, the
+ * startup thread — for as long as MongoDB takes to build every declared
index. Enabling
+ * {@code grails.mongodb.buildIndexesAsync} hands the work to a background
thread and returns
+ * immediately instead.
*/
public void buildIndex() {
Review Comment:
`buildIndex()` is reached from `initialize(settings)`, which the constructor
invokes on its last line as `this.gormEnhancer = initialize(settings)`.
Synchronously that is a self-call on a not-quite-finished object — unpleasant,
but ordered. With `buildIndexesAsync` it publishes `this` to another thread
before the constructor returns: `gormEnhancer` is still null, and for a
subclass neither the subclass constructor body nor its field initializers have
run.
`initializeIndices(PersistentEntity)` is documented in this PR as the
extension point ("subclasses can customise index creation on either path"), so
a subclass that overrides it and reads a field it sets in its own constructor
now sees null or a default, depending on how quickly the worker starts.
`BuildIndexesHookSpec.CustomIndexDatastore` exercises exactly this path and
passes only because it holds no state. The per-connection children are in the
same position — their `initialize()` override calls `super.buildIndex()`.
Deferring the async submit to after construction — an
`InitializingBean`/`afterPropertiesSet` step, or an explicit start hook —
avoids it. At a minimum the hook's javadoc should say it may run on a
background thread before the subclass is fully constructed.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -1241,6 +1500,14 @@ private boolean ownsClient() {
@PreDestroy
public void close() {
MongoClient current = this.mongo;
+ shutDownIndexBuild();
+ // Over a snapshot: the connection sources listener can still add a
child while this runs, and a
+ // ConcurrentModificationException here would escape before anything
below had a chance to close.
+ for (MongoDatastore datastore : new
ArrayList<>(datastoresByConnectionSource.values())) {
Review Comment:
The copy does not provide the protection the comment describes. `new
ArrayList<>(map.values())` calls `values().toArray()`, which iterates
`datastoresByConnectionSource` exactly as the bare for-loop would, so a
`newConnectionSource` callback putting into it concurrently throws
`ConcurrentModificationException` out of the copy — and a concurrent `put` into
a plain `LinkedHashMap` can corrupt the table outright, which copying cannot
help with either.
A child added after the snapshot is also never shut down, so its
`gorm-mongo-index-build-<name>` thread keeps running against a client
`connectionSources.close()` is about to close.
Making `datastoresByConnectionSource` a `ConcurrentHashMap` (or
synchronizing the put and the iteration on the same lock) would do what the
comment claims; the copy and the comment can then both go.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -574,17 +598,188 @@ public ConnectionSources<MongoClient,
MongoConnectionSourceSettings> getConnecti
}
/**
- * Builds the MongoDB index for this datastore
+ * Builds the MongoDB index for this datastore.
+ *
+ * <p>Each index is created by a command that the server answers only once
the index has been built,
+ * so with the default settings this blocks whoever creates the datastore
— in an application, the
+ * startup thread — for as long as MongoDB takes to build every declared
index. Enabling
+ * {@code grails.mongodb.buildIndexesAsync} hands the work to a background
thread and returns
+ * immediately instead.
*/
public void buildIndex() {
- for (PersistentEntity entity :
this.mappingContext.getPersistentEntities()) {
- // Only create Mongo templates for entities that are mapped with
Mongo
- if (!entity.isExternal()) {
- if (entity.isMultiTenant() && multiTenancyMode ==
MultiTenancySettings.MultiTenancyMode.SCHEMA) continue;
+ if (!buildIndexes) {
+ LOG.info("Index creation is disabled by [{} = false]. The indexes
declared by the domain classes " +
+ "will not be created or reconciled; the indexes already
present on the server are left untouched.",
+ MongoSettings.SETTING_BUILD_INDEXES);
+ return;
+ }
+ if (indexBuildExecutor == null) {
+ buildDeclaredIndexes();
+ return;
+ }
+ LOG.info("Building the indexes declared by the domain classes on a
background thread ([{} = true]). " +
+ "Startup does not wait for them, so a query issued before its
index exists is served without it.",
+ MongoSettings.SETTING_BUILD_INDEXES_ASYNC);
+ indexBuildExecutor.execute(() -> {
Review Comment:
`close()` calls `shutDownIndexBuild()`, which calls
`indexBuildExecutor.shutdownNow()`, and a shut-down executor stays shut down.
Any later `buildIndex()` reaches this `execute` and throws
`RejectedExecutionException` at the caller — after the INFO line just above has
already announced that the build is under way on a background thread.
`buildIndex()` is public, documented, and called directly by several specs
in this PR (`TextIndexViaAttributesSpec`, `BuildIndexesDisabledSpec`,
`BuildIndexesSummaryLogSpec`). The synchronous path in the same situation fails
only if the client is actually closed, and fails with a driver error, so the
two modes now diverge on a call that previously behaved the same either way.
A guard on `indexBuildExecutor.isShutdown()` — logging and returning, or
falling back to the synchronous path — plus a test that calls `buildIndex()`
after `close()` would settle it.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -1269,6 +1536,36 @@ public void close() {
}
}
+ private void shutDownIndexBuild() {
+ if (indexBuildExecutor != null) {
+ // Interrupt every connection's build before closing its client. A
build can run for minutes,
+ // so shutdown must not wait for it; the server carries on
building what it was asked for.
+ indexBuildExecutor.shutdownNow();
+ }
+ }
+
+ /**
+ * Names the background index build thread after the connection it serves,
so that a log line or a
+ * thread dump says which datastore is building indexes. The thread is a
daemon: an index build in
+ * flight must not hold the JVM open, and abandoning the wait does not
abandon the build — the server
+ * finishes an index it has been asked for whether or not a client is
still listening.
+ */
+ private static final class IndexBuildThreadFactory implements
ThreadFactory {
Review Comment:
Minor: this module already has `api 'org.springframework:spring-context'`,
so `org.springframework.scheduling.concurrent.CustomizableThreadFactory` is on
the classpath. `new CustomizableThreadFactory("gorm-mongo-index-build-" +
connectionName)` with `setDaemon(true)` produces the same thread and handles
thread group and priority as well, which would let this class go.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -166,6 +171,19 @@ public class MongoDatastore extends AbstractDatastore
implements MappingContext.
protected final boolean stateless;
protected final boolean codecEngine;
protected final boolean transactionsEnabled;
+ protected final boolean buildIndexes;
+ protected final boolean buildIndexesAsync;
+
+ /**
+ * Runs the startup index build off the thread that creates the datastore
when
+ * {@code grails.mongodb.buildIndexesAsync} is enabled; {@code null}
otherwise. A single thread,
+ * so the indexes are still built one at a time per connection. The worker
expires after one idle
+ * second, releasing the worker while allowing subsequent calls to {@link
#buildIndex()}.
+ */
+ private final ExecutorService indexBuildExecutor;
+
+ /** The summary for the current build, scoped to its thread so the
protected index hook is preserved. */
+ private final ThreadLocal<IndexBuildSummary> indexBuildSummary = new
ThreadLocal<>();
Review Comment:
Design note: the summary is threaded through two mechanisms at once.
`buildDeclaredIndexes` sets this ThreadLocal, `initializeIndices(entity)` reads
it back out, and hands it to `initializeIndices(entity, summary)`, which passes
it on to `createOrUpdateIndex` and `ExistingIndexes`.
The ThreadLocal exists only to preserve the one-argument protected hook's
signature, and it brings two costs with it: the `previousSummary` save/restore
in the `finally` guards a re-entrancy case that requires a subclass to call
`buildIndex()` from inside its own `initializeIndices` override, and the
`summary != null ? summary : new IndexBuildSummary()` fallback means a subclass
that forgets `super.initializeIndices(entity)` silently counts nothing while
still appearing to work.
A small non-static `IndexBuild` holding the summary and the per-entity
`ExistingIndexes`, passed explicitly, with the protected one-arg hook
delegating to it, removes the ThreadLocal, the restore branch and the fallback.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -574,17 +598,188 @@ public ConnectionSources<MongoClient,
MongoConnectionSourceSettings> getConnecti
}
/**
- * Builds the MongoDB index for this datastore
+ * Builds the MongoDB index for this datastore.
+ *
+ * <p>Each index is created by a command that the server answers only once
the index has been built,
+ * so with the default settings this blocks whoever creates the datastore
— in an application, the
+ * startup thread — for as long as MongoDB takes to build every declared
index. Enabling
+ * {@code grails.mongodb.buildIndexesAsync} hands the work to a background
thread and returns
+ * immediately instead.
*/
public void buildIndex() {
- for (PersistentEntity entity :
this.mappingContext.getPersistentEntities()) {
- // Only create Mongo templates for entities that are mapped with
Mongo
- if (!entity.isExternal()) {
- if (entity.isMultiTenant() && multiTenancyMode ==
MultiTenancySettings.MultiTenancyMode.SCHEMA) continue;
+ if (!buildIndexes) {
+ LOG.info("Index creation is disabled by [{} = false]. The indexes
declared by the domain classes " +
+ "will not be created or reconciled; the indexes already
present on the server are left untouched.",
+ MongoSettings.SETTING_BUILD_INDEXES);
+ return;
+ }
+ if (indexBuildExecutor == null) {
+ buildDeclaredIndexes();
+ return;
+ }
+ LOG.info("Building the indexes declared by the domain classes on a
background thread ([{} = true]). " +
+ "Startup does not wait for them, so a query issued before its
index exists is served without it.",
+ MongoSettings.SETTING_BUILD_INDEXES_ASYNC);
+ indexBuildExecutor.execute(() -> {
+ try {
+ buildDeclaredIndexes();
+ }
+ catch (Throwable e) {
+ // Nothing is waiting on this thread, so an error that would
have failed startup has to be
+ // reported here or it is lost entirely.
+ if (indexBuildExecutor.isShutdown() ||
Thread.currentThread().isInterrupted()) {
+ // toString rather than the message: an interrupted driver
call can arrive wrapped in
+ // an exception that carries no message of its own.
+ LOG.debug("The background index build was abandoned
because the datastore is shutting down: {}",
+ e.toString(), e);
+ }
+ else {
+ LOG.error("The background index build failed: {}. The
application is running without the " +
+ "indexes that were not created.", e.getMessage(),
e);
+ }
+ }
+ });
+ }
+
+ /**
+ * Creates and reconciles the indexes declared by every entity mapped to
this datastore, and reports
+ * what that cost. MongoDB answers each {@code createIndex} only once the
index exists, so the elapsed
+ * time is the time the caller — startup, or the background build thread —
actually spent waiting.
+ */
+ private void buildDeclaredIndexes() {
+ long startedAt = System.nanoTime();
+ IndexBuildSummary summary = new IndexBuildSummary();
+ IndexBuildSummary previousSummary = indexBuildSummary.get();
+ indexBuildSummary.set(summary);
+ try {
+ for (PersistentEntity entity :
this.mappingContext.getPersistentEntities()) {
+ // Only create Mongo templates for entities that are mapped
with Mongo
+ if (!entity.isExternal()) {
+ if (entity.isMultiTenant() && multiTenancyMode ==
MultiTenancySettings.MultiTenancyMode.SCHEMA) continue;
- initializeIndices(entity);
+ summary.entities++;
+ initializeIndices(entity);
+ }
}
}
+ finally {
+ if (previousSummary == null) {
+ indexBuildSummary.remove();
+ }
+ else {
+ indexBuildSummary.set(previousSummary);
+ }
+ }
+ long elapsedMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() -
startedAt);
+ if (summary.applied() == 0 && summary.failures == 0) {
+ LOG.debug("No indexes are declared by the {} domain class(es)
mapped to database [{}]",
+ summary.entities, defaultDatabase);
+ return;
+ }
+ String outcome = summary.classified ?
+ summary.created + " created, " + summary.alreadyPresent + "
already present" :
+ summary.applied() + " index declaration(s) applied";
+ if (summary.failures == 0) {
+ LOG.info("Index build for database [{}] finished in {}ms: {}, from
{} domain class(es)",
+ defaultDatabase, elapsedMillis, outcome, summary.entities);
+ }
+ else {
+ LOG.warn("Index build for database [{}] finished in {}ms: {}, {}
failed, from {} domain class(es). " +
+ "The failures are reported above.",
+ defaultDatabase, elapsedMillis, outcome, summary.failures,
summary.entities);
+ }
+ }
+
+ /**
+ * The indexes a collection already had when the build reached it, listed
once on first use and then
+ * reused. {@code createIndex} is idempotent and answers the same way
whether or not it had to build
+ * anything — the driver hands back only the index name, discarding the
{@code numIndexesBefore} /
+ * {@code numIndexesAfter} the server reports — so what was there
beforehand is what distinguishes an
+ * index this build created from one it merely confirmed.
+ *
+ * <p>Listed lazily so that an entity declaring no indexes costs no round
trip, and reused by the
+ * conflict path, which would otherwise list them again. Successful
changes are recorded so later
+ * declarations on the same keys see the current name and TTL, and are not
counted as new indexes.
+ */
+ private static final class ExistingIndexes {
+
+ private final com.mongodb.client.MongoCollection<Document> collection;
+
+ private final IndexBuildSummary summary;
+
+ private List<Document> indexes;
+
+ private boolean listed;
+
+ private ExistingIndexes(com.mongodb.client.MongoCollection<Document>
collection, IndexBuildSummary summary) {
+ this.collection = collection;
+ this.summary = summary;
+ }
+
+ /**
+ * @return the known current indexes, or {@code null} if they could
not be listed
+ */
+ private List<Document> get() {
+ if (!listed) {
+ listed = true;
+ try {
+ indexes = collection.listIndexes().into(new ArrayList<>());
Review Comment:
This adds a `listIndexes` round trip per indexed entity on every startup,
and its only product is the wording of one log line.
`get()` is triggered by the first `contains(keys)` in `createOrUpdateIndex`,
so every entity declaring at least one index pays for it; previously
`listIndexes` ran only on the IndexOptionsConflict path. An application with
200 indexed domain classes issues 200 commands it did not issue before. At
30-50ms RTT against a remote deployment that is several seconds added to
startup — the cost the `buildIndexesAsync` half of this PR exists to remove,
reintroduced for everyone, including those who leave async off.
The server already reports `numIndexesBefore`/`numIndexesAfter` on
`createIndexes`; the driver's `createIndex` helper discards them, but
`database.runCommand(new Document("createIndexes", ...))` would give the same
created/already-present split with no extra round trip. Failing that, gate the
snapshot on `LOG.isInfoEnabled()`, or make the classification opt-in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
