-
[OPIK-4933] [BE] feat: add workspaces metadata table for version, first-trace, migration state (#6615)
frostbyte_neo 发布于
2026-05-08 10:10:01 +00:00 - [OPIK-4933] [BE] feat: add workspaces metadata table and persistence layer
New per-workspace state table consolidates three previously ad-hoc workarounds:
- Workspace V1/V2 version (was Redis-only, 5-min TTL).
- First-trace analytics dedup (was Redis, 30-day TTL).
- Migration trapped-workspace flag (was process-local in-memory set).
The schema is intentionally generic (NULLable feature columns, audit columns)
so it can grow into the long-term workspace metadata table without further
migrations. Mutations go through idempotent upserts; first-trace uses a
single-statement upsert + read-back-equality to give correct first-writer-wins
semantics under concurrency without depending on the connector's
useAffectedRows flag or the lock-prone two-step pattern.- [OPIK-4933] [BE] feat: persist V1/V2 determinations and emit analytics event
After every successful version determination (cache-miss, not allowlist /
forced-version override), upsert the result into the workspaces table and
emit a workspace_version_determined analytics event with previous/new version
and a version_changed boolean. Both side effects are fire-and-forget on the
boundedElastic scheduler and never propagate to the response, mirroring the
existing cacheResult pattern. Cache hits still bypass both side effects so
subsequent reads do not pollute the data.- [OPIK-4933] [BE] refactor: migrate first-trace dedup from Redis to MySQL
The Redis-backed firstTraceCreatedDedupTtl key (30-day TTL) is replaced with
the workspaces.first_trace_reported_at column. Dedup is now permanent: a
workspace's first trace is forever its first trace, and the state survives
JVM restarts and is visible from outside the running process.Removes the analytics.firstTraceCreatedDedupTtl config key and its YAML
defaults; existing event-emission paths (BI event + analytics event) are
unchanged.- [OPIK-4933] [BE] refactor: replace in-memory trapped-workspace set with table
The static TRAPPED_WORKSPACE_IDS set is removed; trapped workspaces are now
persisted via workspaces.migration_skipped_at and the cycle-start exclusion
list and cycleTrappedWorkspaces gauge read from the table.Behavior change worth calling out: the skip is now permanent until a deleted
project is restored or an operator clears the row. The previous
reset-on-JVM-restart behavior was a workaround artifact, not a feature.- [OPIK-4933] [BE] refactor: address review feedback on workspaces persistence
- persistAndEmit is now truly fire-and-forget via Schedulers.boundedElastic().schedule(...)
instead of being chained through .flatMap, so the workspace-version response no longer
waits on the DB write or the analytics call. Errors are caught inside the scheduled
runnable so nothing propagates. - Single Instant.now() snapshot reused for both the upsert timestamp and the analytics
event "date" property. - DAO naming consistency: markMigrationSkipped renamed to upsertMigrationSkipped to
match the other upsert verbs. - Javadoc additions: persistAndEmit now documents the auth one-way V2 gate behaviour
(it persists, intentionally, since the auth gate is the authoritative source for
those workspaces). findLastKnownVersion notes the silent fallback when the stored
string does not match a current OpikVersion enum value. markFirstTraceReported
documents the microsecond-collision trade-off as conscious — the SELECT FOR UPDATE
alternative deadlocks under contention because of InnoDB lock-upgrade semantics on
shared INSERT-IGNORE locks.
- [OPIK-4933] [BE] test: end-to-end coverage for workspaces table flows
Closes the gap between the DAO-level WorkspacesServiceTest and the ticket's acceptance
criteria by asserting on the workspaces table state from the resource-level tests:- WorkspaceVersionResourceTest.EntityWorkspaceVersionTest:
- versionDetermination__persistsDeterminedVersionToWorkspacesTable: a real
determination lands in workspaces.last_known_version (awaited because persist is
fire-and-forget). - versionDetermination__updatesPersistedRowWhenVersionChanges: pre-populates the
row with V1, triggers a fresh determination of V2, and asserts the row was
overwritten — implicitly verifies the analytics event would carry
version_changed=true.
- versionDetermination__persistsDeterminedVersionToWorkspacesTable: a real
- WorkspaceVersionResourceTest.V2WorkspaceAllowlistTest:
- overridePaths__doNotPersistRowToWorkspacesTable: both the allowlist (→V2) and
forceWorkspaceVersion (→V1) override paths must skip persistAndEmit so the
table is not polluted with non-determined values.
- overridePaths__doNotPersistRowToWorkspacesTable: both the allowlist (→V2) and
- ExperimentProjectMigrationJobTest.skipExperimentWhenInferredProjectWasDeleted:
asserts the trapped workspace appears in workspacesService.findMigrationSkippedWorkspaceIds()
after the cycle, so the next cycle's exclusion set has the right contents.
- [OPIK-4933] [BE] refactor: trim Javadoc to non-obvious facts; tighten override-skip test
Doc cleanup — keep only what would surprise a future reader:
- WorkspacesService method docs reduced to the non-obvious contract bits
(overwrite vs idempotent, first-writer-wins boolean semantics, unknown-enum
fallback). Class-level Javadoc removed; the package and method names are
self-explanatory. - persistAndEmit Javadoc trimmed to the auth one-way V2 gate caveat (the only
detail not derivable from the implementation). - markFirstTraceReported impl Javadoc reduced to the microsecond-truncation
rationale; the alternative-patterns analysis belongs in the PR description.
Test:
- overridePaths__doNotPersistRowToWorkspacesTable now uses Awaitility
during(1s).atMost(3s) so the assertion must hold continuously, proving no
late-firing background write surfaces.
- [OPIK-4933] [BE] test: shorten override-skip Awaitility window to 300ms
boundedElastic schedules within ms; a 300ms continuous-absence window is
sufficient to prove no late-firing background write surfaces. Cuts the
overridePaths__doNotPersistRowToWorkspacesTable active wait from 1s to 300ms
(test runtime 32s → 22.7s, dominated by container startup).- [OPIK-4933] [BE] test: rewrite WorkspacesServiceTest as a Mockito unit test
The previous version booted the full Dropwizard stack (MySQL + ClickHouse +
Redis + WireMock) to drive 9 DAO-level integration assertions through the
service. That work belongs at the controller layer, not on the service test.Replaced with a true unit test that mocks TransactionTemplate, Handle, and
WorkspacesDAO and verifies the service-level contract: enum-to-wire-value
mapping on upsertVersion, unknown-enum fallback on findLastKnownVersion,
microsecond truncation on markFirstTraceReported, read-back-equality boolean
semantics, and DAO method delegation for the migration-skip operations.Runtime drops from ~57s (containers) to ~0.7s (13 tests, no containers).
The SQL-level behaviours previously asserted here (idempotency, COALESCE,
TIMESTAMP(6) precision, concurrency under contention) remain covered at the
controller layer via BiEventListenerTest, WorkspaceVersionResourceTest, and
ExperimentProjectMigrationJobTest, all of which exercise the real DB through
the resource API.- [OPIK-4933] [BE] test: WireMock-based coverage for workspace_version_determined event
Closes the three remaining acceptance-criteria gaps that were previously only
guaranteed by chain ordering:- firstDetermination__emitsEventWithVersionChangedTrue (AT: emit on first determination)
- cacheHit__doesNotEmitAdditionalEvent (AT: cache hit must not write/emit)
- sameVersionRecompute__emitsVersionChangedFalse (AT: version_changed=false)
- differentVersionRecompute__emitsVersionChangedTrue (AT: version_changed=true)
New top-level test class with caching enabled (PT5M TTL) and the analytics
endpoint stubbed via the existing WireMock plumbing. Each assertion is scoped
to the test's unique workspace_id so events from sibling tests don't bleed in.4 tests, ~25s runtime (dominated by container startup).
- [OPIK-4933] [BE] test: replace inline org.mockito.Mockito.never() with static import
Backend code style requires static imports over inline fully-qualified class
names except on name collisions. The three readOnlyMethodsHaveNoWriteSideEffects
verify(...) calls were qualifying never() inline; switched to a static import
matching the rest of the file's Mockito usage.- [OPIK-4933] [BE] test: rename cacheHit test to cover the no-write half of the AT
The acceptance criterion is "subsequent cache-hit reads do not write or emit"
— the existing test was only named for the no-emit half. persistAndEmitBlocking
writes and emits as a single unit (no early return between them), so an
unchanged event count after a cache hit is logically sufficient to prove the
upsert also did not run; the test now states this in a method-level comment
and asserts the row state is consistent with a single determination.- [OPIK-4933] [BE] test: assert pre-marked trapped workspace is excluded from cycle
The previous skipExperimentWhenInferredProjectWasDeleted asserted that the
trap flag is set in the workspaces table after a workspace becomes trapped
(persistence side). The acceptance criterion also requires that a trapped
workspace is excluded from the next cycle's eligible set (consumption side).Adds skipPreMarkedTrappedWorkspaces — mirrors the existing skipExcludedWorkspaces
pattern but uses the skip flag instead of migration.excludedWorkspaceIds config.
A workspace is pre-marked via workspacesService.markMigrationSkipped before
seeding an eligible experiment; the experiment must remain unmigrated across
cycles, proving the workspace is omitted from the union exclusion set in
runMigrationCycle.- [OPIK-4933] [BE] refactor: review fixes — schema, Workspace record, DAO/service rewrite
Address review feedback on the workspaces persistence layer:
Schema (000067):
- Rename PK column workspace_id → id (matches the entity-table convention).
- Drop workspaces_version_idx (no DAO query reads by version yet).
- Keep workspaces_migration_skipped_idx (used by the cycle's exclusion query).
Model + DAO:
- New api/Workspace record (raw DB shape; service layer converts last_known_version
to OpikVersion via findByValue, treating unrecognised values as empty). - DAO collapsed to a single findById(id) returning Optional; service does
field selection. Per-field finders removed. - All upserts populate created_by/last_updated_by explicitly with SYSTEM_USER and
touch last_updated_by on real changes (IF guard keeps the audit unchanged on
COALESCE no-ops, so row counts still distinguish first-writer vs no-op). - markFirstTraceReported now uses ROW_COUNT-based first-writer detection
(Connector/J default useAffectedRows=false: 1=insert, 2=transition, 0=no-op),
eliminating the microsecond-collision false positive baz-reviewer flagged.
Service:
- Drop Instant parameters; Instant.now() is sourced internally to prevent wrong
uses (audit columns / version_determined_at / first_trace_reported_at). - Drop @NonNull on findById's workspaceId; blank input returns Optional.empty()
(callers receive natural empty result instead of NPE). - DAO row counts propagated through int returns on upsertVersion and
markMigrationSkipped (caller can ignore but the data is available). - Replace findLastKnownVersion with findById; persistAndEmitBlocking maps
Workspace.lastKnownVersion through OpikVersion.findByValue. - Replace "none" with "unknown" for previous_version on the
workspace_version_determined analytics event.
Consumers updated for the new signatures (BiEventListener,
ExperimentProjectMigrationService).- [OPIK-4933] [BE] refactor: move persistAndEmit into computeVersion
Move the fire-and-forget persistAndEmit call into the terminal step of
computeVersion (both the auth one-way V2 gate and the entity-scan branches),
so future call sites of computeVersion can't accidentally skip persistence.
getWorkspaceVersion no longer needs the .doOnNext wrappers.- [OPIK-4933] [BE] test: consolidate workspaces tests into existing classes
Per review feedback (don't add a separated class for analytics; inline into
existing tests; drop negative emission tests; remove the redundant unit test):- Delete WorkspaceVersionAnalyticsEmissionTest. WireMock for the analytics
endpoint is now wired into EntityWorkspaceVersionTest's AppContextConfig
(usageReportUrl + usageReportEnabled + analytics.enabled custom config + a
catch-all stub). Class-level helper verifyEvent(...) is shared across the
nested classes. - Inline the persistence + analytics-event payload assertions into the
existing happy-path test workspaceVersion__whenDatasetEntities__returnsExpectedVersion;
it already exercises a V2 -> V1 entity-driven recompute, which now also
asserts findById(id) reflects the persisted version after each call and
the analytics event payload includes version_changed=true with the right
previous/new values. - Drop the dedicated tests
versionDetermination__persistsDeterminedVersionToWorkspacesTable and
versionDetermination__updatesPersistedRowWhenVersionChanges (their coverage
is folded into the inline assertions above). - Drop overridePaths__doNotPersistRowToWorkspacesTable: per the reviewer,
negative emission tests slow the suite down and add little value beyond
what chain ordering already mechanically guarantees. - Delete WorkspacesServiceTest. Its DAO contracts are exercised end-to-end
through the controller-level tests; the only behaviours that were
unit-only (microsecond truncation, equality-based dedup) no longer exist
with the ROW_COUNT-based dedup in the previous refactor. - ExperimentProjectMigrationJobTest.skipPreMarkedTrappedWorkspaces updated
for the new markMigrationSkipped(workspaceId, reason) signature.
- [OPIK-4933] [BE] fix: split markFirstTraceReported into UPDATE-then-INSERT
CI surfaced that the single-statement upsert + ROW_COUNT-based detection is
unreliable: Connector/J defaults to useAffectedRows=false (CLIENT_FOUND_ROWS=on),
which makes a matched-but-unchanged upsert return 1 — indistinguishable from a
fresh insert. BiEventListenerTest.shouldReportFirstTraceEvents observed both
the first and second trace creations emitting the analytics event because both
calls returned true.Replaced with two atomic primitives in a single transaction, exactly matching
the reviewer's suggestion (one extra query under the same transaction, handle
the conflict exception):- updateFirstTraceIfNull: UPDATE ... WHERE first_trace_reported_at IS NULL.
Row count is unambiguous — only the writer that flipped the column from NULL
to the timestamp gets a row count of 1. - insertFirstTrace: plain INSERT (no upsert). Throws on duplicate-key when
another writer inserted first; caller catches SQLSTATE 23000 and returns
false.
Race-safe: both queries run in the same WRITE transaction, the UPDATE WHERE
clause is atomic at the row level, the INSERT is atomic at the PK level, and
the lock acquisition order is uniform so no deadlock cycle is possible.
BiEventListenerTest.shouldReportFirstTraceEvents now passes locally.- [OPIK-4933] [BE] refactor: bump migration to 000068 + move Workspace record to domain
Round-4 review fixes (Andrés):
- Bump the create-workspaces-table migration from 000067 to 000068. main now
carries 000067_reseed_default_environment_colors.sql, so our changeset
number was outdated. - Move com.comet.opik.api.Workspace → com.comet.opik.domain.workspaces.Workspace.
The api/ package is for request/response DTOs; this record is an internal
DB model and lives next to the DAO/service that use it. - Drop all @JsonProperty annotations on the record. JDBI3's ConstructorMapper
is the only consumer; the record is never serialised to JSON, so the
annotations were dead weight.
- [OPIK-4933] [BE] refactor: split markMigrationSkipped into UPDATE-then-INSERT
Round-4 review fix (Andrés): the previous ON DUPLICATE KEY UPDATE with
COALESCE looked like an update on duplicate but always preserved the existing
values, which was misleading. Per the reviewer's suggestion, an INSERT IGNORE
would communicate intent better — but pure INSERT IGNORE silently fails to
mark workspaces whose row already exists with migration_skipped_at = NULL
(common path: a prior version-determination upsert created the row before the
migration job got there).Switched to the same UPDATE-then-INSERT pattern used for markFirstTraceReported:
- updateMigrationSkippedIfNull: UPDATE ... WHERE migration_skipped_at IS NULL
flips a NULL flag to ts atomically; row count is unambiguous. - insertMigrationSkipped: plain INSERT for the missing-row branch; caller
catches SQLSTATE 23000 if another writer trapped the workspace first.
Same idempotent semantics as before (already-trapped → no-op), but the SQL
honestly reflects what we actually do.- [OPIK-4933] [BE] feat: plumb API user via reactive context for audit columns
Round-4 review fix (Andrés): the SYSTEM_USER constant was being stamped on
every workspaces row, regardless of whether the call originated from a user
request or a background job. Per the reviewer:- upsertVersion / markFirstTraceReported: API call → API user.
- markMigrationSkipped: system job → SYSTEM user (kept internal, unchanged).
WorkspacesService API:
- upsertVersion(workspaceId, version, userName)
- markFirstTraceReported(workspaceId, userName)
- markMigrationSkipped(workspaceId, reason) // SYSTEM_USER stays internal
Plumbing:
- WorkspacesResource.getWorkspaceVersion now wraps the call with
.contextWrite(ctx -> setRequestContext(ctx, requestContext)) so the API
user lands in the Reactor context. - AbstractWorkspaceVersionService.getWorkspaceVersion reads RequestContext.USER_NAME
via Mono.deferContextual (no Provider.get() inside the chain
— that anti-pattern would break on Schedulers.boundedElastic where the
request scope is gone). userName is threaded through computeVersion and
persistAndEmit/persistAndEmitBlocking down to WorkspacesService.upsertVersion. - BiEventListener already had event.userName(); pass it directly to
markFirstTraceReported.
Also fixes the changeset id inside 000068_create_workspaces_table.sql
(file rename in the previous commit didn't update the inline--changeset
declaration, causing a Liquibase checksum mismatch on existing test DBs).- [OPIK-4933] [BE] fix: race in version persistence via single-tx SELECT FOR UPDATE
The two-step findById + upsertVersion across separate transactions could
emit duplicate version_changed=true events when concurrent determinations
ran for the same workspace (both readers saw the same pre-write snapshot).Replace with upsertVersionAndReturnPrevious, which performs SELECT … FOR
UPDATE → upsert in one WRITE transaction so each writer observes the prior
writer's commit.- [OPIK-4933] [BE] refactor: extract single-flip helper, drop duplicate SYSTEM_USER, @NonNull id
- WorkspacesService: extract transitionFlagAtomically + isDuplicateKeyViolation helpers
shared by markFirstTraceReported and markMigrationSkipped (both are UPDATE-if-null →
INSERT-or-ignore-duplicate). markMigrationSkipped now returns boolean (callers discard
the value). - AbstractWorkspaceVersionService: drop the private SYSTEM_USER constant in favor of
RequestContext.SYSTEM_USER. - Workspace: mark id (NOT NULL PK) with @NonNull so the builder/JDBI ConstructorMapper
enforce the invariant. - Migration 000068: trailing blank line per migrations.md.
- [OPIK-4933] [BE] fix: retry UPDATE on duplicate-key in single-flip helper
The duplicate-key on the INSERT branch was wrongly treated as "another
writer flipped this flag." A duplicate-key just means the row exists —
it may have been inserted by an unrelated writer (version determination,
migration job) that didn't touch our target column. Retry the
UPDATE-if-null after duplicate-key to disambiguate: if the column is
still NULL, this caller flips it (true); otherwise some prior caller
already set it (false).Fixes the concurrency hole flagged on markFirstTraceReported and
markMigrationSkipped — both go through the helper.- [OPIK-4933] [BE] style: static-import RequestContext.SYSTEM_USER
Match the convention already used in WorkspacesService — code-style rule
prefers static imports over inline fully-qualified class names.- [OPIK-4933] [BE] style: @NonNull on findById workspaceId for consistency
Every other public method in WorkspacesService annotates
workspaceId
with @NonNull. findById was the lone exception. Keeping the blank-tolerance
(empty Optional on blank id) since that's the documented contract.下载附件