nice catch!

Reviewed 1 of 1 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @petermattis and @sumeerbhola)

2. A change to libroach.h to modify 2 inline functions (I think this might be my preferred approach)

I would do this.

(And my least favorite, but I suppose a possibility as well, is to define a static emtpySlice sentinel in libroach.h (empty DBSlice w/ data initialized to ""). This is not super desirable since, unfortunately, DBSlice is, in fact, not a constant data structure, and so there might be a chance that some code might mutate data ptr).

This sounds ok too, but sounds equivalent to option 2. Maybe I'm misunderstanding the problem you see. You'd define static const rocksdb::Slice emptySlice("", 0). There would be no way to modify emptySlice.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @miretskiy and @sumeerbhola)

pkg/storage/rocksdb.go, line 2643 at r1 (raw file):

// assertions checking for slice.data != nullptr.
// Therefore, we represent an empty slice with an empty []byte array.
var emptySlice = []byte{0}

I realize this approach doesn't work, but wanted to point out this is a bit of overkill. You're create a slice of length 1 in order to get a non-nil *byte pointer. There is an easier way:

var emptyByte byte
...
  if len(b) == 0 {
    return C.DBSlice(data: (*C.char)(&emptyByte), len : 0}
  }

This is equivalent to what you wrote, but just goes directly to a *byte rather than using a []byte. The insight here is that []byte is really just sugar for:

type byteSliceHeader {
  ptr *byte
  len int
  cap int
}

It's just I saw that, even though DBSlice should be an immutable data structure, the fact that we do use it
in a mutable way (e.g. chunked_buffer) made me a bit worried. If you're okay w/ inline function changes, I'd do that.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @petermattis and @sumeerbhola)

pkg/storage/rocksdb.go, line 2643 at r1 (raw file):

Ahh.. Yeah... I didnt' try ptr *byte. I tried creating e.g. []byte{} and pass in the value of that, but cgo was complaining.
Thanks for tip.

The libroach/db.h change looks good, but don't you need to revert the rocksdb.go change? Or did you revert it and reviewable is messing up?

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @petermattis and @sumeerbhola)

good work!

Reviewed 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @petermattis and @sumeerbhola)

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @sumeerbhola)

So, the initial approach w/ changing rocksdb.go produced many test failures.

I did not follow what was causing those test failures -- could you elaborate?

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @sumeerbhola)