schema_registry: Add support for `DELETE /config/{subject}` #13557

oleiman · 2023-09-20T00:43:07Z

Introduces DELETE /config/{subject} endpoint for Schema Registry.

This makes use of (mostly) existing primitives on the sequenced write path to avoid duplicating critical code. Key observation is that a config write message with k: {subject}, v: std::nullopt results in durably (I think) clearing the compatibility config for that subject.

Fixes #13229

Backports Required

Release Notes

Features

Schema Registry: Add DELETE /config/{subject} endpoint

src/v/pandaproxy/schema_registry/sharded_store.cc

src/v/pandaproxy/schema_registry/storage.h

src/v/pandaproxy/api/api-doc/schema_registry.json

src/v/pandaproxy/schema_registry/handlers.cc

BenPope · 2023-09-21T14:04:45Z

src/v/pandaproxy/schema_registry/sharded_store.h

@@ -118,6 +118,10 @@ class sharded_store {
    ss::future<compatibility_level>
    get_compatibility(subject sub, default_to_global fallback);

+    ///\brief Check the compatibility level for a subject w/o fallback.
+    /// If none exists, the underlying store returns subject_not_found
+    ss::future<compatibility_level> check_compatibility(subject sub);


I don't love this name because it feels a lot like check_compatible. Naming is hard!.

Perhaps the conversion can be done in the handler?

Yeah, that's what I had in my first draft. Less code better code.

I agree, perhaps the check could be done within seq_writer::do_delete_config and that method can return a result<optional<bool>>?

I agree, perhaps the check could be done within seq_writer::do_delete_config and that method can return a result<optional>?

Specially since it seems that check_compatible is only used for the delete path.

Removed the extraneous function. Since the seq_writer implementations need to return bool, we'll still need to call get_compatibility from the handler (I think) in order to conform to the API. Am I reading that right?

Oh, I see delete_subject_permanent_inner does something similar to what you're describing @NyaliaLui. Makes sense.

Played around with this a bit, and I think performing the check up in the handler is more or less in line with surrounding code (and less of a pain). Happy to revisit if that interpretation sounds wrong.

BenPope · 2023-09-21T14:09:50Z

src/v/pandaproxy/schema_registry/seq_writer.cc

+    }
+
+    auto key = config_key{.seq{write_at}, .node{seq._node_id}, .sub{sub}};
+    auto batch = as_record_batch(key, std::nullopt);


I think we need to build tombstones so that the original config can be compacted, you may take some inspiration from delete_subject_permanent_inner

Ack. Will put this in a follow on commit. Need a bit of time to grok.

nit: I would squash that commit with this one

src/v/pandaproxy/schema_registry/seq_writer.cc

oleiman · 2023-09-21T18:00:22Z

src/v/pandaproxy/schema_registry/handlers.cc

+    // ensure we see latest writes
+    co_await rq.service().writer().read_sync();
+
+    compatibility_level lvl;


Not very nice. Normally I'd put a lambda or something around the try/catch, but I don't think that works here because coro. Helper function?

michael-redpanda

looks good, just a couple of comments

src/v/pandaproxy/schema_registry/seq_writer.cc

tests/rptest/tests/schema_registry_test.py

michael-redpanda · 2023-09-27T20:31:55Z

src/v/pandaproxy/schema_registry/seq_writer.cc

+    }
+
+    auto key = config_key{.seq{write_at}, .node{seq._node_id}, .sub{sub}};
+    auto batch = as_record_batch(key, std::nullopt);


nit: I would squash that commit with this one

vbotbuildovich · 2023-09-28T00:52:34Z

ducktape was retried in job https://buildkite.com/redpanda/redpanda/builds/37777#018ad906-125c-4bf4-a1fe-d2388fe70bf5

michael-redpanda · 2023-09-28T18:21:23Z

src/v/pandaproxy/schema_registry/seq_writer.cc

+    // co_return true;
+
+    // TODO(oren): seems like this might not be necessary?


chore: remove commented out code and is this a TODO where we need to file a ticket?

Ah, meant to transform this into a PR comment. This is an open question from my perspective, maybe for @BenPope? After we've tombstoned the history of this subject's config, do we still need to finally write an empty record at the front?

michael-redpanda

lgtm

NyaliaLui

lgtm

oleiman · 2023-09-29T05:19:18Z

/ci-repeat 1

BenPope

Nice. I'm seeing correct tombstoning now.

BenPope · 2023-10-18T18:41:45Z

src/v/pandaproxy/schema_registry/store.h

+                return sm.key_type == marker.key_type && sm.seq && marker.seq
+                       && *sm.seq == *marker.seq;


Like this?

std::erase_if(sub_it->second.written_at, [marker](auto sm) { return sm.key_type == marker.key_type && sm.seq == marker.seq; });

It's possible to have read a "compatible" config on the topic - the compat format has no seq or node, e.g.:

{ "topic": "_schemas", "key": "{\"keytype\":\"CONFIG\",\"subject\":\"foo\",\"magic\":0}", "value": "{\"compatibilityLevel\":\"BACKWARD\"}", "timestamp": 1697654666281, "partition": 0, "offset": 5 }

