changes eventLog with Logger #1203

gabru-md · 2020-01-21T08:05:29Z

Provides a fix for #1193

@jarifibrahim, please review and let me know If it is correct. If it isn't then please mention what needs to be done so that I can try to do that as well.

Thanks for the issue

This change is

jarifibrahim

Hey @gabru-md, thank you so much for the PR. It looks great 🎉

I have added some minor comments. Please address those and we should be able to merge this PR.
Thank you once again for your contribution :)

jarifibrahim · 2020-01-21T09:11:38Z

db.go

@@ -68,7 +67,7 @@ type DB struct {
 	valueDirGuard *directoryLockGuard

 	closers   closers
-	elog      trace.EventLog
+	logger    Logger


You can remove this. The options struct has the logger. Look at db.opt.Logger.

jarifibrahim · 2020-01-21T09:14:48Z

db.go

-		elog = trace.NewEventLog("Badger", "DB")
+	logger := NilLogger
+	if opt.Logging {
+		logger = defaultLogger


You can remove the 3 lines above. The y.NoEventLog was added because eventLog would take up too much of CPU/memory. Now we don't need it.

jarifibrahim · 2020-01-21T09:14:51Z

db.go

@@ -284,7 +283,7 @@ func Open(opt Options) (db *DB, err error) {
 		writeCh:       make(chan *request, kvWriteChCapacity),
 		opt:           opt,
 		manifest:      manifestFile,
-		elog:          elog,
+		logger:        logger,


This would go away once you remove logger from the DB struct :)

jarifibrahim · 2020-01-21T09:17:07Z

levels.go

@@ -37,7 +37,7 @@ import (

 type levelsController struct {
 	nextFileID uint64 // Atomic
-	elog       trace.EventLog
+	logger     Logger


You can remove this logger. The levelsController contains DB struct which contains the logger.

You can do levelsController.kv.opt.Logger where ever you need this.

jarifibrahim · 2020-01-21T09:17:42Z

levels.go

@@ -77,7 +77,7 @@ func newLevelsController(db *DB, mf *Manifest) (*levelsController, error) {
 	y.AssertTrue(db.opt.NumLevelZeroTablesStall > db.opt.NumLevelZeroTables)
 	s := &levelsController{
 		kv:     db,
-		elog:   db.elog,
+		logger: db.logger,


This would also go away once you change the levelsController struct.

jarifibrahim · 2020-01-21T09:20:38Z

logger.go

@@ -83,3 +83,15 @@ func (l *defaultLog) Infof(f string, v ...interface{}) {
 func (l *defaultLog) Debugf(f string, v ...interface{}) {
 	l.Printf("DEBUG: "+f, v...)
 }
+
+var NilLogger Logger = nilLogger{}


The nilLogger is of no use to use once you remove the opt.Logging options. You can remove everything related to nilLogger.

jarifibrahim · 2020-01-21T09:21:08Z

options.go

@@ -48,7 +48,7 @@ type Options struct {
 	Truncate            bool
 	Logger              Logger
 	Compression         options.CompressionType
-	EventLogging        bool
+	Logging             bool


This is also not necessary. The user can set the logger via the opt.Logger option.

jarifibrahim · 2020-01-21T09:21:56Z

value.go

@@ -827,7 +827,7 @@ type lfDiscardStats struct {

 type valueLog struct {
 	dirPath string
-	elog    trace.EventLog
+	logger  Logger


You can use valueLog.db.opt.Logger . We don't need this logger.

jarifibrahim · 2020-01-21T09:34:50Z

I just realized that the logger in badger doesn't support leveled logging. We might have to change the default logger. The Travis build failed because we're logging too much information.

gabru-md · 2020-01-21T11:28:09Z

y/watermark.go

 }

 // Init initializes a WaterMark struct. MUST be called before using it.
-func (w *WaterMark) Init(closer *Closer, eventLogging bool) {
+func (w *WaterMark) Init(closer *Closer, opt badger.Options) {


instead of passing a boolean value I am passing the Options object so that logger can be initialized for the Watermark struct as well.
@jarifibrahim, please see if this is the correct way to do it?

this may cause the circular import error. I'm not clear about how would I do it otherwise. will creating the logger as a different module be a better approach?

You can move the logger inside the y package. The y package contains a bunch of utility types.

golangcibot · 2020-01-27T14:14:41Z

y/logger.go

 type defaultLog struct {
 	*log.Logger
 }

 var defaultLogger = &defaultLog{Logger: log.New(os.Stderr, "badger ", log.LstdFlags)}

+func DefaultLogger() *defaultLog {


exported func DefaultLogger returns unexported type *github.com/dgraph-io/badger/v2/y.defaultLog, which can be annoying to use (from golint)

stale · 2020-02-26T19:44:45Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2020-03-04T19:59:45Z

This issue was marked as stale and no activity has occurred since then, therefore it will now be closed. Please, reopen if the issue is still relevant.

jarifibrahim · 2020-03-05T11:02:09Z

This is a useful PR but we cannot merge it until we figure out a way to add leveled logging. Without leveled logging this PR would generate too much log.

CLAassistant · 2020-03-05T11:02:18Z

All committers have signed the CLA.

jarifibrahim · 2020-03-06T13:18:50Z

This PR can be merged after #1249 is merged.

stale · 2020-04-05T13:26:49Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2020-04-12T14:07:16Z

This issue was marked as stale and no activity has occurred since then, therefore it will now be closed. Please, reopen if the issue is still relevant.

This reverts commit 0b81618.

jarifibrahim

Reviewable status: 0 of 7 files reviewed, 10 unresolved discussions (waiting on @ashish-goswami, @gabru-md, @jarifibrahim, and @manishrjain)

jarifibrahim · 2020-04-13T12:29:19Z

Thanks for the PR @gabru-md 🎉

changes eventLog with Logger

a6c3079

gabru-md requested review from ashish-goswami, jarifibrahim, manishrjain and a team as code owners January 21, 2020 08:05

jarifibrahim suggested changes Jan 21, 2020

View reviewed changes

removes unnecessary Logger from files

7b98dce

gabru-md commented Jan 21, 2020

View reviewed changes

gabru-md requested a review from jarifibrahim January 25, 2020 15:34

gabru-md added 2 commits January 27, 2020 19:14

moves logger to y

0b81618

changes to logger_test.go

8e88f85

golangcibot reviewed Jan 27, 2020

View reviewed changes

renames defaultLogger to DefaultLogger

2dfd75d

stale bot added the status/stale The issue hasn't had activity for a while and it's marked for closing. label Feb 26, 2020

stale bot closed this Mar 4, 2020

jarifibrahim reopened this Mar 5, 2020

stale bot removed the status/stale The issue hasn't had activity for a while and it's marked for closing. label Mar 5, 2020

stale bot added the status/stale The issue hasn't had activity for a while and it's marked for closing. label Apr 5, 2020

stale bot closed this Apr 12, 2020

jarifibrahim reopened this Apr 13, 2020

stale bot removed the status/stale The issue hasn't had activity for a while and it's marked for closing. label Apr 13, 2020

Remove logging from watermark

cf2db8d

Ibrahim Jarif added 4 commits April 13, 2020 16:04

Revert "moves logger to y"

85402ce

This reverts commit 0b81618.

Merge branch 'master' into pr-1203

a420219

fixup

4d6465b

Run go mod tidy

cc58591

jarifibrahim approved these changes Apr 13, 2020

View reviewed changes

jarifibrahim merged commit 09dd2e1 into dgraph-io:master Apr 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changes eventLog with Logger #1203

changes eventLog with Logger #1203

gabru-md commented Jan 21, 2020 •

edited by manishrjain

Loading

jarifibrahim left a comment

jarifibrahim Jan 21, 2020

jarifibrahim Jan 21, 2020

jarifibrahim Jan 21, 2020

jarifibrahim Jan 21, 2020

jarifibrahim Jan 21, 2020

jarifibrahim Jan 21, 2020

jarifibrahim Jan 21, 2020

jarifibrahim Jan 21, 2020

jarifibrahim commented Jan 21, 2020

gabru-md Jan 21, 2020

gabru-md Jan 21, 2020

jarifibrahim Jan 27, 2020

golangcibot Jan 27, 2020

stale bot commented Feb 26, 2020

stale bot commented Mar 4, 2020

jarifibrahim commented Mar 5, 2020 •

edited

Loading

CLAassistant commented Mar 5, 2020

jarifibrahim commented Mar 6, 2020

stale bot commented Apr 5, 2020

stale bot commented Apr 12, 2020

jarifibrahim left a comment

jarifibrahim commented Apr 13, 2020

changes eventLog with Logger #1203

changes eventLog with Logger #1203

Conversation

gabru-md commented Jan 21, 2020 • edited by manishrjain Loading

jarifibrahim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarifibrahim commented Jan 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Feb 26, 2020

stale bot commented Mar 4, 2020

jarifibrahim commented Mar 5, 2020 • edited Loading

CLAassistant commented Mar 5, 2020

jarifibrahim commented Mar 6, 2020

stale bot commented Apr 5, 2020

stale bot commented Apr 12, 2020

jarifibrahim left a comment

Choose a reason for hiding this comment

jarifibrahim commented Apr 13, 2020

gabru-md commented Jan 21, 2020 •

edited by manishrjain

Loading

jarifibrahim commented Mar 5, 2020 •

edited

Loading