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

fastsync/rpc: add TotalSyncedTime & RemainingTime to SyncInfo in /status RPC #6620

Merged
merged 13 commits into from
Jul 7, 2021

Conversation

JayT106
Copy link
Contributor

@JayT106 JayT106 commented Jun 25, 2021

closes #3365 once #6619 merged

internal/blockchain/v0/reactor_test.go Outdated Show resolved Hide resolved
internal/blockchain/v0/pool.go Outdated Show resolved Hide resolved
internal/blockchain/v0/pool.go Outdated Show resolved Hide resolved
internal/blockchain/v0/reactor.go Outdated Show resolved Hide resolved
@JayT106 JayT106 force-pushed the fastSync-time-metrics branch 2 times, most recently from 286514f to f273668 Compare June 28, 2021 16:11
@codecov
Copy link

codecov bot commented Jun 28, 2021

Codecov Report

Merging #6620 (0ff9cb6) into master (007eeb9) will decrease coverage by 5.06%.
The diff coverage is 17.31%.

@@            Coverage Diff             @@
##           master    #6620      +/-   ##
==========================================
- Coverage   65.99%   60.92%   -5.07%     
==========================================
  Files         234      297      +63     
  Lines       20269    28210    +7941     
==========================================
+ Hits        13376    17187    +3811     
- Misses       5825     9291    +3466     
- Partials     1068     1732     +664     
Impacted Files Coverage Δ
abci/client/grpc_client.go 0.00% <0.00%> (ø)
abci/client/local_client.go 0.00% <0.00%> (ø)
abci/types/application.go 0.00% <0.00%> (ø)
abci/types/pubkey.go 0.00% <0.00%> (ø)
abci/types/result.go 23.07% <0.00%> (ø)
abci/types/util.go 0.00% <0.00%> (ø)
cmd/tendermint/commands/gen_node_key.go 0.00% <ø> (ø)
cmd/tendermint/commands/gen_validator.go 18.18% <ø> (+18.18%) ⬆️
cmd/tendermint/commands/init.go 3.27% <ø> (+3.27%) ⬆️
cmd/tendermint/commands/light.go 24.27% <ø> (ø)
... and 429 more

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. Nice work @JayT106. Mind taking a look @cmwaters?

rpc/openapi/openapi.yaml Show resolved Hide resolved
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

Looks good. Just a few tweaks

internal/blockchain/v0/pool.go Show resolved Hide resolved
internal/blockchain/v0/pool.go Outdated Show resolved Hide resolved
internal/blockchain/v0/pool.go Outdated Show resolved Hide resolved
internal/blockchain/v0/pool.go Show resolved Hide resolved
internal/blockchain/v2/reactor.go Outdated Show resolved Hide resolved
internal/blockchain/v2/reactor.go Outdated Show resolved Hide resolved
@cmwaters cmwaters changed the title statesync/rpc: add TotalSyncedTime & RemainingTime to SyncInfo in /status RPC blockchain/rpc: add TotalSyncedTime & RemainingTime to SyncInfo in /status RPC Jun 29, 2021
@JayT106 JayT106 changed the title blockchain/rpc: add TotalSyncedTime & RemainingTime to SyncInfo in /status RPC fastsync/rpc: add TotalSyncedTime & RemainingTime to SyncInfo in /status RPC Jun 29, 2021
@JayT106 JayT106 force-pushed the fastSync-time-metrics branch 2 times, most recently from eb53e88 to fb69406 Compare June 29, 2021 15:38
Copy link
Contributor

@cmwaters cmwaters 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 Jay

On a different node, do we want to look at the interplay between state sync and the SyncInfo RPC response. Right now, there's no mention of state syncing in the query

@JayT106
Copy link
Contributor Author

JayT106 commented Jun 30, 2021

LGTM 👍 Thanks Jay

On a different node, do we want to look at the interplay between state sync and the SyncInfo RPC response. Right now, there's no mention of state syncing in the query

I think it's a good idea. Should we make a different PR for addressing the state sync info?

Copy link
Contributor

@tychoish tychoish left a comment

Choose a reason for hiding this comment

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

This code seems decent, but I think there are some edge cases about nodes in different situations around some node configurations that I think are important to think about.

internal/blockchain/v0/reactor.go Outdated Show resolved Hide resolved
rpc/core/status.go Show resolved Hide resolved
@alexanderbez
Copy link
Contributor

@JayT106 yes, sure, we can tackle that in a separate PR 👍

@JayT106 JayT106 force-pushed the fastSync-time-metrics branch from 822e351 to 79340b6 Compare June 30, 2021 18:46
libs/sync/atomic_bool.go Outdated Show resolved Hide resolved
libs/sync/atomic_bool.go Show resolved Hide resolved
libs/sync/atomic_bool_test.go Outdated Show resolved Hide resolved

import "testing"

func TestDefaultValue(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I might also add a few tests that attempt to modify the structure a bunch of times from multiple threads, in a way that the race detector would catch. The interesting part of this type is that it's thread safe/atomic, not that it's a boolean, and it would be nice if the tests would help provide confidence there.

@JayT106 JayT106 force-pushed the fastSync-time-metrics branch from cbf899d to d270f0f Compare July 6, 2021 20:37
@alexanderbez alexanderbez merged commit d4cda54 into tendermint:master Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Information related to remaining blocks need to be synced and remaining time
4 participants