Schema registry design proposal #511

slinkydeveloper · 2023-06-16T10:35:37Z

Part of #43.

This PR contains a design proposal for the schema registry. The idea of the next steps is the following:

This package hopefully will contain little to nothing "public types", except Schemas and the interfaces. All the other types should live in restate_types, as described in Refactor common #420
Schemas will get an implementation of each of the defined interfaces, all of them sharing the same tree of Endpoint/Service/DescriptorPool metadata under a single ArcSwap. We can also reason about potential optimizations using thread locals, as described here: Investigate whether Arc<ArcSwap> for accessing shuffle senders in network is more efficient than Arc<Mutex> #136 (comment)
We get rid of restate_service_metadata and restate_key_extractor modules
We implement the write side of the schema in Schemas#register_new_endpoint. This will contain down the road the logic for:
- Register a new service
- Update a service
- Schema inconsistency checks
- Registration in "Development mode"
This Schemas object in the distributed mode will be updated by the "node service" somehow, perhaps not through register_new_endpoint but a rather simple method that will apply some "diff" calculated by the meta service. Perhaps moving forward register_new_endpoint will be split between doing checks, calculating the diff and then applying the diff itself.

tillrohrmann

The idea of the schema package make sense to me. Having a single source Schemas which contains the schema information and then exposes the different views to it via traits sounds like a good idea to me. This will hopefully help us keeping schema information consistent.

Once we go distributed then there will probably be a small difference between the schema registry running on the meta and the ones being local to the worker which is that the latter might want to ask the former for an update if a resolution operation failed because some information was missing.

slinkydeveloper · 2023-06-30T13:12:59Z

@tillrohrmann this PR is ready to be reviewed again I think. I updated it with the latest changes and rebased on #551.

tillrohrmann

Thanks for creating this PR @slinkydeveloper. The changes make sense to me. I had a few minor comments. +1 for merging this PR once they are resolved and the preceding PR gets merged.

src/schema/src/endpoint.rs

tillrohrmann · 2023-07-03T08:29:20Z

src/schema/src/proto_symbol.rs

+//  We should try to not leak prost_reflect types as they can be problematic to implement
+//  the consistent schema view in future.


Can you add an explanation why this is the case?

This comment doesn't apply with last commits.

src/types/src/identifiers.rs

tillrohrmann · 2023-07-03T08:39:47Z

src/schema_impl/build.rs

+    // I need to run this twice to make sure I split the file descriptor set between prod and test
+    prost_build::Config::new()
+        .file_descriptor_set_path(
+            PathBuf::from(env::var("OUT_DIR").expect("OUT_DIR environment variable not set"))
+                .join("file_descriptor_set_test.bin"),
+        )
+        .bytes(["."])
+        .compile_protos(&["tests/proto/test.proto"], &["tests/proto"])?;


I guess this can't be within a if cfg!(test) or so?

No, build.rs is once per project: rust-lang/cargo#1581 rust-lang/cargo#1430

src/schema_impl/src/lib.rs

src/schema_impl/src/proto_symbol.rs

src/schema_impl/src/lib.rs

tillrohrmann · 2023-07-03T09:38:42Z

src/schema_impl/src/proto_symbol.rs

@@ -89,7 +89,7 @@ impl ProtoSymbols {
            for file in &files {
                // We rename files prepending them with the endpoint id
                // to avoid collision between file names of unrelated endpoints
-                // TODO this should probably go in https://github.com/restatedev/restate/issues/43
+                // TODO with schema checks in place, should we move this or remove this?


Remove prepending the endpoint_id you mean?

yep, changed the comment to specify that.

Designed the various interfaces around schema registry. Define `Schemas`, the schema registry. Port most of the code from `restate_key_extraction`.

Implement the logic to register a endpoint in `register_new_endpoint`. This is copy pasted to meta::Service::reload

…s copy-pasted from restate_service_key_extractor

…e logic to parse InvokeRequest from the ingress.

…ftovers)

slinkydeveloper requested a review from tillrohrmann June 16, 2023 10:35

slinkydeveloper force-pushed the issues/43 branch from 1ebf613 to 91b7d27 Compare June 16, 2023 10:46

tillrohrmann reviewed Jun 21, 2023

View reviewed changes

slinkydeveloper force-pushed the issues/43 branch from 91b7d27 to bbd8b36 Compare June 30, 2023 10:19

slinkydeveloper requested a review from tillrohrmann June 30, 2023 13:12

slinkydeveloper force-pushed the issues/43 branch from a76f259 to bb3a239 Compare June 30, 2023 13:13

tillrohrmann approved these changes Jul 3, 2023

View reviewed changes

slinkydeveloper force-pushed the issues/43 branch from 91d6d96 to c02b103 Compare July 4, 2023 14:30

slinkydeveloper mentioned this pull request Jul 4, 2023

Integrate the new schema registry #559

Merged

slinkydeveloper force-pushed the issues/43 branch from 5706bcc to 50b8f0f Compare July 5, 2023 07:37

slinkydeveloper added 14 commits July 6, 2023 16:06

Introduced restate_schema.

da6775f

Designed the various interfaces around schema registry. Define `Schemas`, the schema registry. Port most of the code from `restate_key_extraction`.

A bunch of changes after refactoring

fa59a37

Split schema crate in schema_api and schema_impl (in the next commit)

cb86972

Scaffold schema_impl crate.

72bc8e3

Implement the logic to register a endpoint in `register_new_endpoint`. This is copy pasted to meta::Service::reload

Moved key extraction/expansion/json conversion in schema_impl. This i…

612e3e6

…s copy-pasted from restate_service_key_extractor

Implement EndpointMetadataResolver

a0937c4

Implement JsonMapperResolver. This will be used by the ingress.

17676e9

Load in memory the descriptor of Restate built-in services.

11a8b8e

Implement ProtoSymbols. This is copy paste from ingress::reflections

0fed3ec

Implement ingress built-in services in the schema registry. Copied th…

21321e4

…e logic to parse InvokeRequest from the ingress.

Feedback

470881e

Change load_full to load

47ced28

Move EndpointId to restate_types::identifiers

84439e7

Remove KeyExtractor/KeyExpander trait from schema_impl (these were le…

b42d9cc

…ftovers)

slinkydeveloper force-pushed the issues/43 branch from 50b8f0f to b42d9cc Compare July 6, 2023 14:07

slinkydeveloper merged commit 9a546e0 into restatedev:main Jul 6, 2023

slinkydeveloper deleted the issues/43 branch July 6, 2023 15:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Schema registry design proposal #511

Schema registry design proposal #511

slinkydeveloper commented Jun 16, 2023

tillrohrmann left a comment

slinkydeveloper commented Jun 30, 2023

tillrohrmann left a comment

tillrohrmann Jul 3, 2023

slinkydeveloper Jul 3, 2023

tillrohrmann Jul 3, 2023

slinkydeveloper Jul 3, 2023

tillrohrmann Jul 3, 2023

slinkydeveloper Jul 3, 2023

		// We should try to not leak prost_reflect types as they can be problematic to implement
		// the consistent schema view in future.

Schema registry design proposal #511

Schema registry design proposal #511

Conversation

slinkydeveloper commented Jun 16, 2023

tillrohrmann left a comment

Choose a reason for hiding this comment

slinkydeveloper commented Jun 30, 2023

tillrohrmann left a comment

Choose a reason for hiding this comment

tillrohrmann Jul 3, 2023

Choose a reason for hiding this comment

slinkydeveloper Jul 3, 2023

Choose a reason for hiding this comment

tillrohrmann Jul 3, 2023

Choose a reason for hiding this comment

slinkydeveloper Jul 3, 2023

Choose a reason for hiding this comment

tillrohrmann Jul 3, 2023

Choose a reason for hiding this comment

slinkydeveloper Jul 3, 2023

Choose a reason for hiding this comment