matrei commented on PR #16208:
URL: https://github.com/apache/grails-core/pull/16208#issuecomment-5680524291
## AI Review
**Head:** `ee362fa1f8`
(`codeconsole:feature/mongodb-skip-index-build-8.0.x`) · **Base:** `8.0.x` at
`3067d0a855` · merge-base is clean, the branch is `8.0.x` plus the feature
commits and merges from `8.0.x`.
**Run locally (Docker, MongoDB via Testcontainers):**
- `./gradlew :grails-data-mongodb-core:cleanTest
:grails-data-mongodb-core:test --no-build-cache` for the nine new specs plus
`MongoConnectionSourceSettingsSpec`, `MongoStaticApiMultiTenancySpec`,
`TtlIndexSpec`, `TextIndexViaAttributesSpec`, `IndexWithInheritanceSpec`,
`IndexAttributesAndCompoundKeySpec`: 38 tests, 0 failures, result XML freshly
written.
- `./gradlew :grails-data-mongodb-core:cleanTest
:grails-data-mongodb-core:check --no-build-cache --continue` (whole module):
698 tests, 0 failures, 45 skipped, across 214 result files; `checkstyleMain`
and `codenarcMain` clean (the `*Test` variants are SKIPPED by the build config;
PMD and SpotBugs are not wired into this module's `check`). BUILD SUCCESSFUL.
- `./gradlew :grails-data-mongodb-docs:asciidoctor -x aggregateGroovydoc`:
BUILD SUCCESSFUL. Asciidoctor prints `possible invalid reference: queryIndexes`
for the new `<<queryIndexes,…>>` xref, but it prints the same INFO for the
pre-existing `advancedConfig`, `idGeneration` and `textSearch` xrefs; the
anchors are defined in `index.adoc` ahead of the includes, so the link resolves.
Overall this is a solid PR: the three features are cleanly separated, the
reporting is genuinely useful, and the test coverage of the failure and
degradation paths is unusually thorough. The findings below are mostly about
the multi-connection case, which the async design does not quite finish, and
about one behaviour change the description undersells.
### Findings
**1. Child datastores' index-build executors are never shut down (medium)**
`close()` (`MongoDatastore.java:1454`) calls `shutdownNow()` on *this*
datastore's `indexBuildExecutor` only. With more than one connection
configured, every child datastore in `datastoresByConnectionSource` is a full
`MongoDatastore` with its own executor and thread
(`gorm-mongo-index-build-<connection>`), and nothing ever closes the children —
`close()` does not iterate them, and `AbstractDatastore.destroy()` does not
know about them.
Two consequences when `buildIndexesAsync = true` and a non-default
connection is configured:
- If the application shuts down while a child's build is still in flight,
`connectionSources.close()` closes that child's client underneath it. The
child's executor is *not* shut down, so the catch block at
`MongoDatastore.java:614` takes the `else` branch and logs `The background
index build failed: ...` at **ERROR** — exactly the noise
`BuildIndexesBackgroundFailureSpec` guarantees does not happen, but only for
the default connection.
- After a child's build completes, its single-thread executor keeps its core
thread alive forever (see 3), so each non-default connection leaks one idle
daemon thread until the JVM exits.
Suggested fix: in `close()`, before `connectionSources.close()`, shut down
the executor of every value in `datastoresByConnectionSource` other than `this`
(a small `shutDownIndexBuild()` helper the children can share).
`BuildIndexesPerConnectionSpec` only exercises `buildIndexes`; a feature that
enables `buildIndexesAsync` with a second connection and closes the datastore
mid-build would pin this.
**2. The supplied-`MongoClient` fix also switches on multi-tenancy — say so
(medium, docs/release notes)**
`createDefaultConnectionSources` (`MongoDatastore.java:1526`) now binds the
settings from configuration instead of `new MongoConnectionSourceSettings()`.
The description lists `stateless`, `transactional`, `engine`, flush mode and
`decimalType` as the settings that were silently ignored on that path. The
bigger one is `multiTenancy`: with the bare settings, `multiTenancy.mode` was
`NONE`, and that value is what both `MongoDatastore.multiTenancyMode`
(`MongoDatastore.java:213`) and `GormStaticApi.resolveMultiTenancyMode`
(`grails-datamapping-core/.../GormStaticApi.groovy:108`, reading
`defaultConnectionSource.settings.multiTenancy.mode`) are driven by. So an
application on the Spring Boot auto-configuration path with a `MongoClient`
bean and a configured tenancy mode was running with tenant discrimination
effectively off at the datastore and static-API level; after this PR it is on.
That is the right outcome, but it is a security-relevant behaviour change,
not just a "`stateless = true` now takes effect" one. Please:
- name it in the release note bullet and in the PR description, and
- add a feature to `SuppliedMongoClientSettingsSpec` asserting
`datastore.multiTenancyMode` follows the configuration, so the fix cannot
regress silently the way the original bug did.
**3. The async executor's thread lives for the life of the application
(low)**
`Executors.newSingleThreadExecutor` (`MongoDatastore.java:223`) never times
out its core thread, so once the startup build is done
`gorm-mongo-index-build-DEFAULT` sits idle until `close()`. Daemon, so harmless
for JVM exit, but a permanent thread per connection for a task that runs once
at startup. A `ThreadPoolExecutor(0, 1, …)` with `allowCoreThreadTimeOut(true)`
and a short keep-alive, or a plain daemon `Thread` created per `buildIndex()`
call and remembered for interruption in `close()`, would release it. This also
shrinks the surface of finding 1.
**4. The protected `initializeIndices(PersistentEntity)` hook is bypassed by
the startup build (low, API)**
Before this PR `buildIndex()` called the protected
`initializeIndices(entity)`; a subclass overriding it customised index creation
everywhere. Now `buildDeclaredIndexes()` (`MongoDatastore.java:642`) calls the
private `initializeIndices(entity, summary)` directly, so an override is
honoured only on `persistentEntityAdded`. Nothing in-tree overrides it, so this
is a note rather than a blocker: either make the two-argument overload the
protected one (with `IndexBuildSummary` package-visible), or accept the
narrowing and say so in the Javadoc of the one-argument method.
**5. The created / already-present split double-counts a key pattern
declared twice on one entity (low, reporting only)**
`ExistingIndexes` is a snapshot taken before the entity's first
`createIndex` and is never updated. `name index: true` together with
`compoundIndex name: 1` both produce `{name: 1}`; the second call is idempotent
on the server but is counted as *created* (`MongoDatastore.java:1211`). The
same staleness means that after a `recreateOnConflict` drop-and-recreate, a
later declaration on the same keys would look up the old index name. Recording
each key pattern this build created (or recreated) into the snapshot after a
successful `createIndex` fixes both, and costs nothing.
**6. Documentation nits (low)**
- `queryIndexes.adoc`: "on a single daemon thread per connection … so
enabling this does not launch several index builds against the server at once"
is true per connection only. With three connections against one server there
are three concurrent builders. Worth one clause.
- `advancedConfig.adoc`: the NOTE that `grails.mongodb.*` keys are not
relaxed-bound (`database-name` is ignored) is a claim no test in this PR backs.
I believe it is right for the `PropertyResolver` path the builder uses, but it
is unrelated to the feature and would be better either backed by a tiny feature
in `MongoConnectionSourceSettingsSpec` or dropped from this PR.
- The summary's "from N domain class(es)" counts every entity mapped to the
connection, including those declaring no index at all (`summary.entities++`
runs before `initializeIndices`). The doc wording "how many domain classes it
covered" is consistent with that, so fine, but "2 created, 0 already present,
from 40 domain class(es)" may surprise a reader who expects 40 to be the number
with indexes.
### Verified as correct
- **Settings.** `buildIndexes` defaults to `true`, `buildIndexesAsync` to
`false`; both bind through `MongoConnectionSourceSettingsBuilder` and resolve
per connection (`BuildIndexesPerConnectionSpec` covers global-off /
connection-on and inheritance). Constants and Javadoc on `MongoSettings` and
`AbstractMongoConnectionSourceSettings` are consistent with the docs.
- **`buildIndexes = false`.** No `createIndex`/`collMod` is issued:
`buildIndex()` returns early (`MongoDatastore.java:594`) and
`initializeIndices` guards `persistentEntityAdded` too
(`MongoDatastore.java:1111`). Queries and writes are unaffected. Verified
against the container with `BuildIndexesDisabledSpec` and its control
`BuildIndexesEnabledByDefaultSpec`.
- **Async path, default connection.** One single-thread executor named after
the connection, daemon thread, created only when both flags are on. `close()`
calls `shutdownNow()` *before* `super.destroy()` and the client close, so an
interrupted build is classified as shutdown (DEBUG) rather than failure
(ERROR); an uninterrupted failure is logged at ERROR. Both branches are
exercised by `BuildIndexesBackgroundFailureSpec` (the proxy-based
`FailingMongoClient` is a neat way to get there without mocking the driver).
`CommandListener` runs on the calling thread in the sync driver, so
`BuildIndexesAsyncSpec`'s thread assertions are sound.
- **Reporting.** Lazy `listIndexes` per entity, reused by the conflict path
(which used to list on its own); on an unreadable list `classified` flips and
the summary falls back to "N index declaration(s) applied", while the conflict
that cannot be reconciled blind is still logged at ERROR and counted as failed
(`BuildIndexesUnreadableIndexListSpec`). `reconcileIndexConflict` now returns
whether the index ended in the declared state and the counts follow it. Summary
at INFO on success, WARN with a failure count otherwise
(`BuildIndexesFailureSummarySpec`, `BuildIndexesSummaryLogSpec`). Per-index
DEBUG line goes through the `AbstractDatastore` logger, so the documented
`org.grails.datastore.mapping.core: DEBUG` does enable it.
- **Supplied client.** `databaseName` is still overridden from the mapping
context after binding; the `MongoClientSettings.Builder`/credentials the
builder may populate are unused on this path, as the description says.
`SuppliedMongoClientSettingsSpec` proves the bound settings take effect, not
merely report.
- **Test logging setup.** `logback-classic` replaces `slf4j-nop` on the test
classpath only (`compileOnly slf4j-nop` for main is untouched);
`logback-test.xml` pins root to WARN and every log-asserting spec raises and
then restores the level of the one logger it touches and detaches its appender.
No other module consumes `grails-data-mongodb-core`'s test output, so the
config file does not leak. Specs filter captured events by database name, so
sharing one JVM fork with other specs is safe.
- **Docs.** Release notes are under the `8.0` heading; `queryIndexes.adoc`
and `advancedConfig.adoc` cross-reference each other and show both YAML and
Groovy forms.
- **CI.** Codecov reports 96 % patch coverage; the single TestLens failure
is the known-flaky scaffolding `UserControllerSpec > User list` (#16030),
unrelated to this change.
--
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]
