Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

obj: introduce pmemobj_tx_set_abort_on_failure #4654

Merged
merged 1 commit into from
May 4, 2020

Conversation

igchor
Copy link
Contributor

@igchor igchor commented Mar 16, 2020

This function can be used to disable or enable aborting tx
in case of an error (for example when allocation fails).

This patch also introduces pmemobj_tx_is_abort_on_failure
which allows to query current setting of a transaction.


This change is Reviewable

@pbalcer
Copy link
Member

pbalcer commented Mar 16, 2020

Maybe we should do this generically with some pmemobj_tx_flags() ?

Copy link
Contributor Author

@igchor igchor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also thinking about some pmemobj_tx_set_policy function but @marcinslusarz suggested to just add a function.

Reviewable status: 0 of 1 files reviewed, all discussions resolved

@igchor igchor force-pushed the abort_on_failure branch 2 times, most recently from 510596a to 33d6373 Compare March 17, 2020 15:10
@igchor igchor marked this pull request as ready for review March 17, 2020 15:33
@igchor igchor force-pushed the abort_on_failure branch 2 times, most recently from 2379952 to 66e7940 Compare March 18, 2020 16:13
@igchor igchor changed the title obj: introduce pmemobj_tx_abort_on_failure obj: introduce pmemobj_tx_set_abort_on_failure Mar 18, 2020
@codecov-io
Copy link

codecov-io commented Mar 19, 2020

Codecov Report

Merging #4654 into master will increase coverage by 0.23%.
The diff coverage is 78.78%.

@@            Coverage Diff             @@
##           master    #4654      +/-   ##
==========================================
+ Coverage   74.51%   74.75%   +0.23%     
==========================================
  Files         167      169       +2     
  Lines       27414    27665     +251     
==========================================
+ Hits        20428    20680     +252     
+ Misses       6986     6985       -1     

@@ -412,6 +418,19 @@ If **pmemobj_tx_set_user_data**() was not called for a current transaction,
during **TX_STAGE_WORK** or **TX_STAGE_ONABORT** or **TX_STAGE_ONCOMMIT** or
**TX_STAGE_FINALLY**.

**pmemobj_tx_set_abort_on_failure**() enables or disables automatic transaction
abort in case **pmemobj_tx_add_rage**, **pmemobj_tx_add_rage_direct**, **pmemobj_tx_alloc**,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will have to remember to update this list. I'm not sure if it wouldn't make more sense to just write all transactional functions except for X, Y and Z.

ASSERT_IN_TX(tx);
ASSERT_TX_STAGE_WORK(tx);

struct tx_data *txd = PMDK_SLIST_FIRST(&tx->tx_entries);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this pattern is repeated multiple times, wrap it into a function:
flags |= tx_abort_flag(tx);

Copy link

@szyrom szyrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 32 of 32 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @igchor)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 422 at r1 (raw file):

**pmemobj_tx_set_abort_on_failure**() enables or disables automatic transaction
abort in case **pmemobj_tx_add_rage**, **pmemobj_tx_add_rage_direct**, **pmemobj_tx_alloc**,

add rage? :)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 428 at r1 (raw file):

**pmemobj_tx_set_abort_on_failure** is not called (or called with **enable** == 1),
the transaction will abort if any of the previously listed functions fails (unless
*_NO_ABORT flag was passed expliciltly). This functions affects the current transaction

s/This/These

Copy link
Contributor Author

@igchor igchor left a 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, 4 unresolved discussions (waiting on @igchor, @pbalcer, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 422 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

We will have to remember to update this list. I'm not sure if it wouldn't make more sense to just write all transactional functions except for X, Y and Z.

I have changed it slightly diffrently. If you want I can do what you suggested but I'm not sure if "all transactional functions" is not too broad. What about tx_errno? tx_commit? tx_process? (there quite a few of those functions).


doc/libpmemobj/pmemobj_tx_begin.3.md, line 422 at r1 (raw file):

Previously, szyrom (Szymon Romik) wrote…

add rage? :)

reworked :)


src/libpmemobj/tx.c, line 836 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

this pattern is repeated multiple times, wrap it into a function:
flags |= tx_abort_flag(tx);

Done, I'm just not sure about the name

