Re: [Tarantool-patches] [PATCH v2] box: add binary search for _session_settings space

[Chris Sosnin <k.sosnin@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 1887 bytes --]

Hi! Thank you for the review and your fixes!
See my answers below.

> 3. I would rather say to '*an* existing'. Because there is no a one
> certain setting to which all the iterators point after setting that
> flag.

Thank you, I agree.

> 4. Even though that bsearch passes the tests, I failed to understand
> it. Please, consider a more canonical bsearch on the branch in a
> separate commit. Feel free to comment, if someone catches your attention
> in my version and you don't agree.

Your version is better, I agree, my code just implements lower/upper bound,
but there is no point in it, because all settings are unique. Thanks!

>> +	if (low < count) {
>> +		int cmp = strcmp(name[low], key);
>> +		if ((cmp == 0 && is_including) ||
>> +		    (cmp > 0 && !is_eq)) {
>> +			*sid = low;
>> +			return 0;
>> +		}
>> +	}
> 
> 5. This last strcmp can be avoided. Or at least
> moved into the cycle. See my commit.
> 
> All the same about 'reverse' version.

I believe with your fixes it doesn’t really matter anymore.

> 
> 6. What I don't like here is that we check is_set as many times,
> as many modules we have. Moreover, if set_forward didn't find
> anything, you fallback to a fullscan in that module, even though
> it wont find anything with 100% guarantee.
> 
> I fixed it, see my commit.

The thing is that if set_forward fails, then sid = count and next_in_module
will immediately return -1. But if we are going to remove modules, then this
part will be rewritten anyway.

> I fixed settings tests also - there was no a check that
> real settings space, and a user defined one return the same tuple count. It
> could happen, that one of them returns 0 tuples, and it would be considered
> success.
> 
> Additionally, I added a test, which tries all existing iterators on every
> setting.

Thanks, I agree with those.


[-- Attachment #2: Type: text/html, Size: 20312 bytes --]