[OPIK-7707] [BE] refactor: make dataset version count updates atomic by JetoPistola · Pull Request #7928 · comet-ml/opik

JetoPistola · 2026-08-20T08:30:51Z

Details

Replaces the read-modify-write on the dataset version counters with a single incrementing SQL statement, and drops the post-insert re-read of the row that was just written. Both were deferred from OPIK-7705 as follow-ups.

The insert path previously did findById → add in Java → updateCounts, then called getVersionById to re-read the same row: three MySQL round-trips per batch, all inside the per-dataset lock that serialises an upload, so a 100-batch upload paid them 100 times in sequence.

incrementCounts applies the deltas in the database (SET items_total = COALESCE(items_total, 0) + :delta), so the counter arithmetic no longer depends on withDatasetVersionLock for mutual exclusion. The COALESCE matters: the columns are INT DEFAULT 0 (nullable) and map to boxed Integer, so a NULL would survive a bare increment and later unbox to an NPE — the absolute update this replaces happened to repair a NULL by overwriting it.
Both delete call sites are converted too. They already passed a plain delta and only needed currentVersion for logging, so insert and delete now write counts the same way — this closes the FR asking that the delete path not be left disagreeing with insert.
The delete path stays idempotent when the version row is gone. A concurrent DatasetService.delete can remove dataset_versions without holding withDatasetVersionLock; the previous absolute update discarded its result, so that race was a silent 204. It logs and continues rather than turning into a 404. The insert path does still throw — it threw from findById before, so that 404 is existing behaviour.
updateCounts had no callers left after both paths moved over, so it is removed.
The returned DatasetVersion is no longer re-read. The resource .block()s and returns 204, and saveBatch maps it to items.size(), so no caller reads its fields; the Mono still emits so saveBatch's map is reached.

Counter values are unchanged. This is a performance and correctness-hardening change with no intended behavioural surface.

Change checklist

User facing
Documentation update

Issues

Resolves #
OPIK-7707

AI-WATERMARK

AI-WATERMARK: yes

Tools: Claude Code
Model(s): Claude Opus 5
Scope: Traced the call graph, wrote the implementation and the two new concurrency tests, ran the suites and the baseline comparison below.
Human verification: Pending author review.

Testing

mvn test -Dtest='DatasetVersionResourceTest*'   # 123/123 pass
mvn spotless:apply                              # clean

Scenarios validated:

Counters unchanged — the whole DatasetVersionResourceTest suite (123 tests) is green, including InsertClassificationCounts (new-vs-update classification), BatchVersioningDeleteTests, DeleteItemsWithVersioning, and MutateLatestVersion. This is the AC that counter values match the previous implementation.
Concurrency invariant — two new tests in ConcurrentUploads, reusing that class's existing barrier harness so the HTTP calls genuinely overlap: 8 concurrent inserts into one version, and 8 concurrent deletes from one version, each asserting items_total equals the rows actually stored.
Round-trip reduction — verified by reading the path: the count update is now one statement, and no getVersionById follows it.
Post-merge — origin/main merged in (no conflicts) and the full 123 re-run green on the merged tree, after the review fixes.

Not verified, stated plainly:

The two new tests also pass without this change. The per-dataset lock does serialise these writes today, so they pin the invariant rather than demonstrating a fixed race. The AC asked for correctness demonstrated "with the lock removed in a test"; that is not what these do, and doing it properly needs a DAO-level test that bypasses the service lock (no such test class exists for this DAO yet). Flagging rather than claiming the AC is met.
DatasetsResourceTest$FindDatasets has 30 pre-existing failures on this branch. Confirmed unrelated: the identical 30 fail on a clean tree with all of this PR's changes stashed. Separately, a ClickHouse testcontainer failed to boot on one run — flaky infra, passed on retry.

Documentation

No documentation impact — internal refactor with no API or behavioural change.

Replace the read-modify-write on the version counters with a single incrementing statement, and drop the post-insert re-read of the row that was just written. The insert path previously did findById -> add in Java -> updateCounts, then called getVersionById to re-read the same row. Three MySQL round-trips per batch, all inside the per-dataset lock that serialises an upload, so a 100-batch upload paid them 100 times in sequence. incrementCounts applies the deltas in the database, so the arithmetic no longer depends on the lock for mutual exclusion. Both delete call sites are converted too -- they already passed a plain delta and only needed currentVersion for logging -- so insert and delete write counts the same way. updateCounts had no callers left and is removed. The returned DatasetVersion is not re-read: the resource blocks and returns 204, and saveBatch maps it to the item count, so no caller reads its fields. The Mono still emits so saveBatch's map is reached. Counter values are unchanged; this is a performance and correctness-hardening change with no intended behavioural surface. Adds two concurrency tests asserting items_total agrees with the rows actually stored after 8 concurrent inserts and 8 concurrent deletes against one version. Note both also pass without this change: the per-dataset lock does serialise these writes today, so the tests pin the invariant rather than demonstrating a fixed race. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-08-20T08:34:14Z

⏱️ pre-commit per-hook timing

Hook	Description	Result	Duration
☕ spotless — java backend	Format Java code	✅	7.42s
Total (1 ran)			7.42s

⏭️ 42 skipped (no matching files changed)

Hook	Description	Result
🐍 trim trailing whitespace — python sdk	Strip trailing whitespace	⏭️
🐍 fix end of files — python sdk	Ensure files end in a newline	⏭️
🐍 ruff — python sdk	Lint + autofix Python (ruff)	⏭️
🐍 ruff-format — python sdk	Format Python code (ruff)	⏭️
🐍 mypy — python sdk	Static type check	⏭️
🤖 trim trailing whitespace — optimizer	Strip trailing whitespace	⏭️
🤖 fix end of files — optimizer	Ensure files end in a newline	⏭️
🤖 check yaml — optimizer	Validate YAML syntax	⏭️
🤖 check json — optimizer	Validate JSON syntax	⏭️
🤖 check toml — optimizer	Validate TOML syntax	⏭️
🤖 check for added large files — optimizer	Block large files (>1MB)	⏭️
🔐 detect private key — optimizer	Block committed private keys	⏭️
🤖 check for merge conflicts — optimizer	Block merge-conflict markers	⏭️
🤖 check for case conflicts — optimizer	Block case-only name clashes	⏭️
🤖 pyupgrade — optimizer	Modernize Python syntax	⏭️
🤖 ruff — optimizer	Lint + autofix Python (ruff)	⏭️
🤖 ruff-format — optimizer	Format Python code (ruff)	⏭️
🤖 mypy — optimizer	Static type check	⏭️
📓 nbstripout — optimizer notebooks	Strip notebook output	⏭️
📝 markdownlint — optimizer	Lint Markdown	⏭️
🔤 codespell — optimizer	Fix common misspellings	⏭️
📊 radon cc — optimizer	Cyclomatic-complexity gate	⏭️
📊 radon raw — optimizer	Raw size metrics gate	⏭️
📊 xenon — optimizer	Fail on complexity thresholds	⏭️
📊 lizard — optimizer	Cyclomatic-complexity gate	⏭️
🧹 vulture — optimizer	Find dead code	⏭️
🛡️ trim trailing whitespace — guardrails	Strip trailing whitespace	⏭️
🛡️ fix end of files — guardrails	Ensure files end in a newline	⏭️
🛡️ ruff — guardrails	Lint + autofix Python (ruff)	⏭️
🛡️ ruff-format — guardrails	Format Python code (ruff)	⏭️
🛡️ mypy — guardrails	Static type check	⏭️
⚓ helm-docs	Regenerate Helm chart README	⏭️
block non-public FE plugins	Block non-public FE plugins	⏭️
🧪 pre-commit wrapper smoke tests	Self-test the wrapper scripts	⏭️
🧪 rebaseline script tests	Self-test the changelog re-baseline script	⏭️
🌐 eslint — frontend	Lint + autofix JS/TS	⏭️
🌐 typecheck — frontend	Whole-project tsc type check	⏭️
📘 eslint — typescript sdk	Lint + autofix JS/TS	⏭️
📘 typecheck — typescript sdk	Whole-project tsc type check	⏭️
⚙️ actionlint — github workflows	Lint GitHub Actions workflows	⏭️
🐳 hadolint — dockerfiles	Lint Dockerfiles	⏭️
🌈 zizmor — github workflows security	Security-scan GitHub Actions workflows	⏭️

CometActions · 2026-08-20T08:37:02Z

No test needed here.

The counter arithmetic is unchanged — read-modify-write of (total, added, modified, deleted) becomes the same deltas applied in one UPDATE — so the only behaviour that differs is under concurrent writers, and that isn't something an e2e run can reproduce deterministically. Our spec e2e/tests/datasets/dataset-version-counters.spec.ts (@cap:datasets.version-history-view, green 18/18) already asserts items_total/items_added/items_modified after a multi-batch insert, both sequential and with num_threads=8, cross-checks the stored total against the item ids actually in the dataset, and asserts the Version history tab renders it — so a swapped or sign-flipped delta in incrementCounts fails an existing test. The two integration tests you added in DatasetVersionResourceTest cover the race at the layer where it is actually reproducible. Also checked the stub DatasetVersion.builder().id(versionId).build(): both callers do discard it (DatasetsResource returns 204, saveBatch maps to items.size()), so nothing reads the fields you stopped populating.

Run

Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review.

…mic-version-count-update

Addresses review feedback on #7928. COALESCE all four counters in incrementCounts. The columns are `INT DEFAULT 0` (nullable) and DatasetVersion maps them as boxed Integer, so a NULL survives the increment and later unboxes to an NPE. The absolute update this replaced happened to repair a NULL by overwriting it; a bare `col + :delta` propagates it instead. Stop throwing NotFoundException from the delete path when the version row is gone. A concurrent dataset delete removes dataset_versions without holding withDatasetVersionLock, and the previous absolute update ignored its result, so that race stayed a 204. Turning it into a 404 would have been an unintended contract change; log and move on. Demote the delete count log to DEBUG and label the field -- it duplicated the caller's "Deleted ... items" INFO line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The counter columns are `INT DEFAULT 0` (migration 000036), which is nullable -- DEFAULT only applies when the column is omitted on INSERT, and DatasetVersion maps all four as boxed Integer. A bare `col + :delta` leaves NULL as NULL, which then unboxes to an NPE on read. The absolute update this replaced happened to repair a NULL by overwriting it, so the increment has to COALESCE explicitly or it is a regression in NULL tolerance. Adds a test that nulls the counters directly and asserts the increment treats them as zero. Verified it fails without the COALESCE. Ported from the duplicate work on #7928, which is being closed in favour of this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

JetoPistola · 2026-08-23T08:28:19Z

Closing as a duplicate of #7705.

Both PRs implement OPIK-7707 and arrived at nearly the same design independently — same incrementCounts method name, same signature, same SQL, same three files. #7705 is the older PR (Aug 3 vs Aug 20); I opened this one without first checking whether a PR already existed for the ticket. My mistake.

#7705 is the better base:

It has a DAO-level concurrency test (incrementCounts__whenConcurrentWritersBypassTheLock__thenCountersAreExact, 8 writers × 25 increments straight at the DAO) that satisfies the acceptance criterion this PR's description explicitly flagged as unmet. The resource-level tests here pass without the fix, because the per-dataset lock serialises them.
It also covers negative deltas, unknown version, and cross-workspace isolation at the DAO level.
It folds the two count-update helpers into one, which is tidier than the two kept here.

Ported across to #7705 in 7ad36cf: the COALESCE(col, 0) NULL guard, plus a test that nulls the counters directly and which I verified fails without the guard.

Not ported — and a correction to this PR's second commit: the delete-path NotFoundException I removed here was not actually a 404 regression. Both delete callers call getVersionById first, which already throws if the version is missing, so incrementCounts returning 0 was unreachable. #7705 deliberately added that throw (0eb0865) to keep insert and delete symmetric for future work that drops the pre-fetch. That reasoning is sound and this PR's change to it was based on a review finding I accepted too readily.

The Long→int narrowing finding is answered in the thread above; it predates both PRs.

🤖 Comment posted via /address-github-pr-comments

