Add a DB Session ID #6959
Add a DB Session ID #6959
Conversation
There was a problem hiding this comment.
@gg814 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Thanks for working on this! I'm thinking it is a little misleading using 'Session', the first impression is like a session in a transaction or something similar. How about DBObjectID? @pdillinger what do you think? |
Roughly quoting Andrew from internal group chat: that requires the user to know that Open() is only offered as a static function (otherwise, someone could reopen the DB using the same object). So I think we decided to keep the name used here. |
Yeah. Agree, keep DbSessionID will be better (post is just one minutes ahead of the discussion>_<) |
|
|
||
| #ifndef ROCKSDB_LITE | ||
| Close(); | ||
| ASSERT_OK(ReadOnlyReopen(options)); |
There was a problem hiding this comment.
How about add a test to compare the id between regular reopen and ReadonlyReopen?
| @@ -1538,6 +1538,9 @@ class DB { | |||
| // Returns Status::OK if identity could be set properly | |||
| virtual Status GetDbIdentity(std::string& identity) const = 0; | |||
|
|
|||
| // Return a unique identifier for each DB object that is opened | |||
| virtual Status GetDbSessionId(std::string& session_id) const = 0; | |||
There was a problem hiding this comment.
Since you add new interfaces to include/rocksdb/*, please also change HISTORY.md and add the comments to Public API changes
There was a problem hiding this comment.
I think we should say how unique the value is. (Unique only among currently open DBs on the host?) Perhaps add this:
This DB session ID should be unique among all open DB instances on all hosts, and should be unique among re-openings of the same or other DBs. (Two open DBs have the same identity from other function GetDbIdentity when one is physically copied from the other.)
| @@ -980,6 +982,7 @@ class DBImpl : public DB { | |||
| protected: | |||
| const std::string dbname_; | |||
| std::string db_id_; | |||
| std::string db_session_id_; | |||
There was a problem hiding this comment.
Add comment to the new variable being added.
db/db_impl/db_impl.h
Outdated
| @@ -1161,6 +1164,8 @@ class DBImpl : public DB { | |||
| // bump up the version set's next_file_number_ to be 1 + largest_file_number. | |||
| Status FinishBestEffortsRecovery(); | |||
|
|
|||
| void SetDbSessionId() { db_session_id_ = env_->GenerateUniqueId(); } | |||
There was a problem hiding this comment.
Also, a comment here to explain why we use GenerateUniqueId() as the ID and also when SetDbSessionId() should be called.
|
@gg814 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@gg814 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
HISTORY.md
Outdated
| @@ -23,6 +23,7 @@ | |||
| * ldb now uses options.force_consistency_checks = true by default and "--disable_consistency_checks" is added to disable it. | |||
| * DB::OpenForReadOnly no longer creates files or directories if the named DB does not exist, unless create_if_missing is set to true. | |||
| * The consistency checks that validate LSM state changes (table file additions/deletions during flushes and compactions) are now stricter, more efficient, and no longer optional, i.e. they are performed even if `force_consistency_checks` is `false`. | |||
| * `DB::GetDbSessionId(std::string& session_id)` is added. `session_id` stores a unique identifier that gets updated every the DB is opened. This identifier is recorded in the `LOG` file on the line starting with `DB Session ID:`. | |||
There was a problem hiding this comment.
I would say, "that gets reset every time the DB is opened." ("updated" can mean new value depends on old value.)
There was a problem hiding this comment.
Maybe follow what Peter stated before "This DB session ID should be unique among all open DB instances on all hosts, and should be unique among re-openings of the same or other DBs." to explain session_id
| @@ -1538,6 +1538,9 @@ class DB { | |||
| // Returns Status::OK if identity could be set properly | |||
| virtual Status GetDbIdentity(std::string& identity) const = 0; | |||
|
|
|||
| // Return a unique identifier for each DB object that is opened | |||
| virtual Status GetDbSessionId(std::string& session_id) const = 0; | |||
There was a problem hiding this comment.
I think we should say how unique the value is. (Unique only among currently open DBs on the host?) Perhaps add this:
This DB session ID should be unique among all open DB instances on all hosts, and should be unique among re-openings of the same or other DBs. (Two open DBs have the same identity from other function GetDbIdentity when one is physically copied from the other.)
|
@pdillinger Thanks for the review! Will update correspondingly. |
|
@gg814 has updated the pull request. Re-import the pull request |
|
@gg814 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@gg814 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: Added DB::GetDbSessionId by using the same format and machinery as DB::GetDbIdentity. The DB Session ID is generated (and therefore, updated) each time a DB object is opened. It is written to the LOG file right after the line of “DB SUMMARY”. A test for the uniqueness, for different openings and during the same opening, is also added. Test plan: Passed make check
|
@gg814 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@gg814 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary:
Added DB::GetDbSessionId by using the same format and machinery as DB::GetDbIdentity.
The DB Session ID is generated (and therefore, updated) each time a DB object is opened. It is written to the LOG file right after the line of “DB SUMMARY”.
A test for the uniqueness, for different openings and during the same opening, is also added.
Test plan:
Passed make check