[OPIK-7707] [BE] perf: make dataset version count updates atomic - #7705
Conversation
github-actions
Bot
added
java
⏱️ pre-commit per-hook timing
⏭️ 42 skipped (no matching files changed)
|
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
Backend Tests - Integration Group 13 39 files + 3 39 suites +3 4m 37s ⏱️ +41s Results for commit 47a56f8. ± Comparison against base commit 89ce756. This pull request removes 109 and adds 218 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
|
Consolidated duplicate work from #7928 into this PR (now closed). I'd independently implemented the same change there without noticing this PR existed. This one is the better base — it has the DAO-level lock-bypass test that actually proves the arithmetic is atomic rather than merely serialised by Ported here in 7ad36cf, one substantive gap the other PR had caught:
Added a test that nulls the counters directly and asserts the increment treats them as zero — verified it fails without the COALESCE and passes with it. Nothing else from #7928 was worth porting. In particular I did not bring over its removal of the delete-path Suite is green locally: 125/125 in Note this branch is still based on Aug 3 and hasn't had main merged in — worth doing before merge. 🤖 Comment posted via /address-github-pr-comments |
|
Already covered by a test in this PR. The delta rewrite of the version counters is covered on both write paths. Insert: tests_end_to_end/e2e/tests/datasets/dataset-version-counters.spec.ts pins items_total/added/modified after multi-batch and 8-thread parallel inserts and reads the same numbers back off the Version history tab, so a wrong sign or a swapped argument in incrementCounts fails it. Delete: the draft stacked on this branch, #7705 -> #7965, adds dataset-version-delete-counters.spec.ts, which asserts the full total/added/modified/deleted quadruple across interleaved inserts, upserts and deletes (the existing delete specs only count rows). Nothing new to propose. The ITEMS_TOTAL_NOT_MIGRATED (-1) seeding in ensureVersion1Exists is the one part we can't reach: it only appears on a dataset lazily migrated from the pre-versioning tables, which a fresh OSS install never produces -- your DatasetVersionResourceTest covers those semantics including the deliberate 404, so we're recording it as deferred rather than proposing a spec we can't run. Not testable yet. ensureVersion1Exists now seeds items_total with the -1 not-migrated sentinel instead of 0, and incrementCounts / updateItemsTotal / batchUpdateItemsTotal are all gated on it. That is user-facing on a legacy dataset: while the row holds the sentinel the Version history tab has no real Item count to show, and an item batch written into that window is refused with 404 by design. It is only reachable on an install that already has pre-versioning dataset_items rows with no dataset_versions row, which the e2e estate cannot produce -- every dataset it creates is versioned from birth. also touches Backend (Java API / internal) Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review. Re-checked after a push on 27 Aug 16:33 UTC. |
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
|
🌙 Nightly cleanup: The test environment for this PR ( |
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
|
🌙 Nightly cleanup: The test environment for this PR ( |
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>
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>
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>
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
|
🌙 Nightly cleanup: The |
|
🌙 Nightly cleanup: The test environment for this PR ( |
|
🌙 Nightly cleanup: The |
JetoPistola
added
test-environment
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
5 participants
Details
Every batch appended to an existing dataset version paid three MySQL round-trips inside the per-dataset lock that serialises the upload:
findById, thenupdateCountswith totals computed in Java, then agetVersionByIdre-read of the row just written. Because the lock serialises batches, those round-trips are additive across an upload — a 100-batch upload pays them 100 times in sequence. This replaces the read-modify-write with a single atomic SQL increment and drops the re-read, taking the insert path from three statements to one.DatasetVersionDAO.updateCounts(absolute values) becomesincrementCounts, which applies signed deltas in the database (SET items_total = items_total + :delta). One method serves both paths:+non insert,-ntotal /+ndeleted on delete. It returns the affected-row count so the service preserves theNotFoundExceptionthatfindById().orElseThrow()used to provide.updateVersionCounts(...)that owns the transaction, the DAO call, and the zero-rowNotFoundExceptionguard.updateVersionCountsForInsert/...ForDeleteremain as thin named wrappers documenting which counters each path moves.getVersionByIdis gone. No consumer reads the returnedDatasetVersion— the REST endpoint returns 204 with the.block()result unassigned, andsaveBatchmaps it toitems.size()— soinsertItemsIntoVersionnow returnsMono<Void>rather than fabricating a partially-populated object that would look real to a future caller.withDatasetVersionLockfor mutual exclusion, and insert/delete now write counts the same way.Two notes for reviewers, since they differ from the ticket as filed:
InsertClassificationCountsas a regression net for this path. No test by that name exists in the tree; the real coverage is inDatasetVersionResourceTest(VersionSnapshotTests,ApplyDatasetItemChanges,MutateLatestVersion).updateCountshad zero remaining callers once both paths were converted, so it was removed rather than left as dead code.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
Added a
AtomicVersionCountUpdatesnested class inDatasetVersionResourceTestwith six tests:itemsModifiedbut notitemsTotalbatch_group_identrypoint accumulates into one versionwithDatasetVersionLockbypassed, asserting exact counters — this is the acceptance criterion that the arithmetic no longer depends on the lock; the previous read-modify-write would lose updates hereitems_totaland raisesitems_deleted, leaving added/modified untouchedNotFoundExceptionpath)Commands run:
mvn -o compile -DskipTests— passesmvn -o test-compile -DskipTests— passesmvn -o spotless:check—BUILD SUCCESSCI result: all 16 backend integration groups pass.
DatasetVersionResourceTest$AtomicVersionCountUpdatesreportsTests run: 6, Failures: 0, Errors: 0(Integration Group 6), so all six new tests executed and passed — including the lock-bypassed concurrency test. Pre-existing regression nets on these paths are green as well:MutateLatestVersion(6),ConcurrentUploads(4),DeleteItemsWithVersioning(5),BatchVersioningTests,BatchVersioningDeleteTests(10).Note on local runs:
mvn -o test -Dtest=DatasetVersionResourceTestcould not be brought to green on my machine — three attempts each died in the test-class constructor, before any test method, withClickHouse exception, code: 159 ... Read timed outonON CLUSTERDDL during Liquibase migration, on a different pre-existing changeset each time (000097,000109,000070). That is local distributed-DDL latency, not this diff: the change is MySQL-only and all three changesets are already onmain. CI, which has a healthy container environment, runs the suite clean.Also verified by inspection: the counter columns are signed
INT(000036_add_dataset_versions_tables.sql), so the delete path's negative delta cannot hit an unsigned-underflow error under strict mode — same semantics as the previous Java-side subtraction. No migration is required.Review follow-up (
0eb086558e,0153bc85dc)updateVersionCountsForDeletenow checks the affected-row count fromincrementCountsand throwsNotFoundExceptionon zero, matching the insert path; the helper's duplicatelog.infowas dropped (the callers already logdeletedCount,versionId, and the resulting total). Note this failure mode is not reachable today — both delete callers callgetVersionByIdfirst, which already throws — but insert and delete were handling a zero-row result inconsistently, which would become a real bug once the lock is narrowed or the pre-fetch removed.That guard left the two helpers identical apart from their delta arguments, so a second follow-up folded the shared transaction / DAO-call / guard into one
updateVersionCounts(...)taking signed deltas. Pure refactor, no behavioural change: verified byAtomicVersionCountUpdates(6),MutateLatestVersion(6, insert wrapper) andDeleteItemsWithVersioning(5, delete wrapper) all green on0153bc85dc.Documentation
N/A — internal performance and correctness-hardening change with no user-facing or API surface.