feat: add store proto

[honeyAcorn]

New Store proto for Persistent Store

[anp13]

we don't need a store_id for client facing protos. We will need the store_name I think but I am not sure because in the cache get/set/delete apis we don't pass the cache_name.

@cprice404 could you comment on this?

[PDXKimani]

That rides in a header on cache APIs. That does not mean that is what we should do here, but that's how it works there. I'd personally prefer it to be in the proto, but if Chris has other thoughts I'm open to them.

[cprice404]

tbh I'd prefer it to be in the proto too. But I feel like someone must have had some motivation for putting the cache name in the header originally. @kvcache ?

If @kvcache doesn't remember a compelling reason for why it was in the header for cache then I'm #TeamProto

[kvcache]

There were compelling reasons but they no longer exist. Putting it in the message is better in this case, and most cases.

[honeyAcorn]

okay fff5d40

[anp13]

Following the same naming convention as our cache apis this should be:

bytes store_key = 1;
bytes store_value = 2;

But I am not sure if we want to name the value as store_value or store_body as we call it cache_body in the cache apis. @cprice404 thoughts?

[cprice404]

idk where body came from, value makes sense to me.

[honeyAcorn]

I think body came from cache_body in cacheclient.proto

[cprice404]

sorry, what i'm saying is that I don't know how we ultimately chose body in the cache proto. value makes more sense to me. And overall I do not feel compelled to make this proto match the cache proto, I'd rather just take everything we've learned and make this one the best it can be.

[honeyAcorn]

So we have some options:
store_id vs store_name or even remove it?
key vs store_key
value vs store_value or store_body

If we want to keep the naming convention as cacheclient,
Then it should be store_key and store_body, and no store_name? But from Dylan's reply I think having store_name won't hurt.

[cprice404]

do we want to explicitly model other data types here while we have the chance? There has been a lot of consternation about how in the cache proto we didn't model numeric values in the proto and that forced us to do weird things with them, like requiring them to be strings and then calling increment on strings. I think that if we had to do over with again we probably would have done a oneof for the values, and supported string/bytes/int/float as discrete types. cc: @kvcache

[honeyAcorn]

like this? fff5d40

[kvcache]

Suggested change

	message _ValueType {
	bytes bytes_value = 1;
	string string_value = 2;
	int64 integer_value = 3;
	double double_value = 4;
	}
	message _ValueType {
	oneof {
	bytes bytes_value = 1;
	string string_value = 2;
	int64 integer_value = 3;
	double double_value = 4;
	}
	}

[anp13]

I would call this message just Value

message _Value {
  oneof value {
    bytes bytes_value = 1;
    string string_value = 2;
    int64 integer_value = 3;
    double double_value = 4;
  }
}

Or may be we can directly use oneof in the request and response like this:

message _SetRequest {
  bytes key = 1;
  oneof value {
    bytes bytes_value = 2;
    string string_value = 3;
    int64 integer_value = 4;
    double double_value = 5;
  }
}

message _GetResponse {
  oneof value {
    bytes bytes_value = 1;
    string string_value = 2;
    int64 integer_value = 3;
    double double_value = 4;
  }
}

[honeyAcorn]

Initially I was separating like this, but then I thought, maybe it makes more sense to define them outside since we don't want these two to have chance to diverge. What do you think?

[honeyAcorn]

@cprice404 , @kvcache , @anp13 and I had a meeting to talk about this. Here's the summary:

1. We have the store_name in header instead of proto message.
2. We will change the bytes key to string key for more clarity
3. We will not need hit/miss for GET cause this is not like cache, store don't have ttl concept. We will throw NOT FOUND grpc error if the key doesn't exist.
4. We will first try to change to bytes value to oneof value where value can be of int/string/double/bytes type. Once it make sense on the sdk and on the server side, we will stick to it and support more type if needed. This would allow us to do operations to numeric types directly without having to convert from string.

Here's the change: 7b8fdf2

[kvcache]

@honeyAcorn
1 - we flip flopped on this. Store name belongs in the header.
2 - would you like to do that now?
3 - yep.
4 - yep.

[honeyAcorn]

@honeyAcorn 1 - we flip flopped on this. Store name belongs in the header. 2 - would you like to do that now? 3 - yep. 4 - yep.

@kvcache Thanks for correcting it for me! I've modified previous comment too. Now it's all good to be reviewed! It looks nice and clean now :) 7b8fdf2

[kvcache]

This looks good to me.

[cprice404]