Eliminates a no-op compaction upon snapshot release when disabling auto compactions

[Connor1996]

Summary:

After releasing a snapshot, it checks whether it is suitable to trigger bottom compactions.
When disabling auto compactions, it may still schedule compaction when releasing a snapshot. Whereas no compaction job will be actually handled, so the state of LSM is not changed and compaction will be triggered again and again every time releasing a snapshot.

Too frequent compactions lead to high CPU usage and high db_mutex lock contention which affects foreground write duration finally.

Test Plan:

• make check
• manual test

[Connor1996]

@ajkr PTAL

[ajkr]

Nice find! Can you mention it in "Bug Fixes" section in HISTORY.md?

[facebook-github-bot]

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

[ajkr]

Hm, actually upon looking further I'm not able to repro the problem using the provided test (without the fix). The test causes one CFD to be enqueued for compaction unnecessarily. But no compaction gets picked due to

rocksdb/db/db_impl/db_impl_compaction_flush.cc

Line 2630 in ce41923

if (!mutable_cf_options->disable_auto_compactions && !cfd->IsDropped()) {

. Then I see one occurrence of "Compaction nothing to do" in the LOG, and then nothing further gets scheduled.

[Connor1996]

Hm, actually upon looking further I'm not able to repro the problem using the provided test (without the fix). The test causes one CFD to be enqueued for compaction unnecessarily. But no compaction gets picked due to

rocksdb/db/db_impl/db_impl_compaction_flush.cc

Line 2630 in ce41923

if (!mutable_cf_options->disable_auto_compactions && !cfd->IsDropped()) {

. Then I see one occurrence of "Compaction nothing to do" in the LOG, and then nothing further gets scheduled.

Yes, this unit test is just to ensure that the compaction shouldn't be scheduled when disabling auto compaction. For the case, it has to get and release snapshot persistently which is typical for scan workload and causes a lot of Compaction nothing to do. But it is hard for a unit test to do that, so I only check it by manual.

Do you have any suggestion?

[facebook-github-bot]

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

[facebook-github-bot]

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

[facebook-github-bot]

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

[facebook-github-bot]

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

[ajkr]

Hm, actually upon looking further I'm not able to repro the problem using the provided test (without the fix). The test causes one CFD to be enqueued for compaction unnecessarily. But no compaction gets picked due to

rocksdb/db/db_impl/db_impl_compaction_flush.cc

Line 2630 in ce41923

if (!mutable_cf_options->disable_auto_compactions && !cfd->IsDropped()) {

. Then I see one occurrence of "Compaction nothing to do" in the LOG, and then nothing further gets scheduled.

Yes, this unit test is just to ensure that the compaction shouldn't be scheduled when disabling auto compaction. For the case, it has to get and release snapshot persistently which is typical for scan workload and causes a lot of Compaction nothing to do. But it is hard for a unit test to do that, so I only check it by manual.

Do you have any suggestion?

I would just suggest making it clear in the title/description/release note that this eliminates a no-op compaction upon snapshot release. "trigger compaction endlessly" sounds like there was an infinite loop somewhere, but my understanding now is that's not the case. Besides that, the fix and test LGTM!

[ajkr]

Also regarding the unit test, you can add a change like this (https://gist.github.com/ajkr/7681bf61b1cdcf76e5510dc9ead2d482) to verify the fix is effective in preventing any no-op compaction.

[facebook-github-bot]

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

[Connor1996]

Thanks for your advice. PTAL again @ajkr

[ajkr]

LGTM. It looks like a lot of HISTORY.md changes related to the markdown.

[facebook-github-bot]

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

[ajkr]

LGTM. It looks like a lot of HISTORY.md changes related to the markdown.

Regarding the markdown changes, if you did intend to include them, would you mind separating them into another PR? Also, we've recently decided to adopt ISO 8601 date format (YYYY-MM-DD). So if you do decide to format all of HISTORY.md, it'd be nice to also change the dates (e.g., "6/12/2020" -> "2020-06-12").

[facebook-github-bot]

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

[facebook-github-bot]

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

[Connor1996]

@ajkr Not intend to do that, it's just changed automatically by markdown-lint. Already revert it.

[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 416943b.