[Tarantool-patches] [PATCH v10 3/4] limbo: filter incoming synchro requests

[Vladislav Shpilevoy]

>>>>> +/**
>>>>> + * Common chain for any incoming packet.
>>>>> + */
>>>>> +static int
>>>>> +filter_in(struct txn_limbo *limbo, const struct synchro_request *req)
>>>>> +{
>>>>> +	(void)limbo;
>>>>
>>>> 6. So you have the filtering enabled dynamically in the limbo, but
>>>> you do not use the limbo here? Why? Maybe at least add an assertion
>>>> that the filter is enabled?
>>>
>>> All chains are having same interface it is just happen that for common
>>> filter I don't need to use limbo. I could add some operations here
>>> but not sure if it worth it. As far as I see leave unused args is
>>> pretty fine in our code base.
>>
>> You didn't answer the second question:
>>
>> 	Maybe at least add an assertion that the filter is enabled?
> 
> I did
> 
>   | I could add some operations here but not sure if it worth it.
> 
> Letme state it clear then - I could add this assert() if you insist
> but I think we aready spread too many assertions all over the code,
> and if it is possible I would be glad not to add new ones. After all
> either we should add this assert() to each filter chain or not add
> at all, otherwise there will be kind of code imbalance.

What is wrong with the assertions that you don't like adding them?
You add panics quite often, and they cost some perf. But asserts
just help to catch bugs and cost nothing in Release build.

>>>>> +/**
>>>>> + * Filter CONFIRM and ROLLBACK packets.
>>>>> + */
>>>>> +static int
>>>>> +filter_confirm_rollback(struct txn_limbo *limbo,
>>>>> +			const struct synchro_request *req)
>>>>> +{
>>>>> +	/*
>>>>> +	 * When limbo is empty we have nothing to
>>>>> +	 * confirm/commit and if this request comes
>>>>> +	 * in it means the split brain has happened.
>>>>> +	 */
>>>>> +	if (!txn_limbo_is_empty(limbo))
>>>>> +		return 0;
>>>>
>>>> 9. What if rollback is for LSN > limbo's last LSN? It
>>>> also means nothing to do. The same for confirm LSN < limbo's
>>>> first LSN.
>>>
>>> static void
>>> txn_limbo_read_rollback(struct txn_limbo *limbo, int64_t lsn)
>>> {
>>> -->	assert(limbo->owner_id != REPLICA_ID_NIL || txn_limbo_is_empty(limbo));
>>>
>>> txn_limbo_read_confirm(struct txn_limbo *limbo, int64_t lsn)
>>> {
>>> -->	assert(limbo->owner_id != REPLICA_ID_NIL || txn_limbo_is_empty(limbo));
>>>
>>> Currently we're allowed to process empty limbo if only owner is not nil,
>>> I think I should add this case here.
>>
>> My question is not about the owner ID. I asked what if rollback/confirm
>> try to cover a range not present in the limbo while it is not empty. If
>> it is not empty, it has an owner obviously. But it does not matter.
>> What if it has an owner, has transactions, but you got ROLLBACK/CONFIRM
>> for data out of the LSN range present in the limbo?
> 
> Since the terms are matching I think such scenarion should be fine, right?
> IOW, some old replica has been stopped for some reason and been living out
> of quorum for some time thus such requests should be considered as OK to
> pass and when filter accepts them the will reach txn_limbo_read_confirm
> or txn_limbo_read_rollback where they will be simply ignored as far as I
> unrestand. IOW, such requests are valid, no?

If a replica is outdated, it should not matter. It will receive the needed
data in order anyway. Like if the data was just sent. Hence, it seems
irrelevant whether it is outdated. And still looks the same as the thing
you are trying to filter out (when the limbo is empty = confirm/rollback
do not cover anything too).