Open new commitlogs async

[richardartoul]

The README.md has more details, but the basic idea with this P.R is that everytime we rotated the commitlog file we would see a really big spike in the commit log queue length. One of the primary causes of the queue backing up was that the single-threaded commitlog writer would stop writing completely while it waited for syscalls and I/O to complete creating a new commitlog file.

This P.R modifies the commitlog to always keep two files open at any time (while only writing to one) and when the commitlog needs to be rotated, the secondary file becomes the primary, and the primary files becomes the secondary and is asynchronously reset in preparation for the next rotation.

Before

After

[codecov]

Codecov Report

Merging #1576 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1576   +/-   ##
======================================
  Coverage      72%     72%           
======================================
  Files         950     950           
  Lines       78950   78950           
======================================
  Hits        56851   56851           
  Misses      18404   18404           
  Partials     3695    3695

Flag	Coverage Δ
#aggregator	82.3% <0%> (ø)	⬆️
#cluster	85.7% <0%> (ø)	⬆️
#collector	63.7% <0%> (ø)	⬆️
#dbnode	80.1% <0%> (ø)	⬆️
#m3em	73.2% <0%> (ø)	⬆️
#m3ninx	73.9% <0%> (ø)	⬆️
#m3nsch	51.1% <0%> (ø)	⬆️
#metrics	17.6% <0%> (ø)	⬆️
#msg	74.7% <0%> (ø)	⬆️
#query	66.9% <0%> (ø)	⬆️
#x	86.8% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91ed20b...fce7dec. Read the comment docs.

[Haijuncao]

LGTM.
some minor comment about additional test.

[robskillington]

Hm, why don't you add the onFlush(err error) method to asyncResettableWriter and then make it call the commit log back.

i.e.

type asyncResettableWriter struct {
  // .. other
  commitLog *commitLog
}

func (w *asyncResettableWriter) onFlush(err error) {
  w.commitLog.onFlush(w, err)
}

// now on commit log, retain how this used to work
func (l *commitLog) onFlush(writer *asyncResettableWriter, err error) {
  // as before
}

Then construct more simple to:

l.writerState.primary.writer = l.newCommitLogWriterFn(&l.writerState.primary.onFlush, l.opts)
l.writerState.secondary.writer = l.newCommitLogWriterFn(&l.writerState.secondary.onFlush, l.opts)

[richardartoul]

Yeah I thought about this and didn't do it because i really hate cyclic datastructures as they can be really confusing. Probably simpler to do it in this case though, I'll make the change

[robskillington]

Does it make slightly more sense to access the "last index" than just 1? i.e.

primaryFile = l.writerState.activeFiles[len(l.writerState.activeFiles)-1]

I know it's longer but seems more easy to read (strangely) since I know it's always getting the last entry.

Not too fussed though.

[richardartoul]

I prefer the existing one because there can only ever be two and the "last" logic feel more confusing to me because it implies there is may be more than two

[robskillington]

Sure thing.

[robskillington]

nit: If you really want to you could create a forEachWriters(func(writer) { ... }) helper function to make sure all for loops does this.

[richardartoul]

Yeah that would be a little cleaner, but I'd rather be explicit and have it crash in places where we don't expect to ever be nil than have it silently do something weird.