jdaugherty commented on code in PR #16208:
URL: https://github.com/apache/grails-core/pull/16208#discussion_r4066763557
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -574,19 +631,331 @@ 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() {
+ 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;
+ }
+ ExecutorService executor = this.indexBuildExecutor;
+ if (executor == null) {
+ runIndexBuild(null);
+ return;
+ }
+ if (!executor.isShutdown()) {
+ 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);
+ try {
+ executor.execute(() -> runIndexBuild(executor));
+ return;
+ }
+ catch (RejectedExecutionException e) {
+ // Shut down between the check and the submission.
+ }
+ }
+ if (closed) {
+ LOG.warn("An index build was requested after the datastore was
closed, so it was not started.");
+ }
+ else {
+ indexBuildPending = true;
+ LOG.info("An index build was requested while the datastore is
stopped; it will run when the datastore is restarted.");
+ }
+ }
+
+ /**
+ * Runs one build and reports it, whether it finished or not.
+ *
+ * @param executor the executor running it, or {@code null} for a build on
the caller's thread, whose
+ * failure is left to propagate to the caller
+ */
+ private void runIndexBuild(ExecutorService executor) {
+ long startedAt = System.nanoTime();
+ // Telling a created index from one that was already there costs a
listIndexes per indexed
+ // collection, and its only product is the summary, so it is skipped
when that would not be logged.
+ IndexBuildSummary summary = new IndexBuildSummary(LOG.isInfoEnabled());
+ Exception failure = null;
+ try {
+ buildDeclaredIndexes(summary);
+ }
+ catch (Exception e) {
+ // An Error is left to propagate as it is, without the summary.
+ failure = e;
+ if (e instanceof InterruptedException) {
+ // Caught here rather than by whoever interrupted, so the flag
it cleared is put back.
+ Thread.currentThread().interrupt();
+ }
+ }
+ long elapsedMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() -
startedAt);
+ if (failure == null) {
+ logFinishedIndexBuild(summary, elapsedMillis);
+ return;
+ }
+ if (executor == null) {
+ logUnfinishedIndexBuild(summary, elapsedMillis, false);
+ // Unchanged, checked or not: an initializeIndices override may
throw one without declaring it.
+ MongoDatastore.<RuntimeException>rethrow(failure);
+ return;
+ }
+ // Nothing is waiting on this thread, so an error that would have
failed startup has to be
+ // reported here or it is lost entirely.
+ boolean abandoned = executor.isShutdown() ||
Thread.currentThread().isInterrupted();
+ if (abandoned) {
+ indexBuildPending = true;
+ // 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: {}",
+ failure.toString(), failure);
+ }
+ else {
+ LOG.error("The background index build failed: {}. The application
is running without the " +
+ "indexes that were not created.", failure.getMessage(),
failure);
+ }
+ logUnfinishedIndexBuild(summary, elapsedMillis, abandoned);
+ }
+
+ @SuppressWarnings("unchecked")
+ private static <E extends Exception> void rethrow(Exception failure)
throws E {
+ throw (E) failure;
+ }
+
+ private void logFinishedIndexBuild(IndexBuildSummary summary, long
elapsedMillis) {
+ if (summary.applied() == 0 && summary.recreated == 0 &&
summary.failures == 0) {
+ LOG.debug("No indexes are declared by the {} domain class(es)
mapped to database [{}]",
+ summary.entities, defaultDatabase);
+ return;
+ }
+ if (summary.failures == 0) {
+ LOG.info("Index build for database [{}] finished in {}ms: {}, from
{} domain class(es)",
+ defaultDatabase, elapsedMillis, summary.describe(),
summary.entities);
+ }
+ else {
+ LOG.warn("Index build for database [{}] finished in {}ms: {}, from
{} domain class(es). " +
+ "The failures are reported above.",
+ defaultDatabase, elapsedMillis, summary.describe(),
summary.entities);
+ }
+ }
+
+ /**
+ * Reports how far a build got before it stopped, which is what an
operator needs when a background
+ * build fails partway: the error names the cause, this says what was
applied before it.
+ */
+ private void logUnfinishedIndexBuild(IndexBuildSummary summary, long
elapsedMillis, boolean abandoned) {
+ String message = "Index build for database [{}] did not finish,
stopping after {}ms at {} of {} domain class(es): {}";
+ if (abandoned) {
+ LOG.debug(message, defaultDatabase, elapsedMillis,
summary.entities, summary.entitiesTotal, summary.describe());
+ }
+ else {
+ LOG.warn(message, defaultDatabase, elapsedMillis,
summary.entities, summary.entitiesTotal, summary.describe());
+ }
+ }
+
+ /**
+ * Creates and reconciles the indexes declared by every entity mapped to
this datastore. MongoDB answers
+ * each {@code createIndex} only once the index exists, so the time this
takes is the time the caller —
+ * startup, or the background build thread — actually spends waiting.
+ */
+ private void buildDeclaredIndexes(IndexBuildSummary summary) {
+ List<PersistentEntity> entities = new ArrayList<>();
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 (!entity.isExternal() &&
+ !(entity.isMultiTenant() && multiTenancyMode ==
MultiTenancySettings.MultiTenancyMode.SCHEMA)) {
+ entities.add(entity);
+ }
+ }
+ summary.entitiesTotal = entities.size();
+ IndexBuildSummary previousSummary = indexBuildSummary.get();
+ indexBuildSummary.set(summary);
+ try {
+ for (PersistentEntity entity : entities) {
initializeIndices(entity);
+ summary.entities++;
+ }
+ }
+ finally {
+ if (previousSummary == null) {
+ indexBuildSummary.remove();
+ }
+ else {
+ indexBuildSummary.set(previousSummary);
}
}
}
+ /**
+ * 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 not at all when the
+ * summary is not being classified. Successful changes are recorded so
later declarations on the same
+ * keys see the current name and TTL, and are not counted as new indexes.
A conflict re-lists instead
+ * of trusting the snapshot: whatever the server reports as conflicting
may have appeared since.
+ */
+ private static final class ExistingIndexes {
+
+ private final com.mongodb.client.MongoCollection<Document> collection;
+
+ private final IndexBuildSummary summary;
+
+ private List<Document> indexes;
+
+ private boolean listed;
+
+ /** Why the last listing failed, for the conflict it leaves
unreconciled to report. */
+ private RuntimeException listingFailure;
+
+ 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() {
+ return listed ? indexes : refresh();
+ }
+
+ /**
+ * Lists the indexes from the server now, replacing what was known.
+ *
+ * @return the current indexes, or {@code null} if they could not be
listed
+ */
+ private List<Document> refresh() {
+ listed = true;
+ try {
+ indexes = collection.listIndexes().into(new ArrayList<>());
+ listingFailure = null;
+ } 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);
+ indexes = null;
+ listingFailure = e;
+ summary.classified = false;
Review Comment:
A listing that fails here takes the classification down for the whole build,
including the entities that were already counted correctly.
`refresh()` has two callers. From `contains()` it is a collection's first
listing, and a failure costs nothing that was not already unknown. From
`reconcileIndexConflict` it is a re-listing, issued after the server reported a
conflict, and by then the build can be holding an accurate breakdown for every
collection it has been through.
Both set `summary.classified = false`. So a build over ten entities that has
counted `2 created, 5 already present` correctly, and then loses the re-listing
for the last one to a step-down or a timeout, reports `7 index declaration(s)
applied` for the whole run — and because `contains()` returns false from then
on, anything declared after it is counted as created whether it was or not.
Not being able to re-list one collection is a reason not to classify that
collection, not a reason to discard what the other listings established.
Keeping the flag on `ExistingIndexes`, and leaving the summary's own flag for
"classification was never asked for", would hold the rest of the breakdown —
with the summary needing some way to say it is partial.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -574,19 +631,331 @@ 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() {
+ 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;
+ }
+ ExecutorService executor = this.indexBuildExecutor;
+ if (executor == null) {
+ runIndexBuild(null);
+ return;
+ }
+ if (!executor.isShutdown()) {
+ LOG.info("Building the indexes declared by the domain classes on a
background thread ([{} = true]). " +
Review Comment:
Two things about this line, both of which matter more now that it is logged
per connection.
It names no connection. With `grails.mongodb.connections` configured, every
child logs this identical sentence at startup and `start()` logs it again for
each — nothing in it says which datastore is speaking. The same goes for the
disabled message a few lines up, which always names the global
`grails.mongodb.buildIndexes` even when the `false` came from
`connections.<name>.buildIndexes`: an operator who greps their configuration
for that key finds it set to `true` and concludes the setting is broken. The
name is on `connectionSources.getDefaultConnectionSource().getName()`, which
`newIndexBuildExecutor` already uses for the thread.
It is also logged before the submission it describes. If the executor is
shut down between the `isShutdown()` check above and `execute` below, the
`RejectedExecutionException` is swallowed and control reaches the block at the
end of the method, which logs either "requested after the datastore was closed,
so it was not started" or "will run when the datastore is restarted" — directly
after a line announcing that the build is under way on a background thread.
Logging it once `execute` has returned would make it true whenever it appears.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -166,6 +174,38 @@ 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()}.
+ *
+ * <p>Not final: a shut down executor cannot be restarted, so {@link
#start()} replaces the one
+ * {@link #stop()} shut down.
+ */
+ private volatile ExecutorService indexBuildExecutor;
Review Comment:
This executor is the first thing in the class to read the datastore's
mutable state from another thread, and `codecRegistry` is not set up for that.
The build calls `getCollection(entity)`, which ends
`.withCodecRegistry(codecRegistry)`. That field is a plain `protected
CodecRegistry`, and `setCodecRegistries`, `setCodecProviders` and `setCodecs`
are each `@Autowired(required = false)` and each do an unsynchronised
read-modify-write on it. With the synchronous build the two could not overlap:
the build finished inside the constructor, before Spring injected anything.
With `buildIndexesAsync` the build is submitted from inside the constructor and
the setters run after it returns, so the build thread can read the field while
they are rewriting it, with no happens-before between them.
Nothing the index build does with the registry matters today — the keys are
`Document`s and the default registry encodes them — so this is not a failure
you can produce. It is a field that was single-threaded before this PR and is
not any more, and `volatile` on it costs nothing.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -574,19 +631,331 @@ 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() {
+ 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;
+ }
+ ExecutorService executor = this.indexBuildExecutor;
+ if (executor == null) {
+ runIndexBuild(null);
+ return;
+ }
+ if (!executor.isShutdown()) {
+ 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);
+ try {
+ executor.execute(() -> runIndexBuild(executor));
+ return;
+ }
+ catch (RejectedExecutionException e) {
+ // Shut down between the check and the submission.
+ }
+ }
+ if (closed) {
+ LOG.warn("An index build was requested after the datastore was
closed, so it was not started.");
+ }
+ else {
+ indexBuildPending = true;
+ LOG.info("An index build was requested while the datastore is
stopped; it will run when the datastore is restarted.");
+ }
+ }
+
+ /**
+ * Runs one build and reports it, whether it finished or not.
+ *
+ * @param executor the executor running it, or {@code null} for a build on
the caller's thread, whose
+ * failure is left to propagate to the caller
+ */
+ private void runIndexBuild(ExecutorService executor) {
+ long startedAt = System.nanoTime();
+ // Telling a created index from one that was already there costs a
listIndexes per indexed
+ // collection, and its only product is the summary, so it is skipped
when that would not be logged.
+ IndexBuildSummary summary = new IndexBuildSummary(LOG.isInfoEnabled());
+ Exception failure = null;
+ try {
+ buildDeclaredIndexes(summary);
+ }
+ catch (Exception e) {
+ // An Error is left to propagate as it is, without the summary.
+ failure = e;
+ if (e instanceof InterruptedException) {
+ // Caught here rather than by whoever interrupted, so the flag
it cleared is put back.
+ Thread.currentThread().interrupt();
+ }
+ }
+ long elapsedMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() -
startedAt);
+ if (failure == null) {
+ logFinishedIndexBuild(summary, elapsedMillis);
+ return;
+ }
+ if (executor == null) {
+ logUnfinishedIndexBuild(summary, elapsedMillis, false);
+ // Unchanged, checked or not: an initializeIndices override may
throw one without declaring it.
+ MongoDatastore.<RuntimeException>rethrow(failure);
+ return;
+ }
+ // Nothing is waiting on this thread, so an error that would have
failed startup has to be
+ // reported here or it is lost entirely.
+ boolean abandoned = executor.isShutdown() ||
Thread.currentThread().isInterrupted();
Review Comment:
`abandoned` describes the datastore rather than the failure, so a real error
that arrives at the wrong moment is reported as an orderly shutdown.
A background build that fails on a bad index specification, a missing
`createIndex` privilege, or a duplicate key on a `unique` index, and is
overtaken by `close()` while it is winding that error up, evaluates
`executor.isShutdown()` as true. The error then goes to `LOG.debug` on the next
line and `logUnfinishedIndexBuild` follows it down to DEBUG, so at a production
level nothing is logged at all: the index is missing and there is no line
anywhere saying why. `indexBuildPending` is set as well, so a restore runs the
same failing build again.
The shutdown explains two kinds of failure: an interruption
(`MongoInterruptedException`, or an `InterruptedException` out of an override)
and the driver's `IllegalStateException` on the client `close()` has just
closed. A failure that is neither is not explained by the shutdown, and is
worth keeping at ERROR even when the executor is down.
##########
grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/connections/SuppliedMongoClientSettingsSpec.groovy:
##########
@@ -0,0 +1,206 @@
+/*
+ * 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.connections
+
+import com.mongodb.client.MongoClient
+import com.mongodb.client.MongoClients
+import grails.gorm.MultiTenant
+import grails.gorm.annotation.Entity
+import spock.lang.AutoCleanup
+import spock.lang.Shared
+
+import org.apache.grails.testing.mongo.AutoStartedMongoSpec
+import org.grails.datastore.gorm.events.DefaultApplicationEventPublisher
+import org.grails.datastore.mapping.core.DatastoreUtils
+import org.grails.datastore.mapping.mongo.MongoDatastore
+import org.grails.datastore.mapping.mongo.config.MongoMappingContext
+import org.grails.datastore.mapping.mongo.config.MongoSettings
+import
org.grails.datastore.mapping.multitenancy.MultiTenancySettings.MultiTenancyMode
+import org.grails.datastore.mapping.multitenancy.resolvers.NoTenantResolver
+import
org.grails.datastore.mapping.multitenancy.resolvers.SystemPropertyTenantResolver
+
+/**
+ * An application that hands GORM an existing {@code MongoClient} - which is
what happens whenever a
+ * {@code MongoClient} bean is already present, as with Spring Boot's MongoDB
auto-configuration - must
+ * still have its {@code grails.mongodb} settings applied. Only the connection
details are taken from the
+ * supplied client; everything describing how the datastore behaves still
comes from the configuration.
+ */
+class SuppliedMongoClientSettingsSpec extends AutoStartedMongoSpec {
+
+ @Shared
+ @AutoCleanup
+ MongoDatastore datastore
+
+ @Shared
+ MongoClient mongoClient
+
+ @Override
+ boolean shouldInitializeDatastore() {
+ false
+ }
+
+ void setupSpec() {
+ mongoClient =
MongoClients.create(dbContainer.getReplicaSetUrl('suppliedClientDb'))
+ Map config = [
+ 'grails.mongodb.databaseName' : 'suppliedClientDb',
+ (MongoSettings.SETTING_BUILD_INDEXES): false,
+ 'grails.mongodb.transactional' : true
Review Comment:
Verified, and your diagnosis is better than the one I gave: the trigger was
the dropped `config` database, not this spec being first to write against a
fresh container.
`:grails-data-mongodb-core:test` on JDK 25, which is where I saw the seven
failures, is now 799 tests, 0 failures, 45 skipped — twice in a row, with no
`mongod` exit recorded either time. Resolving.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -574,19 +612,317 @@ 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() {
+ 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;
+ }
+ ExecutorService executor = this.indexBuildExecutor;
+ if (executor == null) {
+ runIndexBuild(null);
+ return;
+ }
+ if (!executor.isShutdown()) {
+ 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);
+ indexBuildsInFlight.incrementAndGet();
+ try {
+ executor.execute(() -> {
+ try {
+ runIndexBuild(executor);
+ }
+ finally {
+ indexBuildsInFlight.decrementAndGet();
+ }
+ });
+ return;
+ }
+ catch (RejectedExecutionException e) {
+ // Shut down between the check and the submission.
+ indexBuildsInFlight.decrementAndGet();
+ }
+ }
+ if (running) {
+ LOG.warn("An index build was requested after the datastore was
closed, so it was not started.");
+ }
+ else {
+ indexBuildPending = true;
+ LOG.info("An index build was requested while the datastore is
stopped; it will run when the datastore is restarted.");
+ }
+ }
+
+ /**
+ * Runs one build and reports it, whether it finished or not.
+ *
+ * @param executor the executor running it, or {@code null} for a build on
the caller's thread, whose
+ * failure is left to propagate to the caller
+ */
+ private void runIndexBuild(ExecutorService executor) {
+ long startedAt = System.nanoTime();
+ // Telling a created index from one that was already there costs a
listIndexes per indexed
+ // collection, and its only product is the summary, so it is skipped
when that would not be logged.
+ IndexBuildSummary summary = new IndexBuildSummary(LOG.isInfoEnabled());
+ Throwable failure = null;
+ try {
+ buildDeclaredIndexes(summary);
+ }
+ catch (Throwable e) {
+ failure = e;
+ }
+ long elapsedMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() -
startedAt);
+ if (failure == null) {
+ logFinishedIndexBuild(summary, elapsedMillis);
+ return;
+ }
+ if (executor == null) {
+ logUnfinishedIndexBuild(summary, elapsedMillis, false);
+ if (failure instanceof RuntimeException) {
+ throw (RuntimeException) failure;
+ }
+ if (failure instanceof Error) {
+ throw (Error) failure;
+ }
+ throw new IllegalStateException(failure);
Review Comment:
Verified — `catch (Exception)`, the interrupt flag put back, an `Error` left
to propagate without the summary, and the original exception rethrown with its
type intact. `BuildIndexesHookSpec` pins the `InterruptedException` reaching
the constructor's caller and the thread being interrupted again. Resolving.
##########
grails-data-mongodb/core/build.gradle:
##########
@@ -149,7 +149,11 @@ dependencies {
// test: GenericWebApplicationContext requires ServletContext on the
classpath
}
- testImplementation 'org.slf4j:slf4j-nop' // Prevents warning about missing
slf4j implementation during compilation and tests
+ testImplementation 'ch.qos.logback:logback-classic', {
Review Comment:
Verified — plain dependency with the note above it. Resolving.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -1282,7 +1740,14 @@ 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 - so a configured URL is
dropped: its database would
+ // otherwise take precedence over the mapping context's, which is the
one this path always used.
+ MongoConnectionSourceSettings settings =
buildConnectionSourceSettings(configuration);
+ settings.url(null);
Review Comment:
The `authSource` trap is real and I agree it is not worth the
connection-string surgery. One correction to what I wrote, and one thing worth
writing down.
The correction: it is not equally default on both sides. Only the connection
string is dropped — `host`, `port`, `username` and `password` stay on the
settings that become the fallback, and `getUrl()` synthesises
`mongodb://user:pass@host:port/<database>` out of them. So which key the
application used decides what a named connection without a `url` of its own
gets on this path: configured as `grails.mongodb.host` it reaches the right
server, and does so better than it did before this PR; configured as
`grails.mongodb.url` the host goes with the scrub and it falls back to
`localhost:27017`.
That split is the part worth a line in the upgrade note. "Configuration Is
Now Applied to an Externally-Supplied Client" lists everything that starts
applying and does not say that the connection details are the exception, or
that a named connection on this path wants its own `url`. One sentence there
and I am happy to leave the code as it is.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -1103,9 +1511,9 @@ private void reconcileIndexConflict(PersistentEntity
entity,
* existing text index is unambiguously the one a newly-declared text
index conflicts with —
* match it regardless of its key shape or name so {@code
recreateOnConflict} can absorb it.</p>
*/
- private static Document
findIndexByKeyPattern(com.mongodb.client.MongoCollection<Document> collection,
Document keys) {
+ private static Document findIndexByKeyPattern(Iterable<Document> indexes,
Document keys) {
Review Comment:
Verified — the keys are compared position by position with the text-index
case still checked first, and `BuildIndexesKeyOrderSpec` asserts `1 created, 1
already present` with both indexes present on the server. Resolving.
##########
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:
Verified — `stop()` and `start()` both walk `datastoresAndChildren()` now,
and `BuildIndexesPerConnectionSpec` pins a named connection's build being
abandoned as a shutdown rather than an error, then run again on restart.
Resolving.
On the part you kept out: agreed that it is its own issue, and there is more
in it than the child clients. The SCHEMA-mode `AllTenantsResolver` built in the
constructor resolves tenant ids through the captured
`defaultConnectionSource.getSource()` rather than `getMongoClient()`, so after
a restore it is still calling `listDatabaseNames()` on the client `stop()`
closed. That one predates this PR as well, so the issue has at least three
parts to it.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -1033,23 +1433,27 @@ 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 how the conflict was resolved
*/
- private void reconcileIndexConflict(PersistentEntity entity,
+ private Reconciliation 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) {
+ // Listed afresh: the index the server just reported may not have
existed when this collection was
+ // first listed - another instance or another connection can have
created it since.
+ List<Document> indexes = existingIndexes.refresh();
+ if (indexes == null) {
LOG.error("Failed to create index for entity [{}] {} and could not
inspect existing indexes: {}",
Review Comment:
Verified — `ExistingIndexes` keeps the exception the listing failed with,
the error reports its message with the conflict attached as the throwable, and
`indexes == null` is reachable only from the catch, so there is nothing to
dereference on the path where the listing worked. The spec asserts the
authorization text rather than the literal phrase. Resolving.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -925,34 +1284,50 @@ protected void
registerEventListeners(ConfigurableApplicationEventPublisher even
}
/**
- * Indexes any properties that are mapped with index:true
+ * Indexes any properties that are mapped with index:true. Called for both
startup builds and
+ * entities registered later, so subclasses can customise index creation
on either path.
+ *
+ * <p>With {@code grails.mongodb.buildIndexesAsync} enabled the startup
build calls this on a background
+ * thread, and it can do so before the constructor of a subclass has
finished. An override must not
+ * depend on state that its own constructor or field initializers set up.
*
* @param entity The entity
*/
protected void initializeIndices(final PersistentEntity entity) {
+ IndexBuildSummary summary = indexBuildSummary.get();
+ // Outside a build nothing reports the counts, so there is nothing to
classify for.
+ initializeIndices(entity, summary != null ? summary : new
IndexBuildSummary(false));
+ }
+
+ private void initializeIndices(final PersistentEntity entity, final
IndexBuildSummary summary) {
+ if (!buildIndexes) {
+ LOG.debug("Index creation is disabled by [{} = false]. Skipping
the indexes declared by entity [{}].",
+ MongoSettings.SETTING_BUILD_INDEXES, entity.getName());
+ return;
+ }
final com.mongodb.client.MongoCollection<Document> collection =
getCollection(entity);
+ final ExistingIndexes existingIndexes = new
ExistingIndexes(collection, summary);
Review Comment:
Verified — one `ExistingIndexes` per `MongoNamespace`, held on the summary,
so a collection is listed once however many classes map to it and they share
the snapshot. `BuildIndexesClassificationCostSpec` pins both halves: two
listings for three classes over two collections, and the second class's
declaration reported as already present. Resolving.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -1269,6 +1698,35 @@ public void close() {
}
}
+ /**
+ * Interrupts the background index build, if there is one. A build can run
for minutes, so shutdown
+ * must not wait for it; the server carries on building what it was asked
for.
+ *
+ * @return whether a build was running or queued, and so was cut short
+ */
+ private boolean shutDownIndexBuild() {
+ ExecutorService executor = this.indexBuildExecutor;
+ if (executor == null) {
+ return false;
+ }
+ boolean cutShort = indexBuildsInFlight.get() > 0;
Review Comment:
The counter is gone and the case I described is right now: a build on its
last `createIndex` when `stop()` runs records nothing, and `start()` starts no
second build for it.
What the wait put in its place is worth another look. `resumeIndexBuild()`
calls `awaitTermination` for `INDEX_BUILD_EXIT_TIMEOUT_SECONDS` on the executor
`stop()` shut down, so a build that does not exit inside that window costs the
full ten seconds on the thread calling `start()` — the restore path this
setting exists to keep short — and then gets a second build running alongside
the first.
Reproduced on d8adb08fcf with a datastore whose `initializeIndices` blocks
in a step that does not answer an interrupt, over two entities:
```
PROBE start() waited 10010ms, builds announced: 1 -> 2
[gorm-mongo-index-build-default-1] Created index for entity
[...StuckThingTwo] on property [name] in 35ms
[gorm-mongo-index-build-default-1] Created index for entity
[...StuckThingTwo] on property [name] in 45ms
[gorm-mongo-index-build-default-1] Index build for database [probeResumeDb]
finished in 10131ms: 2 created, 0 already present, from 2 domain class(es)
[gorm-mongo-index-build-default-1] Index build for database [probeResumeDb]
finished in 61ms: 1 created, 1 already present, from 2 domain class(es)
```
Both builds ran to the end and each counted `StuckThingTwo` as created. A
blocking driver call is the realistic version of that hook —
`Thread.interrupt()` does not unblock a socket read, and for a named connection
`stop()` does not close the client that would.
There is also a case that needs no stuck build at all. `stop()` shuts down
the children it can see; a connection added while the datastore is stopped is
not one of them, so its executor is live and was never shut down. `start()`
then calls `awaitTermination` on a running executor, which cannot terminate,
waits the full ten seconds and starts a duplicate build for a connection that
was never interrupted.
A build that is going to exit does so in milliseconds once it has been
interrupted, so a much shorter wait would still cover the case this is for;
skipping the wait when `shutdownNow()` was never called on that executor would
cover the second.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -174,7 +203,11 @@ public class MongoDatastore extends AbstractDatastore
implements MappingContext.
protected final GormEnhancer gormEnhancer;
protected final ConnectionSources<MongoClient,
MongoConnectionSourceSettings> connectionSources;
protected final FlushModeType defaultFlushMode;
- protected final Map<String, MongoDatastore> datastoresByConnectionSource =
new LinkedHashMap<>();
+ /**
Review Comment:
The argument holds for an asynchronous child, and
`BuildIndexesPerConnectionSpec` covers the case it can reach. It does not cover
a synchronous one.
`close()` reaches a child through `shutDownIndexBuild()`, which returns
immediately when `indexBuildExecutor` is null — which is what it is for a child
with `buildIndexesAsync` off, or with the setting off globally, which is the
default. For that child, "either `close()` finds the child and shuts its build
down before `connectionSources.close()`" does nothing at all: the listener's
`!closed` check can still have passed, and `childDatastore.buildIndex()` then
takes the `executor == null` branch and runs the build inline on the caller's
thread while `close()` is closing the connection sources underneath it. The
driver's `IllegalStateException` comes back out of `addConnectionSource`.
It is the same window the comment describes; it is just that the thing that
closes it only exists on the asynchronous path. Re-checking `closed` in the
synchronous branch of `buildIndex()`, where the post-close warning already
lives, would cover both with what is already there.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -1015,13 +1391,37 @@ private void createOrUpdateIndex(PersistentEntity
entity,
indexOptions.expireAfter(expireAfterSeconds, TimeUnit.SECONDS);
}
+ // Asked before the index is created, while the answer still means
something.
+ boolean present = existingIndexes.contains(keys);
+ long startedAt = System.nanoTime();
try {
- collection.createIndex(keys, indexOptions);
+ String indexName = collection.createIndex(keys, indexOptions);
+ existingIndexes.record(keys, indexName, expireAfterSeconds);
+ if (present) {
+ summary.alreadyPresent++;
+ }
+ else {
+ summary.created++;
+ }
+ LOG.debug("{} index for entity [{}] {} in {}ms", present ?
"Confirmed" : "Created",
Review Comment:
Verified — the line says `Applied` when the summary is not classified, and
`BuildIndexesClassificationCostSpec` asserts it for a class registered through
`persistentEntityAdded`, with no `listIndexes` issued. Resolving.
##########
grails-data-mongodb/docs/src/docs/asciidoc/querying/queryIndexes.adoc:
##########
@@ -142,6 +142,107 @@ WARNING: Dropping and recreating an index rebuilds it
from scratch, during which
A change to a TTL index's `expireAfterSeconds` is handled automatically and
does not require `recreateOnConflict`, because GORM updates the expiry in place
rather than rebuilding the index.
+==== Disabling Index Creation on Startup
+
+
+By default GORM creates and reconciles every index declared in a mapping block
when the datastore starts. Set `buildIndexes` to `false` in
`grails-app/conf/application.yml` to switch that off:
+
+[source,yaml]
+----
+grails:
+ mongodb:
+ buildIndexes: false
+----
+
+or, in `application.groovy`:
+
+[source,groovy]
+----
+grails {
+ mongodb {
+ buildIndexes = false
+ }
+}
+----
+
+With this setting no `createIndex` or `collMod` command is issued for any
domain class, and the indexes already present on the server are left exactly as
they are. Queries are unaffected and continue to use whichever indexes exist.
This is useful when deploying against live data whose indexes are managed
separately — by a DBA or a migration step — so that a deployment does not build
an index against a large production collection, and so an application running
against an older index set does not have those indexes reconciled underneath it.
+
+It also suppresses index creation for domain classes registered after startup.
Declared at the top level the setting applies to every connection, and each
connection can override it:
+
+[source,yaml]
+----
+grails:
+ mongodb:
+ buildIndexes: false
+ connections:
+ reporting:
+ url: mongodb://localhost/reporting
+ buildIndexes: true
+----
+
+NOTE: Because nothing is created, a collection that has never been initialised
with `buildIndexes` enabled will have no declared indexes at all. Turn the
setting off only where the indexes are already in place or are applied by other
means. The setting governs only the indexes GORM derives from the mapping
blocks; an explicit `createIndex` call made by application code against a
collection is unaffected.
+
+==== Building Indexes in the Background
+
+
+MongoDB answers a `createIndex` command only once the index has been built, so
by default the thread that creates the datastore — in an application, the
startup thread — waits for every declared index before the application finishes
starting. On an empty collection that is instant; on a large existing
collection an index build can take minutes, and a deployment waits for all of
them in turn.
+
+Set `buildIndexesAsync` to have the startup index build run on a background
thread instead:
+
+[source,yaml]
+----
+grails:
+ mongodb:
+ buildIndexesAsync: true
+----
+
+Startup then continues without waiting. The indexes are still built one at a
time per connection, on a daemon thread named
`gorm-mongo-index-build-<connection>-<n>`, where `<n>` counts the workers
started for that connection. Different connections can build indexes
concurrently, including when they use the same MongoDB server. The thread is
released after the build finishes and it has been idle for one second.
Review Comment:
Verified — the guide gives an example name and no longer explains the
counter. Resolving.
--
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]
