Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

backend: implement disk quota #493

Merged
merged 41 commits into from
Jan 29, 2021
Merged

backend: implement disk quota #493

merged 41 commits into from
Jan 29, 2021

Conversation

kennytm
Copy link
Collaborator

@kennytm kennytm commented Nov 26, 2020

What problem does this PR solve?

Implement #446.

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Possible performance regression
  • Increased code complexity

Related changes

  • Need to update the documentation
  • Need to be included in the release note

@kennytm kennytm added the status/WIP Work in progress label Nov 26, 2020
sort.Slice(sizes, func(i, j int) bool {
a, b := &sizes[i], &sizes[j]
if a.IsImporting != b.IsImporting {
return a.IsImporting
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it mean? Do you want to sort all importing engine before other engines?

Copy link
Collaborator Author

@kennytm kennytm Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. We can't import engines that are already importing, but their size do count towards the disk quota.

@kennytm kennytm added status/DNM Do not merge, test is failing or blocked by another PR and removed status/WIP Work in progress labels Dec 2, 2020
@kennytm kennytm force-pushed the kennytm/disk-quota branch 2 times, most recently from 3f12d8c to f6f87fc Compare December 2, 2020 20:31
@kennytm
Copy link
Collaborator Author

kennytm commented Dec 2, 2020

The test case depends on #505 in order to quickly generate huge SSTs without checking in a 1 GB database or introduce dbgen.

@kennytm kennytm marked this pull request as ready for review December 2, 2020 20:43
@kennytm kennytm changed the title [WIP] Implement disk quota backend: implement disk quota Dec 5, 2020
@kennytm kennytm added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/DNM Do not merge, test is failing or blocked by another PR labels Dec 8, 2020
@kennytm kennytm removed the status/WIP Work in progress label Jan 19, 2021
})
return
}

type LocalWriter struct {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL @Little-Wallace, the LocalWriter is basically entirely rewritten, there may be some performance implication of the original code that I may have missed. Thanks.

Copy link
Contributor

@glorv glorv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rest LGTM

lightning/backend/local.go Outdated Show resolved Hide resolved
lightning/backend/local.go Outdated Show resolved Hide resolved
return
}
func() {
rc.diskQuotaLock.RLock()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the current implementation can not handle this if the previous check isImporting is false and the disk-quote loop holds the lock and do importing.

lightning/backend/importer.go Outdated Show resolved Hide resolved
lightning/backend/local.go Outdated Show resolved Hide resolved
lightning/backend/local.go Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@kennytm kennytm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test

lightning/backend/local.go Outdated Show resolved Hide resolved
lightning/backend/local.go Outdated Show resolved Hide resolved

// append more KV pairs to the kvMemCache.
func (m *kvMemCache) append(kvs []common.KvPair) {
if !m.notSorted {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if the kvMemCache has been sorted, we shall still check the order of kvs. The array kvs may be unordered and the first key of kvs may be greater than the lastkey of m.kvs

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the loop inside does check the order of kvs.

		var lastKey []byte
		if len(m.kvs) > 0 {
			lastKey = m.kvs[len(m.kvs)-1].Key
		}
		for _, kv := range kvs {
			if bytes.Compare(kv.Key, lastKey) <= 0 {    // <----
				m.notSorted = true
				break
			}
			lastKey = kv.Key   // <----
		}

@lance6716
Copy link
Contributor

/lgtm

@ti-srebot ti-srebot removed the status/LGT1 One reviewer already commented LGTM (LGTM1) label Jan 29, 2021
@ti-srebot ti-srebot added the status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) label Jan 29, 2021
@glorv
Copy link
Contributor

glorv commented Jan 29, 2021

/lgtm

@ti-srebot ti-srebot added status/LGT3 and removed status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) labels Jan 29, 2021
@glorv
Copy link
Contributor

glorv commented Jan 29, 2021

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@kennytm merge failed.

@glorv
Copy link
Contributor

glorv commented Jan 29, 2021

/run-all-tests

@glorv glorv merged commit 1f12021 into master Jan 29, 2021
@glorv glorv deleted the kennytm/disk-quota branch January 29, 2021 10:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CanMerge status/LGT3 status/PTAL This PR is ready for review. Add this label back after committing new changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants