PLAT-7040 Disable "force" parameter to EnableFileDeletions() #54
Conversation
… on our older version
…eletions(). Following facebook/rocksdb HEAD.
… leftover assert)
|
NOTE: "Check buck targets" are tests of code formatting. Not going to attempt fixes. The are not required for merging. |
|
The code changed is covered by rocksdb unit test error_handler_fs_test (passing). I have also used delete_scheduler_test previously as a quick check (passing). |
There was a problem hiding this comment.
The large number of changes to config.yml gives me pause. If the pieces that we need still build successfully and if all the unit tests pass, then we're probably okay.
I'm less sure about disabling the force parameter to DBImpl::EnableFileDeletions(bool force), since there may be other code that depends on that functionality. You have already removed the call to DBImpl::EnableFileDeletions() in DBImpl::ResumeImpl(), so that should be sufficient for this fix (I think).
Do we have a way to run the RocksDB unit tests as part of this PR? It would be good to confirm that these changes don't break anything before we try this in Starrocks.
| ROCKS_LOG_INFO(immutable_db_options_.info_log, "Successfully resumed DB"); | ||
| } else { | ||
| ROCKS_LOG_INFO(immutable_db_options_.info_log, "Failed to resume DB [%s]", |
There was a problem hiding this comment.
Should this be logged as an error (or warning)?
There was a problem hiding this comment.
The config.yml change size is due to our branch being at least 2 years behind facebook's HEAD. I did not try to be surgical. I just copied HEAD to our branch.
The sole purpose of this config.yml copy was to enable our CircleCI to run. We have not been running CircleCI for our rocksdb fork for years. It is now executing the same thing as Facebook expects. I can expand the actual test set if you desire. CircleCI does run a subset.
EnableFileDeletions(true) is only called one place in our version (including the default of EnableFileDeletions()). That place is the code I removed. My removal makes our code parallel with facebook HEAD. I also made our EnableFilesDeletion() code look like the HEAD just in case someone else, say Simon, thought it good to add an EnableFileDeletions() with or without the explicit force parameter some time in the future.
The Facebook discussion is here (look down a few lines for EnablefileDeletions): https://github.com/facebook/rocksdb/blob/main/HISTORY.md#900-02162024
There was a problem hiding this comment.
ROCKS_LOG_INFO versus warning or error: I am just copying implementation from HEAD.
I was able to get a partial LOG from Morgan Stanley. I found the one place within rocksdb that uses "force = true" in calling EnableFileDeletions(), namely ResumeImpl(). The LOG lines support the idea that a background error occurred. ResumeImpl() is only called in such situations.
That same function, ResumeImpl(), no longer makes the call in more recent rocksdb versions. And the HISTORY.md file explains that "force" was really, really bad and removed.
Removing "force" from our code base too.