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

SuggestCompactRange() should execute compaction even if the data exists only on the bottommost level #1974

Open
igorcanadi opened this issue Mar 9, 2017 · 8 comments
Assignees

Comments

@igorcanadi
Copy link
Collaborator

This is related to https://jira.percona.com/browse/PSMDB-127. When we drop an index or a collection in MongoRocks, we mark all the keys as dead through a compaction filter. We don't issue any deletes. Compaction filters will slowly get rid of those dead indexes as compactions on the dead data gets executed.

However, we have a pathological case if a data for a dropped index only exists on the bottom-most level. In that case, compaction for the data will never be executed and the data won't be cleaned up.

We do call SuggestCompactRange() on the dropped data, but this also doesn't do anything if data is on the bottommost-level: https://github.com/mongodb-partners/mongo-rocks/blob/master/src/rocks_engine.cpp#L500

We should add an option to SuggestCompactRange() that would let us compact the data even if it exists only on the bottommost level.

@siying
Copy link
Contributor

siying commented Mar 9, 2017

"we mark all the keys as dead through a compaction filter" what does it mean?

@siying
Copy link
Contributor

siying commented Mar 9, 2017

We can issue last level => last level compaction if the files are marked as need compaction. Anyone is welcome to contribute.

@mdcallag
Copy link
Contributor

mdcallag commented Mar 9, 2017

"we mark all the keys as dead through a compaction filter" what does it mean?

I think this means drop index is fast because index entries are not deleted, and then the compaction filter will (or can, or should) drop those entries later and they can be found because the leading bytes in their key is the index-id.

@igorcanadi
Copy link
Collaborator Author

What Mark said.

@siying - thanks. I opened this ticket so that we can track this work if we find it necessary.

@wolfkdy
Copy link
Contributor

wolfkdy commented Oct 24, 2017

@igorcanadi I'm a little superised to see this issue, and checked the code rocks_engine.cpp#L500 you posted, but find unrelated code. It is because you posted the code from the master-branch. git master always changes.
I also digged into rocks, the related code in rocks should be those described like below:

1861       } else if (level == max_level_with_files && level > 0) {
1862         if (options.bottommost_level_compaction ==
1863             BottommostLevelCompaction::kSkip) {
1864           // Skip bottommost level compaction
1865           continue;
1866         } else if (options.bottommost_level_compaction ==
1867                        BottommostLevelCompaction::kIfHaveCompactionFilter &&
1868                    cfd->ioptions()->compaction_filter == nullptr &&
1869                    cfd->ioptions()->compaction_filter_factory == nullptr) {
1870           // Skip bottommost level compaction since we don't have a compaction
1871           // filter
1872           continue;
1873         }
1576 enum class BottommostLevelCompaction {
1577   // Skip bottommost level compaction
1578   kSkip,
1579   // Only compact bottommost level if there is a compaction filter
1580   // This is the default option
1581   kIfHaveCompactionFilter,
1582   // Always compact bottommost level
1583   kForce, 
1584 }; 

bottom-most level should be compacted if the option is figured as kIfHaveCompactionFilter

So, would you please repost the line triggered the problem in rocks_engine.cpp ?
it may help us a lot, thanks !

@igorcanadi
Copy link
Collaborator Author

@wolfkdy the code you quote is from DBImpl::CompactRange(), while this issue is for a different function, SuggestCompactRange()

@wolfkdy
Copy link
Contributor

wolfkdy commented Oct 25, 2017

@igorcanadi I've found the related code

1322 void VersionStorageInfo::ComputeFilesMarkedForCompaction() {
1323   files_marked_for_compaction_.clear();
1324   int last_qualify_level = 0;
1325 
1326   // Do not include files from the last level with data
1327   // If table properties collector suggests a file on the last level,
1328   // we should not move it to a new level.
1329   for (int level = num_levels() - 1; level >= 1; level--) {
1330     if (!files_[level].empty()) {
1331       last_qualify_level = level - 1;
1332       break;
1333     }
1334   }
1335 
1336   for (int level = 0; level <= last_qualify_level; level++) {
1337     for (auto* f : files_[level]) {
1338       if (!f->being_compacted && f->marked_for_compaction) {
1339         files_marked_for_compaction_.emplace_back(level, f);
1340       }
1341     }
1342   }
1343 }
1344 

It seems suggestCompactRange will compact data exist only above the bottom-most level.
mongoRocks3.2.10 uses suggestCompactRange, which means 90% of data will never be recycled if the db is dropped.
I saw mongoRocks master-branch has done compaction pretty better, it does not depend on the schedule framework of rocksdb, instead, it implements a priority_queue itself.

@igorcanadi
Copy link
Collaborator Author

Exactly. That's what this issue is about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants