From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 19 Sep 2018 04:46:32 +0300 From: Konstantin Osipov Subject: Re: [PATCH v2 2/8] vinyl: update compact priority usual way on range split/coalesce Message-ID: <20180919014632.GH31150@chai> References: <8da96f62e4b6b8a75c91ce4007c1e10bc38fee5b.1537115208.git.vdavydov.dev@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8da96f62e4b6b8a75c91ce4007c1e10bc38fee5b.1537115208.git.vdavydov.dev@gmail.com> To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: * Vladimir Davydov [18/09/17 15:05]: > When a few ranges are coalesced, we "force" compaction of the resulting > range by raising its compaction priority to max (slice count). There's > actually no point in that, because as long as the shape of the resulting > LSM tree is OK, we don't need to do extra compaction work. Moreover, it > actually doesn't work if a new slice is added to the resulting range by > dump before it gets compacted, which is fairly likely, because then its > compaction priority will be recalculated as usual. So let's simply call > vy_range_update_compact_priority() for the resulting range. > > - /* > - * Coalescing increases read amplification and breaks the log > - * structured layout of the run list, so, although we could > - * leave the resulting range as it is, we'd better compact it > - * as soon as we can. > - */ > - result->compact_priority = result->slice_count; > + vy_range_update_compact_priority(result, &lsm->opts); Looks like a drastic change of opinion to me :). I would leave reasons you give in the old comment why we may need a compaction and the new reasons you give in the CS comment why we may not need a compaction in a consolidated comment for this line. The patch itself is OK to push. > vy_lsm_acct_range(lsm, result); > vy_lsm_add_range(lsm, result); > lsm->range_tree_version++; > -- > 2.11.0 > -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov