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

Extraction of configuration from packages #1172

Closed
wants to merge 26 commits into from
Closed

Conversation

tzdybal
Copy link
Contributor

@tzdybal tzdybal commented Jul 11, 2017

For library usage, individual packages cannot call flag to setup configuration options. All the command line parsing should be done in cmd/dgraph/main.

This change is related to #673.


This change is Reviewable

@tzdybal tzdybal force-pushed the feature/embedded branch from 35ffe62 to 2ecae7e Compare July 11, 2017 05:43
@janardhan1993
Copy link
Contributor

Reviewed 1 of 24 files at r1.
Review status: 1 of 24 files reviewed at latest revision, 2 unresolved discussions.


cmd/dgraph/main.go, line 99 at r1 (raw file):

		"Number of pending mutation proposals. Useful for rate limiting.")
	flag.Float64Var(&worker.Config.Tracing, "trace", 0.0, "The ratio of queries to trace.")
	flag.StringVar(&worker.Config.GroupIds, "groups", "0,1", "RAFT groups handled by this server.")

100 chars


worker/groups.go, line 43 at r1 (raw file):

var (
	pstore           *badger.KV
	workerServer     *grpc.Server

These variables are used/initialized in worker.go. Shouldn't these stay there ?


Comments from Reviewable

@tzdybal
Copy link
Contributor Author

tzdybal commented Jul 12, 2017

Review status: 1 of 24 files reviewed at latest revision, 2 unresolved discussions.


cmd/dgraph/main.go, line 99 at r1 (raw file):

Previously, janardhan1993 (Janardhan Reddy) wrote…

100 chars

Done.


worker/groups.go, line 43 at r1 (raw file):

Previously, janardhan1993 (Janardhan Reddy) wrote…

These variables are used/initialized in worker.go. Shouldn't these stay there ?

Variables are used everywhere (for example pstore is accessed in 8 files). Entire var block moved to worker.go.


Comments from Reviewable

@manishrjain
Copy link
Contributor

Some comments. Otherwise, looks good. I'll defer to @janardhan1993 for the final approval.


Reviewed 21 of 24 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 9 unresolved discussions.


cmd/dgraph/main.go, line 666 at r2 (raw file):

	// Posting will initialize index which requires schema. Hence, initialize
	// schema before calling posting.Init().
	schema.Init(dgraph.State.PStore)

Pstore (small s)


cmd/dgraphloader/main.go, line 208 at r2 (raw file):

func main() {
	flag.Parse()
	x.Init()

Isn't x.Init() calling flag.Parse?


dgraph/server.go, line 41 at r2 (raw file):

	ShutdownCh chan struct{} // channel to signal shutdown.

	PStore   *badger.KV

Pstore


dgraph/server.go, line 42 at r2 (raw file):

	PStore   *badger.KV
	WALStore *badger.KV

WALstore


dgraph/server.go, line 80 at r2 (raw file):

	opt.Dir = Config.PostingDir
	opt.ValueDir = Config.PostingDir
	opt.MapTablesTo = table.MemoryMap

Make this a flag, so user can decide how they want this. There're 3 options -- talk to @janardhan1993 .


posting/config.go, line 20 at r2 (raw file):

type Options struct {
	MaxMemory      float64

Call it AllottedMemory (not allocated).


posting/lists.go, line 211 at r2 (raw file):

			x.NumGoRoutines.Set(int64(runtime.NumGoroutine()))
			if setLruMemory && inUse > 0.75*(Config.MaxMemory) {
				go lcache.UpdateMaxSize()

This doesn't need to be a goroutine.


Comments from Reviewable

@tzdybal tzdybal force-pushed the feature/embedded branch from d03cb5d to 0fcff13 Compare July 13, 2017 00:28
@tzdybal
Copy link
Contributor Author

tzdybal commented Jul 13, 2017

Review status: 14 of 26 files reviewed at latest revision, 9 unresolved discussions.


cmd/dgraph/main.go, line 666 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Pstore (small s)

Done.


cmd/dgraphloader/main.go, line 208 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Isn't x.Init() calling flag.Parse?

Removed it from x.Init(), because we need to call x.Init() in library initialization, and we don't want the library to touch the flag library at all.


dgraph/server.go, line 41 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Pstore

Done.


dgraph/server.go, line 42 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

WALstore

Done.


dgraph/server.go, line 80 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Make this a flag, so user can decide how they want this. There're 3 options -- talk to @janardhan1993 .

OK, I'll talk to him.


posting/config.go, line 20 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Call it AllottedMemory (not allocated).

Done.


posting/lists.go, line 211 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

This doesn't need to be a goroutine.

Nothing changed in here, but I'll change it to normal function invocation.


Comments from Reviewable

@janardhan1993
Copy link
Contributor

:lgtm:


Review status: 14 of 26 files reviewed at latest revision, 9 unresolved discussions.


Comments from Reviewable

@tzdybal
Copy link
Contributor Author

tzdybal commented Jul 13, 2017

Merged to master.

@tzdybal tzdybal closed this Jul 13, 2017
jarifibrahim pushed a commit that referenced this pull request Mar 16, 2020
Important changes
```
 - Changes to overlap check in compaction.
 - Remove 'this entry should've been caught' log.
 - Changes to write stalling on levels 0 and 1.
 - Compression is disabled by default in Badger.
 - Bloom filter caching in a separate ristretto cache.
 - Compression/Encryption in background.
 - Disable cache by default in badger.
```

The following new changes are being added from badger
`git log ab4352b00a17...91c31ebe8c22`

```
91c31eb Disable cache by default (#1257)
eaf64c0 Add separate cache for bloom filters (#1260)
1bcbefc Add BypassDirLock option (#1243)
c6c1e5e Add support for watching nil prefix in subscribe API (#1246)
b13b927 Compress/Encrypt Blocks in the background (#1227)
bdb2b13 fix changelog for v2.0.2 (#1244)
8dbc982 Add Dkron to README (#1241)
3d95b94 Remove coveralls from Travis Build(#1219)
5b4c0a6 Fix ValueThreshold for in-memory mode (#1235)
617ed7c Initialize vlog before starting compactions in db.Open (#1226)
e908818 Update CHANGELOG for Badger 2.0.2 release. (#1230)
bce069c Fix int overflow for 32bit (#1216)
e029e93 Remove ExampleDB_Subscribe Test (#1214)
8734e3a Add missing package to README for badger.NewEntry (#1223)
78d405a Replace t.Fatal with require.NoError in tests (#1213)
c51748e Fix flaky TestPageBufferReader2 test (#1210)
eee1602 Change else-if statements to idiomatic switch statements. (#1207)
3e25d77 Rework concurrency semantics of valueLog.maxFid (#1184) (#1187)
4676ca9 Add support for caching bloomfilters (#1204)
c3333a5 Disable compression and set ZSTD Compression Level to 1 (#1191)
0acb3f6 Fix L0/L1 stall test (#1201)
7e5a956 Support disabling the cache completely. (#1183) (#1185)
82381ac Update ristretto to version  8f368f2 (#1195)
3747be5 Improve write stalling on level 0 and 1
5870b7b Run all tests on CI (#1189)
01a00cb Add Jaegar to list of projects (#1192)
9d6512b Use fastRand instead of locked-rand in skiplist (#1173)
2698bfc Avoid sync in inmemory mode (#1190)
2a90c66 Remove the 'this entry should've caught' log from value.go (#1170)
0a06173 Fix checkOverlap in compaction (#1166)
0f2e629 Fix windows build (#1177)
03af216 Fix commit sha for WithInMemory in CHANGELOG. (#1172)
23a73cd Update CHANGELOG for v2.0.1 release. (#1181)
465f28a Cast sz to uint32 to fix compilation on 32 bit (#1175)
ea01d38 Rename option builder from WithInmemory to WithInMemory. (#1169)
df99253 Remove ErrGCInMemoryMode in CHANGELOG. (#1171)
8dfdd6d Adding changes for 2.0.1 so far (#1168)
```
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.

3 participants