Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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

@jarifibrahim jarifibrahim requested a review from a team January 8, 2020 15:39
@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
Port hypermodeinc#1186

Co-authored-by: Ibrahim Jarif <jarifibrahim@gmail.com>
@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.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants