-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat(datastore): Adding reserve IDs support #9027
Conversation
@@ -1220,6 +1220,21 @@ func TestIntegration_Projection(t *testing.T) { | |||
}) | |||
} | |||
|
|||
func TestIntegration_ReserveIDs(t *testing.T) { |
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.
Are there any assertions you want to make here about either the request or the response?
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.
The response does not have any public fields. So, there is nothing to assert.
https://github.com/googleapis/go-genproto/blob/50ed04b929172f52f906d64b8689663a5afbc9e2/googleapis/datastore/v1/datastore.pb.go#L1374-L1378
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.
Are there any assert statements that could be added to make sure the code did "prevent the supplied keys' IDs from being auto-allocated by Cloud Datastore"? Perhaps by making a read call against the keys and ensuring the result of the read is nil?
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.
The result of read is nil with and without reserving IDs.
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.
I see. Maybe I misunderstood the meaning of "prevent the supplied keys' IDs from being auto-allocated by Cloud Datastore". Is there any test with an assert statement that ensures calling reserveIds will "prevent the supplied keys' IDs from being auto-allocated by Cloud Datastore"? Is there anything we are doing to ensure that behaviour is captured?
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.
I could add a test like this but 1000 operations does not exhaust the possibility that the id could be allocated later. So, not a definite test:
reservedKeyIDs := []int64{1234567, 8900000, 7654897}
keys := make([]*Key, len(reservedKeyIDs))
for i := range reservedKeyIDs {
keys[i] = IDKey("ReserveIDs", reservedKeyIDs[i], nil)
}
err := client.ReserveIDs(ctx, keys)
if err != nil {
t.Fatalf("ReserveIDs failed: %v", err)
}
type value struct{ N int }
v := &value{N: 5}
for i := 0; i < 1000; i++ {
autoAllocatedKey, err := client.Put(ctx, IncompleteKey("ReserveIDs", nil), v)
if err != nil {
t.Fatal(err)
}
if slices.Contains(reservedKeyIDs, autoAllocatedKey.ID) {
t.Fatalf("Reserved ID auto-allocated by Datastore: %v\n", autoAllocatedKey.ID)
}
}
Ideally, Datastore service should return an error if reservation failed. So, just checking for error should suffice, right?
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.
Ok. I see your point and understand that this could be very difficult to test since the allocated key for client.Put is basically random. This LGTM.
When an entity is created in Datastore, a key is auto-allocated to the entity if none is provided at the entity creation time.
This PR adds ReserveIDs method using which a user can prevent the supplied keys' IDs from being auto-allocated by Cloud Datastore. This will remove the feature gap in Go client library. Java and NodeJS both support reserveIDs
The implementation is similar to AllocateIDS
google-cloud-go/datastore/key.go
Lines 242 to 259 in 620e6d8