binarylog: export Sink #3879
binarylog: export Sink #3879
Conversation
menghanl
force-pushed
the
blog
branch
2 times, most recently
from
b791cb8 to
e38967d
Compare
So the sink can be updated (instead of only being noop)
binarylog/sink.go
Outdated
| Close() error | ||
| } | ||
|
|
||
| type bufWriteCloserSink struct { |
There was a problem hiding this comment.
I think we should merge the functionality of writerSink into this one. It is not very easy to read at this point.
There was a problem hiding this comment.
They cannot be merged, because test needs writerSink.
I moved it back to internal, right next to the writerSink.
binarylog/sink.go
Outdated
|
|
||
| func (fs *bufWriteCloserSink) Write(e *pb.GrpcLogEntry) error { | ||
| // Start the write loop when Write is called. | ||
| fs.writeStartOnce.Do(fs.startFlushGoroutine) |
There was a problem hiding this comment.
We could simply start this goroutine from the new* function and get rid of this Once. And the ticker would then become a local variable inside the goroutine. It would also make Write more readable, since we would only be writing stuff here.
There was a problem hiding this comment.
I don't want to start the flush goroutine unless Write happens.
|
|
||
| // NewTempFileSink creates a temp file and returns a Sink that writes to this | ||
| // file. | ||
| func NewTempFileSink() (Sink, error) { |
There was a problem hiding this comment.
Would it make more sense to get the name of the file to write to from the user? The way it is right now, the user would just to go over to the tmp dir and figure out the file for themselves.
There was a problem hiding this comment.
That is reasonable, and in that case we would just export NewBufferedSink from internal. But there's no tool to read the file content anyway, so I would expect users to just implement their own Sink.
I'm leaving this as is for now (I added comment to remind us later). We can decide what to export based on what users want.
So the sink can be updated (instead of only being noop)
fixes #3878