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

Update Feast Core API to support Project namespacing #359

Closed
wants to merge 4 commits into from

Conversation

woop
Copy link
Member

@woop woop commented Dec 11, 2019

Updated Feast Core gRPC API based on: Feast Projects RFC

This will be one of many PRs to the project-isolation branch. I am purposefully not merging this into master (as that will break master) and not building one massive PR (which will be too hard to parse).

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: woop

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@@ -112,6 +133,9 @@ message ListStoresResponse {
}

message ApplyFeatureSetRequest {
// Name of project that the feature set belongs to.
string project = 2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this becomes a mandatory parameter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

}

// Request for the archival of a project
message ArchiveProjectRequest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens to the features ingested when a project is archived?

Copy link
Member Author

Choose a reason for hiding this comment

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

It continues to work. Ingestion is unaffected.

@woop
Copy link
Member Author

woop commented Dec 11, 2019

I just realized that we should probably change the test scripts to also code gen from protos. Right now these tests are passing but they shouldnt. cc @davidheryanto @zhilingc

Updated Golang SDK to Feast Serving API
@feast-ci-bot
Copy link
Collaborator

@woop: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
test-core-and-ingestion 44a4820 link /test test-core-and-ingestion
test-serving 44a4820 link /test test-serving
test-end-to-end 44a4820 link /test test-end-to-end
test-python-sdk 44a4820 link /test test-python-sdk
test-end-to-end-batch 44a4820 link /test test-end-to-end-batch

Full PR test history

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@davidheryanto
Copy link
Collaborator

davidheryanto commented Dec 16, 2019

Should we have a spec for the project name? For example,

Project name can be up to 253 characters long. The characters allowed in names are: digits (0-9), lower case letters (a-z), -, and .

So that when we're combining project name with feature name, there won't be ambiguity (e.g. if we use colon as a separator and project name includes colon as well). And the storage system where we persist the project name information may only support certain character sets, e.g. ASCII but not Unicode. And if we plan to use the project name in some URI, probably it should be DNS friendly too?

@woop
Copy link
Member Author

woop commented Dec 16, 2019

Should we have a spec for the project name? For example,

Project name can be up to 253 characters long. The characters allowed in names are: digits (0-9), lower case letters (a-z), -, and .

So that when we're combining project name with feature name, there won't be ambiguity (e.g. if we use colon as a separator and project name includes colon as well). And the storage system where we persist the project name information may only support certain character sets, e.g. ASCII but not Unicode. And if we plan to use the project name in some URI, probably it should be DNS friendly too?

Yip, makes sense. Where should this be documented?

@woop woop closed this Dec 17, 2019
@woop woop deleted the project-isolation branch April 10, 2020 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants