From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Serge Petrenko Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_45DBC573-22B2-4C84-A2DC-46ACEA9B6840" Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: Re: [tarantool-patches] Re: [PATCH] test: rework box/misc test part regarding on_shutdown triggers Date: Fri, 12 Apr 2019 17:12:11 +0300 In-Reply-To: <20190412140023.geyzk47gkwws7wns@esperanza> References: <20190411133720.39993-1-sergepetrenko@tarantool.org> <20190412140023.geyzk47gkwws7wns@esperanza> To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: --Apple-Mail=_45DBC573-22B2-4C84-A2DC-46ACEA9B6840 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Hi! Thank you for review. > 12 =D0=B0=D0=BF=D1=80. 2019 =D0=B3., =D0=B2 17:00, Vladimir Davydov = =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0= ): >=20 > On Thu, Apr 11, 2019 at 04:37:20PM +0300, Serge Petrenko wrote: >> Rewrite the test so that it doesn't depend on timeouts, which can be >> exceeded under high load, too much and simplify it a little. >>=20 >> Also extract the whole part regarding on_shutdown triggers into a >> separate test, so that its sporadic failures would be easier to >> investigate. >>=20 >> Part of #4134 >> --- >> https://github.com/tarantool/tarantool/issues/4134 >> = https://github.com/tarantool/tarantool/tree/sp/gh-4134-fix-on-shutdown-tes= ts >>=20 >> test/box/misc.result | 161 = ------------------------- >> test/box/misc.test.lua | 62 ---------- >> test/box/on_shutdown_triggers.result | 160 ++++++++++++++++++++++++ >> test/box/on_shutdown_triggers.test.lua | 62 ++++++++++ >=20 > Better call simply box/on_shutdown.test.lua. >=20 > Anyway, this patch is impossible to review. Please split it in two - = one > moves the test case to a separate file, another - fixes it. Will do. >=20 > I managed to extract the diff. Pasting it here for review. >=20 >> @@ -33,7 +33,7 @@ >> box.space.shutdown:select{} >> box.space.shutdown:drop() >>=20 >> --- Check that os.exit invokes triggers >> +-- Check that os.exit invokes on_shutdown triggers >> fiber =3D require("fiber") >> test_run:cmd("create server test with script=3D'box/proxy.lua'") >> test_run:cmd("start server test") >> @@ -46,19 +46,17 @@ >> _ =3D box.ctl.on_shutdown(function() print("on_shutdown 5") end) >> -- Check that we don't hang infinitely after os.exit() >> -- even if the following code doesn't yield. >> -fiber =3D require("fiber") >> -_ =3D fiber.create(function() fiber.sleep(0.05) os.exit() while true = do end end) >> +fiber =3D require('fiber') >=20 > This 'require' isn't needed anymore, no? >=20 >> test_run:cmd("switch default") >> +test_run:eval("test", "_ =3D fiber.new(function() os.exit() while = true do end end)") >=20 > This is supposed to stop the default instance?! No, it=E2=80=99s evaled on =E2=80=99test=E2=80=99, which is the other = instance. And that=E2=80=99s why we need the require(=E2=80=98fiber=E2=80=99= ) above. Anyways, will split the patch in two and resend. >=20 >> fiber.sleep(0.1) >> -- The server should be already stopped by os.exit(), >> -- but start doesn't work without a prior call to stop. >> test_run:cmd("stop server test") >> test_run:cmd("start server test") >> -test_run:cmd("switch test") >> -test_run:grep_log('test', 'on_shutdown 5', nil, {noreset=3Dtrue}) >> +test_run:wait_log('test', 'on_shutdown 5', nil, 5, {noreset=3Dtrue}) >> -- make sure we exited because of os.exit(), not a signal. >> test_run:grep_log('test', 'signal', nil, {noreset=3Dtrue}) >> -test_run:cmd("switch default") >> test_run:cmd("stop server test") >> test_run:cmd("cleanup server test") >> test_run:cmd("delete server test") --Apple-Mail=_45DBC573-22B2-4C84-A2DC-46ACEA9B6840 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Hi! = Thank you for review.

12 =D0=B0=D0=BF=D1=80. 2019 = =D0=B3., =D0=B2 17:00, Vladimir Davydov <vdavydov.dev@gmail.com> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0= =B0=D0=BB(=D0=B0):

On Thu, Apr = 11, 2019 at 04:37:20PM +0300, Serge Petrenko wrote:
Rewrite= the test so that it doesn't depend on timeouts, which can be
exceeded under high load, too much and simplify it a = little.

Also extract the whole part = regarding on_shutdown triggers into a
separate test, so = that its sporadic failures would be easier to
investigate.

Part of #4134
---
https://github.com/tarantool/tarantool/issues/4134
https://github.com/tarantool/tarantool/tree/sp/gh-4134-fix-on-s= hutdown-tests

test/box/misc.result =             &n= bsp;     | 161 -------------------------
test/box/misc.test.lua =             &n= bsp;   |  62 ----------
test/box/on_shutdown_triggers.result   | 160 = ++++++++++++++++++++++++
test/box/on_shutdown_triggers.test.lua |  62 = ++++++++++

Better call simply box/on_shutdown.test.lua.

Anyway, this patch is impossible = to review. Please split it in two - one
moves the test case to a separate file, another - fixes = it.

Will = do.
I managed to extract the diff. = Pasting it here for review.

@@ -33,7 +33,7 @@
box.space.shutdown:select{}
box.space.shutdown:drop()

--- = Check that os.exit invokes triggers
+-- Check that os.exit = invokes on_shutdown triggers
fiber =3D require("fiber")
test_run:cmd("create server test with = script=3D'box/proxy.lua'")
test_run:cmd("start server = test")
@@ -46,19 +46,17 @@
_ =3D = box.ctl.on_shutdown(function() print("on_shutdown 5") end)
-- Check that we don't hang infinitely after os.exit()
-- even if the following code doesn't yield.
-fiber =3D require("fiber")
-_ =3D = fiber.create(function() fiber.sleep(0.05) os.exit() while true do end = end)
+fiber =3D require('fiber')

This 'require' isn't needed anymore, no?

test_run:cmd("switch default")
+test_run:eval("test", "_ =3D fiber.new(function() os.exit() = while true do end end)")

This is supposed to stop the = default instance?!

No, it=E2=80=99s evaled on =E2=80=99test=E2=80=99, = which is the other instance. And that=E2=80=99s why we need the = require(=E2=80=98fiber=E2=80=99) above.
Anyways, will split the = patch in two and resend.

fiber.sleep(0.1)
-- The = server should be already stopped by os.exit(),
-- but = start doesn't work without a prior call to stop.
test_run:cmd("stop server test")
test_run:cmd("start server test")
-test_run:cmd("switch test")
-test_run:grep_log('test', 'on_shutdown 5', nil, = {noreset=3Dtrue})
+test_run:wait_log('test', 'on_shutdown = 5', nil, 5, {noreset=3Dtrue})
-- make sure we exited = because of os.exit(), not a signal.
test_run:grep_log('test', 'signal', nil, {noreset=3Dtrue})
-test_run:cmd("switch default")
test_run:cmd("stop server test")
test_run:cmd("cleanup server test")
test_run:cmd("delete server = test")

= --Apple-Mail=_45DBC573-22B2-4C84-A2DC-46ACEA9B6840--