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

what about implement a buffer writer syncer? #663

Closed
sysulq opened this issue Dec 28, 2018 · 12 comments
Closed

what about implement a buffer writer syncer? #663

sysulq opened this issue Dec 28, 2018 · 12 comments

Comments

@sysulq
Copy link
Contributor

sysulq commented Dec 28, 2018

Support sync through a buffer writer, zap should be faster...

Just like golang/glog does.
https://github.com/golang/glog/blob/master/glog.go#L841

Glad to send a PR if you guys are interested :-)

Simple Demo:

type bufferWriterSync struct {
	buf *bufio.Writer
}

func Buffer(ws WriteSyncer, size int) WriteSyncer {
        bw := &bufferWriterSync{
		buf: bufio.NewWriterSize(ws, size),
	}

        ws = zapcore.Lock(bw)
        return ws
}

// Sync syncs data to file
func (w bufferWriterSync) Sync() error {
	return w.buf.Flush()
}

// Write writes data to buffer
func (w bufferWriterSync) Write(p []byte) (int, error) {
	return w.buf.Write(p)
}
@akshayjshah
Copy link
Contributor

We added Sync to the zapcore.WriteSyncer API for exactly this use case, but haven't actually shipped a buffering sink yet. Any buffer we ship will need to flush periodically, though; otherwise low-output loggers may retain logs indefinitely without actually writing them to disk.

@sysulq
Copy link
Contributor Author

sysulq commented Jan 9, 2019

Yes, and this can be done by a timer loop goroutine.

Like this:

	bw := &bufferWriterSync{
		buf: bufio.NewWriterSize(ws, size),
	}

	// bufio is not goroutine safe, so add lock writer here
	ws = zapcore.Lock(bw)

	// flush buffer every 5s
	go func() {
		for range time.NewTicker(5 * time.Second).C {
			ws.Sync()
		}
	}()

	return ws

@akshayjshah
Copy link
Contributor

Exactly! I'd prefer to automatically start the goroutine in whatever constructor we expose (rather than making the user do it), and we'll also need to expose the ability to cleanly shut down the goroutine to avoid resource leaks.

@pavelnemirovsky
Copy link

@akshayjshah when u plan to release buffer sink capability ? Would be glad to test it. Thx for great project.

@BGraco
Copy link

BGraco commented Nov 9, 2019

I am also very interested in this, mostly to reduce ops/s as that drives cost on cloud storage instances.

@zplzpl
Copy link

zplzpl commented Feb 8, 2020

anyone currently doing this?

@sysulq
Copy link
Contributor Author

sysulq commented Feb 8, 2020

@zplzpl
Maybe I could send a pull request for this feature, unfortunately I just get stuck in Wuhan...
which means I got a lot of time for coding...

@zplzpl
Copy link

zplzpl commented Feb 8, 2020

@hnlq715 bro , take care yoursellf,bless for you

@uber-go uber-go deleted a comment from BGraco Feb 10, 2020
@uber-go uber-go deleted a comment from BGraco Feb 10, 2020
@uber-go uber-go deleted a comment from BGraco Feb 10, 2020
@michelaquino
Copy link

Hi, any update about that?

@sysulq
Copy link
Contributor Author

sysulq commented Jun 9, 2021

FYI~

This issue has been fixed by #782 and #961 :-)

@sysulq sysulq closed this as completed Jun 9, 2021
@YevSent
Copy link

YevSent commented Jul 29, 2022

Is there any documentation on how to use BufferedWriteSyncer? The source code comments don't contain many details and from the tests, it's also difficult to understand how exactly BufferedWriteCyncer should be initialized.

@abhinav
Copy link
Collaborator

abhinav commented Aug 1, 2022

Is there any documentation on how to use BufferedWriteSyncer? The source code comments don't contain many details and from the tests, it's also difficult to understand how exactly BufferedWriteCyncer should be initialized.

Oops, you're right, there's significant room for improvement here.
I've tried to add more documentation for BufferedWriteSyncer in #1139.
Hope this helps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

8 participants