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

core: do not run bloom in case of ephemeral node #2953

Merged
merged 7 commits into from
Jul 10, 2016

Conversation

Kubuxu
Copy link
Member

@Kubuxu Kubuxu commented Jul 8, 2016

I will do nice and full refactor of the Online vs Ephemeral modes.

This PR includes parts of #2942 which cleanup passing of the context and options to the caches.

The important change is 697a345


func DefaultCacheOpts() CacheOpts {
return CacheOpts{
512 * 8 * 1024,
Copy link
Member

Choose a reason for hiding this comment

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

dont leave out the field names please

License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu Kubuxu force-pushed the feature/blocks-bloom-no-rebuild branch from d225a1d to 3b8980d Compare July 8, 2016 18:54
@Kubuxu
Copy link
Member Author

Kubuxu commented Jul 8, 2016

@whyrusleeping sorry for messing it up at first try. This should be working as intended.

Kubuxu added 4 commits July 8, 2016 21:17
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu Kubuxu force-pushed the feature/blocks-bloom-no-rebuild branch from 3b8980d to af77782 Compare July 8, 2016 19:17
func CachedBlockstore(bs GCBlockstore,
ctx context.Context, opts CacheOpts) (cbs GCBlockstore, err error) {
if ctx == nil {
ctx = context.TODO() // For tests
Copy link
Member

Choose a reason for hiding this comment

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

I dont like this, youre only saving yourself typing this in two other places.

Kubuxu added 2 commits July 10, 2016 15:16
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu
Copy link
Member Author

Kubuxu commented Jul 10, 2016

@whyrusleeping updated

@whyrusleeping
Copy link
Member

This LGTM

@whyrusleeping whyrusleeping merged commit 4e67003 into master Jul 10, 2016
@whyrusleeping whyrusleeping deleted the feature/blocks-bloom-no-rebuild branch July 10, 2016 20:34
@@ -27,6 +27,10 @@ type BuildCfg struct {
// If online is set, the node will have networking enabled
Online bool

// If permament then node should run more expensive processes
// that will improve performance in long run
Permament bool
Copy link
Member

Choose a reason for hiding this comment

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

this is not a good name. the node wont ever be Permanent. you're using it here to signify "NotEphemeral", so just use Ephemeral and set it to false.

Copy link
Member Author

@Kubuxu Kubuxu Aug 28, 2016

Choose a reason for hiding this comment

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

Didn't use Ephemeral as it would be hard to find all tests and places where node is created and set it there to true.

This part is on my list to refactor for the split to: Online, Offline and Ephemeral.

Copy link
Member

Choose a reason for hiding this comment

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

split to: Online, Offline and Ephemeral

those aren't good distinctions. an ephemeral node may be online and may be offline.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently we don't have Online Ephemeral, but very good point.

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.

3 participants