chore(storage): implement Notification methods #6138
Conversation
product-auto-label
bot
added
size: l
storage/grpc_client.go
Outdated
| err = run(ctx, func() error { | ||
| gitr := c.raw.ListNotifications(ctx, req, s.gax...) | ||
| for { | ||
| items, nextPageToken, err := gitr.InternalFetch(int(req.PageSize), req.PageToken) |
There was a problem hiding this comment.
I was initially confused about how the page token was being preserved between loop iterations but looks like InternalFetch modifies the request to handle this. Might be worth adding a comment about this since it does seem confusing!
Also @noahdietz can you just verify that this will preserve the page token if there is a failure from internalFetch mid-way through? Otherwise we might end up with duplicate results.
There was a problem hiding this comment.
looks like InternalFetch modifies the request to handle this
I'm not sure what you mean by this. InternalFetch will return the nextPageToken, but not modify the next request to use it. That's how I read its code at least. I think we need to set req.PageToken = nextPageToken right after line 869 before the next iteration.
There was a problem hiding this comment.
Ahh I was not preserving the nextPageToken between iterations >"< updated. Thank you both for the clarification!
There was a problem hiding this comment.
Hah @noahdietz that's what I thought originally looking through the code and I talked myself out of it for some reason. Yup that sounds like the right answer.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return notificationsToMap(res.Items), nil |
There was a problem hiding this comment.
Interestingly it seems like this implementation doesn't use page tokens-- so if there are more than 1000 notifications you won't get all of them. (I think this would be very uncommon though)
There was a problem hiding this comment.
Might be worth adding a Note or TODO for later mentioning this. I too was surprised to see an un-paginated list api, even if it predates the design standards.
There was a problem hiding this comment.
Ah so I did a bit of digging and found the following:
- The JSON API doesn't actually support page tokens, probably because...
- There is a quota of 100 notifications per bucket.
I'm not sure how this would effect gRPC, if at all.
There was a problem hiding this comment.
I think we should keep page size unspecified. The server will "do the right thing" and if for some reason that quota is raised to 500, we won't have to change our code or be worried about what might happen - it will "just work".
Can we add a comment that the server will use a default page_size (just sent an email to inquire what the default actually is), and that our code will not set a page_size? We could leave the param just in case we want to set it later. WDYT?
There was a problem hiding this comment.
Sounds fine to me! Agreed that we don't want to depend of behavior that may change going forward.
There was a problem hiding this comment.
Gotcha... IIUC, based on the same block of code, req.GetPageSize() is not set here and the zero value fallbacks to the API default pageSize of 100.
So I've added a comment but no change in our code. PTAL and let me know if my understanding is correct.
storage/grpc_client.go
Outdated
| err = run(ctx, func() error { | ||
| gitr := c.raw.ListNotifications(ctx, req, s.gax...) | ||
| for { | ||
| items, nextPageToken, err := gitr.InternalFetch(int(req.PageSize), req.PageToken) |
There was a problem hiding this comment.
looks like InternalFetch modifies the request to handle this
I'm not sure what you mean by this. InternalFetch will return the nextPageToken, but not modify the next request to use it. That's how I read its code at least. I think we need to set req.PageToken = nextPageToken right after line 869 before the next iteration.
storage/http_client.go
Outdated
|
|
||
| // ListNotifications returns all the Notifications configured for this bucket, as a map indexed by notification ID. | ||
| // | ||
| // Note: Pagination is not currently supported. |
There was a problem hiding this comment.
I would amend this to say something like:
Note: This API does not support pagination. However, entity limits cap the number of notifications on a single bucket, so all results will be returned in the first response. See https://cloud.google.com/storage/quotas#buckets
storage/grpc_client.go
Outdated
| ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.grpcStorageClient.CreateNotification") | ||
| defer func() { trace.EndSpan(ctx, err) }() | ||
|
|
||
| if n.ID != "" { |
There was a problem hiding this comment.
Remove client-side validation here as well (to be left above the interface).
product-auto-label
bot
added
size: m
This adds interface Notification methods
Tests pending on testbench new release with fix -googleapis/storage-testbench#360
CreateNotificationGetNotificationDeleteNotificationListNotifications