Add EventLogging option #1035
Add EventLogging option #1035
Conversation
This introduces an option to disable or enable trace.EventLog logging of events. For backwards compatability this defaults to true. The reason for making this configurable is that under some workloads the cost of event logging is significant and so can cause performance issues. Fixes #938
mjgarton
requested review from
ashish-goswami,
jarifibrahim,
manishrjain and
a team
as code owners
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ A review job has been created and sent to the PullRequest network.
@mjgarton you can click here to see the review status or cancel the code review job.
|
|
There was a problem hiding this comment.
This code is not passing AppVeyor build due to invalid checksum - https://ci.appveyor.com/project/manishrjain/badger/builds/27425337.
You may need to fix it by merging in the latest version of master.
Had a question in-line but otherwise looks good to me.
Reviewed with ❤️ by PullRequest
nil_event_log.go
Outdated
| noEventLog trace.EventLog = nilEventLog{} | ||
| ) | ||
|
|
||
| type nilEventLog struct{} |
There was a problem hiding this comment.
What is the need for this struct to be a part of the repo, and why is it the default?
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ashish-goswami, @manishrjain, and @mjgarton)
|
@mjgarton We need to propagate above change https://github.com/dgraph-io/badger/blob/86a77bb28c44052be16ce878ca1115ec0b83824e/value.go#L795 here also. |
This also applies the event logging config option to the value log.
Remove the additional eventLogging parameter because it's already available as db.opts.
It is going to need to be used from there and elsewhere, so move into `y`
The option to enable/disable event logging now also works for watermarks, which was previously missed.
|
I missed another event logger in EDIT: pushed now. |
|
Please advise whether |
|
|
||
| func (nel nilEventLog) Errorf(format string, a ...interface{}) {} | ||
|
|
||
| func (nel nilEventLog) Finish() {} |
There was a problem hiding this comment.
You might want to implement SetMaxEvents function also. As it is used below.
https://github.com/dgraph-io/badger/blob/c6681c181835f3846b859d6b546e5da8d9f86ea9/levels.go#L845
There was a problem hiding this comment.
@mjgarton Having the null event logger in y package should be ok. We keep some common functions and helper methods in the y package.
Reviewed 1 of 3 files at r3, 5 of 5 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ashish-goswami, @manishrjain, and @mjgarton)
y/event_log.go, line 31 at r4 (raw file):
Previously, ashish-goswami (Ashish Goswami) wrote…
You might want to implement
SetMaxEventsfunction also. As it is used below.
https://github.com/dgraph-io/badger/blob/c6681c181835f3846b859d6b546e5da8d9f86ea9/levels.go#L845
The elog in this context is trace.Trace type and not trace.EventLog. We don't need to make this change in this PR. We can do it separately.
|
Thank you for fixing this @mjgarton 🎉 |
This introduces an option to disable or enable trace.EventLog logging of events. For backward compatibility this defaults to true. The reason for making this configurable is that under some workloads the cost of event logging is significant and so can cause performance issues. Fixes #938 (cherry picked from commit 75c6a44)
This introduces an option to disable or enable trace.EventLog logging of
events. For backwards compatability this defaults to true.
The reason for making this configurable is that under some workloads the
cost of event logging is significant and so can cause performance
issues.
Fixes #938
This change is