Copy link
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 32 files at r1, 1 of 2 files at r2.
Reviewable status: 31 of 32 files reviewed, 11 unresolved discussions (waiting on @igchor, @pbalcer, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 11 at r2 (raw file):

[comment]: <> (SPDX-License-Identifier: BSD-3-Clause)
[comment]: <> (Copyright 2017-2019, Intel Corporation)

-2020


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r2 (raw file):

**pmemobj_tx_set_abort_on_failure**() enables or disables automatic transaction
abort in case any transactional function accepting NO_ABORT flag fails. If called with
**enable** == 0 *_NO_ABORT flag will be implicitly passed to all of those functions. If
  1. shouldn't this asterisk be escaped?
  2. a .. flag?

doc/libpmemobj/pmemobj_tx_begin.3.md, line 426 at r2 (raw file):

**pmemobj_tx_set_abort_on_failure** is not called (or called with **enable** == 1),
the transaction will abort if any of the above mentioned functions fails (unless
*_NO_ABORT flag was passed expliciltly). This functions affects the current transaction

\* ?


doc/libpmemobj/pmemobj_tx_begin.3.md, line 427 at r2 (raw file):

the transaction will abort if any of the above mentioned functions fails (unless
*_NO_ABORT flag was passed expliciltly). This functions affects the current transaction
and all inner transactions. It does not affect any outer transactions.

any of outer tx's


src/libpmemobj/libpmemobj.def, line 3 at r2 (raw file):

;;;; Begin Copyright Notice
; SPDX-License-Identifier: BSD-3-Clause
; Copyright 2015-2018, Intel Corporation

-2020


src/libpmemobj/libpmemobj.link.in, line 2 at r2 (raw file):

# SPDX-License-Identifier: BSD-3-Clause
# Copyright 2014-2019, Intel Corporation

.


src/test/obj_basic_integration/obj_basic_integration.c, line 532 at r2 (raw file):

	} TX_END

	UT_OUT("%s", pmemobj_errormsg());

twice the same? is it expected?

Copy link
Contributor Author

@igchor igchor left a 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, 11 unresolved discussions (waiting on @lukaszstolarczuk, @pbalcer, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 428 at r1 (raw file):

Previously, szyrom (Szymon Romik) wrote…

s/This/These

it was the other way around - it should be just one function, not functions.


doc/libpmemobj/pmemobj_tx_begin.3.md, line 11 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

-2020

Done.


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
  1. shouldn't this asterisk be escaped?
  2. a .. flag?

Done. (I removed the asterisk)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 426 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

\* ?

Done.


doc/libpmemobj/pmemobj_tx_begin.3.md, line 427 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

any of outer tx's

Done.


src/libpmemobj/libpmemobj.def, line 3 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

-2020

Done.


src/libpmemobj/libpmemobj.link.in, line 2 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

.

Done.


src/test/obj_basic_integration/obj_basic_integration.c, line 532 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

twice the same? is it expected?

It should be have benn above. Done

Copy link
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 32 files at r1, 1 of 2 files at r2, 4 of 4 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @igchor, @pbalcer, and @szyrom)


src/include/libpmemobj/tx_base.h, line 429 at r3 (raw file):

