Conversation
Codecov Report
@@ Coverage Diff @@
## master #944 +/- ##
==========================================
+ Coverage 91.62% 92.89% +1.26%
==========================================
Files 34 34
Lines 3260 3335 +75
==========================================
+ Hits 2987 3098 +111
+ Misses 273 237 -36
|
222d61f
to
3c0db79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 21 files at r1.
Reviewable status: 2 of 21 files reviewed, all discussions resolved
3c0db79
to
410d791
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 13 of 21 files at r1, 2 of 2 files at r2.
Reviewable status: 16 of 21 files reviewed, all discussions resolved
f349845
to
3ae6b29
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 21 files at r1, 11 of 11 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukaszstolarczuk)
tests/config/deprecated_config_cpp.cc, line 8 at r3 (raw file):
#include <string> #include <vector>
no new line
c6f3de6
to
44f36df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 5 of 28 files reviewed, 1 unresolved discussion (waiting on @igchor and @KFilipek)
tests/config/deprecated_config_cpp.cc, line 8 at r3 (raw file):
Previously, KFilipek (Krzysztof Filipek) wrote…
no new line
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 23 of 23 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lukaszstolarczuk)
src/libpmemkv.hpp, line 1532 at r4 (raw file):
inline status config::put_error_if_exists(bool value) noexcept { return put_uint64("error_if_exists", value ? 1 : 0);
logic here is not necessary: (uint64_t)value
src/libpmemkv.hpp, line 1549 at r4 (raw file):
{ return put_uint64("create_if_missing", value ? 1 : 0); }
logic here is not necessary: (uint64_t)value
src/libpmemkv.cc, line 225 at r4 (raw file):
int pmemkv_config_put_create_if_missing(pmemkv_config *config, bool value) { return pmemkv_config_put_uint64(config, "create_if_missing", value ? 1 : 0);
logic here is not necessary: (uint64_t)value
44f36df
to
597817c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 8 of 8 files at r5.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lukaszstolarczuk)
b8484ce
to
cc6c9b2
Compare
cc6c9b2
to
e5ed8c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 23 files at r4, 4 of 12 files at r6.
Reviewable status: 25 of 32 files reviewed, 11 unresolved discussions (waiting on @KFilipek and @lukaszstolarczuk)
ChangeLog, line 13 at r6 (raw file):
Other changes: - Deprecate config's flag 'force_create' and add an alias: 'error_if_exists' (#576) The old flag can still be used and it behaves with no difference. It's
No mention about create_if_missing?
doc/ENGINES-experimental.md, line 22 at r6 (raw file):
+ type: string * **create_if_missing** -- If 1, pmemkv tries to create the file and when that doesn't succeed it tries to open it. If 0, pmemkv uses **error_if_exists** flag to create/open the file.
Hm, shouldn't the default behavior be the same as it is right now? That is, tries to open the database and fails if there is no such pool.
doc/ENGINES-experimental.md, line 25 at r6 (raw file):
+ type: uint64_t + default value: 0 * **error_if_exists** -- If 1, pmemkv creates the file (but it will fail if path exists),
Can we set both of those flags to 1? What will happen?
doc/libpmemkv.7.md, line 71 at r6 (raw file):
+ type: string * **create_if_missing** -- If 1, pmemkv tries to create the file and when that doesn't succeed it tries to open it. If 0, pmemkv uses **error_if_exists** flag to create/open the file.
.
doc/libpmemkv_config.3.md.in, line 39 at r6 (raw file):
int **deprecated** pmemkv_config_put_force_create(pmemkv_config *config, bool value); int pmemkv_config_put_error_if_exists(pmemkv_config *config, bool value);
what about create_if_missing?
src/libpmemkv.hpp, line 1519 at r6 (raw file):
/** * Puts error_if_exists parameter to a config. This flag has lower priority than * **create_if_missing** (see config::put_create_if_missing), setting them both makes no
so maybe disallow setting both?
src/libpmemkv.hpp, line 1522 at r6 (raw file):
*sense. Works only with engines supporting this flag and it means: If true: pmemkv *creates the file, unless it exists - than it fails. If false: pmemkv opens the file *specified by 'path' (see config::put_path), unless the path does not exist - than it
then?
src/libpmemkv.hpp, line 1534 at r6 (raw file):
/** * Puts create_if_missing parameter to a config. This flag is prioritized before * **error_if_exists** (see config::put_error_if_exists) and is encouraged to use.
.
e5ed8c6
to
a934f3c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 25 of 32 files reviewed, 11 unresolved discussions (waiting on @igchor and @KFilipek)
ChangeLog, line 13 at r6 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
No mention about create_if_missing?
Done.
doc/ENGINES-experimental.md, line 22 at r6 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
Hm, shouldn't the default behavior be the same as it is right now? That is, tries to open the database and fails if there is no such pool.
the default stays the same, since we set the new flag to 0 by default
doc/ENGINES-experimental.md, line 25 at r6 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
Can we set both of those flags to 1? What will happen?
discussed below (int other issue)
doc/libpmemkv.7.md, line 71 at r6 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
.
.
doc/libpmemkv_config.3.md.in, line 39 at r6 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
what about create_if_missing?
Done.
src/libpmemkv.hpp, line 1532 at r4 (raw file):
Previously, KFilipek (Krzysztof Filipek) wrote…
logic here is not necessary: (uint64_t)value
Done.
src/libpmemkv.hpp, line 1549 at r4 (raw file):
Previously, KFilipek (Krzysztof Filipek) wrote…
logic here is not necessary: (uint64_t)value
Done.
src/libpmemkv.hpp, line 1519 at r6 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
so maybe disallow setting both?
I'm not sure. Per se, it's not an issue if someone sets them both up to true. On the other hand someone may be confused what's going on (if they set error_if_missing and it has no effect)...
src/libpmemkv.hpp, line 1522 at r6 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
then?
Done.
src/libpmemkv.hpp, line 1534 at r6 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
.
.
src/libpmemkv.cc, line 225 at r4 (raw file):
Previously, KFilipek (Krzysztof Filipek) wrote…
logic here is not necessary: (uint64_t)value
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 12 files at r6, 3 of 4 files at r7.
Reviewable status: 26 of 32 files reviewed, 4 unresolved discussions (waiting on @KFilipek and @lukaszstolarczuk)
doc/ENGINES-experimental.md, line 22 at r6 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
the default stays the same, since we set the new flag to 0 by default
OK, but here you write "If 0, pmemkv uses error_if_exists flag to create/open the file." which is not the default behavior - using error_if_exists means creating the file, not opening it, right?
src/libpmemkv.hpp, line 1532 at r4 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
Done.
nit: static_cast
src/libpmemkv.hpp, line 1549 at r4 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
Done.
.
src/libpmemkv.hpp, line 1519 at r6 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
I'm not sure. Per se, it's not an issue if someone sets them both up to true. On the other hand someone may be confused what's going on (if they set error_if_missing and it has no effect)...
Hm, actually, I just looked more closely at the RocksDB API and it seems like the require setting both of those fields to create or fail if exists. We obviously cannot do the same thing since this would break compatibility so I'm thinking if it wouldn't be better to stay with force_create after all... (this way there would be no misuderstandings - using the same name as RocksDB with slighly different behavior might be misleading).
If we'd leave force_create then we could actually disallow using both flatgs at the same thime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 4 files at r7.
Reviewable status: 27 of 32 files reviewed, 4 unresolved discussions (waiting on @KFilipek and @lukaszstolarczuk)
src/libpmemkv.hpp, line 1519 at r6 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
Hm, actually, I just looked more closely at the RocksDB API and it seems like the require setting both of those fields to create or fail if exists. We obviously cannot do the same thing since this would break compatibility so I'm thinking if it wouldn't be better to stay with force_create after all... (this way there would be no misuderstandings - using the same name as RocksDB with slighly different behavior might be misleading).
If we'd leave force_create then we could actually disallow using both flatgs at the same thime.
Or actually... Maybe we can do it in the same way as RockDB? We could just say that you have to specify both options to get force_create behavior?
a934f3c
to
a7097e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 27 of 32 files reviewed, 4 unresolved discussions (waiting on @igchor and @KFilipek)
doc/ENGINES-experimental.md, line 22 at r6 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
OK, but here you write "If 0, pmemkv uses error_if_exists flag to create/open the file." which is not the default behavior - using error_if_exists means creating the file, not opening it, right?
Maybe the sentence was unfortunate - I re-wrote it a little (only in one place for now) - pls see in libpmemkv.hpp if the wording is correct now. If it's ok - I'll copy it to other docs.
src/libpmemkv.hpp, line 1519 at r6 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
Or actually... Maybe we can do it in the same way as RockDB? We could just say that you have to specify both options to get force_create behavior?
for now, only renamed the new flag to 'create_or_error_if_exists'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 12 files at r6, 23 of 23 files at r8.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @igchor)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @igchor and @lukaszstolarczuk)
ChangeLog, line 12 at r8 (raw file):
Other changes: - Deprecate config's flag 'force_create' and add an alias: 'create_or_error_if_exists' (#576)
are the flags mutually exlusive?
btw, I wouldn't start a changelog entry that adds a new feature with the word "deprecate".
tests/engine_scenarios/all/open.cc, line 9 at r8 (raw file):
* Tests for config flags. * Setting both of them to control kv.open() should not fail in any engine. * If engine supports them, one of them should be prioritzed, and the second one should be
does this mean that the behavior of setting both flags is essentially "random"?
This behavior should be defined explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 23 files at r8.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @lukaszstolarczuk)
doc/ENGINES-experimental.md, line 22 at r8 (raw file):
+ type: string * **create_if_missing** -- If 1, pmemkv tries to create the file and when that doesn't succeed it tries to open it. If 0, pmemkv uses **create_or_error_if_exists** flag to create/open the file.
Hm, this sentence is not entirely true. Maybe we should not write about 0
case here but just have one sentence after both flags are introduces which would say: "If both flags are 0 then pmemkv tries to open the file".?
doc/libpmemkv.7.md, line 70 at r8 (raw file):
* **path** -- Path to a database file or to a poolset file (see **poolset**(5) for details). Note that when using poolset file, size should be 0 + type: string * **create_if_missing** -- If 1, pmemkv tries to create the file and when that doesn't succeed it tries to open it.
Isn't the order (open and then create) reversed in the code?
src/libpmemkv.hpp, line 1518 at r8 (raw file):
/** * Puts create_or_error_if_exists parameter to a config. This flag has lower priority than
Still, I think it would be simpler to just disallow setting both flags to true
. You don;t need to write about priority then.
src/libpmemkv.hpp, line 1522 at r8 (raw file):
* sense. It uses 'path' (as specified by e.g. config::put_path). * Works only with engines supporting this flag and it means: * If true: pmemkv creates the file, unless it exists - then it fails.
Should we use pool
or something similar instead of a file
? I think we use pool
in some other places in the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @igchor and @lukaszstolarczuk)
doc/ENGINES-experimental.md, line 22 at r8 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
Hm, this sentence is not entirely true. Maybe we should not write about
0
case here but just have one sentence after both flags are introduces which would say: "If both flags are 0 then pmemkv tries to open the file".?
Why not try to open the file first? And only if open is fail try to create if missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @lukaszstolarczuk and @vinser52)
doc/ENGINES-experimental.md, line 22 at r8 (raw file):
Previously, vinser52 (Sergei Vinogradov) wrote…
Why not try to open the file first? And only if open is fail try to create if missing?
I think that's what happening in the code right now, my comment was that to make it consistent in the docs.
…_exists force_create naming is confusing and not precise enough, create_or_error_if_exists should be less confusing and now it's better documented.
a7097e8
to
566a0ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 20 of 32 files reviewed, 6 unresolved discussions (waiting on @igchor, @KFilipek, @pbalcer, and @vinser52)
ChangeLog, line 12 at r8 (raw file):
Previously, pbalcer (Piotr Balcer) wrote…
are the flags mutually exlusive?
btw, I wouldn't start a changelog entry that adds a new feature with the word "deprecate".
Done.
doc/ENGINES-experimental.md, line 22 at r8 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
I think that's what happening in the code right now, my comment was that to make it consistent in the docs.
Done.
doc/libpmemkv.7.md, line 70 at r8 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
Isn't the order (open and then create) reversed in the code?
Done.
src/libpmemkv.hpp, line 1518 at r8 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
Still, I think it would be simpler to just disallow setting both flags to
true
. You don;t need to write about priority then.
Done.
src/libpmemkv.hpp, line 1522 at r8 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
Should we use
pool
or something similar instead of afile
? I think we usepool
in some other places in the documentation.
Done.
tests/engine_scenarios/all/open.cc, line 9 at r8 (raw file):
Previously, pbalcer (Piotr Balcer) wrote…
does this mean that the behavior of setting both flags is essentially "random"?
This behavior should be defined explicitly.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 23 files at r4, 12 of 12 files at r9.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @lukaszstolarczuk, @pbalcer, and @vinser52)
doc/ENGINES-experimental.md, line 22 at r8 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
Done.
Still I think this is not entierly true, if it's 0 then i either opens the pool or returns an error (if create_or_error_if_exists is set). I think having a sentence below saying that pmemkv opens the pool only when both of those flags are not set (set to false) would be more clear
tests/engine_scenarios/all/open.cc, line 14 at r9 (raw file):
*/ static void OpenWithCreateIfMissing(std::string path, std::string engine, size_t size,
I gues this test should be run for both existing pool and not existing one. Is this the case?
tests/engine_scenarios/pmemobj/create_or_error_if_exists.cc, line 52 at r9 (raw file):
ASSERT_STATUS(s, pmem::kv::status::OK); }
Is there a test for positive case? (creating not-existent pool with create_or_error_if_exists)
566a0ff
to
465da4c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 30 of 32 files reviewed, 5 unresolved discussions (waiting on @igchor and @pbalcer)
doc/ENGINES-experimental.md, line 22 at r8 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
Still I think this is not entierly true, if it's 0 then i either opens the pool or returns an error (if create_or_error_if_exists is set). I think having a sentence below saying that pmemkv opens the pool only when both of those flags are not set (set to false) would be more clear
Done.
tests/engine_scenarios/all/open.cc, line 14 at r9 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
I gues this test should be run for both existing pool and not existing one. Is this the case?
See answer below for creating new pools.
For opening existing pool - it's tested in scenarios all/open.cc
tests/engine_scenarios/pmemobj/create_or_error_if_exists.cc, line 52 at r9 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
Is there a test for positive case? (creating not-existent pool with create_or_error_if_exists)
yes and no - we use both flags to create database for other tests, see:
- tests/engines/pmemobj_based/pmemobj/create_if_missing.cmake,
- tests/engines/pmemobj_based/pmemobj/put_get_std_map_create_or_error_if_exists.cmake
I was thinking this may be enough - it's not a unit test per se, but it is used. Pls let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r10.
Reviewable status: 31 of 32 files reviewed, 3 unresolved discussions (waiting on @igchor, @pbalcer, and @vinser52)
doc/ENGINES-experimental.md, line 22 at r8 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
Done.
There is the same issue for create_or_error_if_missing I belive. However, I'm not sure if we want to add circular dependency ;d
tests/engine_scenarios/pmemobj/create_or_error_if_exists.cc, line 52 at r9 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
yes and no - we use both flags to create database for other tests, see:
- tests/engines/pmemobj_based/pmemobj/create_if_missing.cmake,
- tests/engines/pmemobj_based/pmemobj/put_get_std_map_create_or_error_if_exists.cmake
I was thinking this may be enough - it's not a unit test per se, but it is used. Pls let me know what you think.
Ok, I think it's fine
it allows to create pool file if it doesn't yet exist and it opens it, if it exists. This flag is mutually exclusive with previously defined config param create_or_error_if_exists - it may be confusing to set them both since one would be quietly ignored. Extend tests for both flags.
465da4c
to
d7358b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 30 of 32 files reviewed, 3 unresolved discussions (waiting on @igchor and @pbalcer)
doc/ENGINES-experimental.md, line 22 at r8 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
There is the same issue for create_or_error_if_missing I belive. However, I'm not sure if we want to add circular dependency ;d
Hah, you're right. I was reading this top-to-bottom... Anyway, I've added another sentence, plus we have the extra description under the table in the manpage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 30 of 32 files reviewed, 2 unresolved discussions (waiting on @igchor and @pbalcer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r11.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lukaszstolarczuk and @pbalcer)
doc/ENGINES-experimental.md, line 125 at r11 (raw file):
+ type: string * **create_if_missing** -- If 1, pmemkv tries to open the pool and if that doesn't succeed, it creates it. If 0, pmemkv will rely on **create_or_error_if_exists** flag setting.
without "setting" sounds also good IMO, but it doesn't matter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lukaszstolarczuk and @pbalcer)
force_create
withcreate_or_error_if_exists
create_if_missing
Fixes #576
This change is