-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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: use gRPC for queries #10997
feat: use gRPC for queries #10997
Conversation
@AmauryM in continue to our discord discussion. this is rather PoC which seems to work |
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.
LGTM, could we get a changlog entry and possibly a test. Could be good to have parallel running queries to test that functionality.
6d7302a
to
4389f2d
Compare
73f3eb4
to
d41ebfb
Compare
some updates to the PR.
questions.
rpcURI, _ := flagSet.GetString(flags.FlagNode)
if clientCtx.Client == nil || flagSet.Changed(flags.FlagNode) {
if rpcURI != "" {
clientCtx = clientCtx.WithNodeURI(rpcURI)
client, err := NewClientFromNode(rpcURI)
if err != nil {
return clientCtx, err
}
clientCtx = clientCtx.WithClient(client)
}
}
isOffline, _ := flagSet.GetBool(flags.FlagOffline)
if !isOffline && clientCtx.ClientConn == nil {
grpcURI, _ := flagSet.GetString(flags.FlagGRPC)
if grpcURI != "" {
if !flagSet.Changed(flags.FlagGRPC) && rpcURI != "" {
if uri, err := url.Parse(rpcURI); err == nil {
guri, _ := url.Parse(grpcURI)
grpcURI = uri.Hostname() + ":" + guri.Port()
}
}
if gcl, err := grpc.DialContext(ctx, grpcURI, grpc.WithInsecure(), grpc.WithReturnConnectionError()); err == nil {
clientCtx = clientCtx.WithClientConn(gcl)
}
}
} |
to replicate current behaviour I think it should be something like default to rpc, unless --grpc is passed. Defaulting to localhost here is fine.
Making the user explicitly set a grpc endpoint there I think is best. There are many endpoint that expose the RPC but not grpc and vice versa. |
do-not-merge
I tend to option 2 as the flag on the client side will be often used by people so they have to type less. As GRPC is not yet widely adopted in cosmos ecosystem, renaming server block to |
i personally prefer option 1, because it's keep backwards compatibility. Renaming app.toml fields requires its own migration path, which is more effort. Also, on the client side, we already have the |
we have a weird race in tests happening when gRPC is on. that's with cosmos v0.44.5. Any thoughts? it happens on the network cleanup |
@troian let me know how I can help push this over the finish line. Secret wants to test this to see if it fixes something else. |
fixes #10996 Signed-off-by: Artur Troian <[email protected]>
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.
Hey @troian, do you think this is ready for review?
For me it looks good, maybe #10997 (comment) is the only comment I have left
@AmauryM this is the race we're seeing. considering we're on the
also running real testnet with clients using grpc reproduces the issue |
@AmauryM thoughts on the last comment? |
Oh wow, it seems like you found a server-side bug that's in production. It was maybe just that no-one ever used the gRPC endpoint directly, that's why we never saw this issue. @troian Would you like to add a mutex on app.checkState? I can also do this tomorrow (maybe it's cleaner to do the mutex in another PR) |
yeah, i have pr in my drafts. still doing some tests to make sure it work |
this is with this branch being used? I know people running queries in parallel and this is the first time seeing this |
I believe 0.44.5 as #10997 (comment). Is there a possibility they all used |
I think this is the case, which explains why teams like terra and secret were running into non parallel queries when trying to do it |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
grpcURI, _ := flagSet.GetString(flags.FlagGRPCNode) | ||
|
||
if grpcURI != "" { | ||
grpcKeepAlive, _ := flagSet.GetDuration(flags.FlagGRPCKeepAlive) |
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.
Check the error.
@@ -147,6 +150,23 @@ func ReadPersistentCommandFlags(clientCtx Context, flagSet *pflag.FlagSet) (Cont | |||
} | |||
} | |||
|
|||
if clientCtx.ClientConn == nil || flagSet.Changed(flags.FlagGRPCNode) { | |||
grpcURI, _ := flagSet.GetString(flags.FlagGRPCNode) |
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.
Check the error.
if err == nil { | ||
clientCtx = clientCtx.WithClientConn(gcl) | ||
} |
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.
Prefer returning a zero-value Context when the error is non-nil.
@@ -91,6 +94,8 @@ var LineBreak = &cobra.Command{Run: func(*cobra.Command, []string) {}} | |||
// AddQueryFlagsToCmd adds common flags to a module query command. | |||
func AddQueryFlagsToCmd(cmd *cobra.Command) { | |||
cmd.Flags().String(FlagNode, "tcp://localhost:26657", "<host>:<port> to Tendermint RPC interface for this chain") | |||
cmd.Flags().String(FlagGRPCNode, "", "<host>:<port> to GRPC interface for this chain") | |||
cmd.Flags().Duration(FlagGRPCKeepAlive, 20*time.Second, "set GRPC keepalive. defaults to 20s") |
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.
No need to re-iterate the default value in the help text, as it is printed anyway.
we reopening this as it's needed for Ethermint |
LFG |
@marbar3778 this issue needs to be addressed first #11102 |
Correct.. I patched the 0.45.2 release with this PR to test this on Ethermint and I am currently seeing the following error
|
@crypto-facs highly possible. last time i worked on the pr more than a month ago. |
@marbar3778 @crypto-facs i know that oKEX forked the SDK and added this functionality. We should look into their implementation |
@alexanderbez sounds good. closing |
fixes #10996
Signed-off-by: Artur Troian [email protected]