Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support catalog register new schema version #1246

Merged
merged 4 commits into from
Sep 16, 2024

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
@@ -42,6 +42,7 @@
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
5 checks passed
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