Badger: Cannot handle errors occurred in badger/y/error.go

[ty2]

log.Fatal() calls os.Exit(1) in the end, the program will exit immediately. Is it possible to use panic() instead, so that errors can be handle by the program.

[peterstace]

In places where some internal invariants have been violated, we will use fatal style error handling, since it indicates a serious programming problem in badger. For any errors that can 'normally' occur, e.g. can't read from file, we return errors back via badger's public API.

If there's a particular place in the code where you feel it would be better to return an error rather than check it fatally, we can look into those on a case-by-case basis.

[ty2]

If Badger internal error is occurred, it should at least return "internal error" message, because badger is not a standalone program, it should let the main program to determine what should do next rather than print error and exit immediately, it is not friendly and it cannot recover by the main program.

For example,
Currently, if a DB file are corrupted, Badger will print error and exit immediately, the program cannot create a new db and then run it normally or shutdown gracefully.

So, I think log.Fatal is bad choice for handle a internal error in a library, it should use panic or better error handling instead.

[peterstace]

If there's a particular place in the code where you feel it would be better to return an error rather than check it fatally, we can look into those on a case-by-case basis.

Currently, if a DB file are corrupted, Badger will print error and exit immediately, the program cannot create a new db and then run it normally or shutdown gracefully.

Agreed, badger should give an error in this case, rather than doing anything fatal.

I had a look for the places we y.Check is used, each case should be reviewed manually.

$ ag y\.Check
manifest.go
109:    y.Check(applyChangeSet(&ret, &changeSet))

table/builder.go
211:            y.Check(err)
218:        y.Check2(b.keyBuf.Read(key))
229:    y.Check(err)

table/table_test.go
57:     y.Check(err)
69:         y.Check(err)
609:    y.Check(err)
613:        y.Check(builder.Add([]byte(k), y.ValueStruct{Value: []byte(v), Meta: 123, UserMeta: 0}))
618:    y.Check(err)
639:    y.Check(err)
643:        y.Check(builder.Add([]byte(k), y.ValueStruct{Value: []byte(v), Meta: 123, UserMeta: 0}))
648:    y.Check(err)
678:        y.Check(err)
684:            y.Check(builder.Add([]byte(k), y.ValueStruct{Value: []byte(v), Meta: 123, UserMeta: 0}))
688:        y.Check(err)

table/table.go
199:    y.Check(err)

util.go
122:    y.Check(err)

value_test.go
32: y.Check(err)
619:                y.Check(err)
622:                y.Check(err)

manifest_test.go
134:        y.Check(err)
150:            y.Check(err)

levels.go
291:            y.Check(builder.Add(it.Key(), it.Value()))
$ ag y\.Check
manifest.go
109:    y.Check(applyChangeSet(&ret, &changeSet))

table/builder.go
211:            y.Check(err)
218:        y.Check2(b.keyBuf.Read(key))
229:    y.Check(err)

table/table_test.go
57:     y.Check(err)
69:         y.Check(err)
609:    y.Check(err)
613:        y.Check(builder.Add([]byte(k), y.ValueStruct{Value: []byte(v), Meta: 123, UserMeta: 0}))
618:    y.Check(err)
639:    y.Check(err)
643:        y.Check(builder.Add([]byte(k), y.ValueStruct{Value: []byte(v), Meta: 123, UserMeta: 0}))
648:    y.Check(err)
678:        y.Check(err)
684:            y.Check(builder.Add([]byte(k), y.ValueStruct{Value: []byte(v), Meta: 123, UserMeta: 0}))
688:        y.Check(err)

table/table.go
199:    y.Check(err)

util.go
122:    y.Check(err)

value_test.go
32: y.Check(err)
619:                y.Check(err)
622:                y.Check(err)

manifest_test.go
134:        y.Check(err)
150:            y.Check(err)

levels.go
291:            y.Check(builder.Add(it.Key(), it.Value()))

Indeed, the first one (in manifest.go) logs fatal if the manifest is corrupted - would be better to bubble the error back up to the public API

[manishrjain]

Currently, if a DB file are corrupted, Badger will print error and exit immediately, the program cannot create a new db and then run it normally or shutdown gracefully.

I'd argue that's the right behavior. If a corrupt DB is used, the program behavior would be unpredictable. A user might ignore this error, and continue using this DB, and waste a lot of time bumping against walls because nothing crashes.

OTOH, the current behavior of crashing immediately on a corrupt DB means the user can create a new DB, or use external tools to try to fix the DB, before restarting. And a successful start would be a strong indication that Badger DB is valid.

[ty2]

I'd argue that's the right behavior. If a corrupt DB is used, the program behavior would be unpredictable. A user might ignore this error, and continue using this DB, and waste a lot of time bumping against walls because nothing crashes.

This is a user(software authors) problem, they can choose repair the db in the same program, continue running the program or exit immediately wherever they want to handle a error because they can get the error signal in the program.

Why Bager don't exit immediately if any error occurred, the program behavior would be unpredictable?

If any error occurred in any third party library and exit immediately, it would be unrecoverable by the main program, it is fault hell which means no fault tolerance.

OTOH, the current behavior of crashing immediately on a corrupt DB means the user can create a new DB, or use external tools to try to fix the DB, before restarting. And a successful start would be a strong indication that Badger DB is valid.

If Bager is a standalone program, I agree with you that user should repair their DB manually outside the program. However, Bager is a embedded DB, which means Badger is integrated with an application software and control by an application software directly, any exception should able to catch by the main program and it will handle the rest because an application software have multiple components need to manage.

As you said corrupted DB can be repaired by external tools, but a repair tools can also embed to the main program and repair it in the main program.

If your house is on fire, you will try to putting out the fire, call the fireman and then run away, right?

Ref:
https://stackoverflow.com/questions/44504354/should-i-use-panic-or-return-error
https://golang.org/doc/effective_go.html#errors

[manishrjain]

Sorry, back after a while.

The article from Dave Cheney actually considers panics like fatals, not exceptions.

panics are always fatal to your program. In panicing you never assume that your caller can solve the problem. Hence panic is only used in exceptional circumstances, ones where it is not possible for your code, or anyone integrating your code to continue.

Let's try to narrow down the scope of work here. What's the expectation here? Is it to replace all log.Fatals with panics, so that they can be captured and re-routed like exceptions?

[manishrjain]

Closing due to inactivity and lack of clarity on what needs to be done here.