codeconsole commented on code in PR #16208:
URL: https://github.com/apache/grails-core/pull/16208#discussion_r4066327100
##########
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:
Fixed in d8adb08fcf, the way you suggested. The in-flight counter is gone. A
build that is running when `stop()` interrupts it records that it was cut short
as it exits, from the abandoned branch, so one that finishes normally records
nothing; a build still queued is picked up from `shutdownNow()`. `start()`
waits for the interrupted worker to have exited, up to ten seconds, before it
decides, which also covers @matrei's note that `stop()` does not wait.
`BuildIndexesLifecycleSpec` has a build that has applied everything and is on
its way out when `stop()` runs, and does not respond to the interrupt;
`start()` then starts no second build. The counter would have re-run it.
##########
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:
I've left this one. Keeping the URL and rewriting its database has a trap: a
connection string's database path is also the default authentication database
whenever the URL gives no `authSource`. A named connection inheriting
`mongodb://user:pass@prod-host/admin` rewritten to `/app` would start
authenticating against `app`. Doing it safely means adding `authSource` when
the credentials need it, which is more connection-string surgery than this PR
should carry.
The other route, taking the datastore's database from the mapping context,
does not work either: per-connection children share the parent's mapping
context, so every named connection would get the parent's database.
As you say, it is not a regression — a named connection without its own
`url` on this path fell back to the defaults before as well — and giving it its
own `url` works on both sides of this PR. I'd rather leave the scrub, which
does what the comment says, than trade it for an authentication change nobody
asked for.
##########
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:
Right. `<n>` counts within one executor, and `start()` makes a new one.
Fixed in d8adb08fcf: the guide now just gives an example name,
`gorm-mongo-index-build-default-1`.
##########
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:
Done in 0a12094fa4.
##########
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:
Taken in d8adb08fcf: `stop()` and `start()` now cover every connection, like
`close()`. `stop()` interrupts each connection's build before closing the
client, and `start()` gives each a fresh executor and re-runs a build that was
cut short, or requested while stopped. `BuildIndexesPerConnectionSpec` blocks a
named connection's `createIndex`, stops the datastore, and asserts the child's
build is abandoned as a shutdown with no error on its worker. It then starts
the datastore and waits for the child's index.
This interrupts the child's build; it does not close the child's client.
`stop()` closes only the default connection's client, as it did before this PR,
so a checkpoint with named connections still sees their sockets. That is the
CRaC support's behaviour for named connections rather than something the index
build introduced, so I've kept it out of this PR; it is worth an issue of its
own.
--
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]
