Add Read only Mode, does not allow inserts#75
Add Read only Mode, does not allow inserts#75movitto wants to merge 2 commits intocppalliance:masterfrom
Conversation
| const std::string& default_log_file() { | ||
| static const std::string dkf("nudb.log"); | ||
| return dkf; | ||
| } |
There was a problem hiding this comment.
nudb should have a <nudb/string_type.hpp> which declares nudb::string_view, copy from here:
https://github.com/boostorg/beast/blob/develop/include/boost/beast/core/string_type.hpp
And these functions should use that string_view. Also I think these functions need to be declared inline.
There was a problem hiding this comment.
@miguelportilla will work up a branch that has just this change for us
include/nudb/impl/basic_store.ipp
Outdated
| , c1(kh_.key_size, kh_.block_size, "c1") | ||
| , kh(kh_) | ||
| { | ||
| static_assert(is_File<File>::value, |
include/nudb/impl/basic_store.ipp
Outdated
| error_code& ec, | ||
| Args&&... args) | ||
| { | ||
| BOOST_ASSERT(boost::filesystem::exists(dir_path)); |
There was a problem hiding this comment.
This shouldn't be an assert, it should be a run-time error code, e.g. dir_not_found. Or perhaps we just propagate the boost::filesystem error if one exists.
There was a problem hiding this comment.
If we don't use filesystem we need to add our own error enum value
include/nudb/basic_store.hpp
Outdated
| }; | ||
|
|
||
| bool open_ = false; | ||
| bool read_only_ = false; |
There was a problem hiding this comment.
bool is_writable_ and it should not have a default value
include/nudb/basic_store.hpp
Outdated
| except @ref open. | ||
| */ | ||
| bool | ||
| is_read_only() const |
| */ | ||
| template<class... Args> | ||
| void | ||
| open( |
There was a problem hiding this comment.
This really feels like it should be a free function which calls into the existing open member function. The function could be open_dir.
include/nudb/basic_store.hpp
Outdated
| */ | ||
| template<class... Args> | ||
| void | ||
| open_read_only( |
There was a problem hiding this comment.
To avoid a combinatorial explosion of signatures, the open mode should be a parameter.
include/nudb/impl/basic_store.ipp
Outdated
| #include <boost/assert.hpp> | ||
| #include <cmath> | ||
| #include <memory> | ||
| #include <boost/filesystem.hpp> |
There was a problem hiding this comment.
This is a hefty dependency to add, and we are using so little of it. I would prefer if we had our own functions to perform the needed operations, e.g. path_cat:
// Append an HTTP rel-path to a local filesystem path.
// The returned path is normalized for the platform.
std::string
path_cat(
beast::string_view base,
beast::string_view path)
{
if(base.empty())
return std::string(path);
std::string result(base);
#ifdef BOOST_MSVC
char constexpr path_separator = '\\';
if(result.back() == path_separator)
result.resize(result.size() - 1);
result.append(path.data(), path.size());
for(auto& c : result)
if(c == '/')
c = path_separator;
#else
char constexpr path_separator = '/';
if(result.back() == path_separator)
result.resize(result.size() - 1);
result.append(path.data(), path.size());
#endif
return result;
}
There was a problem hiding this comment.
We can maybe fall back to boost::filesystem if we can't identify the platform
include/nudb/impl/basic_store.ipp
Outdated
| df.open(file_mode::read, dat_path, ec); | ||
| if(ec) | ||
| return; | ||
| kf.open(file_mode::read, key_path, ec); |
There was a problem hiding this comment.
There's a lot of duplicated code here...
|
This will need tests and coverage (i.e. 100% coverage for all new and changed lines). We should make the indicated changes first, then write the tests. |
|
@vinniefalco sounds good, I'll get back to this & incorporate your feedback in the near future |
|
I believe @miguelportilla is working on adding |
Init files from default names under dir
|
@vinniefalco this was rebased ontop of the updated PR #74 and updated to incorporate feedback. Tests have not been written for this yet, I wanted to make sure this is what you were describing in your feedback. Specifically the 'open_mode' enum which was added to basic_store and specified as a param to 'open'. Is this what you were referring to regarding non-duplicate method signatures? This would cause an API incompatability and in the case of readonly access the log file param is unused. |
You still need the log file in order to know if the database is in a valid state |
2 participants
This would be useful for situations in which the database filesystem / files are not writeable by the local process but read functionality is desired. Mirrors the existing open API with the omission of the 'log path' parameter as this will not be created / written to anyways.
The synchronized write mechanism is skipped when the database is open in read only and attempting to call insert results in failure.
Based on PR #74, will rebase if/when that gets merged.