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

change the way purge works #392

Merged
merged 9 commits into from
Jan 10, 2018
Merged

change the way purge works #392

merged 9 commits into from
Jan 10, 2018

Conversation

janardhan1993
Copy link

@janardhan1993 janardhan1993 commented Jan 8, 2018

This change is Reviewable

@janardhan1993 janardhan1993 changed the title change the way purge works(WIP: Need to fix tests) change the way purge works Jan 8, 2018
Janardhan Reddy added 2 commits January 10, 2018 16:23
Ensure all versions of key are always written to same table.
@manishrjain
Copy link
Contributor

:lgtm:


Reviewed 1 of 7 files at r1, 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks broke.


db.go, line 896 at r1 (raw file):

	defer it.Close()

	var lastPurgeTs uint64

Do a blind write here. And do a select push into a channel, which can be run via a goroutine, which can update the GC stats.


db.go, line 976 at r1 (raw file):

		// or else same entry would be counted as discarded everytime we call PurgeOlderVersions.
		db.vlog.resetGCStats()
		for it.Rewind(); it.Valid(); it.Next() {

No need for all versions.


db.go, line 243 at r2 (raw file):

		flushChan:     make(chan flushTask, opt.NumMemtables),
		writeCh:       make(chan *request, kvWriteChCapacity),
		gcStatsCh:     make(chan updateGcStatTask, 10000),

1000


db.go, line 243 at r2 (raw file):

		flushChan:     make(chan flushTask, opt.NumMemtables),
		writeCh:       make(chan *request, kvWriteChCapacity),
		gcStatsCh:     make(chan updateGcStatTask, 10000),

purgeUpdateCh : chan purgeUpdate


db.go, line 317 at r2 (raw file):

	db.closers.gcStats = y.NewCloser(1)
	go db.periodicUpdateGCStats(db.closers.gcStats)

db.updateGCStats


db.go, line 461 at r2 (raw file):

// get returns the value in memtable or disk for given key.
// Note that value will include meta byte.
// IMP: We should never write an entry with a older timestamp for same key,

IMPORTANT


db.go, line 961 at r2 (raw file):

	}
	select {
	case db.gcStatsCh <- updateGcTask:

default:


db_test.go, line 326 at r1 (raw file):

	err = db.RunValueLogGC(0.2)
	require.NoError(t, err)

Explain in db.go why we should never write a key with an older version to LSM tree.


levels.go, line 673 at r1 (raw file):

			return vs, nil
		}
		if maxVs.Version < vs.Version {

When generating LSM tables, continue iterating until all the versions have been picked up by a single SSTable. That is, don't split a key between 2 tables on the same level.


levels.go, line 661 at r2 (raw file):

// get returns the found value if any. If not found, we return nil.
func (s *levelsController) get(key []byte) (vs y.ValueStruct, err error) {

Don't declare the return vars here.


levels.go, line 676 at r2 (raw file):

			continue
		}
		return

return vs, nil


levels.go, line 678 at r2 (raw file):

		return
	}
	return

return empty vs, nil?


value.go, line 319 at r1 (raw file):

	if err != nil {
		return false, err
	} else if isDeletedOrExpired(vs.Meta, vs.ExpiresAt) {

Add a comment //pass or explain why this.


value.go, line 314 at r2 (raw file):

}

func (vlog *valueLog) purgeEntry(key []byte, version uint64) (bool, error) {

Can just take the fully loaded keyWithTs, and parse the version and key out of it.


y/sparse_linux.go, line 28 at r1 (raw file):

// ErrPurged is returned when a transaction tries to access an entry which
// has been purged.
var ErrPurged = errors.New("This version of key has been purged")

full stop?


Comments from Reviewable

@janardhan1993
Copy link
Author

Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks broke.


db.go, line 896 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Do a blind write here. And do a select push into a channel, which can be run via a goroutine, which can update the GC stats.

Done.


db.go, line 976 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

No need for all versions.

Done.


db.go, line 243 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

1000

Done.


db.go, line 243 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

purgeUpdateCh : chan purgeUpdate

Done.


db.go, line 317 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

db.updateGCStats

Done.


db.go, line 461 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

IMPORTANT

Done.


db.go, line 961 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

default:

Done.


db_test.go, line 326 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Explain in db.go why we should never write a key with an older version to LSM tree.

Done.


levels.go, line 673 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

When generating LSM tables, continue iterating until all the versions have been picked up by a single SSTable. That is, don't split a key between 2 tables on the same level.

Done.


levels.go, line 661 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Don't declare the return vars here.

Done.


levels.go, line 676 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

return vs, nil

Done.


levels.go, line 678 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

return empty vs, nil?

Done.


value.go, line 319 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Add a comment //pass or explain why this.

Done.


value.go, line 314 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Can just take the fully loaded keyWithTs, and parse the version and key out of it.

Done.


y/sparse_linux.go, line 28 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

full stop?

Done.


Comments from Reviewable

@janardhan1993 janardhan1993 merged commit a44a56e into v2.0-candidate Jan 10, 2018
@janardhan1993 janardhan1993 deleted the jan/purge branch January 10, 2018 06:03
deepakjois pushed a commit that referenced this pull request Jan 17, 2018
* change the way purge works
* Stop searching as soon as value is found instead of searching all tables
* Define ErrPurged to be nil for windows/osx
* update gc stats in background
*Ensure all versions of key are always written to same table.
* add test to punch holes twice in same file and delete it later
deepakjois pushed a commit that referenced this pull request Jan 22, 2018
* change the way purge works
* Stop searching as soon as value is found instead of searching all tables
* Define ErrPurged to be nil for windows/osx
* update gc stats in background
*Ensure all versions of key are always written to same table.
* add test to punch holes twice in same file and delete it later
janardhan1993 pushed a commit that referenced this pull request Jan 31, 2018
* change the way purge works
* Stop searching as soon as value is found instead of searching all tables
* Define ErrPurged to be nil for windows/osx
* update gc stats in background
*Ensure all versions of key are always written to same table.
* add test to punch holes twice in same file and delete it later
deepakjois pushed a commit that referenced this pull request Feb 1, 2018
* change the way purge works
* Stop searching as soon as value is found instead of searching all tables
* Define ErrPurged to be nil for windows/osx
* update gc stats in background
*Ensure all versions of key are always written to same table.
* add test to punch holes twice in same file and delete it later
janardhan1993 pushed a commit that referenced this pull request Mar 5, 2018
* change the way purge works
* Stop searching as soon as value is found instead of searching all tables
* Define ErrPurged to be nil for windows/osx
* update gc stats in background
*Ensure all versions of key are always written to same table.
* add test to punch holes twice in same file and delete it later
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants