support etag in redis state provider #579
Conversation
RemindD
force-pushed
the
users/xingdong/redisetag
branch
from
336a107 to
58ee6af
Compare
| @@ -147,9 +177,17 @@ func (r *RedisStateProvider) Upsert(ctx context.Context, entry states.UpsertRequ | |||
| if err != nil { | |||
| return entry.Value.ID, err | |||
| } | |||
| firstWrite := 0 | |||
| if entry.Options.Concurrency == states.FirstWrite { | |||
There was a problem hiding this comment.
Does this mean "first write win"?
There was a problem hiding this comment.
Yes. If two writes with the same ETag coming in, the first write will win and bump etag while the second write fails.
| if etag == "" { | ||
| etag = "0" | ||
| } | ||
| err = r.Client.Do(r.Ctx, "EVAL", setDefaultQuery, 1, key, etag, string(body), 1).Err() |
There was a problem hiding this comment.
Should the last parameter be firstWrite?
There was a problem hiding this comment.
I really want to remove UpdateStatusOnly option because the caller should get the previous state first, apply the update and then upsert with etag.
Here the upsert operation is based on the previous get result so the firstWrite should always be 1.
There was a problem hiding this comment.
I would prefer we still keep UpdateStatusOnly since like K8S state provider, status are sub resources. But seems we should not always default to useFirstWriteWins or lastWriteWins when updating status. As state provider, it is better we align client's input. And for all clients, they need to handle get state at first before composing new state.
| if not etag or type(etag)=="table" or etag == "" or etag == ARGV[1] or (not fwr and ARGV[1] == "0") then | ||
| redis.call("HSET", KEYS[1], "values", ARGV[2]); | ||
| if ARGV[3] == "1" then | ||
| redis.call("HSET", KEYS[1], "first-write", 1); |
There was a problem hiding this comment.
It reads like the first-write field is actually used to distinguish between first write request from other requests? What if two clients sending the request at the same time?
There was a problem hiding this comment.
This logic is from the Dapr project and I think this is to guarantee all the further operations should use proper etag if any one is firstwrite. If you have different expectation, I can change it based on our need.
| redis.call("DEL", KEYS[1]); | ||
| end; | ||
| local fwr = redis.pcall("HGET", KEYS[1], "first-write"); | ||
| if not etag or type(etag)=="table" or etag == "" or etag == ARGV[1] or (not fwr and ARGV[1] == "0") then |
There was a problem hiding this comment.
What does type(etag)=="table" mean?
There was a problem hiding this comment.
This condition is copied from dapr repo. It means the etag is not a proper column name, likely for error case handling.
| @@ -222,11 +267,11 @@ func (r *RedisStateProvider) List(ctx context.Context, request states.ListReques | |||
| continue | |||
| } | |||
| parts := strings.Split(key, separator) | |||
| if len(parts) != 3 { | |||
| if len(parts) != 4 { | |||
There was a problem hiding this comment.
Give an example of new key in comment.
RemindD
force-pushed
the
users/xingdong/redisetag
branch
from
58ee6af to
31f906a
Compare
No description provided.