GeezerVison is showing github.com — FeatureRow proto to wrap FeatureRowKey and FeatureRowMessage · Issue #13 · feast-dev/feast
FeatureRow proto to wrap FeatureRowKey and FeatureRowMessage #13DescriptionSince Feast should support the ingestion of data from Kafka topics (in the format of KV) using KafkaIO, the FeatureRow proto should be wrapped to have FeatureRowKey (K) and FeatureRowMessage (V). Activitywoop commented on Dec 24, 2018 @baskaranz can you propose an example of the proto format that you are suggesting? What would be contained within each of the FeatureRowKey and FeatureRowMessage? baskaranz commented on Dec 24, 2018 ContributorAuthor More actions @woop Here's what is being proposed for the FeatureRow proto.
The FeatureRowKey having all the elements from FeatureRowMessage except/minus the features element.
message FeatureRowKey {
string entityKey = 1;
google.protobuf.Timestamp eventTimestamp = 3;
string entityName = 4;
Granularity.Enum granularity = 5;
}message FeatureRowMessage {
string entityKey = 1;
**repeated Feature features = 2;**
google.protobuf.Timestamp eventTimestamp = 3;
string entityName = 4;
Granularity.Enum granularity = 5;
}Edit: Added formatting for code block woop commented on Dec 24, 2018 @baskaranz, I am assuming that this would replace the existing message format for Pub/Sub, meaning this would be the new standard, right?
Does this impact Pub/Sub at all? If not, then the format looks fine to me.
@tims, can you have a look? tims commented on Dec 26, 2018 It looks like it wouldn't effect PubSub, since you are just renaming FeatureRow to FeatureRowMessage.
What is the key used for in Kafka? If you leave the key as null, the message will be auto assigned to a random partition anyway no? and if you want to use the key to assign partitions, you need to ensure they are well distributed which this key does not (as entityKey is not under Feast's control and often a sequentially assigned integer).
Feast wouldn't care much for what the key was, since it's all in the message anyway. I'm not sure there's a real need for the key to be a proto in feast, or a proto at all.
I'm not necessarily saying we shouldn't do this, but I'd like to know more about the why.
Also, are you working on Kafka support already? if so can you make an issue for it. woop commented on Jan 3, 2019 It seems like it's just for partitioning, so leaving it as null would be a valid approach, at least for now. @ravisuhag can you see any downsides?
Kafka issue: #22 tims commented on Jan 3, 2019 Can we close this now, I don't see a need to change FeatureRow to FeatureRowMessage. And we've already merged in a FeatureRowKey proto. woop commented on Jan 3, 2019 We could close it, but are we happy leaving the code base as is? If we don't need FeatureRowKey, then shouldn't we remove it and add support for null keys during ingestion?
Or should that be a separate issue? @tims tims commented on Jan 10, 2019 I'll add a note to the kafka source issue and close this one.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|