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

feat: Added feast Go operator db stores support #4771

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 62 additions & 4 deletions infra/feast-operator/api/v1alpha1/featurestore_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,13 @@ type OfflineStore struct {
}

// OfflineStorePersistence configures the persistence settings for the offline store service
// +kubebuilder:validation:XValidation:rule="[has(self.file), has(self.store)].exists_one(c, c)",message="One selection required between file or store."
type OfflineStorePersistence struct {
FilePersistence *OfflineStoreFilePersistence `json:"file,omitempty"`
FilePersistence *OfflineStoreFilePersistence `json:"file,omitempty"`
DBPersistence *OfflineStoreDBStorePersistence `json:"store,omitempty"`
}

// OfflineStorePersistence configures the file-based persistence for the offline store service
// OfflineStoreFilePersistence configures the file-based persistence for the offline store service
type OfflineStoreFilePersistence struct {
// +kubebuilder:validation:Enum=dask;duckdb
Type string `json:"type,omitempty"`
Expand All @@ -91,15 +93,35 @@ var ValidOfflineStoreFilePersistenceTypes = []string{
"duckdb",
}

// OfflineStoreDBStorePersistence configures the DB store persistence for the offline store service
type OfflineStoreDBStorePersistence struct {
// +kubebuilder:validation:Enum=snowflake.offline;bigquery;redshift;spark;postgres;feast_trino.trino.TrinoOfflineStore;redis
Type string `json:"type,omitempty"`
SecretRef *corev1.LocalObjectReference `json:"secretRef,omitempty"`
Comment on lines +99 to +100
Copy link
Contributor

Choose a reason for hiding this comment

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

should these be required fields?

Suggested change
Type string `json:"type,omitempty"`
SecretRef *corev1.LocalObjectReference `json:"secretRef,omitempty"`
Type string `json:"type"`
SecretRef *corev1.LocalObjectReference `json:"secretRef"`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmartinol @lokeshrangineni @redhatHameed
I am not sure, what do you think ? anyone ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dmartinol gave a thumbs up. @tmihalac from a code & usability standpoint... aren't these 2 fields required if the user decides to configure a db store? (i believe they are) if so, we need to make them required in the CRD.

SecretKeyName string `json:"secretKeyName,omitempty"`
dmartinol marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

@tchughesiv tchughesiv Nov 21, 2024

Choose a reason for hiding this comment

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

maybe add a comment to SecretKeyName explaining what the default will be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tchughesiv
What do you mean by default ? there is no default , do you mean when that field is not specified in the CR ?

Copy link
Contributor

Choose a reason for hiding this comment

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

do you mean when that field is not specified in the CR ?

yes ... isn't the default here the db store type? i'm saying just add a comment above the field (which will add documentation to the CRD) so the user understands this

}

var ValidOfflineStoreDBStorePersistenceTypes = []string{
"snowflake.offline",
"bigquery",
"redshift",
"spark",
"postgres",
"feast_trino.trino.TrinoOfflineStore",
"redis",
}

// OnlineStore configures the deployed online store service
type OnlineStore struct {
ServiceConfigs `json:",inline"`
Persistence *OnlineStorePersistence `json:"persistence,omitempty"`
}

// OnlineStorePersistence configures the persistence settings for the online store service
// +kubebuilder:validation:XValidation:rule="[has(self.file), has(self.store)].exists_one(c, c)",message="One selection required between file or store."
type OnlineStorePersistence struct {
FilePersistence *OnlineStoreFilePersistence `json:"file,omitempty"`
FilePersistence *OnlineStoreFilePersistence `json:"file,omitempty"`
DBPersistence *OnlineStoreDBStorePersistence `json:"store,omitempty"`
}

// OnlineStoreFilePersistence configures the file-based persistence for the offline store service
Expand All @@ -111,6 +133,28 @@ type OnlineStoreFilePersistence struct {
PvcConfig *PvcConfig `json:"pvc,omitempty"`
}

// OnlineStoreDBStorePersistence configures the DB store persistence for the offline store service
type OnlineStoreDBStorePersistence struct {
// +kubebuilder:validation:Enum=snowflake.online;redis;ikv;datastore;dynamodb;bigtable;postgres;cassandra;mysql;hazelcast;singlestore
Type string `json:"type,omitempty"`
SecretRef *corev1.LocalObjectReference `json:"secretRef,omitempty"`
SecretKeyName string `json:"secretKeyName,omitempty"`
Comment on lines +139 to +141
Copy link
Contributor

Choose a reason for hiding this comment

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

same

}

var ValidOnlineStoreDBStorePersistenceTypes = []string{
"snowflake.online",
"redis",
"ikv",
"datastore",
"dynamodb",
"bigtable",
"postgres",
"cassandra",
"mysql",
"hazelcast",
"singlestore",
}

// LocalRegistryConfig configures the deployed registry service
type LocalRegistryConfig struct {
ServiceConfigs `json:",inline"`
Expand All @@ -119,7 +163,8 @@ type LocalRegistryConfig struct {

// RegistryPersistence configures the persistence settings for the registry service
type RegistryPersistence struct {
FilePersistence *RegistryFilePersistence `json:"file,omitempty"`
FilePersistence *RegistryFilePersistence `json:"file,omitempty"`
DBPersistence *RegistryDBStorePersistence `json:"store,omitempty"`
}

// RegistryFilePersistence configures the file-based persistence for the registry service
Expand All @@ -133,6 +178,19 @@ type RegistryFilePersistence struct {
S3AdditionalKwargs *map[string]string `json:"s3_additional_kwargs,omitempty"`
}

// RegistryDBStorePersistence configures the DB store persistence for the registry service
type RegistryDBStorePersistence struct {
// +kubebuilder:validation:Enum=sql;snowflake.registry
Type string `json:"type,omitempty"`
SecretRef *corev1.LocalObjectReference `json:"secretRef,omitempty"`
SecretKeyName string `json:"secretKeyName,omitempty"`
Comment on lines +184 to +186
Copy link
Contributor

Choose a reason for hiding this comment

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

same?

}

var ValidRegistryDBStorePersistenceTypes = []string{
"snowflake.registry",
"sql",
}

// PvcConfig defines the settings for a persistent file store based on PVCs.
dmartinol marked this conversation as resolved.
Show resolved Hide resolved
// We can refer to an existing PVC using the `Ref` field, or create a new one using the `Create` field.
// +kubebuilder:validation:XValidation:rule="[has(self.ref), has(self.create)].exists_one(c, c)",message="One selection is required between ref and create."
Expand Down
75 changes: 75 additions & 0 deletions infra/feast-operator/api/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading