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 #3898

Merged
merged 2 commits into from
Sep 3, 2019
Merged

make lru_mb optional #3898

merged 2 commits into from
Sep 3, 2019

Conversation

campoy
Copy link
Contributor

@campoy campoy commented Sep 3, 2019

Fixes #3850


This change is Reviewable

@campoy campoy requested review from manishrjain and a team as code owners September 3, 2019 05:05
Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

✅ A review job has been created and sent to the PullRequest network.


@campoy you can click here to see the review status or cancel the code review job.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PR does exactly what it says it does in the description. Looked at the C related things in particular and nothing stood out to me at this time.


Reviewed with ❤️ by PullRequest

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: Got a couple of comments. Please address before merging.

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @campoy)


edgraph/config.go, line 105 at r1 (raw file):

		mb := bytes / 1024 / 1024
		if mb > MinAllottedMemory {
			opt.AllottedMemory = float64(mb / 2)

Set it to 25% of total RAM.


edgraph/config.go, line 106 at r1 (raw file):

		if mb > MinAllottedMemory {
			opt.AllottedMemory = float64(mb / 2)
			fmt.Fprintf(os.Stderr,

Use glog.Infof.


edgraph/config.go, line 108 at r1 (raw file):

			fmt.Fprintf(os.Stderr,
				"LRU memory (--lru_mb) set to %vMB, 50%% of the total RAM found (%vMB)\n"+
					"For more information on --lru_mb please read https://docs.dgraph.io/deploy/#config\n",

100 chars.

Copy link
Contributor Author

@campoy campoy left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @manishrjain)


edgraph/config.go, line 105 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Set it to 25% of total RAM.

Done.


edgraph/config.go, line 106 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Use glog.Infof.

Done.


edgraph/config.go, line 108 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

100 chars.

Done.

@campoy campoy merged commit 7c589be into master Sep 3, 2019
@campoy campoy deleted the lrumb branch September 3, 2019 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Make --lru_mb optional
2 participants