Rename database file to sfs.db #245
Conversation
Use constants DB_FILENAME and DB_WAL_FILENAME to refer to our database filename. Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
|
The last commit in chain has basic migration code. Happy to drop it |
| @@ -414,4 +416,24 @@ void DBConn::maybe_upgrade_metadata() { | |||
| } | |||
| } | |||
|
|
|||
| void DBConn::maybe_rename_database_file() const { | |||
There was a problem hiding this comment.
I suspect we may also have to rename the -wal and -shm files. I think those can be left behind if the database is not properly closed.
There was a problem hiding this comment.
Yeah, if you killall -9 radosgw the -wal and -shm files will remain, then the next time you start it up, you'll see something like [SQLITE] (283) recovered 26 frames from WAL file /scratch/s3gw/qa/s3gw.db-wal in the log. So if we keep the migration code, we'd need to migrate those two as well.
There was a problem hiding this comment.
Thinking about it, the rename code is a bit naive. Perhaps too naive. Not only do we need to rename the extra files, there may also be temporary files (according to docs) that share the basename. If the database is still open renaming is also a big mistake.
A safer option would be the backup API - I think we should rather use that
This would not be feasible in kubernetes. Possible, yes, but asking that from the user would be annoying. We either assume their volumes are to be blown away, or we do the migration. TBH, I'm in favor of doing the migration (it doesn't look too difficult or error prone), and we can always remove it further down the line, before GA. But this also really depends on when we want to consider the on-disk format "stable". If only after this, then blow away; but if we think we are already there, then migration is the way to go. |
That'll work. Put the migration in now, drop it in the next release or next release +1 and expect current early adopters to remain on the train (and mention these changes in release notes as we go). I just didn't want to keep that code forever. |
Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
irq0
force-pushed
the
pr/database-file-rename
branch
from
68c6dea
to
834fe55
Compare
|
Last push changes the migration code to use the sqlite3 backup API |
| if (!std::filesystem::exists(getLegacyDBPath(cct))) { | ||
| return; | ||
| } | ||
| if (std::filesystem::exists(getDBPath(cct))) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
this could have been a single if
| return db_path.string(); | ||
| } | ||
|
|
||
| static std::string getLegacyDBPath(CephContext* cct) { |
There was a problem hiding this comment.
to be perfectly honest, I really dislike that we have this function as camelCase when all other functions are snake case. More of an itch for me than anything else.
| @@ -78,7 +79,7 @@ class TestSFSWALCheckpoint : public ::testing::Test { | |||
| size_t num_threads, size_t num_objects | |||
| ) { | |||
| std::atomic<std::uintmax_t> max_wal_size{0}; | |||
| fs::path wal(test_dir / "s3gw.db-wal"); | |||
| fs::path wal(test_dir / sqlite::DB_WAL_FILENAME); | |||
There was a problem hiding this comment.
can we maybe define this as a function of DB_FILENAME, instead of having to keep a #define in the header solely for this test?
There was a problem hiding this comment.
That would leak an sqlite implementation detail that a reader does not need to know about. I'd rather have this
jecluis
added
kind/enhancement
Since aquarist-labs/ceph#245 rgw/sfs uses 'sfs.db' instead of 's3gw.db' Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Use 'sfs.db' instead of 's3gw.db'. Rename 's3gw.db' -> 'sfs.db' if it
exists on startup.
Fixes: https://github.com/aquarist-labs/s3gw/issues/766
Checklist