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

go/consensus: Move upgrade logic to governance, gracefully handle halt #3755

Merged
merged 1 commit into from
Mar 5, 2021

Conversation

kostko
Copy link
Member

@kostko kostko commented Mar 4, 2021

No description provided.

@kostko kostko added the c:consensus/cometbft Category: CometBFT label Mar 4, 2021
@kostko kostko force-pushed the kostko/fix/upgrade-halt branch 7 times, most recently from 59042fc to a4c8d42 Compare March 5, 2021 10:28
Copy link
Member

@ptrus ptrus 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! left some minor comments

go/consensus/tendermint/full/full.go Outdated Show resolved Hide resolved
go func() {
// Sleep another period so there is some time between when consensus shuts down and
// when all the other services start shutting down.
time.Sleep(waitPeriod)
Copy link
Member

Choose a reason for hiding this comment

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

Would it make more sense to use a different (static, not dependent on TimeoutCommit) period here?

@kostko kostko force-pushed the kostko/fix/upgrade-halt branch from a4c8d42 to 8b391b6 Compare March 5, 2021 10:50
@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #3755 (8b391b6) into master (f030415) will decrease coverage by 0.05%.
The diff coverage is 75.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3755      +/-   ##
==========================================
- Coverage   67.07%   67.02%   -0.06%     
==========================================
  Files         401      401              
  Lines       39927    39970      +43     
==========================================
+ Hits        26783    26790       +7     
- Misses       9363     9368       +5     
- Partials     3781     3812      +31     
Impacted Files Coverage Δ
go/consensus/api/api.go 14.28% <ø> (ø)
go/consensus/tendermint/api/state.go 58.33% <0.00%> (-0.83%) ⬇️
go/consensus/tendermint/seed/seed.go 68.24% <ø> (ø)
go/oasis-node/cmd/node/node.go 53.55% <ø> (-1.66%) ⬇️
go/upgrade/api/api.go 92.30% <ø> (ø)
go/upgrade/upgrade.go 61.48% <0.00%> (-0.83%) ⬇️
go/consensus/tendermint/beacon/beacon.go 70.32% <50.00%> (+2.53%) ⬆️
...consensus/tendermint/apps/governance/governance.go 67.07% <56.25%> (-0.74%) ⬇️
go/consensus/tendermint/full/full.go 63.67% <87.09%> (-0.94%) ⬇️
go/common/grpc/grpc.go 85.34% <100.00%> (+0.44%) ⬆️
... and 44 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8640bf2...d1d1ebc. Read the comment docs.

@kostko kostko force-pushed the kostko/fix/upgrade-halt branch from 8b391b6 to d1d1ebc Compare March 5, 2021 13:54
@kostko kostko enabled auto-merge March 5, 2021 13:55
@kostko kostko merged commit bd10b95 into master Mar 5, 2021
@kostko kostko deleted the kostko/fix/upgrade-halt branch March 5, 2021 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:consensus/cometbft Category: CometBFT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants