jdaugherty commented on code in PR #16208: URL: https://github.com/apache/grails-core/pull/16208#discussion_r4065504576
########## 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: This spec takes the MongoDB container down with it, and with it every later spec in the same Gradle fork. `:grails-data-mongodb-core:test` fails on this branch under JDK 25 — 7 failures, on both MongoDB 7.0 and 8.0 — where CI was green on `4ebb343fce`. I reproduced it locally: `778 tests completed, 7 failed` on JDK 25, clean on JDK 21, which is the same split CI shows. The seven are the two features of this spec that write, plus all five of `MongoStaticApiMultiTenancySpec`. The spec passes on its own; it fails only in a full-module run. The failures are not assertions. `mongod` aborts: ``` Invariant failure: _times._recursionDepth == 1 || !opCtx->writesAreReplicated() src/mongo/db/op_observer/op_observer.cpp:51 ``` reached from `CmdFindAndModify::typedRun` -> `writeConflictRetryUpsert` -> `OpObserverImpl::onUpdate` -> `writeToImageCollection` -> `Helpers::upsert` -> `DatabaseImpl::createCollection` -> `onCreateCollection`: the retryable-write pre-image upsert has to create its collection from inside an op-observer callback, recursion depth reaches 2, and the server dies (container exit 133). The first symptom on the client is `MongoSocketReadException: Prematurely reached end of stream` in "test configured tenant discrimination isolates data with a supplied client". `MongoContainerHolder` keeps one container per test-worker thread and never replaces it, so once the server is gone every subsequent `AutoStartedMongoSpec` in that fork fails with connection-refused against the dead port. That is the whole `MongoStaticApiMultiTenancySpec` cascade, and why the failures read as unrelated to this PR. Two things about this spec put it in the way of that server bug: `transactional: true` on this line makes the writes session-scoped and so retryable, and `SuppliedTenantThing` declares no `ObjectId id`, unlike every other entity in this module, so saving it goes through `findAndModify` sequence generation. Avoiding the retryable `findAndModify` — declaring `ObjectId id` — or not enabling transactions on the shared client for the features that write should take the spec out of it. Why today's commits made JDK 25 start hitting this when the previous head did not is most likely the six specs they add changing how specs distribute across forks, and so which spec is first to write against a freshly started container. Worth confirming, because whatever moved this spec into the window can move another one in later. ########## 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: Making the map concurrent closes the corruption window, but the child is still registered after it has already started work. `createChildDatastore` returns a `MongoDatastore` whose `initialize()` override calls `super.buildIndex()`, so with `buildIndexesAsync` a child has submitted its build to its own executor before its constructor returns — and the `datastoresByConnectionSource.put(...)` at line 286 only happens after that. `connectionSources.addConnectionSource(...)` on one thread while the context shuts down on another: if `close()` iterates in that window it never reaches `shutDownIndexBuild()` for that child, so a daemon `gorm-mongo-index-build-<name>-N` thread keeps running against a client `connectionSources.close()` is about to close. And because that child's executor was never shut down, `runIndexBuild` computes `abandoned == false` and logs "The background index build failed. The application is running without the indexes that were not created." at `ERROR` for what is an orderly shutdown. A flag `close()` sets that the child's `buildIndex()` checks, or a second pass over the map after `connectionSources.close()`, would cover it. ########## 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: `cutShort` is read before `shutdownNow()`, so a build that was about to finish successfully counts as cut short. ```java boolean cutShort = indexBuildsInFlight.get() > 0; indexBuildsInFlight.addAndGet(-executor.shutdownNow().size()); ``` A build on its last `createIndex` when `stop()` runs sets `indexBuildPending`, and `start()` then re-issues every declared index for the connection — a full extra pass over every collection after each restore. It is idempotent, so the result is right, but for the case this setting exists to make cheap it is the wrong default. A flag the worker clears when `buildDeclaredIndexes` returns normally would be exact, and would let `stop()` tell "nothing was running" from "one finished just now". ########## 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: Anchored on the changed signature; the code in question is `sameKeyPattern` a little further down, which is unchanged — but this PR gives it a new job. `sameKeyPattern` compares key *sets*, not key order: ```java if (existingKey.size() != desiredKey.size()) { return false; } for (Map.Entry<String, Object> entry : desiredKey.entrySet()) { if (!existingKey.containsKey(entry.getKey())) { return false; ``` For MongoDB, `{a: 1, b: 1}` and `{b: 1, a: 1}` are two different indexes, so that treats a genuinely new compound index as one that already exists. Until now it only ran after the server had reported `IndexOptionsConflict`, which it does only for the *same* key pattern, so the order-insensitivity was unreachable. `ExistingIndexes.contains()` now calls it for every declaration, before `createIndex`. Collection has `{a: 1, b: 1}`; an entity declares `compoundIndex b: 1, a: 1`. `present` computes as `true`, MongoDB creates a second index, and the summary reports it as already present — per the new guide text, "a restart that changed no mappings". `record()` then removes the `{a:1,b:1}` entry from the snapshot, so a later declaration of `{a:1,b:1}` on the same entity is reported as created. Comparing the key *sequence* — iterating both in order — would fix it, with the text-index special case unchanged. An entity declaring `compoundIndex b: 1, a: 1` against a collection that already has `{a: 1, b: 1}` would pin it. ########## 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: This line always says `Created` for a domain class registered after startup. `initializeIndices(entity)` falls back to `new IndexBuildSummary(false)` when no build is in progress, so `contains()` short-circuits on `!summary.classified` and `present` is always `false`. A class registered through `persistentEntityAdded` therefore logs `Created index for entity [X] on property [y] in 1ms` for an index that already existed and cost nothing to confirm. During a startup build the line is accurate — `DEBUG` implies `INFO`, so classification is on — which is what makes this easy to miss. `queryIndexes.adoc` points at this line as the way to find the one slow index behind a slow summary, so `Created` on a 1ms confirmation is the wrong signal. Either classify on this path too, or make the word depend on `summary.classified` and say `Applied` when it is off. ########## 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: Verified — the attributes come off a `LinkedHashMap` copy and the declaration is left intact, and `BuildIndexesSharedDeclarationSpec` asserts both connections get a unique index. Resolving. ########## 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: Verified — `settings.url(null)` on this path, and the spec asserts `defaultDatabase` is the mapping context's. There is only the one `url(ConnectionString)` overload, so the bare `null` is unambiguous. Resolving. ########## 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: You are right and I had the mechanism wrong: MongoDB reports an existing text index under `{_fts: 'text', _ftsx: 1}`, so a strict comparison would report every unchanged text index as created on every restart. Keeping `findIndexByKeyPattern` here is correct, and the scenario I described is covered by the new `recreated` count in `BuildIndexesRecreateSummarySpec`. Resolving. ########## 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: Verified — `runIndexBuild` reports the summary whether the loop finishes or not, and the synchronous path still propagates afterwards. `BuildIndexesUnfinishedSummarySpec` covers both the background and calling-thread cases. Resolving. ########## 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: Verified — `CustomizableThreadFactory` with `setDaemon(true)`, and the hand-rolled factory is gone. Resolving. ########## 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: You and @matrei are right and I was wrong. `grails.mongodb.databaseName` is not a valid `ConfigurationPropertyName` — it has an uppercase letter — so Boot's attached source returns null rather than answering, and the lookup falls through to the underlying source, which matches only literally. The note holds under Boot, and `MongoConnectionSourceSettingsSpec` now asserts it through an attached environment rather than a bare `MapPropertySource`. Resolving. ########## 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: Verified — `new IndexBuildSummary(LOG.isInfoEnabled())`, and `BuildIndexesClassificationCostSpec` counts the `listIndexes` commands at both levels. The guide also says what the summary falls back to when the gate is closed, which is the part I would otherwise have asked about. Resolving. ########## 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: Verified — `CapturedLog` backs the appender with a `CopyOnWriteArrayList` and hands out snapshots, and the five specs use it in place of their own attach/restore code. 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: This scrubs the connection string but leaves the rest of the connection details bound, and those are the fallback every named connection inherits. `settings` becomes the default connection source's settings, which `InMemoryConnectionSources` passes to `MongoConnectionSourceFactory` as the fallback for each `grails.mongodb.connections.*` entry. `getUrl()` with a null connection string synthesizes one from `host`, `port`, `username` and `password`: ```groovy return new ConnectionString("mongodb://${uAndP}${host}${portStr}/$database") ``` So an application that supplies its own client, sets `grails.mongodb.url = mongodb://prod-host/app` and declares a `reporting` connection with no URL of its own gets `reporting` against `localhost:27017`. Not a regression — this path used a bare `MongoConnectionSourceSettings` before, so the fallback was equally default — but it is the one part of "the configuration is now applied" that still is not, and the dropped URL is what would have carried it. Setting the database name where `getDatabase()` actually reads it, and leaving the URL in place, would keep both the mapping context's database and the configured host. A named connection without its own `url` on the supplied-client constructor would pin it. ########## 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: Accepted — the one-argument hook has to be the one the build calls or a subclass override is skipped on startup, which does force the summary to travel out of band. Resolving. ########## 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: This drops the reason the listing failed, which the version on `8.0.x` reported. Before: ```java } catch (RuntimeException listError) { LOG.error("Failed to create index for entity [{}] {} and could not inspect existing indexes: {}", entity.getName(), descriptor, listError.getMessage(), original); ``` The message argument is now `original.getMessage()` — the `IndexOptionsConflict` — and the listing failure survives only in `ExistingIndexes.refresh()`, at `DEBUG`. So for the case `BuildIndexesUnreadableIndexListSpec` sets up, a role with `createIndex` but not `listIndexes`, an operator at the default level is told the inspection failed but not that it was an authorization failure. The spec asserts only on the literal phrase, so nothing catches it. Keeping the listing failure on `ExistingIndexes` for the caller to report, or logging it at the level of the failure it causes rather than `DEBUG`, restores it. ########## 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: `<n>` is not a count of the workers started for that connection. `newIndexBuildExecutor` builds a fresh `CustomizableThreadFactory` per executor and its counter starts at 1 each time. `start()` builds a new executor after every restore, as does each new `MongoDatastore` for the same connection name, so the worker after a checkpoint/restore is `gorm-mongo-index-build-default-1` again. Since the executor holds at most one thread, `<n>` is really "which worker of this executor" and is almost always 1. Describing it that way, or dropping the explanation and keeping just the example name, would both be accurate. ########## 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: `ExistingIndexes` is per entity, but the listing it caches is per collection. `getCollectionName` returns `entity.getRootEntity().getDecapitalizedName()` for a non-root entity, so every class in an inheritance hierarchy maps to the root's collection — and `collection 'x'` can be declared on several unrelated classes besides. With classification on, a root plus five indexed subclasses issues six `listIndexes` commands against the same collection where one would do, which is the cost the `INFO` gate exists to contain. `IndexBuildSummary` is already per build, so keying the cache off `collection.getNamespace()` there would make it one listing per collection. It would also make the snapshot consistent: today each entity's `record()` updates only its own copy, so two classes declaring the same keys on a shared collection both count as created. ########## 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: Nit: the closure holds only comments. ```groovy testImplementation 'ch.qos.logback:logback-classic', { // test: a real SLF4J binding, so that the log a test asserts on is actually emitted. ... } ``` Every other entry in this file uses the closure to configure something and puts the note beside it. A plain `testImplementation 'ch.qos.logback:logback-classic'` with those three lines as a preceding comment reads the same and drops the empty `Action`. ########## 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: Catching `Throwable` here changes what the synchronous caller sees. On `8.0.x` an exception out of the build loop propagated from `buildIndex()` unchanged. A checked exception now arrives wrapped: Groovy does not require declaring them, and `initializeIndices` is documented as the override point, so a subclass hook that throws `InterruptedException` — `CheckpointedDatastore` in `BuildIndexesLifecycleSpec` calls `CountDownLatch.await()` — reaches the caller as `IllegalStateException`. A caller catching `InterruptedException` no longer sees it, and the interrupt flag `await()` cleared is never restored. Catching `Exception` rather than `Throwable`, and re-interrupting when the failure is an `InterruptedException`, keeps both — and leaves an `Error` to propagate without first running the summary logging. ########## 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: Verified — `reconcileIndexConflict` calls `existingIndexes.refresh()` and the cached listing is replaced with the result, so a conflict is reconciled against current state. 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: The default connection is handled — `stop()` interrupts before closing the client and records that the build was cut short, and `start()` builds a fresh executor and runs it again. I am leaving this open for the per-connection children, which neither covers. `close()` walks `datastoresByConnectionSource` and calls `shutDownIndexBuild()` on every child. `stop()` and `start()` only touch `this`. So with `buildIndexesAsync` and a named connection configured, a checkpoint leaves the child's `gorm-mongo-index-build-<name>-N` worker running against a client that is still open — so the checkpoint still sees those sockets, which is the thing `stop()` exists to prevent — and `start()` never re-runs a child build that was cut short. Given `close()` does loop the map, the asymmetry looks unintended rather than deliberate. The guide's note under Building Indexes in the Background reads as covering every connection: "A datastore stopped for a checkpoint and restored (with CRaC) runs a build it cut short again once it is restarted." A test over the `stop()`/`start()` pair with a second connection configured would pin whichever way you take 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: Fair — there is no lifecycle callback to defer to for a standalone `new MongoDatastore(...)`, so moving the submission would cost standalone users the build entirely. The javadoc now says the hook may run on a background thread before a subclass constructor has finished and must not depend on state that constructor sets up, which is the part that was missing. 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(); + // 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: Verified — `ConcurrentHashMap`, with the copy and the comment gone. Resolving. The registration window you acknowledged in the reply is worth its own look, but it is not this thread. ########## 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: Verified — a build requested after `close()` logs a warning and returns instead of reaching `execute`, and while the datastore is stopped it defers to `start()`. Both are covered in `BuildIndexesLifecycleSpec`. 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]
