inspect: resurrect the inspect command by williambanfield · Pull Request #9655 · tendermint/tendermint

williambanfield · 2022-11-01T22:44:58Z

WIP: resurrect the inspect command from #6785

PR checklist

Tests written/updated, or no tests needed
CHANGELOG_PENDING.md updated, or no changelog entry needed
Updated relevant documentation (docs/) and code comments, or no
documentation updates needed

EDIT: Updated, see [comment below]( #6785 (comment)) This change adds a sketch of the `Debug` mode. This change adds a `Debug` struct to the node package. This `Debug` struct is intended to be created and started by a command in the `cmd` directory. The `Debug` struct runs the RPC server on the data directories: both the state store and the block store. This change required a good deal of refactoring. Namely, a new `rpc.go` file was added to the `node` package. This file encapsulates functions for starting RPC servers used by nodes. A potential additional change is to further factor this code into shared code _in_ the `rpc` package. Minor API tweaks were also made that seemed appropriate such as the mechanism for fetching routes from the `rpc/core` package. Additional work is required to register the `Debug` service as a command in the `cmd` directory but I am looking for feedback on if this direction seems appropriate before diving much further. closes: #5908

thanethomson · 2022-11-03T18:10:46Z

Just to confirm here, the idea's to backport this to v0.34 and v0.37, right?

williambanfield · 2022-11-08T23:11:35Z

Just to confirm here, the idea's to backport this to v0.34 and v0.37, right?

At the moment the goal was to get this up and running with main. It could certainly be backported with a bit of extra work in a non-breaking way if it's important to do so. The changes introduced in v0.36 to remove the package global state in favor of using a type with methods implemented on it is, makes the backport a bit trickier but, again, it can be done definitely if it's important.

@tac0turtle, can you help judge how important it is to backport this to v0.34?

tac0turtle · 2022-11-09T10:12:19Z

I would say it's a nice to have. As we want to encourage users to upgrade, back porting everything enables them to stick around on the older versions for longer.

williambanfield · 2022-11-09T17:26:10Z

Opening this as a full PR to the tip of main. We can continue to discuss backport strategy here.

thanethomson

It looks like this PR pulled in a whole bunch of other unrelated changes to the RPC system as well?

cmd/tendermint/commands/inspect.go

docs/tools/debugging/pro.md

inspect/inspect.go

cmwaters

Nice cleanup as well.

As this PR has a few different additions (not just inspect but modifications to rpc and Canceled instead of Cancelled, make sure you document all of them in the changelog.

Other than a few minor documentation things I see nothing wrong with the PR semantically

cmd/tendermint/commands/inspect.go

docs/tools/debugging/pro.md

inspect/inspect.go

rpc/client/local/local.go

rpc/core/tx.go

rpc/core/routes.go

Co-authored-by: Thane Thomson <connect@thanethomson.com>

Co-authored-by: Callum Waters <cmwaters19@gmail.com>

cmwaters

Looks like you need to appease the linter and add a entry to the changelog. Other than that looks good

inspect/rpc/rpc.go

inspect/inspect.go

tac0turtle · 2022-11-17T10:25:35Z

does setting the db into readonly mode for this provide extra performance benefits?

williambanfield · 2022-11-21T20:34:42Z

@tac0turtle Unlikely. I checked goleveldb. It uses that information to check whether to open the files it uses in readonly mode and if it should perform a scheduled compaction. Compactions occur already when the DB is under load, so this should not be a massive performance issue from the perspective of readonly tendermint workloads.

cmwaters · 2022-11-28T21:04:21Z

rpc/core/status.go

 // hash, app hash, block height and time.
-// More: https://docs.tendermint.com/main/rpc/#/Info/status
-func Status(ctx *rpctypes.Context) (*ctypes.ResultStatus, error) {
+// More: https://docs.tendermint.com/master/rpc/#/Info/status


Don't know why this was changed back to master

cmwaters

👍

thanethomson

It would've been preferable to have this PR as 3 distinct PRs, each making just one substantial change:

The refactoring of the RPC to remove the global state
The refactoring of the indexing functionality
Implementation of the inspect command

That would've made these changes easier to reason about and quicker to review overall.

thanethomson · 2022-11-29T13:30:25Z

rpc/core/status.go

 // hash, app hash, block height and time.
-// More: https://docs.tendermint.com/main/rpc/#/Info/status
-func Status(ctx *rpctypes.Context) (*ctypes.ResultStatus, error) {
+// More: https://docs.tendermint.com/master/rpc/#/Info/status


Suggested change

// More: https://docs.tendermint.com/master/rpc/#/Info/status

// More: https://docs.tendermint.com/main/rpc/#/Info/status

tac0turtle · 2022-11-30T09:33:18Z

Is there a chance this could get backported to 0.34?

williambanfield · 2022-12-01T20:24:45Z

Is there a chance this could get backported to 0.34?

It's possible but complicated. There is a breaking change included here that makes it impossible to perform the simple mechanical backport. I had asked above about how pressing it was to backport this, has anything changed with respect to its priority?

tac0turtle · 2022-12-01T20:27:05Z

I'm building a query only like node and this would help. I can fork the rpc package and pull out what I need as well.

tychoish and others added 15 commits November 1, 2022 16:37

rpc: remove global environment (#6426)

d02ea4e

fixup broken blocks.go methods

5d7f324

rpc: add chunked rpc interface (#6445)

4bcbfd6

fixups

faf009e

wip

d7f2e1c

add indexer constructor

6fae5bd

add db package

a3f1a50

fixup inspect command to use correct indexer constructor

6fb4985

misspell cancelled

4ad837a

fixup! misspell cancelled

0e6f5ca

remove bool param from rpc

107b287

fixup node package

f67a4b5

inspect tests pass

4dd6ff8

pull DB provider from config package in all cases

a467759

williambanfield self-assigned this Nov 3, 2022

williambanfield added 2 commits November 8, 2022 17:42

fmt all edited files

93a1dcf

lint fixups

bc59e16

williambanfield marked this pull request as ready for review November 9, 2022 17:25

williambanfield requested a review from a team November 9, 2022 17:25

thanethomson reviewed Nov 9, 2022

View reviewed changes

cmwaters reviewed Nov 10, 2022

View reviewed changes

williambanfield and others added 4 commits November 10, 2022 13:55

Update cmd/tendermint/commands/inspect.go

fcf16cd

Co-authored-by: Thane Thomson <connect@thanethomson.com>

add defer close statements

0ec7be8

Update rpc/core/routes.go

8ac55ba

Co-authored-by: Callum Waters <cmwaters19@gmail.com>

master -> main

44582a5

williambanfield requested review from adizere, ebuchman and lasarojc as code owners November 14, 2022 15:33

williambanfield and others added 6 commits November 14, 2022 11:58

add comment about state store and block store

ee2fbc6

reduce number of needed close calls

ea065d0

Merge remote-tracking branch 'origin/main' into wb/inspect

f168aee

gofmt

4433e0e

add closes to mocked objects in test

80e1d1d

Merge branch 'main' into wb/inspect

7afb7d4

cmwaters reviewed Nov 16, 2022

View reviewed changes

inspect/rpc/rpc.go Show resolved Hide resolved

inspect/inspect.go Outdated Show resolved Hide resolved

williambanfield and others added 7 commits November 28, 2022 11:16

add header and header by hash

ccc99c1

delete unused eventbus and indexer service

885ae98

remove reset of eventbus

4f3217b

make the linter happier

e2f433f

Merge branch 'main' into wb/inspect

f76ff55

changelog the rpc change

8cd22dc

changelog

c25115e

cmwaters reviewed Nov 28, 2022

View reviewed changes

cmwaters approved these changes Nov 28, 2022

View reviewed changes

thanethomson approved these changes Nov 29, 2022

View reviewed changes

williambanfield added 2 commits December 5, 2022 10:38

Merge branch 'main' into wb/inspect

884eed8

Merge branch 'main' into wb/inspect

243c8e7

williambanfield merged commit 3324f49 into main Dec 7, 2022

williambanfield deleted the wb/inspect branch December 7, 2022 17:15

tnasu mentioned this pull request Dec 20, 2022

Backport e2e-test of the latest tendermint main branch Finschia/ostracon#536

Merged

	// More: https://docs.tendermint.com/master/rpc/#/Info/status
	// More: https://docs.tendermint.com/main/rpc/#/Info/status

Conversation

williambanfield commented Nov 1, 2022 • edited by thanethomson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

thanethomson commented Nov 3, 2022

Uh oh!

williambanfield commented Nov 8, 2022

Uh oh!

tac0turtle commented Nov 9, 2022

Uh oh!

williambanfield commented Nov 9, 2022

Uh oh!

thanethomson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cmwaters left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cmwaters left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tac0turtle commented Nov 17, 2022

Uh oh!

williambanfield commented Nov 21, 2022

Uh oh!

cmwaters Nov 28, 2022

Choose a reason for hiding this comment

Uh oh!

cmwaters left a comment

Choose a reason for hiding this comment

Uh oh!

thanethomson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thanethomson Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

tac0turtle commented Nov 30, 2022

Uh oh!

williambanfield commented Dec 1, 2022

Uh oh!

tac0turtle commented Dec 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

williambanfield commented Nov 1, 2022 •

edited by thanethomson

Loading

thanethomson left a comment •

edited

Loading