From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 786AB43B38; Wed, 14 Feb 2024 17:51:50 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6825842E50; Wed, 14 Feb 2024 17:51:50 +0100 (CET) Received: from mail-pg1-f178.google.com (mail-pg1-f178.google.com [209.85.215.178]) by mails.dpdk.org (Postfix) with ESMTP id 4088042E1F for ; Wed, 14 Feb 2024 17:51:49 +0100 (CET) Received: by mail-pg1-f178.google.com with SMTP id 41be03b00d2f7-5c66b093b86so802517a12.0 for ; Wed, 14 Feb 2024 08:51:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1707929508; x=1708534308; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=KBFZPuMKFvXt6boKijWMbww88eOeyggEgtKKVE/Odk0=; b=RMNIff0A+tAVsx2w0C9sjed4xu43B5trQ5Mg4KhX9snYA2ZQ9MH3P38jS3gRYqX0io reNV6gSSFsy5iL7kcVdeEsJlejM3uBbP6sPhM7Kn93nkOz21dkmPDsOiAIGH6tWGYXkw mgm7vvR8ClOz/Fkv+mIFW5V0l4WbGnhntFiKQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707929508; x=1708534308; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=KBFZPuMKFvXt6boKijWMbww88eOeyggEgtKKVE/Odk0=; b=Yf6PQ1L1qO5fFZ+v+V8m7phoUYtLFnZPEoefqbJdoOtufKwkX4KaF0GFOFdORG9AGD gI8OJ80o7TIPhKdH+EvXD1lICBUO1AaQR/vVf10FkNP1WHmEEbQp8iuooksyHR8EH881 ixS5JuenqBrYgoqsfqaTnMg8Kcn92gJjkNW7sBtBu4IbZoEhRBPtTkA3Rin09p5tgCSb 8G35YTQvbWuvSo39OpjX4sgE+So9cqxom5U4N/ORot2ec3isvphjoND1YD+2n7xYZoWm YX4jVk5JctUKe0gQHM4B0W4+qF6HK3Hu9BAfA3ylw8QDIDkE/A+9UlcOkl4r0Rbd2TBy dUww== X-Forwarded-Encrypted: i=1; AJvYcCUW4TaqVoI0uuogyEUjunlH/Wp0XF9lNrXx7VkcaeVR9JciQkL4mxY3eQNWb922Pq6tKdYYcwSKvi6s5SI= X-Gm-Message-State: AOJu0Yw6LJhAFYKXOX72A5SxWft6Fm2e0tncrDzej5U4JlYmkEIDvhpH wVCRREbNp+f9GcftCRQBy0o4fnuUAuRGmMr/yk42mMaW4DSAddZrokpIVk3YeNdlGfC3g2JcTuF YkOvk/+uatJjmP6j7DcRTjhk8jAnAG2xR7d8Xzg== X-Google-Smtp-Source: AGHT+IFx1Xaq+ysyKSVRLYawDrYct8tj7V28At3jL/iDyE66NM05OgFWnPxjRgTuGriBjz4/F5LcLUCxl9W6A18/fkc= X-Received: by 2002:a17:90a:398f:b0:296:67b:1894 with SMTP id z15-20020a17090a398f00b00296067b1894mr3492222pjb.0.1707929508242; Wed, 14 Feb 2024 08:51:48 -0800 (PST) MIME-Version: 1.0 References: <20231220103331.60888-1-juraj.linkes@pantheon.tech> <20240206145716.71435-1-juraj.linkes@pantheon.tech> <20240206145716.71435-7-juraj.linkes@pantheon.tech> In-Reply-To: From: Jeremy Spewock Date: Wed, 14 Feb 2024 11:51:37 -0500 Message-ID: Subject: Re: [PATCH v2 6/7] dts: refactor logging configuration To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, probb@iol.unh.edu, paul.szczepanek@arm.com, Luca.Vizzarro@arm.com, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Wed, Feb 14, 2024 at 2:49=E2=80=AFAM Juraj Linke=C5=A1 wrote: > > Hi Jeremy, > > Just a reminder, please strip the parts you're not commenting on. > > > > + def __init__(self, *args, **kwargs): > > > + """Extend the constructor with extra file handlers.""" > > > + self._extra_file_handlers =3D [] > > > + super().__init__(*args, **kwargs) > > > > > > - One handler logs to the console, the other one to a file, wi= th either a regular or verbose > > > - format. > > > + def makeRecord(self, *args, **kwargs): > > > > Is the return type annotation here skipped because of the `:meta privat= e:`? > > > > Basically. We're modifying a method defined elsewhere, but there's no > harm in adding the return type. > > > > + logging.setLoggerClass(DTSLogger) > > > + if name: > > > + name =3D f"{dts_root_logger_name}.{name}" > > > + else: > > > + name =3D dts_root_logger_name > > > + logger =3D logging.getLogger(name) > > > + logging.setLoggerClass(logging.Logger) > > > > What's the benefit of setting the logger class back to logging.Logger > > here? Is the idea basically that if someone wanted to use the logging > > module we shouldn't implicitly make them use our DTSLogger? > > > > Yes. We should actually set it to whatever was there before (it may > not be logging.Logger), so I'll change that.+ > > > > > @@ -137,6 +141,7 @@ def run(self): > > > > > > # for all Execution sections > > > for execution in self._configuration.executions: > > > + self._logger.set_stage(DtsStage.execution) > > > > This ends up getting set twice in short succession which of course > > doesn't functionally cause a problem, but I don't exactly see the > > point of setting it twice. > > I'm not sure what you're referring to. The stage is only set once at > the start of each execution (and more generally, at the start of each > stage). It's also set in each finally block to properly mark the > cleanup of that stage. There are two finally blocks in the execution > stage where that could happen, but otherwise it should be set exactly > once. You're right, I misread where it was being set the second time in the code when I was reading this and thought there was a way where it could get set twice but it cannot. Apologies, it makes sense that you weren't sure what I was referring to because what I was referring to doesn't exist. > > > We could either set it here or set it in > > the _run_execution, but i think it makes more sense to set it in the > > _run_execution method as that is the method where we are doing the > > setup, here we are only initializing the nodes which is still in a > > sense "pre execution." > > Init nodes was pre-execution before the patch. I've moved it to > execution because of two reasons: > 1. The nodes are actually defined per-execution. It just made more > sense to move them to execution. Also, if an error happens while > connecting to a node, we don't want to abort the whole DTS run, just > the execution, as the next execution could be connecting to a > different node. > 2. We're not just doing node init here, we're also discovering which > test cases to run. This is essential to do as soon as possible (before > anything else happens in the execution, such as connecting the nodes) > so that we can mark blocked test cases in case of an error. Test case > discovery is definitely part of each execution and putting node init > after that was a natural consequence. There's an argument for > discovering test cases of all executions before actually running any > of the executions. It's certainly doable, but the code doesn't look > that good (when not modifying the original execution config (which we > shouldn't do) - I've tried) and there's actually not much of a point > to do it this way, the result is almost the same. Where it makes a > difference is when there's an error in test suite configuration and > later executions - the user would have to wait for the previous > execution to fully run to discover the error). > These are very good points. I was still thinking of these initializations as part of the pre-execution but I agree with you that the things we are doing are actually part of the execution. Thank you for elaborating! > > > >