/*
 * Enables or disables automatic transaction abort in case of an error.
 * If called with enable == 0 _NO_ABORT flag will be implicitly passed to

for consistency with manpage:
a NO_ABORT flag

Copy link
Member

@pbalcer pbalcer left a 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, 5 unresolved discussions (waiting on @igchor and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 428 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

it was the other way around - it should be just one function, not functions.

I don't understand what are you trying to say. Which functions?

I think this should say "this function affects"


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

**pmemobj_tx_set_abort_on_failure**() enables or disables automatic transaction
abort in case any transactional function accepting NO_ABORT flag fails. If called with
**enable** == 0 a NO_ABORT flag will be implicitly passed to all of those functions. If

So this is a negation? This is bizzare. I'd prefer to change the variable name to disabled.
Maybe we should change the name pmemobj_tx_disable_abort_on_failure ?


src/test/obj_basic_integration/obj_basic_integration.c, line 515 at r3 (raw file):

	errno = 0;
	TX_BEGIN(pop) {
		TX_BEGIN((PMEMobjpool *)(uintptr_t)7) {

this is better suited to tx flow test.

Copy link
Contributor Author

@igchor igchor left a 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, 5 unresolved discussions (waiting on @lukaszstolarczuk, @pbalcer, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 428 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

I don't understand what are you trying to say. Which functions?

I think this should say "this function affects"

Yes, exactly, I accidentally didn't push the changes


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

So this is a negation? This is bizzare. I'd prefer to change the variable name to disabled.
Maybe we should change the name pmemobj_tx_disable_abort_on_failure ?

Well, this will always be a negation one way or the other. If I change it to pmemobj_tx_disable_abort_on_failure and you want to enable the tx abort you have to call pmemobj_tx_disable_abort_on_failure(0). Alternatively we could make something like pmemobj_tx_set_abort_policy (enum policy {enable, disable}) but I'm not sure - we only have 2 posibilities.


src/include/libpmemobj/tx_base.h, line 429 at r3 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

for consistency with manpage:
a NO_ABORT flag

Done.


src/test/obj_basic_integration/obj_basic_integration.c, line 515 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

this is better suited to tx flow test.

Done.

Copy link
Member

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 15 of 18 files reviewed, 4 unresolved discussions (waiting on @igchor, @lukaszstolarczuk, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Well, this will always be a negation one way or the other. If I change it to pmemobj_tx_disable_abort_on_failure and you want to enable the tx abort you have to call pmemobj_tx_disable_abort_on_failure(0). Alternatively we could make something like pmemobj_tx_set_abort_policy (enum policy {enable, disable}) but I'm not sure - we only have 2 posibilities.

pmemobj_tx_disable_abort_on_failure() and pmemobj_tx_enable_abort_on_failure() ?
As is, I think I'd genuinely prefer the enum option.

Copy link
Contributor

@marcinslusarz marcinslusarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 15 of 18 files reviewed, 4 unresolved discussions (waiting on @lukaszstolarczuk, @pbalcer, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

pmemobj_tx_disable_abort_on_failure() and pmemobj_tx_enable_abort_on_failure() ?
As is, I think I'd genuinely prefer the enum option.

What's the difference between pmemobj_tx_set_abort_on_failure and pmemobj_tx_set_abort_policy? I don't see any, other than abort_policy being a too vague term to guess what it is about.

To me pmemobj_tx_disable_abort_on_failure + pmemobj_tx_enable_abort_on_failure is as expressive (or as bad ;) ) as pmemobj_tx_set_abort_on_failure.

How about this:

enum pobj_tx_action_on_failure {
  POBJ_TX_ABORT,
  POBJ_RETURN_ERROR,
};

void pmemobj_tx_set_action_on_failure(enum pobj_tx_action_on_failure action);

enum pobj_tx_action_on_failure pmemobj_tx_get_action_on_failure(void);

Copy link
Contributor Author

@igchor igchor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 15 of 18 files reviewed, 4 unresolved discussions (waiting on @lukaszstolarczuk, @pbalcer, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

What's the difference between pmemobj_tx_set_abort_on_failure and pmemobj_tx_set_abort_policy? I don't see any, other than abort_policy being a too vague term to guess what it is about.

To me pmemobj_tx_disable_abort_on_failure + pmemobj_tx_enable_abort_on_failure is as expressive (or as bad ;) ) as pmemobj_tx_set_abort_on_failure.

How about this:

enum pobj_tx_action_on_failure {
  POBJ_TX_ABORT,
  POBJ_RETURN_ERROR,
};

void pmemobj_tx_set_action_on_failure(enum pobj_tx_action_on_failure action);

enum pobj_tx_action_on_failure pmemobj_tx_get_action_on_failure(void);

I like what Marcin suggested, I can implement that.

Copy link
Member

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 15 of 18 files reviewed, 4 unresolved discussions (waiting on @igchor, @lukaszstolarczuk, @marcinslusarz, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

I like what Marcin suggested, I can implement that.

mhm, that would be better. I think we need to come up with better names though.

tx_set_on_failure();
tx_get_on_failure();

enum pobj_tx_on_failure {
  POBJ_TX_ABORT_ON_FAILURE,
  POBJ_TX_ERR_ON_FAILURE,
};

?

I know this makes the API a little bit less obvious, but it's less verbose this way.

Copy link
Contributor Author

@igchor igchor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 15 of 18 files reviewed, 4 unresolved discussions (waiting on @lukaszstolarczuk, @pbalcer, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

mhm, that would be better. I think we need to come up with better names though.

tx_set_on_failure();
tx_get_on_failure();

enum pobj_tx_on_failure {
  POBJ_TX_ABORT_ON_FAILURE,
  POBJ_TX_ERR_ON_FAILURE,
};

?

I know this makes the API a little bit less obvious, but it's less verbose this way.

Hm, I think I prefer the version with "action" in function name. In your version, there is ON_FAILURE both in function name and enum so it's still quite verbose. One thing I'm not sure about is POBJ_TX_ABORT. What this option effectively does is it makes all functions behave "normally" (if NO_ABORT flag is passed explicitly then it will return error, otherwsie it will abort). So maybe it should be something like "POBJ_DEFAULT"?

Copy link
Member

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 15 of 18 files reviewed, 4 unresolved discussions (waiting on @igchor, @lukaszstolarczuk, @marcinslusarz, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Hm, I think I prefer the version with "action" in function name. In your version, there is ON_FAILURE both in function name and enum so it's still quite verbose. One thing I'm not sure about is POBJ_TX_ABORT. What this option effectively does is it makes all functions behave "normally" (if NO_ABORT flag is passed explicitly then it will return error, otherwsie it will abort). So maybe it should be something like "POBJ_DEFAULT"?

We can drop the on_failure.
I don't like the DEFAULT enum :P We can write that in docs/comment.

POBJ_TX_RETURN_ON_FAILURE and POBJ_TX_ABORT_ON_FAILURE ?

I don't know... :P

Copy link
Contributor

@marcinslusarz marcinslusarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 15 of 18 files reviewed, 4 unresolved discussions (waiting on @lukaszstolarczuk, @pbalcer, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

We can drop the on_failure.
I don't like the DEFAULT enum :P We can write that in docs/comment.

POBJ_TX_RETURN_ON_FAILURE and POBJ_TX_ABORT_ON_FAILURE ?

I don't know... :P

I agree DEFAULT is bad - it doesn't say what it is.

BTW, TX_ABORT also means returning an error where jmp buffer is not used, so you have to be careful with the names.

@marcinslusarz marcinslusarz added the libpmemobj src/libpmemobj label Mar 31, 2020
Copy link
Contributor Author

@igchor igchor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 18 files reviewed, 4 unresolved discussions (waiting on @lukaszstolarczuk, @pbalcer, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Ok, for now I used the Marcin's proposal. If you want I can change that to:

pmemobj_tx_set_on_failure(POBJ_TX_RETURN_ON_FAILURE)

but it's even longer than:

pmemobj_tx_set_action_on_failure(POBJ_TX_ABORT);

So, @marcinslusarz @pbalcer can it stay that way? Or you prefer to change it?

Copy link
Member

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 18 files reviewed, 4 unresolved discussions (waiting on @lukaszstolarczuk, @marcinslusarz, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

So, @marcinslusarz @pbalcer can it stay that way? Or you prefer to change it?

I'd prefer to change this to:

enum pobj_tx_on_failure {
	POBJ_TX_ON_FAILURE_ABORT,
	POBJ_TX_ON_FAILURE_RETURN,
};

void pmemobj_tx_set_on_failure(enum pobj_tx_on_failure failure);
enum pobj_tx_on_failure pmemobj_tx_get_on_failure(void);

I don't like using "action" here. @marcinslusarz ?

Copy link
Contributor

@marcinslusarz marcinslusarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 18 files reviewed, 4 unresolved discussions (waiting on @lukaszstolarczuk, @pbalcer, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

I'd prefer to change this to:

enum pobj_tx_on_failure {
	POBJ_TX_ON_FAILURE_ABORT,
	POBJ_TX_ON_FAILURE_RETURN,
};

void pmemobj_tx_set_on_failure(enum pobj_tx_on_failure failure);
enum pobj_tx_on_failure pmemobj_tx_get_on_failure(void);

I don't like using "action" here. @marcinslusarz ?

What's wrong with "action"? Without any noun telling you what it is setting the API looks a bit... vague(?)

Putting "action" question aside, here's how calling this will look like:

pmemobj_tx_set_action_on_failure(POBJ_TX_ABORT);
vs
pmemobj_tx_set_on_failure(POBJ_TX_ON_FAILURE_ABORT);

The second one duplicates on_failure.

Copy link
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 30 files at r4.
Reviewable status: 7 of 18 files reviewed, 5 unresolved discussions (waiting on @igchor, @lukaszstolarczuk, @pbalcer, and @szyrom)

a discussion (no related file):
in commit message "pmemobj_tx_is_abort_on_failure"



src/libpmemobj/tx.c, line 2124 at r4 (raw file):

pmemobj_tx_set_action_on_failure(enum pobj_tx_action_on_failure action)
{
	LOG(3, "enable %d", action);

string "enable" could be changed

Copy link
Contributor Author

@igchor igchor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 18 files reviewed, 5 unresolved discussions (waiting on @lukaszstolarczuk, @pbalcer, and @szyrom)

a discussion (no related file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

in commit message "pmemobj_tx_is_abort_on_failure"

Done.



doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

What's wrong with "action"? Without any noun telling you what it is setting the API looks a bit... vague(?)

Putting "action" question aside, here's how calling this will look like:

pmemobj_tx_set_action_on_failure(POBJ_TX_ABORT);
vs
pmemobj_tx_set_on_failure(POBJ_TX_ON_FAILURE_ABORT);

The second one duplicates on_failure.

Please see the latest version. I'm not sure whether event is the best word but Piotr has a point that "action" can be ambiguous (we already have action API).


src/libpmemobj/tx.c, line 2124 at r4 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

string "enable" could be changed

Done.

Copy link
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 15 files at r5.
Reviewable status: 9 of 18 files reviewed, 4 unresolved discussions (waiting on @igchor, @pbalcer, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 79 at r5 (raw file):

void *pmemobj_tx_get_user_data(void);

void pmemobj_tx_set_failure_event(enum pobj_tx_failure_event action);

arg name action -> event

Copy link
Member

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: 9 of 18 files reviewed, 7 unresolved discussions (waiting on @igchor and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 425 at r5 (raw file):

If **pmemobj_tx_set_failure_event**() is called with POBJ_TX_FAILURE_RETURN a NO_ABORT
flag is implicitly passed to all functions which accept a NO_ABORT flag. If called
with POBJ_TX_FAILURE_ABORT then all functions behave normally. Event on failure setting is

This setting


doc/libpmemobj/pmemobj_tx_begin.3.md, line 425 at r5 (raw file):

If **pmemobj_tx_set_failure_event**() is called with POBJ_TX_FAILURE_RETURN a NO_ABORT
flag is implicitly passed to all functions which accept a NO_ABORT flag. If called
with POBJ_TX_FAILURE_ABORT then all functions behave normally. Event on failure setting is

behave normally doesn't describe anything. What's normal behavior?


doc/libpmemobj/pmemobj_tx_begin.3.md, line 426 at r5 (raw file):

flag is implicitly passed to all functions which accept a NO_ABORT flag. If called
with POBJ_TX_FAILURE_ABORT then all functions behave normally. Event on failure setting is
inherited by inner transactions. It does not affect any of outer transactions.

of the outer transactions.


src/include/libpmemobj/tx_base.h, line 433 at r5 (raw file):

/*
 * Specifies what should happen in case of an error within the

This shouldn't be copy/pasted from the man page.

Just say "Sets the failure behavior of transactional functions" or something like that.

Copy link
Contributor Author

@igchor igchor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 9 of 18 files reviewed, 7 unresolved discussions (waiting on @lukaszstolarczuk, @pbalcer, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 79 at r5 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

arg name action -> event

Done.


doc/libpmemobj/pmemobj_tx_begin.3.md, line 425 at r5 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

This setting

Done.


doc/libpmemobj/pmemobj_tx_begin.3.md, line 425 at r5 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

behave normally doesn't describe anything. What's normal behavior?

Done.


doc/libpmemobj/pmemobj_tx_begin.3.md, line 426 at r5 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

of the outer transactions.

Done.


src/include/libpmemobj/tx_base.h, line 433 at r5 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

This shouldn't be copy/pasted from the man page.

Just say "Sets the failure behavior of transactional functions" or something like that.

Done.

Copy link
Contributor

@marcinslusarz marcinslusarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 18 files reviewed, 7 unresolved discussions (waiting on @lukaszstolarczuk, @pbalcer, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Please see the latest version. I'm not sure whether event is the best word but Piotr has a point that "action" can be ambiguous (we already have action API).

"Event" usually is something that happens out of your control, not something you want to happen.

"Action API" is supposed to be phased out in favor of "intent API"... :)

Copy link
Member

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 18 files reviewed, 3 unresolved discussions (waiting on @lukaszstolarczuk and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

"Event" usually is something that happens out of your control, not something you want to happen.

"Action API" is supposed to be phased out in favor of "intent API"... :)

"things" generate "events". In this way, you choose which "events" are generated from "things". I think that makes sense :D

As for action, yes - but we won't remove it.

@marcinslusarz
Copy link
Contributor


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

"things" generate "events". In this way, you choose which "events" are generated from "things". I think that makes sense :D

As for action, yes - but we won't remove it.

This article summarises quite well what word "event" means in CS: https://en.wikipedia.org/wiki/Event_(computing)

"Failure event" that you are supposed to "set" is just bizarre.

Copy link
Contributor

@marcinslusarz marcinslusarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 18 files reviewed, 4 unresolved discussions (waiting on @igchor, @lukaszstolarczuk, and @szyrom)

Copy link
Contributor Author

@igchor igchor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 18 files reviewed, 4 unresolved discussions (waiting on @lukaszstolarczuk, @marcinslusarz, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

This article summarises quite well what word "event" means in CS: https://en.wikipedia.org/wiki/Event_(computing)

"Failure event" that you are supposed to "set" is just bizarre.

I agree that usually events mean something generated by the environment/asynchronously but that's not a requirement. However, I think that our event is "an action or occurrence recognized by software" as the article says ;d

What do you think about "behaviour"?

pmemobj_tx_set_failure_behaviour(POBJ_TX_FAILURE_ABORT / POBJ_TX_FAILURE_RETURN);

Copy link
Contributor

@marcinslusarz marcinslusarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 18 files reviewed, 4 unresolved discussions (waiting on @igchor, @lukaszstolarczuk, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

I agree that usually events mean something generated by the environment/asynchronously but that's not a requirement. However, I think that our event is "an action or occurrence recognized by software" as the article says ;d

What do you think about "behaviour"?

pmemobj_tx_set_failure_behaviour(POBJ_TX_FAILURE_ABORT / POBJ_TX_FAILURE_RETURN);

It's much much better than "event". I still think "action" is the best term, but I can live with "behavior" ;) (btw, American English)

Copy link
Member

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 18 files reviewed, 4 unresolved discussions (waiting on @lukaszstolarczuk, @marcinslusarz, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

It's much much better than "event". I still think "action" is the best term, but I can live with "behavior" ;) (btw, American English)

ok, let's go with behavior. At the end of the day - this is not going to be used very frequently from C.

Copy link
Contributor Author

@igchor igchor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 18 files reviewed, 4 unresolved discussions (waiting on @lukaszstolarczuk, @marcinslusarz, and @szyrom)


doc/libpmemobj/pmemobj_tx_begin.3.md, line 423 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

ok, let's go with behavior. At the end of the day - this is not going to be used very frequently from C.

Ok, done.

Copy link
Member

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: 3 of 18 files reviewed, 4 unresolved discussions (waiting on @lukaszstolarczuk, @marcinslusarz, and @szyrom)

Copy link
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 7 of 15 files at r6.
Reviewable status: 10 of 18 files reviewed, 7 unresolved discussions (waiting on @igchor, @marcinslusarz, and @szyrom)

a discussion (no related file):
please update the commit msg



doc/libpmemobj/pmemobj_tx_begin.3.md, line 427 at r6 (raw file):

with POBJ_TX_FAILURE_ABORT then all functions abort the transaction (unless NO_ABORT
flag is passed explicitly). This setting is inherited by inner transactions. It does
not affect any of the outer transactions. Aborting on failure is the default behaviour.

keep US eng. in all places then: "behaviour" -> "behavior"


src/include/libpmemobj/tx_base.h, line 433 at r6 (raw file):

/*
 * Sets the failure behaviour of transactional functions.

"behavior"


src/include/libpmemobj/tx_base.h, line 440 at r6 (raw file):

/*
 * Returns failure event for the current transaction.

event

This function can be used to disable or enable aborting tx
in case of an error (for example when allocation fails).

This patch also introduces pmemobj_tx_get_failure_behavior
which allows to query current setting of a transaction.
Copy link
Contributor Author

@igchor igchor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 10 of 18 files reviewed, 7 unresolved discussions (waiting on @lukaszstolarczuk, @marcinslusarz, and @szyrom)

a discussion (no related file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

please update the commit msg

Done.



doc/libpmemobj/pmemobj_tx_begin.3.md, line 427 at r6 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

keep US eng. in all places then: "behaviour" -> "behavior"

Done.


src/include/libpmemobj/tx_base.h, line 433 at r6 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

"behavior"

Done.


src/include/libpmemobj/tx_base.h, line 440 at r6 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

event

Done.

Copy link
Member

@lukaszstolarczuk lukaszstolarczuk left a 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 r7.
Reviewable status: 10 of 18 files reviewed, 3 unresolved discussions (waiting on @marcinslusarz and @szyrom)

@pmem-bot
Copy link
Contributor

@osalyk and @janekmi please review this pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libpmemobj src/libpmemobj
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants