Introduce flat transaction class and change containers' behavior #932
Conversation
|
For now, I changed all occurrences of transaction:: to flat_transaction in our headers. This should allow using our containers (including segment_vector) by users without changing user's code nor behavior (apart from containers' method behavior). This change is problematic as we will have to always use flat_transaction everywhere in our code (some static analyzer for that?). The alternative is to use |
igchor
force-pushed
the
abort_on_failure_flat_tx
branch
from
53360b8
to
217ff73
Compare
Codecov Report
@@ Coverage Diff @@
## master #932 +/- ##
==========================================
- Coverage 96.09% 95.70% -0.40%
==========================================
Files 48 48
Lines 6255 6290 +35
==========================================
+ Hits 6011 6020 +9
- Misses 244 270 +26
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Reviewed 16 of 18 files at r1.
Reviewable status: 16 of 18 files reviewed, 11 unresolved discussions (waiting on @igchor)
README.md, line 18 at r1 (raw file):
# Compatibility note # In libpmemobj 1.12 we introduced a new transaction handler type: [pmem::obj::flat_transaction](https://pmem.io/libpmemobj-cpp/master/doxygen/classpmem_1_1obj_1_1flat__transaction.html). By defining LIBPMEMOBJ_CPP_USE_FLAT_TRANSACTION you can make pmem::obj::transaction to be an alias to pmem::obj::flat_transaction. In 1.12 we have also changed the default behavior of containers' transactional methods. Now, in case of any failure within such method, the outer transaction (if any) will not be immediately aborted. Instead, an exception will be thrown, which will lead to transaction abort only if it's not caught before tx scope ends. To change the behavior to the old one, you can set LIBPMEMOBJ_CPP_FLAT_TX_USE_FAILURE_RETURN macro to 0. Be aware that the old behavior can lead to segfaults in some cases (see tx_nested_struct_example in this [example](examples/transaction/transaction.cpp))
in this [file], perhaps
README.md, line 18 at r1 (raw file):
# Compatibility note # In libpmemobj 1.12 we introduced a new transaction handler type: [pmem::obj::flat_transaction](https://pmem.io/libpmemobj-cpp/master/doxygen/classpmem_1_1obj_1_1flat__transaction.html). By defining LIBPMEMOBJ_CPP_USE_FLAT_TRANSACTION you can make pmem::obj::transaction to be an alias to pmem::obj::flat_transaction. In 1.12 we have also changed the default behavior of containers' transactional methods. Now, in case of any failure within such method, the outer transaction (if any) will not be immediately aborted. Instead, an exception will be thrown, which will lead to transaction abort only if it's not caught before tx scope ends. To change the behavior to the old one, you can set LIBPMEMOBJ_CPP_FLAT_TX_USE_FAILURE_RETURN macro to 0. Be aware that the old behavior can lead to segfaults in some cases (see tx_nested_struct_example in this [example](examples/transaction/transaction.cpp))
pls add . at the end of the line
examples/transaction/transaction.cpp, line 293 at r1 (raw file):
auto pop = pool_base(pmemobj_pool_by_ptr(this)); /* Using a basic instead of flat one, would lead to an
flat one -> flat transaction
examples/transaction/transaction.cpp, line 294 at r1 (raw file):
/* Using a basic instead of flat one, would lead to an * application terminate. The reason is that allocating the char
terminate -> termination
examples/transaction/transaction.cpp, line 385 at r1 (raw file):
proot->count++; /* Even if there is no explcit commit inner_flat tx will
inner_flat? did you mean (flat) inner_tx will...?
examples/transaction/transaction.cpp, line 385 at r1 (raw file):
proot->count++; /* Even if there is no explcit commit inner_flat tx will
explcit -> explicit
include/libpmemobj++/README.md, line 80 at r1 (raw file):
* Resides on persistent memory property - [p](@ref pmem::obj::p) * Persistent smart pointer - [persistent_ptr](@ref pmem::obj::persistent_ptr) * Persistent memory transactions - [@ref pmem::obj::basic_transaction][@ref pmem::obj::flat_transaction]
you wanted to put 2 links here?
include/libpmemobj++/transaction.hpp, line 96 at r1 (raw file):
} if (IsFlat && LIBPMEMOBJ_CPP_FLAT_TX_USE_FAILURE_RETURN)
what's the behavior when it is not a flat transaction? can we change it using LIBPMEMOBJ_CPP_FLAT_TX_USE_FAILURE_RETURN ?
include/libpmemobj++/transaction.hpp, line 105 at r1 (raw file):
* * End pmemobj transaction. Abort the transaction if * the transaction is not 'flat' or this is outermost
the outermost
tests/transaction/transaction.cpp, line 1031 at r1 (raw file):
void test_tx_error_handling(nvobj::pool<root> &pop)
perhaps add a short test description, test name is rather non descriptive
tests/transaction/transaction_basic.cpp, line 19 at r1 (raw file):
* Test Recommendations. "_MSC_VER" is a workaround. */ #if _MSC_VER < 1900
perhaps you can move this into a sep. helper header for tx tests...?
There was a problem hiding this comment.
Reviewable status: 16 of 18 files reviewed, 11 unresolved discussions (waiting on @igchor and @lukaszstolarczuk)
include/libpmemobj++/README.md, line 80 at r1 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
you wanted to put 2 links here?
yes, but I'm not sure if this is ok. I may just link to the transaction page which has references to both basic and flat txs
include/libpmemobj++/transaction.hpp, line 96 at r1 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
what's the behavior when it is not a flat transaction? can we change it using
LIBPMEMOBJ_CPP_FLAT_TX_USE_FAILURE_RETURN?
The behavior is as it is now - each transactional method failure results in an abort. I don't think there is much sense in allowing anything different in such case because for non-flat transaction something like:
pmem::obj::transaction::run(pop, [&] {
make_persistent();
});
will abort the tx anyway due to the exception catched in run.
That said, the behavior can still be changed by using pmemobj_tx_set_failure_behavior but I'd prefer not to document that at the momenet.
There was a problem hiding this comment.
Reviewable status: 16 of 18 files reviewed, 10 unresolved discussions (waiting on @igchor)
include/libpmemobj++/README.md, line 80 at r1 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
yes, but I'm not sure if this is ok. I may just link to the transaction page which has references to both basic and flat txs
link to a transaction page should be fine, I guess.
If you want to put two links, just make it explicit [flat_tx](@ref pmem::obj::flat_tx) or [basic_tx](@ref pmem::obj::basic_tx)
There was a problem hiding this comment.
Reviewed 18 of 18 files at r1.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @igchor)
include/libpmemobj++/transaction.hpp, line 96 at r1 (raw file):
} if (IsFlat && LIBPMEMOBJ_CPP_FLAT_TX_USE_FAILURE_RETURN)
is_flat is better IMO
tests/transaction/transaction.cpp, line 1041 at r1 (raw file):
UT_ASSERT(0); } catch (pmem::transaction_scope_error &) {
remove empty line or comment
There was a problem hiding this comment.
Reviewed 17 of 18 files at r1.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @igchor)
examples/transaction/transaction.cpp, line 263 at r1 (raw file):
* basic_transaction). */ assert(proot->count == 2); throw;
in order to better illustrate this example, I would suggest adding a comment here that this is pmem::transaction_error
examples/transaction/transaction.cpp, line 272 at r1 (raw file):
} catch (...) { /* some other exception thrown, tx aborted * reacquire locks if necessary */
... and here maybe assert(false) since we do not expect to get there
examples/transaction/transaction.cpp, line 300 at r1 (raw file):
* an exception. This exception will be propagated to the * outermost transaction and only then the abort will happen. * This allows C++ to call ptr1 in A class destructor and unwind
what is a danger in this example by not unwinding the stack correctly? can we better illustrate the danger?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @igchor)
examples/transaction/transaction.cpp, line 300 at r1 (raw file):
Previously, szyrom (Szymon Romik) wrote…
what is a danger in this example by not unwinding the stack correctly? can we better illustrate the danger?
nvm, I understand now - maybe its a good idea to illustrate this
igchor
force-pushed
the
abort_on_failure_flat_tx
branch
from
217ff73
to
ff22a7f
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @KFilipek, @lukaszstolarczuk, and @szyrom)
README.md, line 18 at r1 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
in this [file], perhaps
Done.
README.md, line 18 at r1 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
pls add
.at the end of the line
Done.
examples/transaction/transaction.cpp, line 263 at r1 (raw file):
Previously, szyrom (Szymon Romik) wrote…
in order to better illustrate this example, I would suggest adding a comment here that this is pmem::transaction_error
It's std::runtime_exception (the one which we thrown above) but I wanted to make it a more general construct. In this particular case onle one example can be thrown, but in general the behavior is the same for all of them (hence I used ...).
examples/transaction/transaction.cpp, line 272 at r1 (raw file):
Previously, szyrom (Szymon Romik) wrote…
... and here maybe assert(false) since we do not expect to get there
Well, we don't expect it here, but in general this exception can happen (and this example shows most common error handling).
examples/transaction/transaction.cpp, line 293 at r1 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
flat one->flat transaction
Done.
examples/transaction/transaction.cpp, line 294 at r1 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
terminate -> termination
Done.
examples/transaction/transaction.cpp, line 300 at r1 (raw file):
Previously, szyrom (Szymon Romik) wrote…
nvm, I understand now - maybe its a good idea to illustrate this
Done.
examples/transaction/transaction.cpp, line 385 at r1 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
inner_flat? did you mean(flat) inner_tx will...?
Done.
examples/transaction/transaction.cpp, line 385 at r1 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
explcit -> explicit
Done.
include/libpmemobj++/README.md, line 80 at r1 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
link to a transaction page should be fine, I guess.
If you want to put two links, just make it explicit[flat_tx](@ref pmem::obj::flat_tx) or [basic_tx](@ref pmem::obj::basic_tx)
Done.
include/libpmemobj++/transaction.hpp, line 96 at r1 (raw file):
Previously, KFilipek (Krzysztof Filipek) wrote…
is_flat is better IMO
Done.
include/libpmemobj++/transaction.hpp, line 105 at r1 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
the outermost
Done.
tests/transaction/transaction.cpp, line 1031 at r1 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
perhaps add a short test description, test name is rather non descriptive
Done.
tests/transaction/transaction.cpp, line 1041 at r1 (raw file):
Previously, KFilipek (Krzysztof Filipek) wrote…
remove empty line or comment
Done.
tests/transaction/transaction_basic.cpp, line 19 at r1 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
perhaps you can move this into a sep. helper header for tx tests...?
Done.
igchor
force-pushed
the
abort_on_failure_flat_tx
branch
from
ff22a7f
to
eb222d3
Compare
There was a problem hiding this comment.
Reviewed 3 of 13 files at r2.
Reviewable status: 8 of 18 files reviewed, 25 unresolved discussions (waiting on @igchor, @KFilipek, @lukaszstolarczuk, and @szyrom)
README.md, line 18 at r2 (raw file):
# Compatibility note # In libpmemobj 1.12 we introduced a new transaction handler type: [pmem::obj::flat_transaction](https://pmem.io/libpmemobj-cpp/master/doxygen/classpmem_1_1obj_1_1flat__transaction.html). By defining LIBPMEMOBJ_CPP_USE_FLAT_TRANSACTION you can make pmem::obj::transaction to be an alias to pmem::obj::flat_transaction. In 1.12 we have also changed the default behavior of containers' transactional methods. Now, in case of any failure within such method, the outer transaction (if any) will not be immediately aborted. Instead, an exception will be thrown, which will lead to transaction abort only if it's not caught before tx scope ends. To change the behavior to the old one, you can set LIBPMEMOBJ_CPP_FLAT_TX_USE_FAILURE_RETURN macro to 0. Be aware that the old behavior can lead to segfaults in some cases (see tx_nested_struct_example in this [file](examples/transaction/transaction.cpp)).
perhaps make it explicit here it's not caught before **outer** tx scope ends
examples/transaction/transaction.cpp, line 267 at r2 (raw file):
}); } catch (pmem::transaction_error &) { /* an internal transaction error occurred, tx aborted
perhaps something more precise, like tx aborted -> outer tx aborted just now. (pls, also add . at the end)
examples/transaction/transaction.cpp, line 271 at r2 (raw file):
assert(proot->count == 0); } catch (...) { /* some other exception thrown, tx aborted
.
examples/transaction/transaction.cpp, line 324 at r2 (raw file):
auto pop = pool_base(pmemobj_pool_by_ptr(this)); // Will result in crash!
It would result in a crash!
examples/transaction/transaction.cpp, line 342 at r2 (raw file):
struct root { persistent_ptr<A> ptr1; persistent_ptr<B> ptr2;
perhaps rename to ptrA & ptrB
examples/transaction/transaction.cpp, line 353 at r2 (raw file):
try { // Will result in a crash!
It would
examples/transaction/transaction.cpp, line 376 at r2 (raw file):
* * If we'd use basic_transaction the allocation failure, apart * from throwing an exception would also cause the transaction
an exception , (this apart.. sentence was an inclusion, so it should be surrounded with commas, I believe)
examples/transaction/transaction.cpp, line 389 at r2 (raw file):
* means that the transaction is still in WORK stage during * stack unwinding. Only after it completes, the transaction is * aborted (it's happending at the outermost level, when exiting
happending misspell
examples/transaction/transaction.cpp, line 401 at r2 (raw file):
/* Running create_b can be done both within basic and flat * transaction. However, note that the transaction used in the B * constructor MUST be flat_transaction. This is because
be a flat_transaction
examples/transaction/transaction.cpp, line 404 at r2 (raw file):
performend
misspell
examples/transaction/transaction.cpp, line 405 at r2 (raw file):
* exception. Instead it passes it to the outermost transaction * - the abort is performend at that outermost level. In case of * basic_transaction the abort would be done within the B ctor
a basic_transaction
examples/transaction/transaction.cpp, line 458 at r2 (raw file):
* flat_transaction::manual is destroyed because of an * active exception. */
I've read this comment once more and I'm not sure if I get it ;-) flat_transaction::manual will not abort, even if some exception occur here... and it will abort just only in the outermost tx...?
examples/transaction/transaction.cpp, line 465 at r2 (raw file):
transaction::commit(); } catch (pmem::transaction_error &) { /* an internal transaction error occurred, tx aborted
.
examples/transaction/transaction.cpp, line 468 at r2 (raw file):
* reacquire locks if necessary */ } catch (...) { /* some other exception thrown, tx aborted
.
examples/transaction/transaction.cpp, line 473 at r2 (raw file):
/* In complex cases with library calls, remember to check the status of * the previous transaction. */
perhaps of the last transaction...?
include/libpmemobj++/transaction.hpp, line 573 at r2 (raw file):
/** * C++ transaction handler class. This class should be used with care. * It's recommended to use pmem::obj::flat_transaction.
to rather use or to use pmem::obj::flat_transaction instead.
include/libpmemobj++/transaction.hpp, line 828 at r2 (raw file):
* * To see what is the difference between the two please look at the examples: * @snippet transaction/transaction.cpp manual_tx_example
did you mean manual_flat_tx_example ? and it (by itself) doesn't show the difference, I guess
also, perhaps make this snippet the last one, the next two seems to be better examples
include/libpmemobj++/transaction.hpp, line 840 at r2 (raw file):
* * To see what is the difference between the two please look at the examples: * @snippet transaction/transaction.cpp manual_tx_example
.
tests/transaction/transaction.hpp, line 33 at r2 (raw file):
#endif /* __cpp_lib_uncaught_exceptions */ #endif /* _MSC_VER */ #endif /* LIBPMEMOBJ_CPP_TESTS_TRANSACTION */
no new line at the end
There was a problem hiding this comment.
Reviewable status: 8 of 18 files reviewed, 25 unresolved discussions (waiting on @igchor, @KFilipek, @lukaszstolarczuk, and @szyrom)
include/libpmemobj++/README.md, line 80 at r1 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
Done.
I've generated the docs and the link looks good.
igchor
force-pushed
the
abort_on_failure_flat_tx
branch
2 times, most recently
from
9852483
to
ee09d0e
Compare
There was a problem hiding this comment.
Reviewable status: 8 of 18 files reviewed, 23 unresolved discussions (waiting on @KFilipek, @lukaszstolarczuk, and @szyrom)
examples/transaction/transaction.cpp, line 267 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
perhaps something more precise, like
tx aborted->outer tx aborted just now.(pls, also add.at the end)
Hm, I'm not sure if "outer tx aborted" is
examples/transaction/transaction.cpp, line 271 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
.
Done.
examples/transaction/transaction.cpp, line 324 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
It would result in a crash!
Done.
examples/transaction/transaction.cpp, line 342 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
perhaps rename to
ptrA&ptrB
Done.
examples/transaction/transaction.cpp, line 353 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
It would
Done.
examples/transaction/transaction.cpp, line 376 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
an exception , (this
apart..sentence was an inclusion, so it should be surrounded with commas, I believe)
Done.
examples/transaction/transaction.cpp, line 389 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
happendingmisspell
Done.
examples/transaction/transaction.cpp, line 401 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
be a flat_transaction
Done.
examples/transaction/transaction.cpp, line 404 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
performendmisspell
Done.
examples/transaction/transaction.cpp, line 405 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
a basic_transaction
Done.
examples/transaction/transaction.cpp, line 458 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
I've read this comment once more and I'm not sure if I get it ;-) flat_transaction::manual will not abort, even if some exception occur here... and it will abort just only in the outermost tx...?
I've added some extra info
examples/transaction/transaction.cpp, line 465 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
.
Done.
examples/transaction/transaction.cpp, line 468 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
.
Done.
include/libpmemobj++/transaction.hpp, line 573 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
to rather useorto use pmem::obj::flat_transaction instead.
Done.
include/libpmemobj++/transaction.hpp, line 828 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
did you mean
manual_flat_tx_example? and it (by itself) doesn't show the difference, I guessalso, perhaps make this snippet the last one, the next two seems to be better examples
Done.
include/libpmemobj++/transaction.hpp, line 840 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
.
Done.
tests/transaction/transaction.hpp, line 33 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
no new line at the end
Done.
igchor
force-pushed
the
abort_on_failure_flat_tx
branch
from
ee09d0e
to
fb04acc
Compare
There was a problem hiding this comment.
Reviewed 2 of 5 files at r3.
Reviewable status: 8 of 18 files reviewed, 26 unresolved discussions (waiting on @igchor, @KFilipek, @lukaszstolarczuk, and @szyrom)
examples/transaction/transaction.cpp, line 261 at r3 (raw file):
} catch (...) { /* Transaction is not aborted yet (unlike for * basic_transaction). */
assert(tx_state() == WORK);
? (not sure what's the C++ syntax)
examples/transaction/transaction.cpp, line 461 at r3 (raw file):
* there are manual transaction objects). In case of * flat_transaction, the commit has to be called only * once, at the outermost level.
I don't like this distinction.
Can the commit be called at every level? If not, then this makes code nesting difficult.
include/libpmemobj++/transaction.hpp, line 96 at r3 (raw file):
} if (is_flat &&
can't you just abstract this away somehow? having an if class = this {} else {} seems odd ;)
igchor
force-pushed
the
abort_on_failure_flat_tx
branch
from
fb04acc
to
6783d98
Compare
There was a problem hiding this comment.
Reviewable status: 6 of 18 files reviewed, 26 unresolved discussions (waiting on @KFilipek, @lukaszstolarczuk, @pbalcer, and @szyrom)
examples/transaction/transaction.cpp, line 261 at r3 (raw file):
Previously, pbalcer (Piotr Balcer) wrote…
assert(tx_state() == WORK);
? (not sure what's the C++ syntax)
Done.
examples/transaction/transaction.cpp, line 461 at r3 (raw file):
Previously, pbalcer (Piotr Balcer) wrote…
I don't like this distinction.
Can the commit be called at every level? If not, then this makes code nesting difficult.
You can call commit at each level - you just don't have to. I updated the comment to make it more clear.
include/libpmemobj++/transaction.hpp, line 96 at r3 (raw file):
Previously, pbalcer (Piotr Balcer) wrote…
can't you just abstract this away somehow? having an
if class = this {} else {}seems odd ;)
Done. It' still basically an if, but not in compile time ;d Or did you mean some other approach?
There was a problem hiding this comment.
Reviewable status: 6 of 18 files reviewed, 24 unresolved discussions (waiting on @igchor, @KFilipek, @lukaszstolarczuk, @pbalcer, and @szyrom)
include/libpmemobj++/transaction.hpp, line 96 at r3 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
Done. It' still basically an if, but not in compile time ;d Or did you mean some other approach?
I was thinking more about just having an empty virtual function in the base class, and implementing this in the flat_transaction class.
There was a problem hiding this comment.
Reviewed 8 of 13 files at r2, 3 of 5 files at r3, 2 of 2 files at r4.
Reviewable status: all files reviewed, 26 unresolved discussions (waiting on @igchor, @lukaszstolarczuk, and @szyrom)
examples/transaction/transaction.cpp, line 355 at r4 (raw file):
try { // It would result in a crash! // basic_transaction::run(pop, create_a);
I think it will be removed in the future
tests/transaction/transaction_basic.cpp, line 10 at r4 (raw file):
#include <libpmemobj++/pool.hpp> #include "transaction.hpp"
why not #include <libpmemobj++/transaction.hpp>?
tests/transaction/transaction_flat.cpp, line 7 at r4 (raw file):
Quoted 4 lines of code…
#include <libpmemobj/iterator_base.h> #include <libpmemobj++/make_persistent.hpp> #include <libpmemobj++/persistent_ptr.hpp> #include <libpmemobj++/pool.hpp>
Please reorder the headers to looks like above
tests/transaction/transaction_flat.cpp, line 12 at r4 (raw file):
#include <libpmemobj/iterator_base.h> #include "transaction.hpp"
why not #include <libpmemobj++/transaction.hpp>?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 26 unresolved discussions (waiting on @igchor, @lukaszstolarczuk, @pbalcer, and @szyrom)
include/libpmemobj++/transaction.hpp, line 96 at r3 (raw file):
Previously, pbalcer (Piotr Balcer) wrote…
I was thinking more about just having an empty virtual function in the base class, and implementing this in the flat_transaction class.
For that, I'd have to actually implement a base class for flat_transaction::manual (I can't call virtual function on transaction_base which is an outer class). And it would also broke run() method from transaction_base class. There, I use manual class directly which is always transaction_base::manual. I'd have to add some factory function or just re implement run() in flat_transaction class. I'm not sure if it's worth it ... and it's probably slightly faster this way ;d
igchor
force-pushed
the
abort_on_failure_flat_tx
branch
from
6783d98
to
3fe34ae
Compare
There was a problem hiding this comment.
Reviewable status: 17 of 18 files reviewed, 26 unresolved discussions (waiting on @KFilipek, @lukaszstolarczuk, @pbalcer, and @szyrom)
examples/transaction/transaction.cpp, line 355 at r4 (raw file):
Previously, KFilipek (Krzysztof Filipek) wrote…
I think it will be removed in the future
Probably not until libpmemobj-cpp 2.0
tests/transaction/transaction_basic.cpp, line 10 at r4 (raw file):
Previously, KFilipek (Krzysztof Filipek) wrote…
why not #include <libpmemobj++/transaction.hpp>?
Because it;s different file, I'm including the tests/transaction/transaction.hpp here
tests/transaction/transaction_flat.cpp, line 7 at r4 (raw file):
Previously, KFilipek (Krzysztof Filipek) wrote…
#include <libpmemobj/iterator_base.h> #include <libpmemobj++/make_persistent.hpp> #include <libpmemobj++/persistent_ptr.hpp> #include <libpmemobj++/pool.hpp>Please reorder the headers to looks like above
Done. (but with the ordering enforced of clang-format)
tests/transaction/transaction_flat.cpp, line 12 at r4 (raw file):
Previously, KFilipek (Krzysztof Filipek) wrote…
why not #include <libpmemobj++/transaction.hpp>?
.
There was a problem hiding this comment.
Reviewed 5 of 13 files at r2, 3 of 5 files at r3, 1 of 2 files at r4.
Reviewable status: 17 of 18 files reviewed, 13 unresolved discussions (waiting on @igchor, @KFilipek, @pbalcer, and @szyrom)
examples/transaction/transaction.cpp, line 267 at r2 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
Hm, I'm not sure if "outer tx aborted" is
you're not sure... what? it looks like you didn't end the sentence above
examples/transaction/transaction.cpp, line 458 at r2 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
I've added some extra info
Thanks!
examples/transaction/transaction.cpp, line 464 at r5 (raw file):
* have to call commit() at each level (as many times as * there are manual transaction objects). In case of * flat_transaction, the commit has to be called only
a flat_transaction
tests/transaction/transaction.cpp, line 18 at r5 (raw file):
#include <libpmemobj++/shared_mutex.hpp> #include "transaction.hpp"
you can have this include just next to "unittest.hpp"
tests/transaction/transaction_basic.cpp, line 346 at r5 (raw file):
UT_ASSERT(r->p1 == nullptr); } void
pls add an empty line
tests/transaction/transaction_basic.cpp, line 363 at r5 (raw file):
test_tx_nested_behavior_scope<nvobj::transaction::manual, nvobj::flat_transaction::manual>(pop); test_tx_nested_behavior_scope<nvobj::transaction::automatic,
isn't automatic supported only from some C++ standard (17) ?
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @igchor, @pbalcer, and @szyrom)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @igchor, @lukaszstolarczuk, @pbalcer, and @szyrom)
examples/transaction/transaction.cpp, line 464 at r5 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
a flat_transaction
Not 'a', because 'the' flat_transaction was mentioned earlier
There was a problem hiding this comment.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @igchor, @KFilipek, @pbalcer, and @szyrom)
examples/transaction/transaction.cpp, line 464 at r5 (raw file):
Previously, KFilipek (Krzysztof Filipek) wrote…
Not 'a', because 'the' flat_transaction was mentioned earlier
I disagree
- I can't see that we've mentioned "flat_tx" class in this section
- it is any flat_tx, not the one specific instance or something
hence, I'd put here "a flat_tx"
Rename pmem::obj::transaction to pmem::obj::basic_transaction and make transaction a macro. The common code between basic_transaction and flat_transaction is moved to a base class. All the doxygen comments are placed next to the actual transaction implementation.
They will work after introducing flat_transaction.
and changing containers' transactional methods behavior.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @lukaszstolarczuk, @pbalcer, and @szyrom)
examples/transaction/transaction.cpp, line 267 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
you're not sure... what? it looks like you didn't end the sentence above
I think I just didn't remove my previous answer. It's ok now, right?
examples/transaction/transaction.cpp, line 464 at r5 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
I disagree
- I can't see that we've mentioned "flat_tx" class in this section
- it is any flat_tx, not the one specific instance or something
hence, I'd put here "a flat_tx"
Done.
tests/transaction/transaction.cpp, line 18 at r5 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
you can have this include just next to
"unittest.hpp"
Done.
tests/transaction/transaction_basic.cpp, line 346 at r5 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
pls add an empty line
Done.
tests/transaction/transaction_basic.cpp, line 363 at r5 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
isn't automatic supported only from some C++ standard (17) ?
Yes, because of std::uncaught_exceptions, but in the test, for previous C++ versions we just mock the uncaught_exceptions (see tests/transaction/transaction.hpp)
igchor
force-pushed
the
abort_on_failure_flat_tx
branch
from
3fe34ae
to
98c3f96
Compare
There was a problem hiding this comment.
Reviewed 2 of 4 files at r6.
Reviewable status: 16 of 18 files reviewed, 4 unresolved discussions (waiting on @KFilipek, @pbalcer, and @szyrom)
tests/transaction/transaction_basic.cpp, line 363 at r5 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
Yes, because of std::uncaught_exceptions, but in the test, for previous C++ versions we just mock the uncaught_exceptions (see tests/transaction/transaction.hpp)
och, ok. I thought we should ifdef it.
There was a problem hiding this comment.
Reviewed 6 of 18 files at r1, 4 of 13 files at r2, 2 of 5 files at r3, 1 of 4 files at r6.
Reviewable status: 16 of 18 files reviewed, 4 unresolved discussions (waiting on @KFilipek, @pbalcer, and @szyrom)
There was a problem hiding this comment.
Reviewable status: 16 of 18 files reviewed, 3 unresolved discussions (waiting on @KFilipek and @szyrom)
There was a problem hiding this comment.
Reviewed 2 of 4 files at r6.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @szyrom)
There was a problem hiding this comment.
Reviewed 2 of 4 files at r6.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @szyrom)
By using flat_transaction user can disable automatic tx abort in case of
any failure. Error handling can be then done using exceptions.
If an exception is caught inside of the transaction then the transaction
simply commits. Otherwise, exception is ignored by all inner transactions
and propagated to the outer one which calls pmemobj_tx_abort().
This patch also changes the behavior of all containers' transnational methods.
Now, they do not cause an abort on failure but only throw an exception. This
can be changed by setting a macro.
Requires: pmem/pmdk#4654
Closes #516
This change is