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.

Sign up for GitHub

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

Jump to bottom

Pruning cleanup #1533

Merged
merged 1 commit into from
Jul 13, 2018
Merged

Pruning cleanup #1533

merged 1 commit into from
Jul 13, 2018

Conversation

UnitylChaos
Copy link
Contributor

@UnitylChaos UnitylChaos commented Jul 4, 2018 •
edited
Loading

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG.md
  • Updated Gaia/Examples
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)
  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)

ref: #1516

@UnitylChaos UnitylChaos requested a review from ebuchman as a code owner July 4, 2018 01:37
@UnitylChaos UnitylChaos force-pushed the jlandrews/pruningcleanup branch from a2f958a to 43db01b Compare July 4, 2018 01:38
@codecov
Copy link

codecov bot commented Jul 4, 2018 •
edited
Loading

Codecov Report

Merging #1533 into master will decrease coverage by 0.2%.
The diff coverage is 27.02%.

@@            Coverage Diff             @@
##           master    #1533      +/-   ##
==========================================
- Coverage   63.04%   62.84%   -0.21%     
==========================================
  Files         123      124       +1     
  Lines        6960     6992      +32     
==========================================
+ Hits         4388     4394       +6     
- Misses       2321     2346      +25     
- Partials      251      252       +1

@UnitylChaos UnitylChaos force-pushed the jlandrews/pruningcleanup branch from 43db01b to d66e5a7 Compare July 6, 2018 01:36
ebuchman
ebuchman reviewed Jul 6, 2018
View reviewed changes
store/rootmultistore.go Outdated
@@ -40,6 +42,7 @@ func NewCommitMultiStore(db dbm.DB) *rootMultiStore {
storesParams: make(map[StoreKey]storeParams),
stores: make(map[StoreKey]CommitStore),
keysByName: make(map[string]StoreKey),
pruning: viper.GetString("pruning"),
Copy link
Member

Choose a reason for hiding this comment

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

;)

@UnitylChaos UnitylChaos mentioned this pull request Jul 6, 2018
Closed
7 tasks
@jackzampolin jackzampolin mentioned this pull request Jul 6, 2018
Open
@UnitylChaos UnitylChaos mentioned this pull request Jul 7, 2018
Merged
7 tasks
@UnitylChaos UnitylChaos force-pushed the jlandrews/pruningcleanup branch from d66e5a7 to 242a1d7 Compare July 10, 2018 16:45
@ebuchman ebuchman changed the base branch from develop to master July 11, 2018 00:29
@UnitylChaos UnitylChaos force-pushed the jlandrews/pruningcleanup branch from 242a1d7 to 7d147d4 Compare July 11, 2018 01:58
@UnitylChaos UnitylChaos removed the wip label Jul 11, 2018
@UnitylChaos UnitylChaos changed the title WIP - Pruning cleanup Pruning cleanup Jul 11, 2018
@UnitylChaos
Copy link
Contributor Author

Updated to use functional options. We should still do some CLI / gaiad testing, but that could wait on a refactor / clean up of test_cli in general.

alexanderbez
alexanderbez reviewed Jul 11, 2018
View reviewed changes
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.

LGTM 👍

@UnitylChaos UnitylChaos mentioned this pull request Jul 11, 2018
Closed
@UnitylChaos UnitylChaos force-pushed the jlandrews/pruningcleanup branch from 1fa9460 to 2546b8e Compare July 11, 2018 23:24
@zmanian
Copy link
Member

zmanian commented Jul 12, 2018

Is there any chance we can get this in before gaia-7000 launches?

alexanderbez
alexanderbez approved these changes Jul 12, 2018
View reviewed changes
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.

Yeah, this LGTM 🎉

@UnitylChaos UnitylChaos force-pushed the jlandrews/pruningcleanup branch from 47040cf to 0a51c56 Compare July 12, 2018 17:41
cwgoes
cwgoes reviewed Jul 12, 2018
View reviewed changes
store/iavlstore.go Outdated
// Implements Committer.
func (st *iavlStore) SetPruning(pruning string) {
switch pruning {
case "everything":
Copy link
Contributor

Choose a reason for hiding this comment

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

Can pruning be an enum instead of a string, or is that hard to use with functional options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could. Would just have to move this switch case into the baseapp.SetPruning function and have it translate to enum there, so that the Committer SetPruning function could take enums.

Copy link
Contributor

Choose a reason for hiding this comment

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

An enum would be better I think; if the user specifies an invalid option we should fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

cwgoes
cwgoes suggested changes Jul 12, 2018
View reviewed changes
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Logic LGTM; a few suggestions.

store/iavlstore.go Outdated
@@ -16,18 +16,19 @@ import (
const (
defaultIAVLCacheSize = 10000
defaultIAVLNumRecent = 100
defaultIAVLStoreEvery = 1
defaultIAVLStoreEvery = 10000
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we choose one strategy (syncable) as default instead of duplicating constants?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do. The constants aren't actually being used currently anyway, just there for the constructor before SetPruning is called. I could change the constructor to not take them in as parameters and only use SetPruning, but that would change all the tests. Simpler solution is to just have LoadIAVLStore pass in zero for both and then call SetPruning.

baseapp/options.go
sdk "github.com/cosmos/cosmos-sdk/types"
)

// File for storing in-package BaseApp optional functions,
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool - is it also possible to set these options in config.toml or a Gaia-specific file / is there some way we could architect that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should definitely have this able to be set in config, but I think currently everything in the toml is managed by tendermint, so we would either have to add it there, or make a another config file. I would say we should make a separate issue to plan out how we want to do gaia/app specific config files.

Copy link
Contributor

Choose a reason for hiding this comment

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

#1662

@jackzampolin jackzampolin mentioned this pull request Jul 12, 2018
Closed
@UnitylChaos UnitylChaos force-pushed the jlandrews/pruningcleanup branch from 72a3469 to 341e1d0 Compare July 13, 2018 01:04
@cwgoes cwgoes mentioned this pull request Jul 13, 2018
Closed
@cwgoes cwgoes merged commit 43b9cc6 into master Jul 13, 2018
@cwgoes cwgoes deleted the jlandrews/pruningcleanup branch July 13, 2018 01:20
@zmanian
Copy link
Member

zmanian commented Jul 13, 2018

Great work!

@robert-zaremba robert-zaremba mentioned this pull request Mar 11, 2023
Closed
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebuchman ebuchman ebuchman left review comments

@cwgoes cwgoes cwgoes approved these changes

@alexanderbez alexanderbez alexanderbez approved these changes

@rigelrozanski rigelrozanski Awaiting requested review from rigelrozanski

Assignees
No one assigned
Labels
C:CLI T: Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@UnitylChaos @zmanian @ebuchman @alexanderbez @cwgoes