[Tarantool-patches] [PATCH 1/1] txn_limbo: introduce dynamic synchro config
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Jul 10 03:23:47 MSK 2020
>> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
>> index e28e2016f..2575f4c25 100644
>> --- a/src/box/txn_limbo.c
>> +++ b/src/box/txn_limbo.c
>> @@ -159,45 +160,56 @@ txn_limbo_wait_complete(struct txn_limbo *limbo, struct txn_limbo_entry *entry)
>> assert(!txn_has_flag(txn, TXN_IS_DONE));
>> assert(txn_has_flag(txn, TXN_WAIT_SYNC));
>> - bool cancellable = fiber_set_cancellable(false);
>> - bool timed_out = fiber_yield_timeout(txn_limbo_confirm_timeout(limbo));
>> - fiber_set_cancellable(cancellable);
>> - if (timed_out) {
>> - assert(!txn_limbo_is_empty(limbo));
>> - if (txn_limbo_first_entry(limbo) != entry) {
>> - /*
>> - * If this is not a first entry in the
>> - * limbo, it is definitely not a first
>> - * timed out entry. And since it managed
>> - * to time out too, it means there is
>> - * currently another fiber writing
>> - * rollback. Wait when it will finish and
>> - * wake us up.
>> - */
>> - bool cancellable = fiber_set_cancellable(false);
>> - fiber_yield();
>> - fiber_set_cancellable(cancellable);
>> - assert(txn_limbo_entry_is_complete(entry));
>> + double start_time = fiber_clock();
>> + while (true) {
>> + double deadline = start_time + txn_limbo_confirm_timeout(limbo);
>> + bool cancellable = fiber_set_cancellable(false);
>> + double timeout = deadline - fiber_clock();
>
> Why not just timeout = txn_limbo_confirm_timeout(limbo) ?
> It's look like
> fiber_clock()(old) + txn_limbo_confirm_timeout(limbo) - fiber_clock()(new) ~= txn_limbo_confirm_timeout(limbo)
Because it will wait infinitely in case I will constantly update
the parameters. Deadline will move forward and forward. It
shouldn't.
>> + bool timed_out = fiber_cond_wait_timeout(&limbo->wait_cond,
>> + timeout);
>> + fiber_set_cancellable(cancellable);
>> + if (txn_limbo_entry_is_complete(entry))
>> goto complete;
>> - }
>> + if (timed_out)
>> + goto do_rollback;
>> + }
>> @@ -472,17 +484,26 @@ txn_limbo_wait_confirm(struct txn_limbo *limbo)
>> struct txn_limbo_entry *tle = txn_limbo_last_entry(limbo);
>> txn_on_commit(tle->txn, &on_complete);
>> txn_on_rollback(tle->txn, &on_rollback);
>> -
>> - int rc = fiber_cond_wait_timeout(&cwp.confirm_cond,
>> - txn_limbo_confirm_timeout(limbo));
>> - fiber_cond_destroy(&cwp.confirm_cond);
>> - if (rc != 0) {
>> - /* Clear the triggers if the timeout has been reached. */
>> - trigger_clear(&on_complete);
>> - trigger_clear(&on_rollback);
>> - diag_set(ClientError, ER_SYNC_QUORUM_TIMEOUT);
>> - return -1;
>> + double start_time = fiber_clock(); > + while (true) {
>> + double deadline = start_time + txn_limbo_confirm_timeout(limbo);
>> + bool cancellable = fiber_set_cancellable(false);
>> + double timeout = deadline - fiber_clock();
>
> Maybe add a comment about the possible reasons of wake up
> (reconfiguration, triggers, timeout).
Wait_cond has a comment about that already.
>> + int rc = fiber_cond_wait_timeout(&limbo->wait_cond, timeout);
>
> If I understand correctly, you use a trick for wake up from triggers.
> In this case fiber_wakeup invoke manually without using the fiber_cond
> API. In this context, I have few questions/comments:
> - IMHO, this is not true way to use implemantation details of fiber_cond
> passing over the fiber_cond API.
> - In the case, we don't interact with waiters list inside condition
> variable. Are you sure this is ok?
I am sure that it is ok to use fiber_wakeup(). Moreover, you can't use
fiber_cond API here, because you will wakeup other fibers, not related
to the waiter. So the direct wakeup is the only way.
More information about the Tarantool-patches
mailing list