-
[OPIK-6703] [BE] fix: zero-rows retry guard + v2-client INSERT...SELECT on dataset_item_versions (#6871)
frostbyte_neo 发布于
2026-05-28 08:27:41 +00:00 - [OPIK-6703] [BE] fix: zero-rows retry guard + v2-client INSERT...SELECT on dataset_item_versions
Fix for the silent data-loss bug documented in OPIK-6674: INSERT...SELECT on
dataset_item_versions can return 0 rows when the SELECT runs on a stale
ClickHouse replica, and the new-version row gets committed with a truncated
items_total.Changes:
- Application-side retry guard on COPY and EDIT INSERT...SELECT operations.
Configurable via datasetVersioning.zeroRowsRetry (maxAttempts, backoff bounds).
On exhaustion: typed ZeroRowsWrittenException instead of silent success. - COPY path (copyVersionItems / copyUnchangedItems) moved to the v2 ClickHouse
client. The v2 client reports authoritative written_rows from Statistics;
the r2dbc driver reads from interim Progress events and is unreliable for
this query shape (clickhouse-java#2860). - EDIT path (editItemsViaSelectInsert) also moved to v2 client. Retries are
idempotent: newRowIds are caller-supplied and ReplacingMergeTree dedupes on
the (workspace_id, dataset_id, dataset_version_id, id) sort key. - New v2-client entry points on FilterQueryBuilder: toAnalyticsDbFiltersV2Client
and bindV2Client share the existing operator/strategy templates with the
r2dbc bind path via an internal bindUsing(BiConsumer) core. - formatStringArrayLiteral hardens against SQL injection: backslashes doubled
first, then single quotes doubled (ClickHouse accepts both '' and ' escapes). - Non-blocking query path: Mono.fromFuture(Supplier) with outer
subscribeOn(Schedulers.boundedElastic()) keeps clickHouseClient.query(),
SQL/params build, and the blocking QueryResponse.close() off the reactive
event loop while not parking a thread during the in-flight query.
Tests: DatasetVersionResourceTest 112/112; FilterQueryBuilderV2ClientTest 14/14
(includes injection-vector tests).Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- [OPIK-6703] [BE] refactor: delegate CH string escaping to ClickHouseUtil; Collection signature
formatStringArrayLiteral previously did the SQL-injection escape inline via two
.replace() passes. Switch to ru.yandex.clickhouse.ClickHouseUtil.escape(), the
upstream driver helper, which handles the full C-style escape set ClickHouse
accepts (, ', `, \n, \t, \b, \f, \r, \0). Same injection-safety contract,
fewer hand-rolled escape rules to audit.API changes:
- Signature: String[] -> Collection. Drops three .toArray(new String[0])
call sites in DatasetItemVersionDAO and one internal use in bindV2Client. - Null elements now rejected with an explicit NullPointerException carrying a
useful message instead of NPEing inside .replace() on the null receiver.
Escape output changes cosmetically — ' is now ' (was ''), and \ is \ (was
\\). Both forms are valid ClickHouse string-literal escapes; the injection
tests verify both attack vectors still resolve to a single-element array on the
server side.Tests: FilterQueryBuilderV2ClientTest 16/16 (added empty-collection and
null-element-rejection cases); DatasetVersionResourceTest 112/112.Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- [OPIK-6703] [BE] fix: inline UUID arrays in SQL body + enable v2 client async requests
CSV bulk-upload (DatasetsCsvUploadResourceTest#uploadCsvFile__largeBatch, 2500
rows) was timing out at 2000/2500 items on this branch. Two contributing causes:-
clickHouseClient.query(...) was synchronous by default. Without
useAsyncRequests(true) on Client.Builder, the v2 client runs the entire HTTP
round-trip on the calling thread and returns CompletableFuture.completedFuture,
so Mono.fromFuture() did not actually defer anything. We were also pinning the
call to Schedulers.boundedElastic, doubling the thread-hop cost vs r2dbc's
truly-async pipeline. Enabling async on the client lets the future genuinely
defer; the outer subscribeOn is no longer needed. -
The UUID arrays for COPY_VERSION_ITEMS (uuids pool + excludedIds) were bound
as v2 client query parameters, which the v2 client URL-encodes as
?param_=... on the request line. With 2000 UUIDs the URL crossed the
Apache HttpClient request-line limit (~8 KB). The SQL itself is sent in the
request body and has no such limit, so we inline the arrays directly via
StringTemplate <uuids_literal>/<excluded_ids_literal>. Safe because
UUID.toString() is [0-9a-f-] only — no injection vector.
Tests: DatasetsCsvUploadResourceTest#uploadCsvFile__largeBatch passes in 25s
(was timing out at 30s); DatasetVersionResourceTest 112/112;
FilterQueryBuilderV2ClientTest 16/16.Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- [OPIK-6703] [BE] refactor: drop formatUuidArrayParam, reuse formatStringArrayLiteral
PR review: formatUuidArrayParam was a near-duplicate of the FilterQueryBuilder
helper. Replaced with a thin uuidsToArrayLiteral delegate that maps each UUID
through UUID::toString and calls formatStringArrayLiteral. UUID.toString output
is [0-9a-f-] only, so the per-element escape is a no-op.Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- [OPIK-6703] [BE] refactor: address review — extract retry infra, Duration config, FilterQueryBuilder cleanup
Addresses review feedback on PR #6871:
- Extract the zero-rows retry guard out of the DAO into reusable client
infrastructure: new ZeroRowsWrittenException + ZeroRowsRetryPolicy in
infrastructure/db, provided via DatabaseAnalyticsModule. The DAO is now a
user, not the owner. Documented why the retry is reactor-based (the v2
client's setMaxRetries/retryOnFailures only cover transport faults, not the
business-level "INSERT succeeded but wrote 0 rows" condition). - Move expectedRowsFromPool into FilterUtils, next to generateUuidPool whose
sizing it reverses. - DatasetVersioningConfig: convert to a record using dropwizard Duration
(minBackoff/maxBackoff) with @MinDuration/@MaxDuration; drop the Java-code
defaults so they live only in config.yml / config-test.yml. - FilterQueryBuilder cleanup:
- rename bindV2Client -> populateV2ClientParams (it populates a Map, it does
not bind to a Statement). - formatStringArrayLiteral: build via Collectors.joining(",","[","]") instead
of a hand-rolled StringBuilder; signature @NonNull Collection<@NonNull String>. - mark rewritePlaceholdersForV2Client @VisibleForTesting.
- use imported BiConsumer instead of the fully-qualified name.
- rename bindV2Client -> populateV2ClientParams (it populates a Map, it does
Investigation (client-side collection binding): confirmed the v2 client
serializes params via String.valueOf at the HTTP boundary, so a Collection
can't be bound directly for Array(String) — pre-formatting the literal is
required. r2dbc does bind collections directly, but that only affects the
shared bind(Statement) path used by many other queries and is out of scope here.Tests: DatasetVersionResourceTest 112/112; DatasetsCsvUploadResourceTest
large-batch 1/1; FilterQueryBuilderV2ClientTest 15/15; spotless clean.Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- [OPIK-6703] [BE] test: unit-test ZeroRowsRetryPolicy retry/bypass paths
Adds focused coverage for the extracted retry policy: expectedRows<=0 bypass,
non-zero pass-through, 0-rows-then-success retry, and exhaustion surfacing
ZeroRowsWrittenException. Uses StepVerifier with tiny backoffs.Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- test(backend): pin dataset type in createFromTraces regular-dataset test
createFromTraces__whenRegularDataset__thenDataContainsAllEnrichedFields used
the shared createDataset() helper, which lets PODAM randomize DatasetType
(~50/50 DATASET vs TEST_SUITE). On TEST_SUITE the enrichment unwraps the trace
input instead of keeping the enriched input/expected_output/tags keys, so the
test failed intermittently. Pin type to DATASET, mirroring the TEST_SUITE
siblings.- fix(datasets): bypass zero-rows guard on filtered copyVersionItems
copyVersionItems sized the zero-rows retry guard from the UUID pool, which is
always > 0. But filter-based callers (delete, batch-update carry-forward) can
legitimately exclude every source row, so a filter matching all items wrote 0
rows yet retried maxAttempts times and threw ZeroRowsWrittenException for a
valid 'copy none' outcome. Gate expectedRows on excludeFilters: assert the
guard only on the unfiltered carry-forward path (OPIK-6674); bypass it
(expectedRows=0) when filters are present. Addresses PR #6871 review.
Co-authored-by: Claude Opus 4.7 (1M context) noreply@anthropic.com
下载附件