Skip to content

Support catalog register new schema version#1246

Merged
jfallows merged 4 commits intoaklivity:developfrom
akrambek:feature/register-schema
Sep 16, 2024
Merged

Support catalog register new schema version#1246
jfallows merged 4 commits intoaklivity:developfrom
akrambek:feature/register-schema

Conversation

@akrambek
Copy link
Contributor

@akrambek akrambek commented Sep 13, 2024

Description

Support catalog register new schema version.

Fixes #1060

@akrambek akrambek requested a review from jfallows September 13, 2024 16:05
public class SchemaRegistryCatalogHandler implements CatalogHandler
{
private static final String SUBJECT_VERSION_PATH = "/subjects/{0}/versions/{1}";
private static final String SUBJECT_PATH = "subjects/{0}/versions";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing leading slash?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, no slash required. Spec is also correct accept "http://localhost:8081/subjects/items-snapshots-value/versions"

Copy link
Contributor

@jfallows jfallows Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does SUBJECT_VERSION_PATH start with /subjects/... but SUBJECT_PATH starts with subjects/...?
Seems like these should be consistent, and paths tend to start with /, no?
It's possible that the HTTP client is conveniently injecting the / for us as a workaround for missing leading slash perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I see what you mean I got it wrong yep :)

Comment on lines +65 to +71
default int register(
String subject,
String schema)
{
return NO_VERSION_ID;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent.

catalog:
catalog0:
- subject: items-snapshots-value
record: |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

record is something else, this should be schema.

Comment on lines +350 to +353
if (catalog.subject != null && catalog.record != null)
{
handler.register(catalog.subject, catalog.record);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (catalog.subject != null && catalog.record != null)
{
handler.register(catalog.subject, catalog.record);
}
if (catalog.subject != null && catalog.schema != null)
{
handler.register(catalog.subject, catalog.schema);
}

jfallows
jfallows previously approved these changes Sep 13, 2024
@jfallows jfallows merged commit 00e71fd into aklivity:develop Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support catalog register new schema version

2 participants