From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp62.i.mail.ru (smtp62.i.mail.ru [217.69.128.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id D964C445320 for ; Fri, 17 Jul 2020 09:16:28 +0300 (MSK) References: <1594821336-14468-1-git-send-email-alyapunov@tarantool.org> <1594821336-14468-12-git-send-email-alyapunov@tarantool.org> <8257766c-ad0e-e572-9cc2-8b970ba8cb4e@tarantool.org> From: Aleksandr Lyapunov Message-ID: Date: Fri, 17 Jul 2020 09:16:27 +0300 MIME-Version: 1.0 In-Reply-To: <8257766c-ad0e-e572-9cc2-8b970ba8cb4e@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [Tarantool-patches] [PATCH v3 11/13] txm: introduce txm_story List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org Hello, thank for the review! On 7/16/20 3:20 AM, Vladislav Shpilevoy wrote: > 1. Why is the hash called 'history', but the objects are 'story'? > Or as 'history' you mean a list of stories of the same key in one > index? I mean history is a set of all stories. Like a big book with a lot of paragraphs and stories about things in time. > 2. difference -> different. thanks > > 3. The iterator is initialized and assigned to something, but is > never used for anything meaningful. The same for all_stories list. > At least on top of this commit. Implemented garbage collector. > + cord_slab_cache(), item_size); > 4. Is it correct you also create a pool for 0 index count? > Why? > > Probably would be much simpler to just create one mempool with > objects having BOX_INDEX_MAX records in them. It would also solve > the DDL problem. At least partially. > > Can the story objects be allocated on txn's region? AFAIU, > stories are created by transactions, and can't outlive their > creator. Discussed in call, that would be too much memory wasted. > > 5. Diag OOM takes size, allocator, and variable name. Here the > allocator is "mempool_alloc", and the variable name is "story". > Lets be consistent with other diag_sets. The same for all the > other new places. OK > > 6. I know story is NULL, but why not to return NULL explicitly? > This looks confusing. OK > > 7. Nit: would be better to call tuple_ref() right near this assignment. > For the sake of OOM handler below, worth moving this line down to the > tuple_ref() call below. The hash table requires tuple to be set. > > 8. Comments for functions should use imperative mood. The same > in other similar places. OK > > +static struct tuple * > +txm_story_older_tuple(struct txm_story_link *link) > +{ > + return link->older.is_story ? link->older.story->tuple > + : link->older.tuple; > 9. Why so? If older.is_story, it means it stores a story, > not a tuple. But you return vice versa. I don't see any mistake. I return tuple in any case. > > 10. Why do you need all the new structs in txn.h if they aren't > used out of txn.c anyway. At least some of them. OK, I'll try to. I have to make a pause, will return later.