codeconsole commented on code in PR #16208:
URL: https://github.com/apache/grails-core/pull/16208#discussion_r4064844331
##########
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:
Fixed in 4a4733ac09: the attributes are read from a copy, and the
declaration is left as it was. `BuildIndexesSharedDeclarationSpec` maps an
entity with `compoundIndex name: 1, age: -1, indexAttributes: [unique: true]`
to two connections and asserts both indexes are unique. It failed before the
fix with the default synchronous build — the second connection built first and
the default connection got a plain index — so, as you said, this predated the
branch.
##########
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:
Fixed in 8a7e1ff4c4: the bound connection string is dropped on this path, so
the mapping context's database wins again, which is what the comment above the
line said happened. `SuppliedMongoClientSettingsSpec` builds the datastore from
a `MongoMappingContext` for one database with `grails.mongodb.url` naming
another and asserts `defaultDatabase` is the mapping context's; it came back as
the URL's before the fix.
##########
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:
Your scenario did misreport, but through the conflict branch rather than
`contains()`: a `createIndex` that conflicts goes to `reconcileIndexConflict`,
and a successful reconcile was counted as already present whatever `present`
said. 848282548a counts a drop-and-recreate separately, so this case now reads
`0 created, 1 recreated, 0 already present`. `BuildIndexesRecreateSummarySpec`
is exactly that collection: a text index on `title`, and a declared text index
on `body` with `recreateOnConflict`.
I've left `contains()` on `findIndexByKeyPattern`, though. MongoDB reports
an existing text index under the synthetic key `{_fts: 'text', _ftsx: 1}`,
which never equals a declared `{body: 'text'}` under `sameKeyPattern`, so a
strict comparison would report every unchanged text index as created on every
restart.
##########
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:
Done in 848282548a: `reconcileIndexConflict` lists again and refreshes the
cached listing; the up-front listing is still what classification uses.
`BuildIndexesStaleSnapshotSpec` makes the first `listIndexes` return nothing,
as if another instance had created the index after it, and asserts the TTL is
reconciled in place. With the cached listing it fails.
##########
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:
Done in 848282548a: after `close()`, `buildIndex()` logs a warning and
returns. While the datastore is stopped it defers the build to `start()`
instead (see the `stop()` thread). Both are in `BuildIndexesLifecycleSpec`.
##########
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:
Done in 848282548a: `stop()` interrupts the build before closing the client
and records that it was cut short, and `start()` builds a new executor, since
`shutdownNow()` is terminal, and runs the build again.
`BuildIndexesLifecycleSpec` blocks the first build in the hook, stops the
datastore, asserts the build is logged as abandoned rather than failed and that
the index is absent, then starts it again and waits for the index.
##########
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:
Done in 848282548a: the summary is logged whether the loop finishes or not —
`did not finish, stopping after Nms at X of Y domain class(es): …` at WARN, or
DEBUG when the build was abandoned for shutdown — and a synchronous build still
rethrows afterwards. `BuildIndexesUnfinishedSummarySpec` loses the connection
on the second of two entities, once in the background and once on the calling
thread.
--
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]
