[BREAKING CHANGE] proposal: Builder API for Options

[campoy]

Since we're changing our API I'm thinking we should take the opportunity to break a couple more things
that will make our API nicer to use.

One of the complains I've previously seen is how the Open API is not idiomatic nor friendly.
Indeed it forces the user to first fetch the DefaultOptions, then modify them, to finally pass them.

I propose a simple change on which the default options are implicity and instead we simply pass Option
values which provide ways to set specific settings.

For instance the current way to open a database is:

	opts := badger.DefaultOptions
	opts.Dir = "mydir"
	opts.ValueDir = "mydir"

	db, err := badger.Open(opts)

With the new API, the code becomes:

	db, err := badger.Open("mydir")

If the user wants to open it in read-only mode:

	db, err := badger.Open("mydir", options.WithReadOnly(true))

It is much easier for beginners and still provides the same options as previously.

---

This change is 

[manishrjain]

High level comments. You could still keep the current badger.Open(opts), but just change how opts is being created to achieve the same intended effect.

opts := badger.DefaultOptions().SetDir("dir").SetValueDir("vdir").SetReadOnly().SetSomething(...)
badger.Open(opts)

It could be further enhanced in various ways to cut down some boilerplate.

Reviewable status: 0 of 36 files reviewed, all discussions resolved (waiting on @ashish-goswami and @manishrjain)

[campoy]

I thought about having a builder API, I'm not against it but it didn't look as nice because you still had to somehow have the Default function first.

My biggest concern is having the path we need to open a file as part of the options instead of a parameter. That single change makes the code much easier (most users use the same path for both the tables and values dirs)

[campoy]

Yeah, I've been thinking about it and while I like the fact that having a DefaultOptions function in badger makes it so you don't need to also import the options package, the builder API still makes the easiest case still more complicated:

Currently:

	opts := badger.DefaultOptions
	opts.Dir = "mydir"
	opts.ValueDir = "mydir"

	db, err := badger.Open(opts)

Functional Options API:

	db, err := badger.Open("mydir")

Builder API:

	db, err := badger.Open("mydir", badger.DefaultOptions())

Alternatively, we could receive a pointer and assume that a nil value corresponds to the default options, but not sure whether that's easier to understand than the Functional Options API

[campoy]

After a conversation with @manishrjain, I will rework on this API to keep it backwards compatible with the current one and instead offer a more convenient way to create Options values.

The biggest change will be the creation of a options.Default(path string) function, which will allow us to open files like this:

db, err := badger.Open(options.Default(path))`

We will also provide a builder API on options.Options so we can create new values easily:

db, err := badger.Open(options.Default(path).WithReadOnly(true))

As this change is now backwards compatible it can be removed from the v2.0.0 milestone.

[campoy]

As agreed with @manishrjain, I updated the PR to provide a change that will be backwards compatible with badger.Open.

I did decide it was probably better not to be backwards compatible with DefaultOptions by making it a function that receives a parameter rather than a variable.
The reasoning here is that people could change the value of DefaultOptions and expect some behavior. It's better not to allow it at all.

The output of go doc is much nicer than before too.

type Options struct {
        Dir      string
        ValueDir string

        SyncWrites          bool
        TableLoadingMode    options.FileLoadingMode
        ValueLogLoadingMode options.FileLoadingMode
        NumVersionsToKeep   int
        ReadOnly            bool
        Truncate            bool
        Logger              Logger

        MaxTableSize        int64
        LevelSizeMultiplier int
        MaxLevels           int
        ValueThreshold      int
        NumMemtables        int

        NumLevelZeroTables      int
        NumLevelZeroTablesStall int

        LevelOneSize       int64
        ValueLogFileSize   int64
        ValueLogMaxEntries uint32

        NumCompactors     int
        CompactL0OnClose  bool
        LogRotatesToFlush int32

        // Has unexported fields.
}
    Options are params for creating DB object.

    This package provides DefaultOptions which contains options that should work
    for most applications. Consider using that as a starting point before
    customizing it for your own needs.

    Each option X is documented on the WithX method.

func DefaultOptions(path string) Options
func LSMOnlyOptions(path string) Options
func (opt *Options) Debugf(format string, v ...interface{})
func (opt *Options) Errorf(format string, v ...interface{})
func (opt *Options) Infof(format string, v ...interface{})
func (opt *Options) Warningf(format string, v ...interface{})
func (opt Options) WithCompactL0OnClose(val bool) Options
func (opt Options) WithDir(val string) Options
func (opt Options) WithLevelOneSize(val int64) Options
func (opt Options) WithLevelSizeMultiplier(val int) Options
func (opt Options) WithLogRotatesToFlush(val int32) Options
func (opt Options) WithLogger(val Logger) Options
func (opt Options) WithMaxLevels(val int) Options
func (opt Options) WithMaxTableSize(val int64) Options
func (opt Options) WithNumCompactors(val int) Options
func (opt Options) WithNumLevelZeroTables(val int) Options
func (opt Options) WithNumLevelZeroTablesStall(val int) Options
func (opt Options) WithNumMemtables(val int) Options
func (opt Options) WithNumVersionsToKeep(val int) Options
func (opt Options) WithReadOnly(val bool) Options
func (opt Options) WithSyncWrites(val bool) Options
func (opt Options) WithTableLoadingMode(val options.FileLoadingMode) Options
func (opt Options) WithTruncate(val bool) Options
func (opt Options) WithValueDir(val string) Options
func (opt Options) WithValueLogFileSize(val int64) Options
func (opt Options) WithValueLogLoadingMode(val options.FileLoadingMode) Options
func (opt Options) WithValueLogMaxEntries(val uint32) Options
func (opt Options) WithValueThreshold(val int) Options

And the documentation for each option is now on WithX.

$ go doc Options.WithReadOnly
func (opt Options) WithReadOnly(val bool) Options
    WithReadOnly modifies returns a new Options value where ReadOnly has been
    set to the given value. The original Options value is never modified.

    When ReadOnly is true the DB will be opened on read-only mode. Multiple
    processes can open the same Badger DB. Note: if the DB being opened had
    crashed before and has vlog data to be replayed, ReadOnly will cause Open to
    fail with an appropriate message.

    The default value of ReadOnly is false.

Please review.

[martinmr]

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 24 unresolved discussions (waiting on @ashish-goswami, @campoy, and @golangcibot)

---

options.go, line 151 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 104 characters (from lll)

Hard to tell in reviewable if these warnings have been fixed but they are marked as outdated in github. Just go through them once more to make sure.

---

options.go, line 130 at r3 (raw file):

// WithDir returns a new Options value with Dir set to the given value.
// The original Options value is never modified.

How is this statement true? You are changing the value of opt.Dir and then returning the same opt value. Isn't that the opposite of what this sentence says? Or does setting the function signature to func (opt Options) instead of func (opt *Options) means the copy is implicit?

[campoy]

Reviewable status: all files reviewed, 24 unresolved discussions (waiting on @ashish-goswami, @golangcibot, and @martinmr)

---

options.go, line 151 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Hard to tell in reviewable if these warnings have been fixed but they are marked as outdated in github. Just go through them once more to make sure.

Done.

---

options.go, line 162 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 116 characters (from lll)

Done.

---

options.go, line 172 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 122 characters (from lll)

Done.

---

options.go, line 175 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 101 characters (from lll)

Done.

---

options.go, line 182 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 118 characters (from lll)

Done.

---

options.go, line 226 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 108 characters (from lll)

Done.

---

options.go, line 236 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 122 characters (from lll)

Done.

---

options.go, line 240 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 101 characters (from lll)

Done.

---

options.go, line 247 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 102 characters (from lll)

Done.

---

options.go, line 257 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 112 characters (from lll)

Done.

---

options.go, line 260 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 105 characters (from lll)

Done.

---

options.go, line 268 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 108 characters (from lll)

Done.

---

options.go, line 278 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 120 characters (from lll)

Done.

---

options.go, line 288 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 130 characters (from lll)

Done.

---

options.go, line 291 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 128 characters (from lll)

Done.

---

options.go, line 298 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 108 characters (from lll)

Done.

---

options.go, line 308 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 116 characters (from lll)

Done.

---

options.go, line 318 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 120 characters (from lll)

Done.

---

options.go, line 322 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 101 characters (from lll)

Done.

---

options.go, line 329 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 110 characters (from lll)

Done.

---

options.go, line 340 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 116 characters (from lll)

Done.

---

options.go, line 351 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 118 characters (from lll)

Done.

---

options.go, line 354 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 109 characters (from lll)

Done.

---

options.go, line 130 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

How is this statement true? You are changing the value of opt.Dir and then returning the same opt value. Isn't that the opposite of what this sentence says? Or does setting the function signature to func (opt Options) instead of func (opt *Options) means the copy is implicit?

Exactly, the receiver is a copy so the original value is never modified making it easier to understand what's going on.

[manishrjain]

Good change overall. The only concern is that each new field here in Options would now need a corresponding WithX function. Maybe add a note in options to ensure that future changes need to keep that in mind.

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @ashish-goswami, @campoy, @golangcibot, and @martinmr)

---

options.go, line 356 at r3 (raw file):

The original options value is never modified.

Doesn't seem like this needs to be mentioned repeatedly in every With func.

[campoy]

Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @ashish-goswami, @golangcibot, @manishrjain, and @martinmr)

---

options.go, line 356 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

The original options value is never modified.

Doesn't seem like this needs to be mentioned repeatedly in every With func.

Done.

[campoy]

After verifying that the Windows errors are due to flaky tests and not a my PR, I'm merging.