Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

binarylog: export Sink #3879

Merged
merged 3 commits into from Sep 22, 2020
Merged

binarylog: export Sink #3879

merged 3 commits into from Sep 22, 2020

Conversation

menghanl
Copy link
Contributor

So the sink can be updated (instead of only being noop)

fixes #3878

@menghanl menghanl mentioned this pull request Sep 15, 2020
@menghanl menghanl added this to the 1.33 Release milestone Sep 15, 2020
So the sink can be updated (instead of only being noop)
binarylog/sink.go Outdated Show resolved Hide resolved
binarylog/sink.go Outdated Show resolved Hide resolved
binarylog/sink.go Outdated Show resolved Hide resolved
binarylog/sink.go Outdated Show resolved Hide resolved
binarylog/sink.go Outdated Show resolved Hide resolved
binarylog/sink.go Outdated Show resolved Hide resolved
Close() error
}

type bufWriteCloserSink struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should merge the functionality of writerSink into this one. It is not very easy to read at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They cannot be merged, because test needs writerSink.
I moved it back to internal, right next to the writerSink.

binarylog/sink.go Outdated Show resolved Hide resolved

func (fs *bufWriteCloserSink) Write(e *pb.GrpcLogEntry) error {
// Start the write loop when Write is called.
fs.writeStartOnce.Do(fs.startFlushGoroutine)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to start the flush goroutine unless Write happens.

binarylog/sink.go Outdated Show resolved Hide resolved
@menghanl menghanl assigned easwars and unassigned menghanl Sep 18, 2020

// NewTempFileSink creates a temp file and returns a Sink that writes to this
// file.
func NewTempFileSink() (Sink, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@easwars easwars assigned menghanl and unassigned easwars Sep 21, 2020
@menghanl menghanl merged commit 400b4a0 into grpc:master Sep 22, 2020
@menghanl menghanl deleted the blog branch September 22, 2020 16:52
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

binarylog: how to use?
2 participants