Skip to content
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

grpc/proto changes for GetByIndex #355

Merged
merged 9 commits into from
Feb 3, 2021

Conversation

UnitylChaos
Copy link
Contributor

#354

Is there any documentation on how to do the setup for the gogoproto generation stuff? I see the proto-gen make action, but when I tried to set things up manually to do that I ended up with a different version of protoc-gen-grpc-gateway which caused a bunch of extra changes on the generated go files.

@tac0turtle
Copy link
Member

There doesn't seem to be. Ill open a PR to fix that now.

Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

Thanks! This is a breaking change, so it'll have to wait for 0.16.

server/server.go Outdated Show resolved Hide resolved
Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution! Would you mind adding an entry to CHANGELOG.md as well, in particular describing the breaking change to Get()?

CHANGELOG.md Outdated
## Unreleased

### Breaking Changes
- [\#355](https://github.com/cosmos/iavl/pull/355) `Get` in `iavlServer` no longer returns an error if the requested key does not exist. `GetResponse` now contains a `missing` boolean to indicate that a key does not exist, and the returned index will be that of the next occupied key.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need that breaking change?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe there is a supported way of returning structured error information in gRPC, so in order to return the index of the nearest key when accessing a missing key we have to change from an error response to a regular response. See original issue #354.

@robert-zaremba robert-zaremba added the T:breaking Type: Breaking Change label Jan 30, 2021
Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

Let's consider renaming missing.

server/server.go Outdated
}

return &pb.GetResponse{Index: idx, Value: value}, nil
return &pb.GetResponse{Index: idx, Value: value, Missing: false}, nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

you could merge the lines above to one line;

return &pb.GetResponse{Index: idx, Value: value, Missing: value==nil}, nil

@@ -283,6 +296,12 @@ message HasResponse {
message GetResponse {
int64 index = 1;
bytes value = 2;
bool missing = 3;
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about changing to more standard not_found (like in HTTP) or exists (like in SQL)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, not_found sounds good. I think it makes sense for the zero value to default to the typical case where the key exists.

@robert-zaremba robert-zaremba self-assigned this Feb 3, 2021
Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

ukACK

@robert-zaremba robert-zaremba added the S:automerge Automatic merge and/or update Pull requests label Feb 3, 2021
@mergify mergify bot merged commit 290555f into cosmos:master Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S:automerge Automatic merge and/or update Pull requests T:breaking Type: Breaking Change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants