From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 45E936FC8F; Tue, 23 Mar 2021 10:28:50 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 45E936FC8F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1616484530; bh=qxVoedwXqswLuf3+/w24tGokXRfZQVQOgsW9YMrHMls=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=Do2iTqCSjxDe58aeJS810DI2nVegaHDlnct1pLGiHYqZuKivo18CsHNrLLB2487l3 pjSz4KDmxwvc9ROmg4WUmXQ321FfqcvrP2iGJGjWX/JnNB40VfPQgAfGwslG6BT5qR yvIw1mJ6e6XO+B7LE/2QtNMCAHLt9dImwjZKitcM= Received: from mail-lj1-f174.google.com (mail-lj1-f174.google.com [209.85.208.174]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id E8C326FC8F for ; Tue, 23 Mar 2021 10:28:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E8C326FC8F Received: by mail-lj1-f174.google.com with SMTP id 16so24264022ljc.11 for ; Tue, 23 Mar 2021 00:28:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=Abd+9lzqDG6CWY95PeAXW5ZUc+P/eREe6hX34/+L/5U=; b=m7epllMAjnmQIcra63YcusdAK0btbjcBpkHjV6NOUkThyI3BPHjej0woJX16LcU2W2 tNYLuf/JchSE8W4cQ/IiFnV0uw5fxJXSpRstG6PjU0TzaDdLIMo0q6hCIxtGdBP9NDwK P6NRzUTy54mckoHgMOrgXKQjUMWXfJrxD3Dw4LAAUBF0Y1pw9mhMdPEHEozoMKBNZazt I+SjfunCAWmITRBMapEG+0L4H2mSefhWhU1vROYHeZcHzJ8oDWKlBkjbGx5LasJbV6sP Kel1i872mXji0kVSzaq9pRdvQQda3j+vp8ZelNki/4kXHM51RXjask2h3/SpQSAfAz3n fAFw== X-Gm-Message-State: AOAM530njABK5dAs0YdixMrKsnZTTeRA0ILgOjbwGcQArqE8A15TdMjl +1g41imDX0td/wNMn1usU/E= X-Google-Smtp-Source: ABdhPJxoRcYMcuf5mnXePXkPswUfsYb6Wdl7tCZc8hW393eluoYrX07jzj0ZmuwNEfv6Xa1zoUWP9Q== X-Received: by 2002:a2e:910b:: with SMTP id m11mr2266046ljg.179.1616484528378; Tue, 23 Mar 2021 00:28:48 -0700 (PDT) Received: from grain.localdomain ([5.18.171.94]) by smtp.gmail.com with ESMTPSA id v18sm2220718ljg.85.2021.03.23.00.28.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 23 Mar 2021 00:28:47 -0700 (PDT) Received: by grain.localdomain (Postfix, from userid 1000) id 8785A560175; Tue, 23 Mar 2021 10:28:46 +0300 (MSK) Date: Tue, 23 Mar 2021 10:28:46 +0300 To: Vladislav Shpilevoy Cc: tml , Mons Anderson Message-ID: References: <20210320131521.1249747-1-gorcunov@gmail.com> <20210320131521.1249747-2-gorcunov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/2.0.5 (2021-01-21) Subject: Re: [Tarantool-patches] [PATCH v2 1/3] gc/xlog: delay xlog cleanup until relays are subscribed X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Cyrill Gorcunov via Tarantool-patches Reply-To: Cyrill Gorcunov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" On Mon, Mar 22, 2021 at 10:40:34PM +0100, Vladislav Shpilevoy wrote: > Hi! Thanks for working on this! > > See 8 comments below. > > > Current state of `*.xlog` garbage collector can be found in > > `box.info.gc()` output. For example > > > > ``` Lua > > tarantool> box.info.gc() > > --- > > ... > > cleanup_is_paused: false > > ``` > > 1. Isn't it 'is_paused' instead of 'cleanup_is_paused'? Yeah, sorry. Thanks! > > > > +static double > > +box_check_wal_cleanup_delay(void) > > +{ > > + double value = cfg_geti("wal_cleanup_delay"); > > 2. cfg_geti() returns an integer. Please, use cfg_getd(). I did it on a purpose: we accepts time in whole seconds, not milliseconds and etc. But sure, no problem will change it to cfg_getd. > > + > > + /* > > + * Anonymous replicas do not require > > + * delay the cleanup procedure since they > > + * are read only. > > + */ > > + if (cfg_geti("replication_anon") != 0) { > > + if (value != 0) > > + value = 0; > > 3. Still it makes sense to check that if the option is set, > it is valid. Regardless of what is the anon value. OK > > I also think the function should return the delay exactly as > it is set, not corrected by the anon value. Because these > box_check functions are not for configuring. They are for > checking a single option. OK > > Also you should use 'replication_anon' global variable instead > of the config, which might be not installed at this moment yet. What would happen if one setup both 'wal_cleanup_delay' and 'replication_anon' in config at once. Which C's replication_anon value I will be reading? The C's replication_anon is set after the cfg procedure complete, so since I operate on values obrained from Lua level I need to use cfg_geti("replication_anon") because at this moment only Lua level is consistent and replication_anon may have a value from previous box.cfg call. > > > > +int > > +box_set_wal_cleanup_delay(void) > > +{ > > + double delay = box_check_wal_cleanup_delay(); > > + if (delay < 0) > > + return -1; > > 4. Here you could do the correction. Look at the option > value and at 'replication_anon' global variable. Not at > the anon config. Need to think about this moment, thanks! > > > + gc_set_wal_cleanup_delay(delay); > > + return 0; > > +} > > + > > void > > box_set_vinyl_memory(void) > > { > > @@ -3000,7 +3035,7 @@ box_cfg_xc(void) > > rmean_box = rmean_new(iproto_type_strs, IPROTO_TYPE_STAT_MAX); > > rmean_error = rmean_new(rmean_error_strings, RMEAN_ERROR_LAST); > > > > - gc_init(); > > + gc_init(box_check_wal_cleanup_delay()); > > 5. Here is the problem: you rely on replication_anon setting, but it is installed > below, after GC is initialized. Exactly, that's why I use raw cfg_geti("replication_anon") instead of global replication_anon variable. > Perhaps you could set the delay to infinite and after the non-dynamic options > are done, box_set_wal_cleanup_delay() would be called by load_cfg.lua and > would lower the delay to 0 or whatever is configured. > > > engine_init(); > > schema_init(); > > replication_init(); > > diff --git a/src/box/gc.c b/src/box/gc.c > > index 9af4ef958..5418fd31d 100644 > > --- a/src/box/gc.c > > +++ b/src/box/gc.c > > @@ -46,6 +46,7 @@ > > #include > > #include > > > > +#include "cfg.h" > > 6. Not necessary anymore. +1 > > gc_cleanup_fiber_f(va_list ap) > > { > > (void)ap; > > + > > + /* > > + * Stage 1 (optional): in case if we're booting > > + * up with cleanup disabled lets do wait in a > > + * separate cycle to minimize branching on stage 2. > > + */ > > + if (gc.is_paused) { > > + ev_tstamp timeout = gc.wal_cleanup_delay; > > 7. Please, use double for timestamps and timeouts. Because it > is used in almost all the other code working with time. Sure > > + while (!fiber_is_cancelled()) { > > + ev_tstamp clock_start = fiber_clock(); > > + if (fiber_yield_timeout(timeout)) { > > + say_info("wal/engine cleanup is resumed " > > + "due to timeout expiration"); > > + gc.is_paused = false; > > + gc.delay_ref = 0; > > + break; > > + } > > + > > + /* > > + * If a last reference is dropped > > + * we can exit out early. > > + */ > > + if (!gc.is_paused) > > + break; > > + > > + ev_tstamp elapsed = fiber_clock() - clock_start; > > + timeout = gc.wal_cleanup_delay - elapsed; > > 8. In the previous review I said about remembering a timestamp not > accidentally. But because otherwise I can make this loop roll > forever if I update wal_cleanup_delay with some period < timeout. > > For instance, it was 5. You remember clock_start. Then 2.5 secs > pass and I update it to 4.99999. elapsed is 2.5 secs. Timeout is > 4.99999 - 2.5 = ~2.5. It starts again. I update the timeout to > 4.99998 after another 2.5 secs. Elapsed is 2.5 secs again, and the > next timeout is about 2.5 secs. And so on. > > In your patch it will happen even if I increase the timeout, not > only decrease. The fluctuations might be accidental in case I > calculate it from something in Lua, get precision loss, and call > box.cfg{} with a certain period until the instance accepts > requests. > > This might be fixed if clock_start is moved out of the loop. What you're talking about is fp rouding errors and you're to work really hard to enter into a such loop. But I see your point and will update. Btw the use of FP value for seconds is a big mistake in architecure, not only here but all over the code and I think we should get rid of this very strange approach (and I must confess I don't understand *why* on earth the FP is used for delays in ev library). Cyrill