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

Do not stall Level 0 and 1 #1186

Merged
merged 10 commits into from
Jan 15, 2020
Merged

Do not stall Level 0 and 1 #1186

merged 10 commits into from
Jan 15, 2020

Conversation

jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Jan 8, 2020

We don't need to stall writes if Level 1 does not have enough space. Level 1 is stored on the disk and it should be okay to have more number of tables (more size) on Level 1 than the max level 1 size. These tables will eventually be compacted to lower levels.

This commit changes the following

  • We no longer stall writes if L1 doesn't have enough space.
  • We stall L0 only if KeepL0InMemory is true.
  • Upper levels (L0, L1, etc) get priority in compaction (previously, level with higher priority score would get preference)

This change is Reviewable

@coveralls
Copy link

coveralls commented Jan 8, 2020

Coverage Status

Coverage increased (+0.3%) to 69.941% when pulling 952173b on ibrahim/remove-l1-stall into 2a90c66 on master.

Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

:lgtm: but ask Manish for final approval

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ashish-goswami and @manishrjain)

Copy link
Contributor

@ashish-goswami ashish-goswami left a comment

Choose a reason for hiding this comment

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

Please add a test if possible. :lgtm:

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @manishrjain)

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

Also, do the priority as discussed.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @ashish-goswami, @jarifibrahim, @manishrjain, and @martinmr)


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

			// computing compactability in order to guarantee progress.
			// Break the loop once L0 has enough space to accommodate new tables.
			if !s.isLevel0Compactable() {

If L0 not in memory, don't stall here.

The only stall should happen based on memory, not disk.

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @ashish-goswami, @jarifibrahim, and @martinmr)

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jarifibrahim)


levels.go, line 426 at r3 (raw file):

			prios = append(prios, pri)
		}
	}

We used to compact based on the score. But, we decided to compact based on the level, not the priority. So, upper levels always get compacted first, before the lower levels -- this allows us to avoid stalls.

@jarifibrahim
Copy link
Contributor Author

Travis build failed because of #1197 and #1196 .

@jarifibrahim jarifibrahim merged commit 3747be5 into master Jan 15, 2020
@jarifibrahim jarifibrahim deleted the ibrahim/remove-l1-stall branch January 15, 2020 10:27
coocood pushed a commit to pingcap/badger that referenced this pull request Feb 25, 2020
coocood added a commit to pingcap/badger that referenced this pull request Feb 25, 2020
@jarifibrahim jarifibrahim changed the title Do not stall Level 1 Do not stall Level 0 and 1 Aug 14, 2020
jarifibrahim pushed a commit that referenced this pull request Aug 19, 2020
Related to #1459

This PR contains the following changes to compactions
- Use a separate thread for compacting Level 0 and 1 and a separate one for other levels
- Pick levels to compact based on score.
- Stall Level 0 if compactions cannot keep up (we had added this in #1186)
- Limit the number of open table builders to 5 in compactions.
jarifibrahim pushed a commit that referenced this pull request Aug 20, 2020
Related to #1459

This PR contains the following changes to compactions
- Use a separate thread for compacting Level 0 and 1 and a separate one for other levels
- Pick levels to compact based on score.
- Stall Level 0 if compactions cannot keep up (we had added this in #1186)
- Limit the number of open table builders to 5 in compactions.

(cherry picked from commit 0b8eb4c)
jarifibrahim pushed a commit that referenced this pull request Aug 25, 2020
Related to #1459

This PR contains the following changes to compactions
- Use a separate thread for compacting Level 0 and 1 and a separate one for other levels
- Pick levels to compact based on score.
- Stall Level 0 if compactions cannot keep up (we had added this in #1186)
- Limit the number of open table builders to 5 in compactions.

(cherry picked from commit 0b8eb4c)
jarifibrahim pushed a commit that referenced this pull request Oct 2, 2020
Related to #1459

This PR contains the following changes to compactions
- Use a separate thread for compacting Level 0 and 1 and a separate one for other levels
- Pick levels to compact based on score.
- Stall Level 0 if compactions cannot keep up (we had added this in #1186)
- Limit the number of open table builders to 5 in compactions.
manishrjain pushed a commit that referenced this pull request Oct 15, 2020
Related to #1459

This PR contains the following changes to compactions
- Use a separate thread for compacting Level 0 and 1 and a separate one for other levels
- Pick levels to compact based on score.
- Stall Level 0 if compactions cannot keep up (we had added this in #1186)
- Limit the number of open table builders to 5 in compactions.
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.

6 participants