Add buffered write syncer

[moisesvega]

This diff addresses comments in #782

• Add BufferedWriteSyncer
• Use new Clock Interface for testing

API for BufferedWriteSyncer:

type BufferedWriteSyncer struct {
	WriteSyncer

	Size          int
	FlushInterval time.Duration
	Clock         Clock

	// unexported fields for state
}

buf := &bytes.Buffer{}
ws := &BufferedWriteSyncer{WriteSyncer: AddSync(buf)} //  BufferedWriteSyncer with the default values
ws.Write([]byte("foo"))

 // Close closes the buffer, cleans up background goroutines, and flushes remaining, unwritten data.
ws.Close()

[codecov]

Codecov Report

Merging #952 (f32b796) into master (cfe34dc) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #952      +/-   ##
==========================================
+ Coverage   98.12%   98.16%   +0.03%     
==========================================
  Files          44       45       +1     
  Lines        1975     2014      +39     
==========================================
+ Hits         1938     1977      +39     
  Misses         29       29              
  Partials        8        8              

Impacted Files	Coverage Δ
zapcore/buffered_write_syncer.go	100.00% <100.00%> (ø)
zaptest/observer/observer.go	100.00% <0.00%> (ø)

---

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 cfe34dc...f32b796. Read the comment docs.

[abhinav]

Sorry for the delay. I'll look at the next iteration quicker!

[abhinav]

minor nit: we should specify the defaults above the fields as we document the fields:

// Size specifies the maximum amount of data the writer will buffer before
// flushing. Defaults to 256 kB.

// FlushInterval specifies how often the writer should flush data if there
// have been no writes. Defaults to 30 seconds.

[abhinav]

minor naming: can we name this init or initialize because the bool is called initialize. Or we can name the bool "loaded" and call this "load", but I'm more in favor of init or initialize.

[abhinav]

minor optimization on the WriteSyncer:
since BufferedWriteSyncer is already thread-safe, we can optimize this a bit if s.WriteSyncer was wrapped with zapcore.Lock.

writer := s.WriteSyncer
if w, ok := writer.(*lockedWriteSyncer); ok {
  writer = w.ws // don't double lock
}
s.writer = bufio.NewWriterSize(writer, size)

[abhinav]

Mostly LGTM. For the wrap-twice case, can we add a test with an explicit sync to verify that the inner BufWriter was also flushed?

[shirchen]

lgtm, just nits about closing files in tests and not needing to manually closing files.

[shirchen]

lgtm but wait for @abhinav 's stamp too.

[abhinav]

No, we don't want a merge commit for this. Reverting.

[prashantv]

Can we wait for the background goroutine to stop before returning here? It leads to:

• no goroutine running after Close guaranteed
• avoids odd cases where a Sync can happen after the Close returns

[prashantv]

Since this will panic if we do it twice, do we want to explicitly call out that Close must only be called once, or put some protection here in case Close is called twice?

[abhinav]

Going with a doc for now. Guarding against it is possible but seems unnecessary given that standard usage will probably be:

ws := zapcore.BufferedWriteSyncer{...}
defer ws.Stop()

(we do need to guard against Stop without Start, so going to add that too.)

[prashantv]

The warning that this will not close the underlying WriteSyncer makes me wonder if Close is a little surprising. Would this be better named something else like Stop to make it very clear that it doesn't close the underlying writer?

[abhinav]

Reasonable. Renaming.

[moisesvega]

Sounds good. Stop makes more sense in the context of this specific writer.

[abhinav]

Reopened in #961 @prashantv