Skip to content

Add GetKeysSummary Api Signature/protos #4

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 15, 2023
Merged

Add GetKeysSummary Api Signature/protos #4

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 73 additions & 1 deletion app/src/main/proto/vss.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ message GetObjectRequest {
// All APIs operate within a single store_id.
// It is up to clients to use single or multiple stores for their use-case.
// This can be used for client-isolation/ rate-limiting / throttling on the server-side.
// Authorization and billing can also be performed at the storeId level.
// Authorization and billing can also be performed at the store_id level.
string store_id = 1;

// Key for which the value is to be fetched.
Expand Down Expand Up @@ -98,6 +98,78 @@ message PutObjectRequest {
message PutObjectResponse {
}

message ListKeyVersionsRequest {

// store_id is a keyspace identifier.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: might make sense to move repeating doc comments to the top of the file, and just link to them in each request proto.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, i want to do that but there wasn't a clear and nice way to do it.
If we do it the current way, protobuf generated classes have nicely added field docs.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, i want to do that but there wasn't a clear and nice way to do it. If we do it the current way, protobuf generated classes have nicely added field docs.

Are you saying any top-level docs are not copied anywhere in the generated code? In my past experience, the generated code didn't have any docs at all, so developers would just look at the proto file. But perhaps this has changed.

We should ideally end off in a place where the reader has a high-level summary somewhere and have more detailed explanations as they drill down into messages and fields, IMO. Not sure how this will ultimately look, but I could imagine a few possibilities:

  • High-level summary at top, minimal request/response docs, detailed field docs
  • High-level summary at top, detailed request/response docs, minimal field docs
  • No high-level summary, high-level summary in request/response docs, detailed request/response docs

Regardless of which, we should have some high-level summary somewhere so developers don't have to exhaustively read every field doc to know how everything fits together. Feel free to do in a follow-up, of couse.

Copy link
Collaborator Author

@G8XSU G8XSU Jan 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying any top-level docs are not copied anywhere in the generated code?

Yes, they get omitted, only field and struct level docs get into generated code.

My current plan is
"High-level summary in project docs, No high-level summary in proto, high-level summary in request/response docs, detailed field docs"

In this way, docs on top of fields and structs will get copied to generated code, and high-level project doc serves as reference for usage. It will also contain examples from client-side code.
Will take it as follow-up.

// Ref: https://en.wikipedia.org/wiki/Keyspace_(distributed_data_store)
// All APIs operate within a single store_id.
// It is up to clients to use single or multiple stores for their use-case.
// This can be used for client-isolation/ rate-limiting / throttling on the server-side.
// Authorization and billing can also be performed at the store_id level.
string store_id = 1;

// A key_prefix is a string of characters at the beginning of the key. Prefixes can be used as
// a way to organize key-values in a similar way to directories.
//
// If key_prefix is specified, the response results will be limited to those keys that begin with
// the specified prefix.
//
// If no key_prefix is specified or it is empty (""), all the keys are eligible to be returned in
// the response.
optional string key_prefix = 2;

// page_size is used by clients to specify the maximum number of results that can be returned by
// the server.
// The server may further constrain the maximum number of results returned in a single page.
// If the page_size is 0 or not set, the server will decide the number of results to be returned.
optional int32 page_size = 3;

// page_token is a pagination token.
//
// To query for the first page of ListKeyVersions, page_token must not be specified.
//
// For subsequent pages, use the value that was returned as `next_page_token` in the previous
// page's ListKeyVersionsResponse.
optional string page_token = 4;
}

message ListKeyVersionsResponse {

// Fetched keys and versions.
// Even though this API reuses KeyValue struct, the value sub-field will not be set by the server.
repeated KeyValue key_versions = 1;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider: creating a separate struct for KeyVersion instead of re-using KeyValue struct here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say here explicitly that the value will be empty. I think it's OK to reuse the struct.


// next_page_token is a pagination token, used to retrieve the next page of results.
// Use this value to query for next_page of paginated ListKeyVersions operation, by specifying
// this value as the `page_token` in the next request.
//
// If next_page_token is empty (""), then the "last page" of results has been processed and
// there is no more data to be retrieved.
//
// If next_page_token is not empty, it does not necessarily mean that there is more data in the
// result set. The only way to know when you have reached the end of the result set is when
// next_page_token is empty.
//
// Caution: Clients must not assume a specific number of key_versions to be present in a page for
// paginated response.
optional string next_page_token = 2;

// global_version is a sequence-number/version of the whole store.
//
// global_version is only returned in response for the first page of the ListKeyVersionsResponse
// and is guaranteed to be read before reading any key-versions.
//
// In case of refreshing the complete key-version view on the client-side, correct usage for
// the returned global_version is as following:
// 1. Read global_version from the first page of paginated response and save it as local variable.
// 2. Update all the key_versions on client-side from all the pages of paginated response.
// 3. Update global_version on client_side from the local variable saved in step-1.
// This ensures that on client-side, all current key_versions were stored at global_version or later.
// This guarantee is helpful for ensuring the versioning correctness if using the global_version
// in PutObject API and can help avoid the race conditions related to it.
optional int64 global_version = 3;
}

// When HttpStatusCode is not ok (200), the response `content` contains a serialized ErrorResponse
// with the relevant ErrorCode and message
message ErrorResponse {
Expand Down