Conversation
| } | ||
|
|
||
| message GetResponse { | ||
| bytes data = 1; |
There was a problem hiding this comment.
Include objectName for async request/responses?
There was a problem hiding this comment.
I don't really understand how gRPC asyc requests work but it seems that there should be some id associated with the request, rather than in the data. There appears to be a provision for tagging the responses, e.g., in this example.
There was a problem hiding this comment.
You could create separate, async versions of each of the requests, where the request returns a handle that one could wait upon.
| bytes data = 3; | ||
| } | ||
|
|
||
| message Empty {} No newline at end of file |
There was a problem hiding this comment.
Nit: This shouldn't need to be redefined. You can use import "google/protobuf/empty.proto"; and then return google.protobuf.Empty. There are a bunch of commonly used protobuf definitions in that package, which you can find here.
There was a problem hiding this comment.
Ok, let's try that. I was going to go that way but then figured this one is actually simpler to define than to import. But maybe some of the other definitions are useful.
| - What do we want to do for location, e.g., regions? | ||
| - How do object versions work? | ||
| - What concurrency models / consistency guarantees do we support | ||
| - How are we encoding error codes? Can we use standard rRPC status (https://github.com/grpc/grpc/blob/master/doc/statuscodes.md) |
There was a problem hiding this comment.
Are there standard ways to translate those codes into client languages? If not, you can just use protobuf enums, and those are compiled into enums in the corresponding language, which helps with readability quite a bit (e.g., here).
There was a problem hiding this comment.
The main advantage that I see in using the standard error codes, at least in Go, is that you check the error once.
Here's what it could end up looking like with two levels of errors:
func doPut(objectName string, objectData []byte) {
// ... get client and context
r, err := client.Put(ctx, &pb.PutRequest{ObjectName:objectName,ObjectData:data})
if err != nil {
return err
}
if r.ErrCode != pb.OK {
return NewObjStoreErr(r.ErrCode)
}
return nil
}vs
func doPut(objectName string, objectData []byte) {
// ... get client and context
r, err := client.Put(ctx, &pb.PutRequest{ObjectName:objectName,ObjectData:data})
if err != nil {
return err
}
return nil
}There was a problem hiding this comment.
+1 for standard error codes.
There was a problem hiding this comment.
We are planning to go down the path of standard error codes. It definitely makes the code nicer. One disadvantage we should be mindful of is that the errors become a more explicit part of the interface when we define them as part of the protobuf messages. Fully specifying the behavior requires tests either way, though.
|
|
||
| message CreateBucketRequest { | ||
| string bucketName = 1; | ||
| // TODO add permissions |
There was a problem hiding this comment.
Permissions would make sense in a multi-tenant setting. In the worst case, if a store does not support it, it can ignore this value.
| } | ||
|
|
||
| message GetResponse { | ||
| bytes data = 1; |
There was a problem hiding this comment.
You could create separate, async versions of each of the requests, where the request returns a handle that one could wait upon.
| */ | ||
|
|
||
|
|
||
| service ObjectStore { |
There was a problem hiding this comment.
I think deleteBucket(bucketName), delete(objectName) are missing
There was a problem hiding this comment.
Also, if we do decide to add permissions, some way of looking up permissions..
| message CreateBucketRequest { | ||
| string bucketName = 1; | ||
| // TODO add permissions | ||
| // TODO add location |
There was a problem hiding this comment.
What would location correspond to? Geographic region?
There was a problem hiding this comment.
That's what I had in mind. Going off of the S3 API it's a region. In principle it could mean something else though, e.g., some locality group.
| - What do we want to do for location, e.g., regions? | ||
| - How do object versions work? | ||
| - What concurrency models / consistency guarantees do we support | ||
| - How are we encoding error codes? Can we use standard rRPC status (https://github.com/grpc/grpc/blob/master/doc/statuscodes.md) |
There was a problem hiding this comment.
+1 for standard error codes.
|
Would be good to have comments on Protobuf definitions (especially to document decisions to explicitly include/exclude certain things in our existing API) and on the reference implementation to explain what the moving parts in a gRPC object store service are. |
|
Status update:
|
…d gRPC server. Prelim bench shows that the throughput of Anna is 1/4 of that of local file system (1, 10m files, 1 thread). DeleteBucket remains buggy since I'm not sure how to remove a setLattice
We are moving forward with a prototype object storage API implementation using gRPC as the transport. What's important about this contribution is the interface, not the implementation. What's most important to review is the gRPC file:
pkg/objstore/objstore.proto.Some notes:
Some key questions:
protoc? That would make builds simpler by eliminating a dependency?