codeconsole commented on code in PR #16208:
URL: https://github.com/apache/grails-core/pull/16208#discussion_r4067219470
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -574,19 +631,331 @@ public ConnectionSources<MongoClient,
MongoConnectionSourceSettings> getConnecti
}
/**
- * Builds the MongoDB index for this datastore
+ * Builds the MongoDB index for this datastore.
+ *
+ * <p>Each index is created by a command that the server answers only once
the index has been built,
+ * so with the default settings this blocks whoever creates the datastore
— in an application, the
+ * startup thread — for as long as MongoDB takes to build every declared
index. Enabling
+ * {@code grails.mongodb.buildIndexesAsync} hands the work to a background
thread and returns
+ * immediately instead.
*/
public void buildIndex() {
+ if (!buildIndexes) {
+ LOG.info("Index creation is disabled by [{} = false]. The indexes
declared by the domain classes " +
+ "will not be created or reconciled; the indexes already
present on the server are left untouched.",
+ MongoSettings.SETTING_BUILD_INDEXES);
+ return;
+ }
+ ExecutorService executor = this.indexBuildExecutor;
+ if (executor == null) {
+ runIndexBuild(null);
+ return;
+ }
+ if (!executor.isShutdown()) {
+ LOG.info("Building the indexes declared by the domain classes on a
background thread ([{} = true]). " +
+ "Startup does not wait for them, so a query issued before
its index exists is served without it.",
+ MongoSettings.SETTING_BUILD_INDEXES_ASYNC);
+ try {
+ executor.execute(() -> runIndexBuild(executor));
+ return;
+ }
+ catch (RejectedExecutionException e) {
+ // Shut down between the check and the submission.
+ }
+ }
+ if (closed) {
+ LOG.warn("An index build was requested after the datastore was
closed, so it was not started.");
+ }
+ else {
+ indexBuildPending = true;
+ LOG.info("An index build was requested while the datastore is
stopped; it will run when the datastore is restarted.");
+ }
+ }
+
+ /**
+ * Runs one build and reports it, whether it finished or not.
+ *
+ * @param executor the executor running it, or {@code null} for a build on
the caller's thread, whose
+ * failure is left to propagate to the caller
+ */
+ private void runIndexBuild(ExecutorService executor) {
+ long startedAt = System.nanoTime();
+ // Telling a created index from one that was already there costs a
listIndexes per indexed
+ // collection, and its only product is the summary, so it is skipped
when that would not be logged.
+ IndexBuildSummary summary = new IndexBuildSummary(LOG.isInfoEnabled());
+ Exception failure = null;
+ try {
+ buildDeclaredIndexes(summary);
+ }
+ catch (Exception e) {
+ // An Error is left to propagate as it is, without the summary.
+ failure = e;
+ if (e instanceof InterruptedException) {
+ // Caught here rather than by whoever interrupted, so the flag
it cleared is put back.
+ Thread.currentThread().interrupt();
+ }
+ }
+ long elapsedMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() -
startedAt);
+ if (failure == null) {
+ logFinishedIndexBuild(summary, elapsedMillis);
+ return;
+ }
+ if (executor == null) {
+ logUnfinishedIndexBuild(summary, elapsedMillis, false);
+ // Unchanged, checked or not: an initializeIndices override may
throw one without declaring it.
+ MongoDatastore.<RuntimeException>rethrow(failure);
+ return;
+ }
+ // Nothing is waiting on this thread, so an error that would have
failed startup has to be
+ // reported here or it is lost entirely.
+ boolean abandoned = executor.isShutdown() ||
Thread.currentThread().isInterrupted();
+ if (abandoned) {
+ indexBuildPending = true;
+ // toString rather than the message: an interrupted driver call
can arrive wrapped in
+ // an exception that carries no message of its own.
+ LOG.debug("The background index build was abandoned because the
datastore is shutting down: {}",
+ failure.toString(), failure);
+ }
+ else {
+ LOG.error("The background index build failed: {}. The application
is running without the " +
+ "indexes that were not created.", failure.getMessage(),
failure);
+ }
+ logUnfinishedIndexBuild(summary, elapsedMillis, abandoned);
+ }
+
+ @SuppressWarnings("unchecked")
+ private static <E extends Exception> void rethrow(Exception failure)
throws E {
+ throw (E) failure;
+ }
+
+ private void logFinishedIndexBuild(IndexBuildSummary summary, long
elapsedMillis) {
+ if (summary.applied() == 0 && summary.recreated == 0 &&
summary.failures == 0) {
+ LOG.debug("No indexes are declared by the {} domain class(es)
mapped to database [{}]",
+ summary.entities, defaultDatabase);
+ return;
+ }
+ if (summary.failures == 0) {
+ LOG.info("Index build for database [{}] finished in {}ms: {}, from
{} domain class(es)",
+ defaultDatabase, elapsedMillis, summary.describe(),
summary.entities);
+ }
+ else {
+ LOG.warn("Index build for database [{}] finished in {}ms: {}, from
{} domain class(es). " +
+ "The failures are reported above.",
+ defaultDatabase, elapsedMillis, summary.describe(),
summary.entities);
+ }
+ }
+
+ /**
+ * Reports how far a build got before it stopped, which is what an
operator needs when a background
+ * build fails partway: the error names the cause, this says what was
applied before it.
+ */
+ private void logUnfinishedIndexBuild(IndexBuildSummary summary, long
elapsedMillis, boolean abandoned) {
+ String message = "Index build for database [{}] did not finish,
stopping after {}ms at {} of {} domain class(es): {}";
+ if (abandoned) {
+ LOG.debug(message, defaultDatabase, elapsedMillis,
summary.entities, summary.entitiesTotal, summary.describe());
+ }
+ else {
+ LOG.warn(message, defaultDatabase, elapsedMillis,
summary.entities, summary.entitiesTotal, summary.describe());
+ }
+ }
+
+ /**
+ * Creates and reconciles the indexes declared by every entity mapped to
this datastore. MongoDB answers
+ * each {@code createIndex} only once the index exists, so the time this
takes is the time the caller —
+ * startup, or the background build thread — actually spends waiting.
+ */
+ private void buildDeclaredIndexes(IndexBuildSummary summary) {
+ List<PersistentEntity> entities = new ArrayList<>();
for (PersistentEntity entity :
this.mappingContext.getPersistentEntities()) {
// Only create Mongo templates for entities that are mapped with
Mongo
- if (!entity.isExternal()) {
- if (entity.isMultiTenant() && multiTenancyMode ==
MultiTenancySettings.MultiTenancyMode.SCHEMA) continue;
-
+ if (!entity.isExternal() &&
+ !(entity.isMultiTenant() && multiTenancyMode ==
MultiTenancySettings.MultiTenancyMode.SCHEMA)) {
+ entities.add(entity);
+ }
+ }
+ summary.entitiesTotal = entities.size();
+ IndexBuildSummary previousSummary = indexBuildSummary.get();
+ indexBuildSummary.set(summary);
+ try {
+ for (PersistentEntity entity : entities) {
initializeIndices(entity);
+ summary.entities++;
+ }
+ }
+ finally {
+ if (previousSummary == null) {
+ indexBuildSummary.remove();
+ }
+ else {
+ indexBuildSummary.set(previousSummary);
}
}
}
+ /**
+ * The indexes a collection already had when the build reached it, listed
once on first use and then
+ * reused. {@code createIndex} is idempotent and answers the same way
whether or not it had to build
+ * anything — the driver hands back only the index name, discarding the
{@code numIndexesBefore} /
+ * {@code numIndexesAfter} the server reports — so what was there
beforehand is what distinguishes an
+ * index this build created from one it merely confirmed.
+ *
+ * <p>Listed lazily so that an entity declaring no indexes costs no round
trip, and not at all when the
+ * summary is not being classified. Successful changes are recorded so
later declarations on the same
+ * keys see the current name and TTL, and are not counted as new indexes.
A conflict re-lists instead
+ * of trusting the snapshot: whatever the server reports as conflicting
may have appeared since.
+ */
+ private static final class ExistingIndexes {
+
+ private final com.mongodb.client.MongoCollection<Document> collection;
+
+ private final IndexBuildSummary summary;
+
+ private List<Document> indexes;
+
+ private boolean listed;
+
+ /** Why the last listing failed, for the conflict it leaves
unreconciled to report. */
+ private RuntimeException listingFailure;
+
+ private ExistingIndexes(com.mongodb.client.MongoCollection<Document>
collection, IndexBuildSummary summary) {
+ this.collection = collection;
+ this.summary = summary;
+ }
+
+ /**
+ * @return the known current indexes, or {@code null} if they could
not be listed
+ */
+ private List<Document> get() {
+ return listed ? indexes : refresh();
+ }
+
+ /**
+ * Lists the indexes from the server now, replacing what was known.
+ *
+ * @return the current indexes, or {@code null} if they could not be
listed
+ */
+ private List<Document> refresh() {
+ listed = true;
+ try {
+ indexes = collection.listIndexes().into(new ArrayList<>());
+ listingFailure = null;
+ } catch (RuntimeException e) {
+ // Not fatal: the build can still create indexes, it just
cannot report which of them
+ // were new. Losing the breakdown is not worth failing a
startup over.
+ LOG.debug("Could not list the existing indexes of collection
[{}]: {}",
+ collection.getNamespace().getCollectionName(),
e.getMessage(), e);
+ indexes = null;
+ listingFailure = e;
+ summary.classified = false;
Review Comment:
Fixed in c6a64e8d1e, the way you describe. Classification is now per
collection: `ExistingIndexes` has its own `readable` flag, which any failed
listing of that collection clears, whether its first or the re-listing after a
conflict. `contains()` then answers "not known" for that collection alone. The
summary's own flag now only records whether classification was asked for at all.
For the partial case the summary gains a third count, so your example reads
`2 created, 5 already present, 3 applied without a listing`. When nothing could
be classified — the summary not being logged, or no collection readable — it
keeps the existing `N index declaration(s) applied` wording.
`BuildIndexesPartialClassificationSpec` covers both failures at once. One
collection lists normally, one refuses its listing, and a third lists once and
then loses the re-listing its conflict makes; the result is `1 created, 0
already present, 1 applied without a listing, 1 failed`.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -574,19 +631,331 @@ public ConnectionSources<MongoClient,
MongoConnectionSourceSettings> getConnecti
}
/**
- * Builds the MongoDB index for this datastore
+ * Builds the MongoDB index for this datastore.
+ *
+ * <p>Each index is created by a command that the server answers only once
the index has been built,
+ * so with the default settings this blocks whoever creates the datastore
— in an application, the
+ * startup thread — for as long as MongoDB takes to build every declared
index. Enabling
+ * {@code grails.mongodb.buildIndexesAsync} hands the work to a background
thread and returns
+ * immediately instead.
*/
public void buildIndex() {
+ if (!buildIndexes) {
+ LOG.info("Index creation is disabled by [{} = false]. The indexes
declared by the domain classes " +
+ "will not be created or reconciled; the indexes already
present on the server are left untouched.",
+ MongoSettings.SETTING_BUILD_INDEXES);
+ return;
+ }
+ ExecutorService executor = this.indexBuildExecutor;
+ if (executor == null) {
+ runIndexBuild(null);
+ return;
+ }
+ if (!executor.isShutdown()) {
+ LOG.info("Building the indexes declared by the domain classes on a
background thread ([{} = true]). " +
+ "Startup does not wait for them, so a query issued before
its index exists is served without it.",
+ MongoSettings.SETTING_BUILD_INDEXES_ASYNC);
+ try {
+ executor.execute(() -> runIndexBuild(executor));
+ return;
+ }
+ catch (RejectedExecutionException e) {
+ // Shut down between the check and the submission.
+ }
+ }
+ if (closed) {
+ LOG.warn("An index build was requested after the datastore was
closed, so it was not started.");
+ }
+ else {
+ indexBuildPending = true;
+ LOG.info("An index build was requested while the datastore is
stopped; it will run when the datastore is restarted.");
+ }
+ }
+
+ /**
+ * Runs one build and reports it, whether it finished or not.
+ *
+ * @param executor the executor running it, or {@code null} for a build on
the caller's thread, whose
+ * failure is left to propagate to the caller
+ */
+ private void runIndexBuild(ExecutorService executor) {
+ long startedAt = System.nanoTime();
+ // Telling a created index from one that was already there costs a
listIndexes per indexed
+ // collection, and its only product is the summary, so it is skipped
when that would not be logged.
+ IndexBuildSummary summary = new IndexBuildSummary(LOG.isInfoEnabled());
+ Exception failure = null;
+ try {
+ buildDeclaredIndexes(summary);
+ }
+ catch (Exception e) {
+ // An Error is left to propagate as it is, without the summary.
+ failure = e;
+ if (e instanceof InterruptedException) {
+ // Caught here rather than by whoever interrupted, so the flag
it cleared is put back.
+ Thread.currentThread().interrupt();
+ }
+ }
+ long elapsedMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() -
startedAt);
+ if (failure == null) {
+ logFinishedIndexBuild(summary, elapsedMillis);
+ return;
+ }
+ if (executor == null) {
+ logUnfinishedIndexBuild(summary, elapsedMillis, false);
+ // Unchanged, checked or not: an initializeIndices override may
throw one without declaring it.
+ MongoDatastore.<RuntimeException>rethrow(failure);
+ return;
+ }
+ // Nothing is waiting on this thread, so an error that would have
failed startup has to be
+ // reported here or it is lost entirely.
+ boolean abandoned = executor.isShutdown() ||
Thread.currentThread().isInterrupted();
Review Comment:
Fixed in c6a64e8d1e. A build counts as abandoned only if the executor was
shut down and the failure is one the shutdown produces: an interruption
(`InterruptedException`, `MongoInterruptedException`), the driver's
`IllegalStateException` on the closed client, or a `MongoSocketException` from
a socket closed under a call in progress. The cause chain is walked, so an
interruption wrapped by a proxy still counts. Anything else is logged at ERROR
even with the executor down, and does not mark the build pending, so a restore
does not re-run a build that fails in its own right.
Of the failures you list, a bad index specification and a missing
`createIndex` privilege are `MongoCommandException`s, which
`createOrUpdateIndex` already catches and logs per index, so they never reached
this branch. A duplicate key on a `unique` index is not caught there, and is
the one this changes. `BuildIndexesBackgroundFailureSpec` has a `createIndex`
that ignores the interrupt, the datastore closed under it, and then fails with
a duplicate key: it is logged as a failure.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -574,19 +631,331 @@ public ConnectionSources<MongoClient,
MongoConnectionSourceSettings> getConnecti
}
/**
- * Builds the MongoDB index for this datastore
+ * Builds the MongoDB index for this datastore.
+ *
+ * <p>Each index is created by a command that the server answers only once
the index has been built,
+ * so with the default settings this blocks whoever creates the datastore
— in an application, the
+ * startup thread — for as long as MongoDB takes to build every declared
index. Enabling
+ * {@code grails.mongodb.buildIndexesAsync} hands the work to a background
thread and returns
+ * immediately instead.
*/
public void buildIndex() {
+ if (!buildIndexes) {
+ LOG.info("Index creation is disabled by [{} = false]. The indexes
declared by the domain classes " +
+ "will not be created or reconciled; the indexes already
present on the server are left untouched.",
+ MongoSettings.SETTING_BUILD_INDEXES);
+ return;
+ }
+ ExecutorService executor = this.indexBuildExecutor;
+ if (executor == null) {
+ runIndexBuild(null);
+ return;
+ }
+ if (!executor.isShutdown()) {
+ LOG.info("Building the indexes declared by the domain classes on a
background thread ([{} = true]). " +
Review Comment:
Both fixed in c6a64e8d1e. Every line `buildIndex()` logs now names the
connection — the background announcement, the disabled message, and the stopped
and closed ones. The disabled message names the setting as an operator would
look for it: `grails.mongodb.buildIndexes` for the default connection, and
`grails.mongodb.connections.<name>.buildIndexes (or
grails.mongodb.buildIndexes)` for a named one, since a connection inherits the
top-level value when it does not declare its own. The announcement is now
logged after `execute` returns, so it only ever describes a build that was
submitted. `BuildIndexesPerConnectionSpec` asserts the connection name on a
named connection's announcement.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -166,6 +174,38 @@ public class MongoDatastore extends AbstractDatastore
implements MappingContext.
protected final boolean stateless;
protected final boolean codecEngine;
protected final boolean transactionsEnabled;
+ protected final boolean buildIndexes;
+ protected final boolean buildIndexesAsync;
+
+ /**
+ * Runs the startup index build off the thread that creates the datastore
when
+ * {@code grails.mongodb.buildIndexesAsync} is enabled; {@code null}
otherwise. A single thread,
+ * so the indexes are still built one at a time per connection. The worker
expires after one idle
+ * second, releasing the worker while allowing subsequent calls to {@link
#buildIndex()}.
+ *
+ * <p>Not final: a shut down executor cannot be restarted, so {@link
#start()} replaces the one
+ * {@link #stop()} shut down.
+ */
+ private volatile ExecutorService indexBuildExecutor;
Review Comment:
Agreed. It is `volatile` in c6a64e8d1e, with a comment saying why.
##########
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:
Right, the guard only existed on the asynchronous path. Fixed in c6a64e8d1e:
`buildIndex()` checks `closed` before either mode runs, so a synchronous build
requested after `close()` gets the same warning as an asynchronous one, instead
of running against the client being closed. That also makes the two modes agree
for a direct `buildIndex()` call after `close()`, which they did not.
`BuildIndexesLifecycleSpec` pins the synchronous case.
##########
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:
Both fixed in c6a64e8d1e. `resumeIndexBuild()` returns straight away when
the executor was never shut down: a connection added while the datastore was
stopped submitted its build rather than deferring it, so there is nothing to
wait for or resume. The wait itself is now one second instead of ten, since a
build that is going to exit does so within milliseconds of the interrupt.
A build stuck in a call that ignores the interrupt still gets a second build
started alongside it after that second, as in your probe. That second is now
all it costs `start()`, and `createIndex` is idempotent.
`BuildIndexesPerConnectionSpec` adds a connection while stopped, lets its build
finish, and asserts that `start()` starts no second one for 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:
Thanks for the correction — you're right that `host`, `port`, `username` and
`password` survive the scrub, so the key used decides what a named connection
gets. The upgrade note says so in 0ffc9215e7: the connection details are the
exception to the settings that now apply, a connection under
`grails.mongodb.connections` without its own `url` falls back to those four
(`localhost:27017` unless they are set) rather than to `grails.mongodb.url`,
and each such connection should declare its own `url`.
##########
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:
Filed as #16366, in three parts: the named connections' clients `stop()`
leaves open, `start()` rebuilding only the default client, and the SCHEMA-mode
`AllTenantsResolver` still listing through the pre-checkpoint client.
--
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]
