Skip to content

Commit bc4fdfb

Browse files
committed
Stop storing actor identity in ate Redis's value
The key actor:<atespace>:<id> already carries both fields, so storing them in the value duplicates `~27 bytes plus both name lengths` per actor.
1 parent d8f562d commit bc4fdfb

2 files changed

Lines changed: 68 additions & 10 deletions

File tree

cmd/ateapi/internal/store/ateredis/ateredis.go

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -315,9 +315,9 @@ func (s *Persistence) GetActor(ctx context.Context, atespace, id string) (*ateap
315315
return nil, fmt.Errorf("while unmarshaling actor: %w", err)
316316
}
317317

318-
if actor.GetActorId() != id || actor.GetAtespace() != atespace {
319-
return nil, fmt.Errorf("(impossible) mismatch between stored id/atespace and key")
320-
}
318+
// Identity lives in the key, not the value.
319+
actor.Atespace = atespace
320+
actor.ActorId = id
321321

322322
return actor, nil
323323
}
@@ -329,6 +329,9 @@ func (s *Persistence) CreateActor(ctx context.Context, actor *ateapipb.Actor) er
329329
// stomp the caller's copy.
330330
dbActor := proto.Clone(actor).(*ateapipb.Actor)
331331
dbActor.Version = 1
332+
// Clear the identity as they are stored in key instead of value.
333+
dbActor.Atespace = ""
334+
dbActor.ActorId = ""
332335

333336
dbActorBytes, err := protojson.Marshal(dbActor)
334337
if err != nil {
@@ -510,6 +513,9 @@ func (s *Persistence) UpdateActor(ctx context.Context, actor *ateapipb.Actor, ex
510513
// Clone because we will update the version field, and we don't want to
511514
// stomp the caller's copy.
512515
dbActor := proto.Clone(actor).(*ateapipb.Actor)
516+
// Clear the identity as they are stored in key instead of value.
517+
dbActor.Atespace = ""
518+
dbActor.ActorId = ""
513519

514520
err := s.rdb.Watch(ctx, func(tx *redis.Tx) error {
515521
currentVal, err := tx.Get(ctx, dbKey).Bytes()
@@ -529,12 +535,6 @@ func (s *Persistence) UpdateActor(ctx context.Context, actor *ateapipb.Actor, ex
529535
return store.ErrPersistenceRetry
530536
}
531537
dbActor.Version = currentActor.GetVersion() + 1
532-
if currentActor.GetActorId() != dbActor.GetActorId() {
533-
return fmt.Errorf("actor_id is immutable")
534-
}
535-
if currentActor.GetAtespace() != dbActor.GetAtespace() {
536-
return fmt.Errorf("atespace is immutable")
537-
}
538538
if currentActor.GetActorTemplateNamespace() != dbActor.GetActorTemplateNamespace() {
539539
return fmt.Errorf("actor_template_namespace is immutable")
540540
}
@@ -755,7 +755,7 @@ func (s *Persistence) fetchActors(ctx context.Context, master *redis.Client, key
755755
}
756756

757757
var actors []*ateapipb.Actor
758-
for _, cmd := range cmds {
758+
for i, cmd := range cmds {
759759
getCmd, ok := cmd.(*redis.StringCmd)
760760
if !ok {
761761
continue
@@ -771,6 +771,13 @@ func (s *Persistence) fetchActors(ctx context.Context, master *redis.Client, key
771771
if err := protojson.Unmarshal([]byte(getCmd.Val()), actor); err != nil {
772772
return nil, fmt.Errorf("in protojson.Unmarshal: %w", err)
773773
}
774+
// Identity lives in the key, not the value.
775+
parts := strings.Split(keys[i], ":")
776+
if len(parts) != 3 {
777+
return nil, fmt.Errorf("bad key format %q", keys[i])
778+
}
779+
actor.Atespace = parts[1]
780+
actor.ActorId = parts[2]
774781
actors = append(actors, actor)
775782
}
776783
return actors, nil

cmd/ateapi/internal/store/ateredis/ateredis_test.go

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"context"
1919
"errors"
2020
"fmt"
21+
"strings"
2122
"testing"
2223
"time"
2324

@@ -1071,6 +1072,56 @@ func TestDeleteAtespace_EmptyAfterActorsRemoved(t *testing.T) {
10711072
}
10721073
}
10731074

1075+
func TestActorValue_OmitsIdentity(t *testing.T) {
1076+
mr, s, ctx := setupTest(t)
1077+
defer mr.Close()
1078+
1079+
actor := &ateapipb.Actor{
1080+
ActorId: "a1",
1081+
Atespace: "team-a",
1082+
ActorTemplateNamespace: "default",
1083+
ActorTemplateName: "tmpl",
1084+
Status: ateapipb.Actor_STATUS_SUSPENDED,
1085+
}
1086+
if err := s.CreateActor(ctx, actor); err != nil {
1087+
t.Fatalf("CreateActor failed: %v", err)
1088+
}
1089+
1090+
assertNoIdentity := func(operation string) {
1091+
t.Helper()
1092+
raw, err := mr.Get("actor:team-a:a1")
1093+
if err != nil {
1094+
t.Fatalf("raw get after %s failed: %v", operation, err)
1095+
}
1096+
if strings.Contains(raw, `"actorId"`) || strings.Contains(raw, `"atespace"`) {
1097+
t.Errorf("value after %s duplicates key identity: %s", operation, raw)
1098+
}
1099+
}
1100+
assertNoIdentity("create")
1101+
1102+
got, err := s.GetActor(ctx, "team-a", "a1")
1103+
if err != nil {
1104+
t.Fatalf("GetActor failed: %v", err)
1105+
}
1106+
if got.GetAtespace() != "team-a" || got.GetActorId() != "a1" {
1107+
t.Errorf("GetActor identity = (%q, %q), want (team-a, a1)", got.GetAtespace(), got.GetActorId())
1108+
}
1109+
1110+
got.Status = ateapipb.Actor_STATUS_RUNNING
1111+
if err := s.UpdateActor(ctx, got, got.GetVersion()); err != nil {
1112+
t.Fatalf("UpdateActor failed: %v", err)
1113+
}
1114+
assertNoIdentity("update")
1115+
1116+
actors, _, err := s.ListActors(ctx, "team-a", 10, "")
1117+
if err != nil {
1118+
t.Fatalf("ListActors failed: %v", err)
1119+
}
1120+
if len(actors) != 1 || actors[0].GetAtespace() != "team-a" || actors[0].GetActorId() != "a1" {
1121+
t.Errorf("ListActors did not re-populate identity: %v", actors)
1122+
}
1123+
}
1124+
10741125
func TestDeleteAtespace_EmptyWhileOtherAtespaceNonEmpty(t *testing.T) {
10751126
mr, s, ctx := setupTest(t)
10761127
defer mr.Close()

0 commit comments

Comments
 (0)