From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <tarantool-patches-bounce@freelists.org>
Received: from localhost (localhost [127.0.0.1])
	by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id A4D5F221FA
	for <tarantool-patches@freelists.org>; Wed, 18 Jul 2018 07:52:53 -0400 (EDT)
Received: from turing.freelists.org ([127.0.0.1])
	by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024)
	with ESMTP id A7NmfU_fRzHs for <tarantool-patches@freelists.org>;
	Wed, 18 Jul 2018 07:52:53 -0400 (EDT)
Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(No client certificate requested)
	by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 051DA221F7
	for <tarantool-patches@freelists.org>; Wed, 18 Jul 2018 07:52:52 -0400 (EDT)
Subject: [tarantool-patches] Re: [PATCH v2 2/7] box: move checks for key
 findability from space_vtab
References: <cover.1531832794.git.imeevma@gmail.com>
 <38687f411d21ed4ac762f3d5716cf635aaf2ea4c.1531832794.git.imeevma@gmail.com>
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Message-ID: <e2853472-1cb0-fcb3-68d4-c7cdfe96ad20@tarantool.org>
Date: Wed, 18 Jul 2018 14:52:50 +0300
MIME-Version: 1.0
In-Reply-To: <38687f411d21ed4ac762f3d5716cf635aaf2ea4c.1531832794.git.imeevma@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: tarantool-patches-bounce@freelists.org
Errors-to: tarantool-patches-bounce@freelists.org
Reply-To: tarantool-patches@freelists.org
List-help: <mailto:ecartis@freelists.org?Subject=help>
List-unsubscribe: <tarantool-patches-request@freelists.org?Subject=unsubscribe>
List-software: Ecartis version 1.0.0
List-Id: tarantool-patches <tarantool-patches.freelists.org>
List-subscribe: <tarantool-patches-request@freelists.org?Subject=subscribe>
List-owner: <mailto:>
List-post: <mailto:tarantool-patches@freelists.org>
List-archive: <http://www.freelists.org/archives/tarantool-patches>
To: tarantool-patches@freelists.org, Mergen Imeev <imeevma@tarantool.org>

Hi. Thank you for patch. Mostly it is looking good for me.

0. It would be great, if you describe in commit message, why you should move
this checks.

> +/**
> + * Check space for writeability.
> + */
> +static inline int
> +space_check_writable(struct space *space)> +/**
> + * Check if key is good enough
> + * to be used to find tuple.
> + */
> +static inline int
> +key_check_findable(struct space *space, uint32_t index_id, const char *key)
1. I don't like this comments. Please, make doxygen comments for them.

>  int
>  box_process1(struct request *request, box_tuple_t **result)
2. As I see, box_process1 is used only in a single place, in tx_process1; It is also extra small now.
Do we really need it in header? Maybe it worth to inline it?


> -/**
> - * Wrapper around vy_lsm_find() which ensures that
> - * the found index is unique.
> - */
> -static  struct vy_lsm *
> +struct vy_lsm *
>  vy_lsm_find_unique(struct space *space, uint32_t index_id)
3. It also lacks a good comment, like vy_unique_key_validate has.