-
Notifications
You must be signed in to change notification settings - Fork 235
Replace ateredis with atepg #940
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
base: main
Are you sure you want to change the base?
Changes from all commits
2363f2e
2e841ea
792b208
0a08db4
405895c
9033013
b938d6b
ef7821a
572dd53
82f9821
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -353,20 +353,17 @@ func TestCreateActorSnapshotTag_RejectsUnsetScope(t *testing.T) { | |
| } | ||
|
|
||
| // serviceWithActorSnapshotTag seeds an ActorSnapshot and a tag pointing at it | ||
| // in a miniredis-backed store, and returns a Service over it. | ||
| // in a PostgreSQL-backed store, and returns a Service over it. | ||
| func serviceWithActorSnapshotTag(t *testing.T, tag *ateapipb.ActorSnapshotTag) (*Service, *ateapipb.ActorSnapshotTag) { | ||
| t.Helper() | ||
| persistence, cleanup := storetest.SetupTestStore(t) | ||
| t.Cleanup(cleanup) | ||
|
|
||
| atespace, name := tag.GetMetadata().GetAtespace(), tag.GetMetadata().GetName() | ||
| snapshot, err := persistence.CreateActorSnapshot(context.Background(), &ateapipb.ActorSnapshot{ | ||
| snapshot := storetest.MustCreateActorSnapshot(t, context.Background(), persistence, &ateapipb.ActorSnapshot{ | ||
| Metadata: &ateapipb.ResourceMetadata{Atespace: atespace, Name: "snapshot-" + name}, | ||
| SnapshotUri: "gs://my-bucket/snapshots/" + atespace + "/snapshot-" + name, | ||
| }) | ||
| if err != nil { | ||
| t.Fatalf("Failed to CreateActorSnapshot: %v", err) | ||
| } | ||
| tag.Snapshot = &ateapipb.ObjectRef{Atespace: snapshot.GetMetadata().GetAtespace(), Name: snapshot.GetMetadata().GetName()} | ||
| created, err := persistence.CreateActorSnapshotTag(context.Background(), atespace, snapshot.GetMetadata().GetName(), tag) | ||
| if err != nil { | ||
|
|
@@ -383,12 +380,10 @@ func TestUpdateActorSnapshotTag_DeleteRecreateRace(t *testing.T) { | |
| t.Cleanup(cleanup) | ||
|
|
||
| for _, name := range []string{"snapshot-1", "snapshot-2"} { | ||
| if _, err := persistence.CreateActorSnapshot(ctx, &ateapipb.ActorSnapshot{ | ||
| storetest.MustCreateActorSnapshot(t, ctx, persistence, &ateapipb.ActorSnapshot{ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I decided to also do a quick scan for unindexed queries. Postgres automatically indexes the referenced side of a foreign key but not the referencing side, and there's no index on actor_snapshot_tags (snapshot_atespace, snapshot_name) so the queries against are full scans.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch for this, thanks! I missed the usage of this index in the FK trigger when deleting snapshots |
||
| Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: name}, | ||
| SnapshotUri: "gs://bucket/root/snapshots/" + testAtespace + "/" + name, | ||
| }); err != nil { | ||
| t.Fatalf("Failed to CreateActorSnapshot(%s): %v", name, err) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| const tagName = "before-upgrade" | ||
|
|
@@ -461,12 +456,10 @@ func TestUpdateActorSnapshotTag_ConcurrentUnguardedUpdate(t *testing.T) { | |
| persistence, cleanup := storetest.SetupTestStore(t) | ||
| t.Cleanup(cleanup) | ||
|
|
||
| if _, err := persistence.CreateActorSnapshot(ctx, &ateapipb.ActorSnapshot{ | ||
| storetest.MustCreateActorSnapshot(t, ctx, persistence, &ateapipb.ActorSnapshot{ | ||
| Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: "snapshot-1"}, | ||
| SnapshotUri: "gs://bucket/root/snapshots/" + testAtespace + "/snapshot-1", | ||
| }); err != nil { | ||
| t.Fatalf("Failed to CreateActorSnapshot: %v", err) | ||
| } | ||
| }) | ||
|
|
||
| const tagName = "before-upgrade" | ||
| originalTag, err := persistence.CreateActorSnapshotTag(ctx, testAtespace, "snapshot-1", &ateapipb.ActorSnapshotTag{ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same problem as mentioned for CreateActor. If you create a tag for a snapshot that doesn't exist and you get ErrNotFound from the pre-check. But if the snapshot is deleted after the pre-check and before the insert, the FK violation comes back as ErrFailedPrecondition instead.
Generally we need to not rely on pre-checks to guard an operation if it can race with other operations.