transaction: expose API to control transaction behaviour on error #723
Conversation
igchor
force-pushed
the
abort_on_failure
branch
3 times, most recently
from
abf0817
to
a7e258a
Compare
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1.
Reviewable status:complete! all files reviewed, all discussions resolved
Codecov Report
@@ Coverage Diff @@
## master #723 +/- ##
==========================================
- Coverage 95.92% 95.78% -0.14%
==========================================
Files 48 48
Lines 5858 6008 +150
==========================================
+ Hits 5619 5755 +136
- Misses 239 253 +14
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
igchor
force-pushed
the
abort_on_failure
branch
2 times, most recently
from
0600b08
to
d548bfe
Compare
There was a problem hiding this comment.
Reviewable status: 3 of 5 files reviewed, 3 unresolved discussions (waiting on @igchor and @KFilipek)
include/libpmemobj++/transaction.hpp, line 97 at r2 (raw file):
private: failure_behavior failure_behavior_ = failure_behavior::LIBPMEMOBJ_CPP_TX_FAILURE_EVENT;
Setting this default parameter compile time, and exposing setter to change this in runtime may cause user's confusion.
Why not left default value always return_error, and left to user explicitly call setter if need to change it?
include/libpmemobj++/transaction.hpp, line 144 at r2 (raw file):
Quoted 4 lines of code…
if (opts.failure_behavior_ == failure_behavior::abort && pmemobj_tx_get_failure_behavior() == POBJ_TX_FAILURE_RETURN)
if( opts.failure_bahavior != pmemovb_tx_get_failure_bahavior) ??
include/libpmemobj++/transaction.hpp, line 168 at r2 (raw file):
pmemobj_tx_set_failure_behavior( (pobj_tx_failure_behavior)
why you need to cast here?
|
shouldn't we require PMDK 1.9 for this to work...? |
There was a problem hiding this comment.
Reviewable status: 3 of 5 files reviewed, 3 unresolved discussions (waiting on @karczex and @KFilipek)
include/libpmemobj++/transaction.hpp, line 97 at r2 (raw file):
Previously, karczex wrote…
Setting this default parameter compile time, and exposing setter to change this in runtime may cause user's confusion.
Why not left default value alwaysreturn_error, and left to user explicitly call setter if need to change it?
The problem is backward compatibility. If we set this to return_error by default some old code might stop working. On the other hand, if we set it to abort by default we would have to manually use options for all our internal transaction starting. With the macro approach we also break the compatibility, but it's quite easy for user to opt-out - he just needs to add one define when compiling.
include/libpmemobj++/transaction.hpp, line 144 at r2 (raw file):
Previously, karczex wrote…
if (opts.failure_behavior_ == failure_behavior::abort && pmemobj_tx_get_failure_behavior() == POBJ_TX_FAILURE_RETURN)if( opts.failure_bahavior != pmemovb_tx_get_failure_bahavior) ??
No, it's prefectly fine if you want to start "failure return" transaction inside "failure abort" transaction. The problem is only the other way around because the internal transaction would cause abort and that's conflicting with the outer one behavior. (Remember that transactions are flattened-out to if there is an abort in internal tx, it will also cause external tx abort).
include/libpmemobj++/transaction.hpp, line 168 at r2 (raw file):
Previously, karczex wrote…
why you need to cast here?
Because opts.failure_bahvior_is my enum and not C type.
There was a problem hiding this comment.
Yes, done.
Reviewable status: 3 of 7 files reviewed, 3 unresolved discussions (waiting on @karczex and @KFilipek)
igchor
force-pushed
the
abort_on_failure
branch
2 times, most recently
from
6b4dbe4
to
86c6ede
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @igchor and @KFilipek)
include/libpmemobj++/transaction.hpp, line 97 at r2 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
The problem is backward compatibility. If we set this to return_error by default some old code might stop working. On the other hand, if we set it to abort by default we would have to manually use options for all our internal transaction starting. With the macro approach we also break the compatibility, but it's quite easy for user to opt-out - he just needs to add one define when compiling.
As we talked offline:
Consider to left default parameter to be abort with deprecation warning.
igchor
force-pushed
the
abort_on_failure
branch
2 times, most recently
from
83dfcd4
to
7928721
Compare
There was a problem hiding this comment.
Reviewed 1 of 2 files at r2, 2 of 5 files at r3.
Reviewable status: 3 of 7 files reviewed, 7 unresolved discussions (waiting on @igchor and @KFilipek)
include/libpmemobj++/transaction.hpp, line 78 at r3 (raw file):
class manual; /** Specifies failure event in case of transaction error */
of a transaction..
include/libpmemobj++/transaction.hpp, line 95 at r3 (raw file):
*/ struct options { /** Controls behavior of transaction in case of errors. It can
of a transaction
include/libpmemobj++/transaction.hpp, line 96 at r3 (raw file):
struct options { /** Controls behavior of transaction in case of errors. It can * be failure_behavior::abort or failure_behavior::return_error.
add one space before the final . - when they are together, doxygen may not create the link
include/libpmemobj++/transaction.hpp, line 285 at r3 (raw file):
* * @param[in,out] pop pool object. * @param[in] opts options for controlling transaction behavior
add . at the end
include/libpmemobj++/transaction.hpp, line 298 at r3 (raw file):
} template <typename... L>
add description...?
include/libpmemobj++/transaction.hpp, line 461 at r3 (raw file):
} template <typename... Locks>
add desc...?
There was a problem hiding this comment.
Reviewable status: 3 of 7 files reviewed, 7 unresolved discussions (waiting on @karczex, @KFilipek, and @lukaszstolarczuk)
include/libpmemobj++/transaction.hpp, line 97 at r2 (raw file):
Previously, karczex wrote…
As we talked offline:
Consider to left default parameter to be abort with deprecation warning.
Decided to go with the current approach
include/libpmemobj++/transaction.hpp, line 78 at r3 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
of a transaction..
Done.
include/libpmemobj++/transaction.hpp, line 95 at r3 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
of a transaction
Done.
include/libpmemobj++/transaction.hpp, line 96 at r3 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
add one space before the final
.- when they are together, doxygen may not create the link
Done.
include/libpmemobj++/transaction.hpp, line 285 at r3 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
add
.at the end
Done.
include/libpmemobj++/transaction.hpp, line 298 at r3 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
add description...?
Done.
include/libpmemobj++/transaction.hpp, line 461 at r3 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
add desc...?
I initially wanted to have a description only for version with options. But even though I wrote that options param is optional it could still be misleading so: done.
igchor
force-pushed
the
abort_on_failure
branch
from
7928721
to
5f1a4e9
Compare
igchor
force-pushed
the
abort_on_failure
branch
from
5f1a4e9
to
6298f6f
Compare
With this patch, 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().
igchor
force-pushed
the
abort_on_failure
branch
from
6298f6f
to
8073cce
Compare
There was a problem hiding this comment.
Reviewed 2 of 5 files at r3, 4 of 4 files at r4.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @igchor, @karczex, and @lukaszstolarczuk)
include/libpmemobj++/transaction.hpp, line 154 at r4 (raw file):
POBJ_TX_FAILURE_RETURN) throw pmem::transaction_error( "Cannot start transaction with failure_behavior::abort. Outer transaction was configured with failure_behavior::return_error");
Need to discuss how to deal with cases when nested transaction is opened in a third-party library.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r2.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @igchor, @karczex, and @lukaszstolarczuk)
|
Redone #932 |
With this patch, 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().
Requires: pmem/pmdk#4654
Closes #516
This change is