8000 Remove gRPC broadcast API by thanethomson · Pull Request #659 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove gRPC broadcast API #659

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

Merged
merged 11 commits into from
Apr 12, 2023
Merged

Remove gRPC broadcast API #659

merged 11 commits into from
Apr 12, 2023

Conversation

thanethomson
Copy link
Contributor
@thanethomson thanethomson commented Apr 6, 2023

Closes #650.

Commits 11c1f34 and 8e2d5ee can be reviewed independently. The vast majority of code changes here are purely removals of the generated Go code.


PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments

@thanethomson thanethomson added this to the 2023-Q2 milestone Apr 6, 2023
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson thanethomson force-pushed the thane/650-rm-grpc-api branch from f61b0a0 to f62d564 Compare April 6, 2023 11:07
@thanethomson thanethomson marked this pull request as ready for review April 6, 2023 15:56
@thanethomson thanethomson requested a review from a team as a code owner April 6, 2023 15:56
@thanethomson thanethomson removed this from the 2023-Q2 milestone Apr 6, 2023
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Update the config section of the docs with a more recently generated
`config.toml` file (generated using the current code on `main`).

Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson thanethomson force-pushed the thane/650-rm-grpc-api branch from f5183ea to 02795ca Compare April 6, 2023 18:54
This reverts commit ce2bfa1.

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Copy link
Contributor
@cason cason left a comment

Choose a reason for hiding this comment

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

Legit.

This is a superficial review, somehow, based on the file diffs. I haven't checked whether everything from the current gRPC was removed, for instance, nor run tests and similar stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here I assume you are updating the "version" of the config file from docs/toml.go, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I generated a configuration file using cometbft init using the code on main and updated the docs for the configuration file.

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Comment on lines -623 to -649
// we expose a simplified api over grpc for convenience to app devs
grpcListenAddr := n.config.RPC.GRPCListenAddress
if grpcListenAddr != "" {
config := rpcserver.DefaultConfig()
config.MaxBodyBytes = n.config.RPC.MaxBodyBytes
config.MaxHeaderBytes = n.config.RPC.MaxHeaderBytes
// NOTE: GRPCMaxOpenConnections is used, not MaxOpenConnections
config.MaxOpenConnections = n.config.RPC.GRPCMaxOpenConnections
// If necessary adjust global WriteTimeout to ensure it's greater than
// TimeoutBroadcastTxCommit.
// See https://github.com/tendermint/tendermint/issues/3435
if config.WriteTimeout <= n.config.RPC.TimeoutBroadcastTxCommit {
config.WriteTimeout = n.config.RPC.TimeoutBroadcastTxCommit + 1*time.Second
}
listener, err := rpcserver.Listen(grpcListenAddr, config.MaxOpenConnections)
if err != nil {
return nil, err
}
go func() {
//nolint:staticcheck // SA1019: core_grpc.StartGRPCClient is deprecated: A new gRPC API will be introduced after v0.38.
if err := grpccore.StartGRPCServer(env, listener); err != nil {
n.Logger.Error("Error starting gRPC server", "err", err)
}
}()
listeners = append(listeners, listener)

}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This prevents the node from creating any gRPC server attached to the node. #81 will reintroduce something like this.

In some places, the Go names of parameters are used instead of their
TOML versions. This replaces the Go names with TOML versions, and fixes
a few grammatical and formatting issues in the configuration template
and docs.

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson thanethomson merged commit 5e7fb0c into main Apr 12, 2023
@thanethomson thanethomson deleted the thane/650-rm-grpc-api branch April 12, 2023 16:49
thanethomson added a commit that referenced this pull request May 6, 2023
* Remove gRPC functionality and usage

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Remove all gRPC-related configuration functionality

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Add changelog entry

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* ci: Force use of very latest Go version for govulncheck

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* docs: Update config section

Update the config section of the docs with a more recently generated
`config.toml` file (generated using the current code on `main`).

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Revert "ci: Force use of very latest Go version for govulncheck"

This reverts commit ce2bfa1.

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* config: Fix grammar in doc comment

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* config: Remove # for clarity

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* config: Fix comments to clarify

In some places, the Go names of parameters are used instead of their
TOML versions. This replaces the Go names with TOML versions, and fixes
a few grammatical and formatting issues in the configuration template
and docs.

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* docs: Remove extraneous #

Signed-off-by: Thane Thomson <connect@thanethomson.com>

---------

Signed-off-by: Thane Thomson <connect@thanethomson.com>
76C2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Remove the existing gRPC broadcast API
2 participants
0