Fix unexpected compaction error for compact files

[Connor1996]

Summary:
When doing CompactFiles on the files of multiple levels(num_level > 2) with L0 is included, the compaction would fail like this.

The reason is that in VerifyCompactionFileConsistency it checks the levels between the L0 and base level should be empty, but it regards the compaction triggered by CompactFiles as an L0 -> base level compaction wrongly.

The condition is committed several years ago, whereas it isn't correct anymore.

 if (vstorage->compaction_style_ == kCompactionStyleLevel &&
        c->start_level() == 0 && c->num_input_levels() > 2U)

So this PR just deletes the incorrect check.

Test Plan:
make check

[yiwu-arbug]

@ajkr Is there anyone can take a look and merge the PR? Thanks.

CI failure looks irrelevant.

[facebook-github-bot]

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[ajkr]

Agreed the check is incorrect and can be removed.

num_input_levels == output_level - start_level + 1

My reading of the code suggests this is still true today. Is it not? That condition can be true and the check can still be incorrect since that condition doesn't imply anything about whether the input levels are empty.

[yiwu-arbug]

Agreed the check is incorrect and can be removed.

num_input_levels == output_level - start_level + 1

My reading of the code suggests this is still true today. Is it not? That condition can be true and the check can still be incorrect since that condition doesn't imply anything about whether the input levels are empty.

The issue is not about level compaction, but about when we issue CompactFiles to compact some files across multiple levels, where the check is incorrect.

[Connor1996]

@ajkr the test fails but seems unrelated. Could you help to take a look?

[Connor1996]

Agreed the check is incorrect and can be removed.

num_input_levels == output_level - start_level + 1

My reading of the code suggests this is still true today. Is it not? That condition can be true and the check can still be incorrect since that condition doesn't imply anything about whether the input levels are empty.

num_input_levels == output_level - start_level + 1 is not true today. As I understood, num_input_levels should be the number of levels involved in compaction input.

[ajkr]

As I understood, num_input_levels should be the number of levels involved in compaction input.

I thought the below code fills in entries for all the levels not explicitly provided, which would be L1-L4 in your example. num_input_levels comes from the size of that vector.

rocksdb/db/compaction/compaction_picker.cc

Lines 407 to 411 in 08144bc

	for (int level = first_non_empty_level; level <= last_non_empty_level;
	++level) {
	matched_input_files[level].level = level;
	input_files->emplace_back(std::move(matched_input_files[level]));
	}

Anyways, this isn't a blocking issue - I am just waiting on somebody to accept the internal PR to land it.

[Connor1996]

As I understood, num_input_levels should be the number of levels involved in compaction input.

I thought the below code fills in entries for all the levels not explicitly provided, which would be L1-L4 in your example. num_input_levels comes from the size of that vector.

rocksdb/db/compaction/compaction_picker.cc

Lines 407 to 411 in 08144bc

	for (int level = first_non_empty_level; level <= last_non_empty_level;
	++level) {
	matched_input_files[level].level = level;
	input_files->emplace_back(std::move(matched_input_files[level]));
	}

Anyways, this isn't a blocking issue - I am just waiting on somebody to accept the internal PR to land it.

Oh, I see.

[facebook-github-bot]

@Connor1996 has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@ajkr merged this pull request in f06b761.