In which case the config_key::seq will be nullopt, which we want to match.

But why not match on node as well?

std::erase_if(sub_it->second.written_at, [marker](auto sm) { return sm.key_type == marker.key_type && sm.seq == marker.seq && sm.node == marker.node; });

But of course, I'm not opposed to adding seq_marker::operator== (with a comment that matching nullopts are intentional due to the "compatible" records not having seq or node)

BenPope · 2023-10-18T18:58:13Z

src/v/pandaproxy/schema_registry/seq_writer.cc

+      model::record_batch_type::raft_data, model::offset{0}};
+
+    std::vector<config_key> keys;
+    for (auto s : sequences) {


Yeah I see what you mean. This change (current branch HEAD) seems to fix it if somewhat naively. Look reasonable to you?

It's gone, but if it's the last commit, it looks good.

BenPope · 2023-10-18T19:24:49Z

since this is a feature, why are we backporting it?

Last time I added one of these the backport justification was "so console can have uniform access to it right away". That was a GET endpoint though with a clean cherry pick. Worth discussing in this case for sure.

This is also a request from Console team. It's a cross between a feature and a bugfix, since deleting a config is a necessary operation. I often backport this type of fix, since the change almost never touches existing code, so the chance of a regression is minimal.

BenPope · 2023-10-19T17:03:58Z

src/v/pandaproxy/schema_registry/types.h

+    bool operator==(const seq_marker& other) const {
+        return key_type == other.key_type && seq == other.seq
+               && node == other.node;
+    }


I think the general case for operator== should include the version

Suggested change

bool operator==(const seq_marker& other) const {

return key_type == other.key_type && seq == other.seq

&& node == other.node;

}

friend bool operator==(const seq_marker&, const seq_marker&) = default;

BenPope · 2023-10-19T17:08:01Z

src/v/pandaproxy/schema_registry/store.h

+        std::erase_if(sub_it->second.written_at, [marker](const auto& sm) {
+            return marker == sm;
+        });


Suggested change

std::erase_if(sub_it->second.written_at, [marker](const auto& sm) {

return marker == sm;

});

std::erase(sub_it->second.written_at, marker);

oleiman · 2023-10-23T06:04:12Z

force push (add a comment to seq_marker::operator==

Adds `seq_writer::delete_config`, which deletes the stored compatibility level for a given subject by producing an empty-valued config write. Also adds `sharded_store::get_subject_config_written_at` which returns the sequence number history of a subject's config. This differs from `get_subject_written_at` in that the sequence markers returned cannot be used to perm delete the subject itself, only the config. Therefore, there is no need to soft-delete a subject before fetching this info.

Adds a handler for the delete endpoint. To match the behavior of the reference implementation, this endpoint: - Returns the compatibility level of the subject before deletion - OR subject_not_found if no compatibility level was previously set

vbotbuildovich · 2023-10-23T18:38:37Z

new failures detected in https://buildkite.com/redpanda/redpanda/builds/39608#018b5d95-0d4e-4cf6-b1f6-dc9d8f8299c4: "rptest.tests.follower_fetching_test.FollowerFetchingTest.test_basic_follower_fetching.read_from_object_store=False"

oleiman · 2023-10-23T19:43:44Z

CI Failures:

CI Failure (TimeoutError: cli-consumer failed to stop) in FollowerFetchingTest.test_basic_follower_fetching #14374 (NEW) - not likely related to this PR. this is just an SR API endpoint so fairly self-contained.

BenPope

I think it's worth backporting this

vbotbuildovich · 2023-10-23T23:52:36Z

/backport v23.2.x

vbotbuildovich · 2023-10-23T23:52:37Z

/backport v23.1.x

github-actions bot added the area/redpanda label Sep 20, 2023

oleiman commented Sep 20, 2023

View reviewed changes

src/v/pandaproxy/schema_registry/sharded_store.cc Outdated Show resolved Hide resolved

oleiman commented Sep 20, 2023

View reviewed changes

src/v/pandaproxy/schema_registry/storage.h Outdated Show resolved Hide resolved

oleiman requested review from BenPope, NyaliaLui and michael-redpanda September 20, 2023 00:49

oleiman marked this pull request as draft September 20, 2023 21:47

oleiman force-pushed the delete-subject-compat branch 3 times, most recently from 4a4e9cd to d10c047 Compare September 21, 2023 06:11

BenPope reviewed Sep 21, 2023

View reviewed changes

NyaliaLui reviewed Sep 21, 2023

View reviewed changes

src/v/pandaproxy/schema_registry/seq_writer.cc Outdated Show resolved Hide resolved

oleiman force-pushed the delete-subject-compat branch from d10c047 to a365944 Compare September 21, 2023 17:58

oleiman commented Sep 21, 2023

View reviewed changes

oleiman force-pushed the delete-subject-compat branch from a365944 to d972155 Compare September 22, 2023 05:24

oleiman requested review from BenPope and NyaliaLui September 22, 2023 05:26

oleiman marked this pull request as ready for review September 22, 2023 05:26

oleiman marked this pull request as draft September 24, 2023 23:44

oleiman marked this pull request as ready for review September 25, 2023 15:46

oleiman self-assigned this Sep 26, 2023

oleiman changed the title ~~Delete subject compat~~ schema_registry: Add support for DELETE /config/{subject} Sep 26, 2023

michael-redpanda reviewed Sep 27, 2023

View reviewed changes

oleiman force-pushed the delete-subject-compat branch from d972155 to 1287e06 Compare September 27, 2023 22:20

oleiman requested a review from michael-redpanda September 28, 2023 00:04

michael-redpanda reviewed Sep 28, 2023

View reviewed changes

michael-redpanda previously approved these changes Sep 28, 2023

View reviewed changes

NyaliaLui previously approved these changes Sep 29, 2023

View reviewed changes

oleiman requested review from dotnwat, BenPope, NyaliaLui and michael-redpanda October 18, 2023 15:27

BenPope reviewed Oct 18, 2023

View reviewed changes

oleiman force-pushed the delete-subject-compat branch from 82f4010 to 6b8f645 Compare October 19, 2023 02:40

oleiman requested a review from BenPope October 19, 2023 02:43

BenPope reviewed Oct 19, 2023

View reviewed changes

oleiman force-pushed the delete-subject-compat branch 2 times, most recently from 55451af to bebdc51 Compare October 23, 2023 06:03

oleiman added 4 commits October 23, 2023 09:14

schema_registry: Swagger for delete config/{subject}

c36c9ab

schema_registry: Wire up delete_config_subject

df1a49d

oleiman force-pushed the delete-subject-compat branch from bebdc51 to df1a49d Compare October 23, 2023 16:15

oleiman requested a review from BenPope October 23, 2023 19:34

BenPope approved these changes Oct 23, 2023

View reviewed changes

oleiman merged commit 4600a7a into redpanda-data:dev Oct 23, 2023
30 checks passed

github-actions bot mentioned this pull request Dec 22, 2023

update redpanda appVersion from v23.2.21 to v23.3.1 redpanda-data/helm-charts#950

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schema_registry: Add support for `DELETE /config/{subject}` #13557

schema_registry: Add support for `DELETE /config/{subject}` #13557

oleiman commented Sep 20, 2023 •

edited

Loading

BenPope Sep 21, 2023 •

edited

Loading

oleiman Sep 21, 2023

NyaliaLui Sep 21, 2023

NyaliaLui Sep 21, 2023 •

edited

Loading

oleiman Sep 21, 2023

oleiman Sep 21, 2023

oleiman Sep 26, 2023

BenPope Sep 21, 2023 •

edited

Loading

oleiman Sep 21, 2023

michael-redpanda Sep 27, 2023

oleiman Sep 21, 2023

michael-redpanda left a comment

michael-redpanda Sep 27, 2023

vbotbuildovich commented Sep 28, 2023

michael-redpanda Sep 28, 2023

oleiman Sep 28, 2023

michael-redpanda left a comment

NyaliaLui left a comment

oleiman commented Sep 29, 2023

BenPope left a comment

BenPope Oct 18, 2023 •

edited

Loading

BenPope Oct 18, 2023

BenPope commented Oct 18, 2023

BenPope Oct 19, 2023

oleiman Oct 19, 2023

BenPope Oct 19, 2023

oleiman Oct 19, 2023

oleiman commented Oct 23, 2023

vbotbuildovich commented Oct 23, 2023

oleiman commented Oct 23, 2023

BenPope left a comment

vbotbuildovich commented Oct 23, 2023

vbotbuildovich commented Oct 23, 2023

		// co_return true;

		// TODO(oren): seems like this might not be necessary?

		return sm.key_type == marker.key_type && sm.seq && marker.seq
		&& sm.seq == marker.seq;

schema_registry: Add support for DELETE /config/{subject} #13557

schema_registry: Add support for DELETE /config/{subject} #13557

Conversation

oleiman commented Sep 20, 2023 • edited Loading

Backports Required

Release Notes

Features

BenPope Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NyaliaLui Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenPope Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-redpanda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbotbuildovich commented Sep 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-redpanda left a comment

Choose a reason for hiding this comment

NyaliaLui left a comment

Choose a reason for hiding this comment

oleiman commented Sep 29, 2023

BenPope left a comment

Choose a reason for hiding this comment

BenPope Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenPope commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleiman commented Oct 23, 2023

vbotbuildovich commented Oct 23, 2023

oleiman commented Oct 23, 2023

BenPope left a comment

Choose a reason for hiding this comment

vbotbuildovich commented Oct 23, 2023

vbotbuildovich commented Oct 23, 2023

schema_registry: Add support for `DELETE /config/{subject}` #13557

schema_registry: Add support for `DELETE /config/{subject}` #13557

oleiman commented Sep 20, 2023 •

edited

Loading

BenPope Sep 21, 2023 •

edited

Loading

NyaliaLui Sep 21, 2023 •

edited

Loading

BenPope Sep 21, 2023 •

edited

Loading

BenPope Oct 18, 2023 •

edited

Loading