codeconsole commented on code in PR #16208: URL: https://github.com/apache/grails-core/pull/16208#discussion_r4066325735
########## 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: The invariant and the cascade are right, but the trigger is not this spec writing first against a fresh container. `config.image_collection` is created during step-up, before the node becomes primary, so a fresh container already has it. Something dropped it. I caught one crash on JDK 21 with `docker events` and kept the container's own log before it was reaped (exit 133, not a Testcontainers kill). The sequence on that server: ``` 20:07:02.132 OplogApplier-0 createCollection config.image_collection 20:08:29.873 conn30 dropDatabase config <- among asyncIndexDb, blockingIndexDb, test, ... 20:08:30.323 conn61 createCollection config.image_collection -> Invariant failure, abort ``` conn30 is `SchemaBasedMultiTenancySpec`. Its cleanup is `CompanyB.eachTenant { CompanyB.DB.drop() }`, and in SCHEMA mode `resolveTenantIds()` is `listDatabaseNames()`, so every database on the shared server is a tenant, MongoDB's own included. `admin` and `local` refuse the drop and the spec's catch swallows it, but `config` goes. conn61 is this spec's first save, and native id generation is a `findAndModify` that the driver retries by default, so it has to write an image into a collection that is no longer there. Declaring `ObjectId id` here, or turning transactions off, would take this spec out of the way, but it would leave the dropped `config` for whichever spec came next with a `Long` id. As you said, whatever moved this spec into place can move another one. So the fix is at the source, in 6fe37380ea: that cleanup now drops only `test1` and `test2`, the two tenant databases the feature uses. With the connection specs in one fork, the ordering that failed every time it was not run failed-first, it is now 3 of 3 green, with no mongod exit recorded. On why it appeared now: Gradle distributes classes across forks by order, the new spec classes moved `SchemaBasedMultiTenancySpec` ahead of this one in the same fork, and a run straight after a failure runs the failed classes first on a fresh container. That is why it looked flaky locally and was deterministic on CI. It is not specific to JDK 25 — the crash I caught was on JDK 21 — so I'd put the split you saw down to the same distribution rather than the JDK. With the drop gone, both are clean (numbers in the summary comment). ########## 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: Fixed in 9a88f6ecd1: the keys are now compared position by position, with the text-index special case still checked first. `BuildIndexesKeyOrderSpec` is your scenario — `{a: 1, b: 1}` on the server, and the entity declaring both `compoundIndex b: 1, a: 1` and `compoundIndex a: 1, b: 1`. Before the fix it reported `0 created, 2 already present` while MongoDB created the reversed index; now it reports `1 created, 1 already present`, and both indexes are asserted on the server. ########## 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: Restored in d8adb08fcf: `ExistingIndexes` keeps the exception the listing failed with, and the error reports its message again, with the conflict attached as the throwable, as on `8.0.x`. `BuildIndexesUnreadableIndexListSpec` now asserts that the message carries `not authorized on unlistableIndexDb to execute command listIndexes`, not just the literal phrase. ########## 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: Fixed in d8adb08fcf by not letting a child start its build until it is registered. The child's `initialize()` no longer builds; the parent calls `buildIndex()` on it right after putting it in the map, in the constructor for configured connections. For a connection added at runtime, the listener registers the child first and then starts its build only if `closed` is still clear. `close()` sets that flag before walking the map. Both are volatile or `ConcurrentHashMap` operations, so either `close()` finds the child and shuts its build down before `connectionSources.close()`, or the listener sees `closed` and never starts it — no build can begin unseen. `BuildIndexesPerConnectionSpec` adds a connection after `close()` and asserts no build is started for it. The interleaving itself is not something a test can force, so that half rests on the argument above. ########## 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: Fixed in d8adb08fcf: when the summary is not classified, the line says `Applied`, since nothing established whether the index was new. `BuildIndexesClassificationCostSpec` registers a class through `persistentEntityAdded` at DEBUG and asserts `Applied index for entity [...]`, no `Created`, and no `listIndexes` issued. ########## 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: Fixed in d8adb08fcf. `runIndexBuild` catches `Exception`, so an `Error` propagates as it is without the summary. An `InterruptedException` puts the interrupt flag back. A synchronous build rethrows the original exception unchanged, checked or not, instead of wrapping it. `BuildIndexesHookSpec` has an override throw `InterruptedException`, and asserts that the constructor's caller gets that exception and that the thread is interrupted again. ########## 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: Fixed in d8adb08fcf: the build summary hands out one `ExistingIndexes` per `MongoNamespace`, so a collection is listed once however many classes map to it, and they share one snapshot. `BuildIndexesClassificationCostSpec` now maps two classes to one collection with the same keys: the startup build issues two `listIndexes` for three classes over two collections, and reports the second class's declaration as already present. -- 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]
