-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add BufferedWriteSyncer #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is not the actual PR so providing some early feedback here.
WriteSyncer | ||
sync.Mutex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, anything you embed is part of the public API. That's probably not desirable for the mutex, but we can do it for the WriteSyncer if we want to.
https://github.com/uber-go/guide/blob/master/style.md#avoid-embedding-types-in-public-structs
Clock Clock | ||
|
||
// unexported fields for state | ||
size int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused field. can delete.
s.Lock() | ||
defer s.Unlock() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're trying to make Write thread-safe, the initiallized
check should also be thread safe.
The lock should be acquired before we change initialized.
if s.Clock != nil { | ||
s.ticker = s.Clock.NewTicker(flushInterval) | ||
} else { | ||
s.ticker = time.NewTicker(flushInterval) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use a default clock implementation if unset rather than if-else on using the clock versus using the system.
So the systemClock should be available in zapcore to be used.
) | ||
|
||
func TestBufferWriter(t *testing.T) { | ||
defer goleak.VerifyNone(t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not necessary if leak_test provides this via TestMain
This diff addresses comments in uber-go#782
API for BufferedWriteSyncer: