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

Make --lru_mb optional #3850

Closed
campoy opened this issue Aug 23, 2019 · 4 comments · Fixed by #3898
Closed

Make --lru_mb optional #3850

campoy opened this issue Aug 23, 2019 · 4 comments · Fixed by #3898
Labels
area/usability Issues with usability and error messages exp/beginner Something most people could solve. kind/enhancement Something could be better. priority/P2 Somehow important but would not block a release. status/accepted We accept to investigate/work on it.

Comments

@campoy
Copy link
Contributor

campoy commented Aug 23, 2019

Currently, alphas won't work unless you provide a value for --lru_mb of at least 1024MB

We should instead set the default to 1024MB.

@campoy campoy added kind/enhancement Something could be better. exp/beginner Something most people could solve. priority/P2 Somehow important but would not block a release. status/accepted We accept to investigate/work on it. area/usability Issues with usability and error messages labels Aug 23, 2019
@MichelDiz
Copy link
Contributor

MichelDiz commented Aug 23, 2019

Just an opinion, I think we should try catch the RAM available and set 50% of it as default.

e.g (from stackoverflow)

package main

// #include <unistd.h>
import "C"

func main() {
    println(C.sysconf(C._SC_PHYS_PAGES)*C.sysconf(C._SC_PAGE_SIZE), " bytes")
}

Or https://godoc.org/github.com/capnm/sysinfo

If we set 1024MB as default users can think this is some kind of minimal recommendation. But the real recommendation is to use at least 50% of available RAM.

@mangalaman93
Copy link
Contributor

I do not think lru_mb is currently used at all. It was previously used when we had an LRU cache, it was removed from Dgraph at some point.

@campoy
Copy link
Contributor Author

campoy commented Sep 3, 2019

I don't care that much about the value, but making it compulsory when we're not even using it seems unnecessary.

Running the version on master as today I get:

➜  dgraph git:(master) ~/bin/dgraph alpha
[Decoder]: Using assembly version of decoder
2019/09/03 09:30:05 LRU memory (--lru_mb) must be specified. (At least 1024 MB)

campoy added a commit that referenced this issue Sep 3, 2019
@campoy
Copy link
Contributor Author

campoy commented Sep 3, 2019

After a chat with @manishrjain we decided that we should try to guess the total size of RAM and:

  • if we find the size, use 50% of it and log a warning with a link to a page explaining how to set the lru_mb correctly
  • if we don't find the size (as it probably happens with Windows?) fail and log an error with the same link as above

This will be part of the next release, not necessary for 1.1.0

campoy added a commit that referenced this issue Sep 3, 2019
campoy pushed a commit that referenced this issue Sep 3, 2019
* make lru_mb optional

Fixes #3850

* applied Manish's feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/usability Issues with usability and error messages exp/beginner Something most people could solve. kind/enhancement Something could be better. priority/P2 Somehow important but would not block a release. status/accepted We accept to investigate/work on it.
Development

Successfully merging a pull request may close this issue.

3 participants