[CORE-60] Schema Registry: Support `/mode` #17952

BenPope · 2024-04-18T21:19:33Z

Support /mode

TODO:

More Auth tests
Wire-up is_mutable a bit more
Return an error if the mode is incompatible with the operation

Backports Required

Release Notes

Features

Schema Registry: Support /mode endpoints

Signed-off-by: Ben Pope <ben@redpanda.com>

No functional changes Signed-off-by: Ben Pope <ben@redpanda.com>

Signed-off-by: Ben Pope <ben@redpanda.com>

dotnwat · 2024-04-19T04:55:16Z

src/v/pandaproxy/schema_registry/types.h

+template<>
+constexpr std::optional<mode> from_string_view<mode>(std::string_view sv) {
+    return string_switch<std::optional<mode>>(sv)
+      .match(to_string_view(mode::import), mode::import)


i like how to_string_view gets reused here. nice and clena

dotnwat · 2024-04-19T05:06:09Z

src/v/pandaproxy/schema_registry/sharded_store.cc

+    auto sub_shard{shard_for(sub)};
+    co_return co_await _store.invoke_on(
+      sub_shard, _smp_opts, [marker, sub{std::move(sub)}, m, f](store& s) {
+          return s.set_mode(marker, sub, m, f).value();


i guess our default result<> configuration has a policy that throws on .value() (rather than what I always assume to be undefined behavior if no value exists) ?

It's not default; but it does throw.

dotnwat · 2024-04-19T05:06:49Z

src/v/pandaproxy/schema_registry/types.h

@@ -325,6 +331,9 @@ constexpr std::string_view to_string_view(seq_marker_key_type v) {
    case seq_marker_key_type::config:
        return "config";
        break;
+    case seq_marker_key_type::mode:
+        return "mode";
+        break;


break unused here

michael-redpanda

looks nice, a few questions

michael-redpanda · 2024-04-22T13:46:42Z

src/v/pandaproxy/schema_registry/store.h

+    ///\brief Get the mode for a subject, or fallback to global.
+    result<mode>
+    get_mode(const subject& sub, default_to_global fallback) const {
+        auto sub_it = get_subject_iter(sub, include_deleted::yes);


qq: Is include_deleted::yes correct?

michael-redpanda · 2024-04-22T13:47:16Z

src/v/pandaproxy/schema_registry/store.h

+        } else if (fallback) {
+            return _mode;
+        }
+        return mode_not_found(sub);


Should this be subject_not_found rather than mode_not_found? Should mode_not_found be returned if the subject exists and fallback is false?

Should this be subject_not_found rather than mode_not_found? Should mode_not_found be returned if the subject exists and fallback is false?

The mode configuration can exist without a subject. This is required so that a subject can be placed into import without the entire registry being in import.

michael-redpanda · 2024-04-22T13:54:32Z

src/v/pandaproxy/schema_registry/store.h

+    result<void> check_mutable(force f) const {
+        if (!_mutable && !f) {


question: Isn't the force flag for if there are already registered schemas and the mode is going into IMPORT?

michael-redpanda · 2024-04-22T17:23:19Z

src/v/pandaproxy/api/api-doc/schema_registry_definitions.def.json

+      "type": "object",
+      "properties": {
+        "mode": {
+          "type": "string"


qq: can this be an enum?

michael-redpanda · 2024-04-22T17:25:03Z

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

+          {
+            "name": "defaultToGlobal",
+            "in": "query",
+            "required": false,
+            "type": "boolean"
+          }


question: I don't see this in the API. Is this an undocumented option?

oleiman

lgtm. few questions mostly for my own benefit.

oleiman · 2024-04-20T04:51:52Z

src/v/pandaproxy/schema_registry/test/consume_to_store.cc

@@ -202,7 +202,7 @@ model::record_batch as_record_batch(Key key) {

 SEASTAR_THREAD_TEST_CASE(test_consume_to_store_after_compaction) {
    pps::sharded_store s;
-    s.start(ss::default_smp_service_group()).get();
+    s.start(pps::is_mutable::no, ss::default_smp_service_group()).get();


question: why sometimes is_mutable::no? branch coverage?

oleiman · 2024-04-22T17:57:07Z

src/v/pandaproxy/schema_registry/seq_writer.cc

+    auto mode = co_await ss::coroutine::as_future(
+      _store.get_subject_mode_written_at(sub));
+    if (!mode.failed()) {
+        rb(mode.get());
+    }
+


is it generally advisable to explicitly get_exception (or discard or whatever) on the other side of the branch here?

is it generally advisable to explicitly get_exception (or discard or whatever) on the other side of the branch here?

Yes, you need to eat the exception out of ready future to avoid abandoned exceptional future.

same below for conf

EDIT: for conf i think I just noticed this in the PR that introduced that #17905 (review).

oleiman · 2024-04-22T18:03:21Z

src/v/pandaproxy/schema_registry/seq_writer.cc

+    try {
+        co_await seq._store.get_mode(sub, default_to_global::no);
+    } catch (const exception&) {
+        // subject config already blank


nitpick:

Suggested change

// subject config already blank

// subject mode already blank

dotnwat · 2024-04-23T17:14:22Z

src/v/pandaproxy/schema_registry/seq_writer.cc

+    auto mode = co_await ss::coroutine::as_future(
+      _store.get_subject_mode_written_at(sub));
+    if (!mode.failed()) {
+        rb(mode.get());
+    }
+


is it generally advisable to explicitly get_exception (or discard or whatever) on the other side of the branch here?

Yes, you need to eat the exception out of ready future to avoid abandoned exceptional future.

same below for conf

EDIT: for conf i think I just noticed this in the PR that introduced that #17905 (review).

BenPope added 14 commits April 18, 2024 21:47

pandaproxy/error: Introduce error_codes for mode

cfa3473

Signed-off-by: Ben Pope <ben@redpanda.com>

schema_registry/error: Introduce error_codes for mode

0ee456a

Signed-off-by: Ben Pope <ben@redpanda.com>

schema_registry/types: Introduce is_mutable

a831586

Signed-off-by: Ben Pope <ben@redpanda.com>

schema_registry/config: Introduce mode_mutability

840b399

Signed-off-by: Ben Pope <ben@redpanda.com>

schema_registry: Wire-up mode_mutability

2b359b5

Signed-off-by: Ben Pope <ben@redpanda.com>

schema_registry/types: Introduce mode

ee61835

Signed-off-by: Ben Pope <ben@redpanda.com>

schema_registry/store: Support mode

57720d7

Signed-off-by: Ben Pope <ben@redpanda.com>

schema_registry/types: Add sequence_marker_type::mode

5225b6e

Signed-off-by: Ben Pope <ben@redpanda.com>

schema_registry/storage: Support consuming mode records

331a792

Signed-off-by: Ben Pope <ben@redpanda.com>

schema_registry/storage: Support writing mode records

60768c2

Signed-off-by: Ben Pope <ben@redpanda.com>

schema_registry: Support mode endpoints

3e424d3

Signed-off-by: Ben Pope <ben@redpanda.com>

test 3rdparty compat

a9c465e

Signed-off-by: Ben Pope <ben@redpanda.com>

schema_registry/dt: Refactor SchemaRegistryBasicAuthTest

fa2dd06

No functional changes Signed-off-by: Ben Pope <ben@redpanda.com>

schema_registry: Require superuser to mutate /mode

070f318

Signed-off-by: Ben Pope <ben@redpanda.com>

BenPope requested a review from a team as a code owner April 18, 2024 21:19

BenPope self-assigned this Apr 18, 2024

BenPope requested review from oleiman and michael-redpanda and removed request for a team and oleiman April 18, 2024 21:19

github-actions bot added the area/redpanda label Apr 18, 2024

BenPope added area/schema-registry Schema Registry service within Redpanda and removed area/redpanda labels Apr 18, 2024

BenPope changed the title ~~Schema Registry: Support /mode~~ [CORE-60] Schema Registry: Support /mode Apr 18, 2024

BenPope mentioned this pull request Apr 18, 2024

Schema Registry features meta-issue #1878

Open

34 tasks

dotnwat reviewed Apr 19, 2024

View reviewed changes

michael-redpanda reviewed Apr 22, 2024

View reviewed changes

oleiman approved these changes Apr 22, 2024

View reviewed changes

dotnwat requested changes Apr 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CORE-60] Schema Registry: Support `/mode` #17952

[CORE-60] Schema Registry: Support `/mode` #17952

BenPope commented Apr 18, 2024 •

edited

dotnwat Apr 19, 2024

dotnwat Apr 19, 2024

BenPope Apr 23, 2024

dotnwat Apr 19, 2024

michael-redpanda left a comment

michael-redpanda Apr 22, 2024

michael-redpanda Apr 22, 2024

BenPope Apr 23, 2024

michael-redpanda Apr 22, 2024

michael-redpanda Apr 22, 2024

michael-redpanda Apr 22, 2024

oleiman left a comment

oleiman Apr 20, 2024

oleiman Apr 22, 2024

dotnwat Apr 23, 2024 •

edited

oleiman Apr 22, 2024

dotnwat Apr 23, 2024 •

edited

		result<void> check_mutable(force f) const {
		if (!_mutable && !f) {

	// subject config already blank
	// subject mode already blank

[CORE-60] Schema Registry: Support /mode #17952

Are you sure you want to change the base?

[CORE-60] Schema Registry: Support /mode #17952

Conversation

BenPope commented Apr 18, 2024 • edited

Backports Required

Release Notes

Features

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

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

Choose a reason for hiding this comment

oleiman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotnwat Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotnwat Apr 23, 2024 • edited

Choose a reason for hiding this comment

[CORE-60] Schema Registry: Support `/mode` #17952

[CORE-60] Schema Registry: Support `/mode` #17952

BenPope commented Apr 18, 2024 •

edited

dotnwat Apr 23, 2024 •

edited

dotnwat Apr 23, 2024 •

edited