[CORE-16776] cluster_link: prefer the source-reported deleted flag for SR sync#31076
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates schema-registry sync behavior to distinguish an explicitly reported deleted flag from an omitted one, enabling the reconciler to prefer the per-version body’s deleted state when present and fall back to the listing-derived soft-delete set only when the source omits the flag.
Changes:
- Add
source_schema_read::reported_deleted(std::optional<is_deleted>) to capture whetherdeletedwas present in the source response body. - Update JSON parsing to preserve “absent vs explicit false” and keep
schema.deleteddefaulting tonowhen absent. - Adjust schema-registry sync reconciler logic and tests to prefer
reported_deletedover the listing-derived soft-delete set, with explicit fallback behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/v/pandaproxy/schema_registry/types.h |
Adds reported_deleted to represent presence/absence of the deleted flag. |
src/v/pandaproxy/schema_registry/rest_client/parse.cc |
Parses deleted into an optional and wires it into source_schema_read. |
src/v/pandaproxy/schema_registry/rest_client/tests/parse_test.cc |
Adds/updates tests validating reported_deleted semantics for present/absent cases. |
src/v/cluster_link/schema_registry_sync/reconciler.cc |
Prefers reported_deleted when present; otherwise falls back to listing-derived soft-deletes. |
src/v/cluster_link/schema_registry_sync/mirroring_task.cc |
Updates comments to reflect the new “fallback” role of the listing-derived set. |
src/v/cluster_link/schema_registry_sync/http_source_reader.cc |
Updates comments clarifying that reported_deleted is carried through to the reconciler. |
src/v/cluster_link/schema_registry_sync/tests/sr_sync_test_fixtures.h |
Extends the fake source to model sources that omit the deleted flag. |
src/v/cluster_link/schema_registry_sync/tests/reconciler_test.cc |
Adds/adjusts tests for precedence rules between body-reported vs listing-derived deleted state. |
| struct source_schema_read { | ||
| stored_schema schema; | ||
| chunked_vector<unsupported_feature> unsupported; | ||
| /// The soft-delete state the source explicitly reported for this version, | ||
| /// or nullopt when the source omitted `deleted` from the body. When | ||
| /// nullopt, `schema.deleted` is only the `no` default and the real state | ||
| /// must be derived from the active-vs-deleted listing partition rather than | ||
| /// trusted from the body. | ||
| std::optional<is_deleted> reported_deleted; |
There was a problem hiding this comment.
What do you think about an alternative type definition that looks something like this, which would avoid the potential inconsistency between schema.deleted and reported_deleted altogether?
struct source_schema_read {
subject_schema schema;
schema_version version{invalid_schema_version};
schema_id id{invalid_schema_id};
/// As reported by the source; nullopt when the body omitted `deleted`.
std::optional<is_deleted> deleted;
chunked_vector<unsupported_feature> unsupported;
stored_schema into_stored(is_deleted fallback = is_deleted::no) && {
return {std::move(schema), version, id, deleted.value_or(fallback)};
}
};
There was a problem hiding this comment.
That's a much better way to do it, implemented :)
Retry command for Build#87003please wait until all jobs are finished before running the slash command |
93822d4 to
c6d1f6a
Compare
|
force-push 1: implement @pgellert suggestion
force-push 2: clean up a comment force-push 3: formatting force-push 4: align missed tests force-push 5: clang-tidy fix |
7fc13d1 to
e1824c2
Compare
parse_subject_version now distinguishes an absent `deleted` field from an explicit false via source_schema_read::deleted (a std::optional), so a caller can tell "source said active" from "source said nothing". The concrete deleted state is resolved by into_stored(), which applies a caller-supplied fallback when the source omitted the flag.
e1824c2 to
01e85e2
Compare
import_body now trusts the version body's `deleted` when the source reports it (fresher, read on a later call than the listing, and free of the listing set-difference race) and falls back to the listing-derived soft_deleted set only when the source omits it. Tests cover body-wins in both directions and the omit fallback.
01e85e2 to
3d345eb
Compare
CI test resultstest results on build#87013
|
Schema Registry API-mode sync propagates a source version's soft-delete state
to the destination. Until now the imported deleted-state was taken solely from
the listing set-difference (all − active): the per-version body could not be
trusted because a Confluent source omitted the
deletedflag from it bydefault.
Now that the SR REST client opts into Confluent's extended schema fields, the
per-version body carries
deletedreliably for both Redpanda and Confluentsources. This PR prefers that source-reported flag over the listing-derived
set: it is fresher (read on a later call than the listing) and avoids the
listing set-difference, which can race with a concurrent (un)delete on the
source. The listing-derived set is kept as a fallback for a source that still
omits the flag from the body.
Backports Required
Release Notes