The counter columns are `INT DEFAULT 0` (migration 000036), which is nullable -- DEFAULT only applies when the column is omitted on INSERT, and DatasetVersion maps all four as boxed Integer. A bare `col + :delta` leaves NULL as NULL, which then unboxes to an NPE on read. The absolute update this replaced happened to repair a NULL by overwriting it, so the increment has to COALESCE explicitly or it is a regression in NULL tolerance. Adds a test that nulls the counters directly and asserts the increment treats them as zero. Verified it fails without the COALESCE. Ported from the duplicate work on #7928, which is being closed in favour of this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* [OPIK-7707] [BE] perf: make dataset version count updates atomic Replace the read-modify-write count update on the dataset item insert path with a single atomic SQL increment, and drop the version re-read that immediately followed it. MySQL round-trips inside the per-dataset lock drop from three (find, update, re-read) to one. The returned DatasetVersion is read by no consumer -- the REST endpoint returns 204 with the blocked result unassigned, and saveBatch maps it to the item count -- so insertItemsIntoVersion now completes empty instead of fabricating a partially populated object. The delete path is converted to the same atomic increment for consistency. Unlike the insert path it saves no round-trip: both callers already fetch the version for their own logging. What it gains is that the counter arithmetic no longer depends on withDatasetVersionLock for mutual exclusion. Implements OPIK-7707: Make the version count update atomic and drop the post-insert version re-fetch Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(datasets): fail delete count update when no version row matches updateVersionCountsForDelete ignored the affected-row count from incrementCounts, so insert and delete disagreed about how a missing or cross-workspace version is handled. Both callers currently fetch the version first via getVersionById, which already throws, so this is not reachable today -- but the asymmetry is a trap for later work that narrows the lock or drops that pre-fetch. Also drop the helper's log.info: the delete callers already emit a summary with deletedCount, versionId and the resulting total two frames up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(datasets): fold the two count-update helpers into one updateVersionCountsForInsert and updateVersionCountsForDelete had become identical apart from which deltas they pass -- same transaction, same DAO call, same zero-row NotFoundException guard -- so any change to the atomic update or the error handling had to be mirrored in both. Both now delegate to a shared updateVersionCounts(...) taking signed deltas. The named wrappers stay so the call sites still read as insert/delete intent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(datasets): treat NULL version counters as zero when incrementing The counter columns are `INT DEFAULT 0` (migration 000036), which is nullable -- DEFAULT only applies when the column is omitted on INSERT, and DatasetVersion maps all four as boxed Integer. A bare `col + :delta` leaves NULL as NULL, which then unboxes to an NPE on read. The absolute update this replaced happened to repair a NULL by overwriting it, so the increment has to COALESCE explicitly or it is a regression in NULL tolerance. Adds a test that nulls the counters directly and asserts the increment treats them as zero. Verified it fails without the COALESCE. Ported from the duplicate work on #7928, which is being closed in favour of this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(datasets): gate the not-migrated sentinel and correct stale contracts Addresses the eight review findings on #7705. Sentinel (the load-bearing one). Liquibase 000046 leaves un-backfilled versions at items_total = -1, and findVersionsNeedingItemsTotalMigration selects on exactly that value. Incrementing such a row would both corrupt the counter and hide it from the backfill forever. incrementCounts now excludes it, using the NULL-safe <=> so a NULL counter -- which the COALESCE exists to repair -- is still incremented rather than skipped by three-valued logic. The sentinel is a named constant referenced by every statement that depends on it. The absolute writers driven by the backfill (updateItemsTotal, batchUpdateItemsTotal) are gated on the same sentinel, so a total counted before an increment landed can no longer clobber it. The incrementCounts javadoc no longer claims a blanket no-lock guarantee: it holds among delta writers, which is what is actually true. save() narrows to Mono<Void>. Its javadoc promised a DatasetVersion that the append path stopped emitting; the only caller discards it, so the type now says what happens instead of documenting a value that arrives on one path in three. Both delete callers drop the pre-delete getVersionById. Expressing the update as a delta removed the last reason to read the version, so that synchronous MySQL round-trip was surviving purely to enrich a log line -- unwrapped inside Mono.defer, unlike every other blocking hop here. The helper javadoc claimed the callers needed it "regardless", which stopped being true when the snapshot parameter went away. Tests: adds service-level coverage for the updated == 0 guard (reached through the public API via a sentinel-held version, asserting 404 rather than a silent 204) and for the sentinel skip itself. Drops two DAO tests that duplicated existing resource tests, folding their counter assertions into the originals, which also assert row-level state the copies had lost. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(datasets): seed the sentinel on the lazy-migration path The sentinel gate added for the backfill-clobber finding broke lazy migration. ensureVersion1Exists seeds items_total = 0, not -1, so the newly gated updateItemsTotal matched zero rows and countAndUpdateItemsTotal discarded that result: the real count was never written, and the version stayed at 0 permanently. The batch backfill could not repair it either, since it also selects only on the sentinel. Seed ITEMS_TOTAL_NOT_MIGRATED instead. A literal 0 on a row created before its items are counted is indistinguishable from a genuinely empty version, which is what let the failure hide. Also stop discarding the affected-row count in countAndUpdateItemsTotal: zero rows now means an API write already moved the counter off the sentinel, so the stale count is correctly skipped rather than silently dropped. Logged rather than thrown -- skipping is the right outcome there. Addresses review feedback on #7705. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(datasets): scope the v1 existence check by workspace ensureVersion1Exists guarded on NOT EXISTS (SELECT 1 FROM dataset_versions WHERE dataset_id = :dataset_id) with no workspace predicate, so a matching dataset_id in another workspace could suppress v1 creation in this one and leak that state through the affected-row count. Every other statement here scopes by workspace, and the table's uniqueness constraint is (workspace_id, dataset_id, version_hash) -- this subquery was the outlier. Predates this branch, but seeding the sentinel made the affected-row result load-bearing, so a wrong NOT EXISTS now matters more than it did. Also move the skip log out of the inTransaction callback in countAndUpdateItemsTotal: the callback returns the affected-row count and the branch runs outside, per the database-work-only rule in .agents/skills/opik-backend/mysql.md. Addresses review feedback on #7705. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(datasets): label the fields in the backfill skip log Aggregated logs can't tell which value is which in "for dataset '{}' version '{}'". Matches the labelled form used elsewhere in this branch. Addresses review feedback on #7705. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: t <t@t.t>

