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

Add low power init profile #4154

Merged
merged 3 commits into from
Mar 25, 2018
Merged

Add low power init profile #4154

merged 3 commits into from
Mar 25, 2018

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented Aug 20, 2017

For context #4137

@magik6k magik6k force-pushed the feat/lowpower-profile branch 2 times, most recently from cc065aa to ecf2b6a Compare August 20, 2017 16:56
@whyrusleeping
Copy link
Member

I like this, but perhaps we should hold off on merging until we have a few more options set? If someone inits with this profile, and we later add more things to it, they wont get those benefits.

As it stands, i'm not sure if these two options are worth enough to merit their own profile at this point.

@magik6k
Copy link
Member Author

magik6k commented Aug 26, 2017

Agreed, having an option to apply profile to the config after it was generated might be an option too, though for a separate PR.

@whyrusleeping
Copy link
Member

We should add a lower connection limits here, also when we get this implemented, set the default for this to 'dhtclient'

@magik6k
Copy link
Member Author

magik6k commented Oct 29, 2017

This PR actually implements the routing option (under Discovery.Routing)

Stebalien
Stebalien reviewed Oct 30, 2017
View reviewed changes
docs/config.md Outdated
- `dht` (default)
- `dhtclient`
- `none`
- `supernode` (deprecated)
Copy link
Member

Choose a reason for hiding this comment

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

This has now been removed.

cmd/ipfs/init.go
@@ -36,6 +36,8 @@ Available profiles:
running IPFS on machines with public IPv4 addresses.
'test' - Reduces external interference of IPFS daemon, this
is useful when using the daemon in test environments.
'lowpower' - Reduces daemon overhead on the system. May affect node
Copy link
Member

Choose a reason for hiding this comment

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

Maybe 'laptop'? Honestly, laptop users should use this (less problems with random WiFis, fewer ephemeral DHT nodes, etc. Alternatively, we could have a separate laptop mode that occasionally reprovides pin roots.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably the second option, I meant this setting for mobile devices (Android phones, etc.)

Copy link
Member

Choose a reason for hiding this comment

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

"May affect node functionality" should be better described to tell what functionality it would affect. Seemingly, it seems to run the dht in client mode and also set really low ConnMgr values, meaning it would probably take longer to find and download content. Would be nice to add that to the description.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

whyrusleeping
whyrusleeping reviewed Oct 31, 2017
View reviewed changes
repo/config/discovery.go Outdated
@@ -2,6 +2,9 @@ package config

type Discovery struct {
MDNS MDNS

//Routing sets default daemon routing mode.
Routing string
Copy link
Member

Choose a reason for hiding this comment

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

I would make Routing its own top level section, and then give it a Type field (so we can add other parameters later)

@ghost ghost assigned magik6k Oct 31, 2017
@ghost ghost added the status/in-progress In progress label Oct 31, 2017
@magik6k
Copy link
Member Author

magik6k commented Oct 31, 2017

We should chose some connection limiter settings here. Would something like 75/100 work? Or is it a 'more testing is needed' kind of situation?

@whyrusleeping
Copy link
Member

@magik6k I think that with dhtclient mode, even something as low as 20/40 could work well if we set a relatively high grace period (1m or more).

@magik6k magik6k mentioned this pull request Nov 1, 2017
Merged
3 tasks
@magik6k
Copy link
Member Author

magik6k commented Jan 22, 2018

Rebased

@whyrusleeping
Copy link
Member

This LGTM, weird that so much of CI is angry though...

@magik6k magik6k force-pushed the feat/lowpower-profile branch 3 times, most recently from 76dbecf to b1afa87 Compare January 22, 2018 19:59
@magik6k
Copy link
Member Author

magik6k commented Jan 23, 2018

Made CI less angry

@whyrusleeping
Copy link
Member

The travis OSX backlog is pretty nuts...

@magik6k magik6k force-pushed the feat/lowpower-profile branch 2 times, most recently from 1c090bb to 0497f57 Compare January 25, 2018 16:25
@magik6k magik6k requested a review from Kubuxu as a code owner March 23, 2018 14:56
@magik6k
Copy link
Member Author

magik6k commented Mar 23, 2018

Rebased

@Kubuxu Kubuxu added the need/author-input Needs input from the original author label Mar 23, 2018
@Kubuxu
Copy link
Member

Kubuxu commented Mar 23, 2018

"Nothing works, everything is broken" - by "Someone had to say it at some point"

@magik6k
daemon: config option for routing
35da088 
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@magik6k
init: lowpower profile
fae9967 
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@magik6k
config: apply review to lowpower profile
af9b4af 
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@magik6k
Copy link
Member Author

magik6k commented Mar 24, 2018

Fixed

@whyrusleeping
Copy link
Member

Thanks @magik6k! Let's start telling people about this one :)

@whyrusleeping whyrusleeping merged commit 7d84246 into master Mar 25, 2018
@ghost ghost removed the status/in-progress In progress label Mar 25, 2018
@whyrusleeping whyrusleeping deleted the feat/lowpower-profile branch March 25, 2018 03:55
@camelmasa camelmasa mentioned this pull request Apr 23, 2018
Merged
@magik6k magik6k mentioned this pull request May 14, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stebalien Stebalien Stebalien left review comments

@victorb victorb victorb left review comments

@whyrusleeping whyrusleeping whyrusleeping left review comments

@Kubuxu Kubuxu Awaiting requested review from Kubuxu

Assignees

@magik6k magik6k

Labels
need/author-input Needs input from the original author
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@magik6k @whyrusleeping @Kubuxu @Stebalien @victorb