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

[#5817] core(feat): Add server-side REST APIs for model management #5948

Merged
merged 5 commits into from
Dec 26, 2024

Conversation

jerryshao
Copy link
Contributor

What changes were proposed in this pull request?

This PR adds the server-side REST endpoint for model management.

Why are the changes needed?

This is a part of model management for Gravitino.

Fix: #5817

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add UTs for this PR.

@jerryshao jerryshao requested review from FANNG1 and mchades December 23, 2024 06:14
@jerryshao jerryshao self-assigned this Dec 23, 2024
Preconditions.checkArgument(
StringUtils.isNotBlank(alias), "alias must not be null or empty");
Preconditions.checkArgument(
!NumberUtils.isCreatable(alias), "alias must not be a number or a number string");
Copy link
Contributor

Choose a reason for hiding this comment

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

why alias must not be a number or a number string? I don't see the limitation comment at the ModelVersion API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is because of the current implementation limitation, both the version and the alias will be treated as a part of NameIdentifier, if the name can be parsed as a number, then it will be treated as a version number, to avoid this ambiguity, I added this limitation. I can add the javadoc if needed.

docs/open-api/openapi.yaml Show resolved Hide resolved
mchades
mchades previously approved these changes Dec 26, 2024
Copy link
Contributor

@mchades mchades left a comment

Choose a reason for hiding this comment

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

LGTM.

@mchades mchades dismissed their stale review December 26, 2024 04:23

plz resolve the conflicts first

@mchades mchades merged commit a68e5e2 into apache:main Dec 26, 2024
26 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.

[Subtask]Implement the server rest endpoint for model catalog.
3 participants