JetoPistola requested a review from a team as a code owner August 20, 2026 08:30

github-actions Bot assigned JetoPistola Aug 20, 2026

github-actions Bot added java Pull requests that update Java code Backend tests Including test files, or tests related like configuration. 🟡 size/M labels Aug 20, 2026

baz-reviewer Bot reviewed Aug 20, 2026

View reviewed changes

JetoPistola marked this pull request as draft August 20, 2026 08:42

JetoPistola and others added 2 commits August 20, 2026 19:16

Merge remote-tracking branch 'origin/main' into danield/OPIK-7707-ato…

921b78a

…mic-version-count-update

baz-reviewer Bot added the baz: pending label Aug 20, 2026

baz-reviewer Bot reviewed Aug 20, 2026

View reviewed changes

Comment thread apps/opik-backend/src/main/java/com/comet/opik/domain/DatasetItemService.java

baz-reviewer Bot approved these changes Aug 23, 2026

View reviewed changes

JetoPistola closed this Aug 23, 2026

JetoPistola mentioned this pull request Aug 23, 2026

[OPIK-7707] [BE] perf: make dataset version count updates atomic #7705

Merged

2 tasks

Navigation Menu

Uh oh!

[OPIK-7707] [BE] refactor: make dataset version count updates atomic - #7928

[OPIK-7707] [BE] refactor: make dataset version count updates atomic#7928
JetoPistola wants to merge 3 commits into
mainfrom
danield/OPIK-7707-atomic-version-count-update

JetoPistola commented Aug 20, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 20, 2026 •

edited

Loading

Uh oh!

CometActions commented Aug 20, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JetoPistola commented Aug 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

JetoPistola commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Change checklist

Issues

AI-WATERMARK

Testing

Documentation

Uh oh!

github-actions Bot commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⏱️ pre-commit per-hook timing

Uh oh!

CometActions commented Aug 20, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JetoPistola commented Aug 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JetoPistola commented Aug 20, 2026 •

edited

Loading

github-actions Bot commented Aug 20, 2026 •

edited

Loading