matrei commented on PR #16208:
URL: https://github.com/apache/grails-core/pull/16208#issuecomment-5696438605
## Review round 2
**Head:** `211ef8de17`
(`codeconsole:feature/mongodb-skip-index-build-8.0.x`) · **Base:** `8.0.x`,
merge-base still `3067d0a855`; `origin/8.0.x` has moved on to `37f8ca6377` (51
commits). `git merge-tree` of head against `origin/8.0.x` is conflict-free, and
CI ran on that merge revision. This round covers the two new commits,
`1726f84d3a` (lifecycle and review findings) and `211ef8de17` (CI logging
assertions, `LocalMongoUnitSpec`).
**Run locally (Docker, MongoDB via Testcontainers):**
- `./gradlew :grails-data-mongodb-core:cleanTest
:grails-data-mongodb-core:check --no-build-cache --continue` (whole module):
714 tests, 0 failures, 0 errors, 45 skipped, across 215 result files written
minutes before this note (up from 698 in round 1: `BuildIndexesHookSpec` 2,
`BuildIndexesPerConnectionSpec` 4, `SuppliedMongoClientSettingsSpec` 12,
`BuildIndexesAsyncSpec` 2, `TextIndexViaAttributesSpec` 3,
`MongoConnectionSourceSettingsSpec` 5, all green); `checkstyleMain` and
`codenarcMain` clean. BUILD SUCCESSFUL in 4m 16s.
- `./gradlew :grails-test-examples-mongodb-base:cleanTest
:grails-test-examples-mongodb-base:test --tests
functional.tests.LocalMongoUnitSpec --no-build-cache`: both features pass,
including the newly activated `test fail on error`. BUILD SUCCESSFUL.
- `./gradlew :grails-data-mongodb-docs:asciidoctor -x aggregateGroovydoc`:
BUILD SUCCESSFUL; the `possible invalid reference` INFO lines are the same
pre-existing ones as in round 1.
- Not run: the merged tree (head + `origin/8.0.x`). CI's merge-revision jobs
are green, and finding 1 below is derived from reading that merged file, not
from executing it.
All six findings from round 1 are addressed, and the fixes are the ones I
would have written: children's executors are interrupted from the parent's
`close()`, the executor is a `ThreadPoolExecutor(0, 1, 1s)` so the worker exits
when idle, the protected `initializeIndices(PersistentEntity)` hook is back on
the startup path with the summary carried in a `ThreadLocal`, the index
snapshot is refreshed after every successful `createIndex` / `collMod` /
recreate, the multi-tenancy change is called out, and every one of them has a
test. What remains is mostly a consequence of the branch being 51 commits
behind `8.0.x`, plus the supplied-client fix having grown wider than the
release note says.
### Findings
**1. The documented logger category stops working the moment this is merged
(medium, docs)**
`queryIndexes.adoc:225-231` (and the PR description) tell the reader to
enable `org.grails.datastore.mapping.core: DEBUG` to get the per-index line. On
this branch that is right, because `MongoDatastore` logs through the `LOG` that
`AbstractDatastore` declares. On `origin/8.0.x`, `MongoDatastore` has had its
own `private static final Logger LOG =
LoggerFactory.getLogger(MongoDatastore.class)` since #16338 went in, and the
clean merge keeps it (merged file, line 160): the per-index `DEBUG` line, the
summary `INFO`/`WARN` line and the "abandoned because the datastore is shutting
down" line all move to `org.grails.datastore.mapping.mongo.MongoDatastore`.
`211ef8de17` widened the five log-asserting specs to
`org.grails.datastore.mapping` precisely because of this, but the documentation
was left on the old category, so a user following it after the merge sees
nothing.
Suggested fix: merge `8.0.x` into the branch (it is clean) and change the
doc snippet and the PR description to `org.grails.datastore.mapping.mongo` (or
the class name). The `logback-test.xml` root at `WARN` is unaffected.
**2. The supplied-client fix now also applies every `grails.gorm.*` setting
— the upgrade note only mentions multi-tenancy (medium, docs/release notes)**
Round 1 asked for the multi-tenancy switch-on to be named; it is, in
`releaseNotes.adoc` and the description. But `1726f84d3a` went further than
that: `buildConnectionSourceSettings()` (`MongoDatastore.java:1602`) now seeds
the settings from `new ConnectionSourceSettingsBuilder(configuration).build()`,
i.e. the whole `grails.gorm` block becomes the fallback. That is correct — it
is exactly what `AbstractConnectionSourceFactory.create` does for a
GORM-created client — but it means the following also change for a Spring Boot
application handing GORM its `MongoClient` bean:
- `grails.gorm.failOnError: true` now makes `save()` throw
`ValidationException`. `211ef8de17` proves this is user-visible:
`LocalMongoUnitSpec."test fail on error"` in
`grails-test-examples/mongodb/base` had been `@PendingFeature(reason = 'A
ValidationException is not thrown')` since it was written, and now passes. An
application that had `failOnError` on in configuration but was running for
years without it will start throwing on the first invalid save after upgrading.
- `grails.gorm.default.mapping`, `grails.gorm.default.constraints`,
`autoFlush`, `markDirty`, `flushMode` — all previously ignored on this path,
all applied now.
- Multi-tenancy needs one more sentence. On the supplied-client path there
is no `MongoConnectionSourceFactory`, so the `@Autowired(required = false)
setTenantResolver` that injects a `TenantResolver` *bean* on the factory path
never runs; `MongoDbGormAutoConfiguration` does not autowire one either. The
only way to supply a resolver on this path is
`grails.gorm.multiTenancy.tenantResolverClass`. An application that configured
`mode: DISCRIMINATOR` and registered a `TenantResolver` bean was, before this
PR, silently in `NONE` mode; after it, it is in `DISCRIMINATOR` mode with
`NoTenantResolver`, whose `resolveTenantIdentifier()` throws
`TenantNotFoundException` on every tenant-scoped operation. The release note's
"review their tenant resolver" should say that concretely: set
`tenantResolverClass`, a bean is not enough here.
`origin/8.0.x` now has `introduction/upgradeNotes.adoc` with a section
"MongoClient Lifecycle for an Externally-Supplied Client" — the same audience.
I would put a sibling section there ("Configuration is now applied to an
externally-supplied client") listing the above, and keep the release-note
bullet short. On @jdaugherty's 8.1 comment: the two index-build settings are
additive and can land in either release, but this part is a fix to what
existing configuration means at runtime, and 8.0.0 is unreleased and already
carries upgrade notes for supplied clients. Shipping it there, with the note,
is less surprising than a 8.1 in which `failOnError` and multi-tenancy start
working on the Spring Boot path. If the PR is retargeted, the supplied-client
fix is worth splitting out and keeping in 8.0.
**3. `close()` iterates a plain `LinkedHashMap` that the connection-sources
listener still writes to (low)**
`datastoresByConnectionSource` (`MongoDatastore.java:191`) is a
`LinkedHashMap`, and the new loop in `close()` (`MongoDatastore.java:1495`)
iterates its `values()` while the `ConnectionSourcesListener` registered in the
constructor can still `put` a child for a runtime `addConnectionSource`. A
concurrent add during shutdown is unlikely, but if it happens the
`ConcurrentModificationException` escapes `close()` before `super.destroy()`
and `connectionSources.close()` run, so nothing gets closed. Iterating a
snapshot (`new ArrayList<>(datastoresByConnectionSource.values())`) closes that
gap at no cost. The map's thread-safety is pre-existing, so this is a hardening
note, not a regression.
**4. Codecov patch coverage dropped from 96 % to 90 % (low, informational)**
Codecov now reports 8 missing and 4 partial lines in `MongoDatastore.java`.
The upload comes from the CI merge revision, so its line attribution against
the head file is shifted (it marks Javadoc lines as missed), and I did not
chase it. The plausibly uncovered new branches are the `previousSummary !=
null` restore in `buildDeclaredIndexes()` (a nested build, which nothing
exercises) and the `catch` of a failed recreate in `reconcileIndexConflict`.
Neither needs a test; noting it so the number is not read as a regression.
### Verified as correct
- **Round-1 finding 1 (child executors).** `close()` calls
`shutDownIndexBuild()` on itself and on every child before `super.destroy()`
and `connectionSources.close()`, so a child's in-flight build is interrupted
before its client is closed underneath it, and the catch at
`MongoDatastore.java:621` classifies it as shutdown.
`BuildIndexesPerConnectionSpec."test closing interrupts a child index build
added at runtime"` pins both the configured-at-startup and
`addConnectionSource` cases, blocks the child's `createIndex` with a latch via
the proxy client, and asserts the `DEBUG` "abandoned" line and the absence of
any `ERROR` on the `gorm-mongo-index-build-indexedAsync` thread. Closing the
parent twice (once in the feature, once in `cleanup`) is safe:
`MongoClient.close()` and `shutdownNow()` are idempotent and
`gormEnhancer.close()` is guarded.
- **Round-1 finding 3 (idle thread).** `ThreadPoolExecutor(0, 1, 1, SECONDS,
LinkedBlockingQueue)` with core size 0: the first `execute` enqueues and starts
a non-core worker, which times out after one idle second; a later
`buildIndex()` starts a fresh one. `BuildIndexesAsyncSpec` now joins the
startup worker, asserts it is dead, triggers another build and asserts a
second, distinct worker ran it. After `shutdownNow()` a further `buildIndex()`
gets `RejectedExecutionException` exactly as before.
- **Round-1 finding 4 (protected hook).** `buildDeclaredIndexes()` sets a
per-datastore `ThreadLocal<IndexBuildSummary>` around the loop and calls the
protected one-argument `initializeIndices`, which picks the summary up (or
creates a throwaway one on the `persistentEntityAdded` path). The `finally`
restores a previous value for a nested build and removes it otherwise, so the
pooled worker carries nothing over. `BuildIndexesHookSpec` overrides the hook
in a subclass and checks the extra index is present for a startup entity, sync
and async, and for one registered later.
- **Round-1 finding 5 (stale snapshot).** `record()` replaces the entry with
the same key pattern (text indexes included, via `findIndexByKeyPattern`'s
special case) with a synthetic `{key, name, expireAfterSeconds}` document,
which is all `reconcileIndexConflict` reads. `BuildIndexesSummaryLogSpec`
declares `name index: true` and `compoundIndex name: 1` on one entity and
expects `3 created, 1 already present`; `TextIndexViaAttributesSpec."Test
repeated text declarations reconcile against the latest index name"` recreates
a text index twice and ends on the last declared name. The early return when
the list was unreadable is right: `get()` would list from the server later
anyway, and a lazily unlisted snapshot is not the same as an unreadable one.
- **Round-1 finding 2 (multi-tenancy on a supplied client).**
`SuppliedMongoClientSettingsSpec` now covers every `MultiTenancyMode` under
both `grails.gorm` and `grails.mongodb` prefixes, the mongo prefix overriding
the gorm fallback, and an end-to-end discriminator isolation check through
`withTenant`. `multiTenancyMode` and
`defaultConnectionSource.settings.multiTenancy.mode` are asserted together,
which is what `GormStaticApi.resolveMultiTenancyMode` reads.
- **Round-1 finding 6 (docs).** Per-connection concurrency, the one-second
worker release, the domain-class count and the duplicate-declaration count are
all now stated in `queryIndexes.adoc`. The camel-case-only claim in
`advancedConfig.adoc` is backed by `MongoConnectionSourceSettingsSpec."test
MongoDB setting names require the documented camel case spelling"`.
- **Scope of `buildConnectionSourceSettings()`.** It is used by
`createDefaultConnectionSources` and by `createMappingContext(PropertyResolver,
Class...)`, and the latter is reached only from the `MongoClient`-taking
constructors (`MongoDatastore.java:353-394`); every `PropertyResolver`-only
constructor goes through `ConnectionSourcesInitializer` and
`createMappingContext(ConnectionSources, ...)`, whose settings already carried
the fallback. So the mapping context and the connection source now agree on the
supplied-client path and nothing changes on the others.
- **Logger widening in tests.** `org.grails.datastore.mapping` is a parent
of both `...mapping.core.AbstractDatastore` and
`...mapping.mongo.MongoDatastore`, every spec still filters by database name or
thread name, and each restores the level and detaches its appender in
`cleanupSpec`/`cleanup`.
- **`LocalMongoUnitSpec`.** `MongoSpec` hands the `EmbeddedMongoClient` to
`new MongoDatastore(mongoClient, config, ...)`, the test app's
`application.yml` sets `grails.gorm.failOnError: true`, and `GormEnhancer`
reads `settings.isFailOnError()` from the default connection source. Dropping
`@PendingFeature` is the right call; it is the regression test for the fallback.
- **CI.** The MongoDB matrix, coverage and Groovy canary jobs are green on
`211ef8de17`; the single TestLens failure is the known-flaky scaffolding
`UserControllerSpec > User list` (#16030), unrelated.
--
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]
