On Sat, 27 May 2023 at 17:32, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 2023-05-26 Fr 20:35, vignesh C wrote:
>
> On Fri, 26 May 2023 at 04:09, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, May 25, 2023 at 06:34:20PM +0100, Dagfinn Ilmari Mannsåker wrote:
>
> However, none of the other functions in ::Utils know anything about node
> objects, which makes me think it should be a method on the node itself
> (i.e. in PostgreSQL::Test::Cluster) instead. Also, I think log_contains
> would be a better name, since it just returns a boolean. The name
> find_in_log makes me think it would return the log lines matching the
> pattern, or the position of the match in the file.
>
> In that case, the slurp_file() call would have to be fully qualified,
> since ::Cluster uses an empty import list to avoid polluting the method
> namespace with imported functions.
>
> Hmm. connect_ok() and connect_fails() in Cluster.pm have a similar
> log comparison logic, feeding from the offset of a log file. Couldn't
> you use the same code across the board for everything? Note that this
> stuff is parameterized so as it is possible to check if patterns match
> or do not match, for multiple patterns. It seems to me that we could
> use the new log finding routine there as well, so how about extending
> it a bit more? You would need, at least:
> - One parameter for log entries matching.
> - One parameter for log entries not matching.
>
> I felt adding these to log_contains was making the function slightly
> complex with multiple checks. I was not able to make it simple with
> the approach I tried. How about having a common function
> check_connect_log_contents which has the common log contents check for
> connect_ok and connect_fails function like the v2-0002 patch attached.
>
>
> + $offset = 0 unless defined $offset;
>
>
> This is unnecessary, as slurp_file() handles it appropriately, and in fact doing this is slightly inefficient, as it
willcause slurp_file to do a redundant seek.
>
> FYI there's a simpler way to say it if we wanted to:
>
> $offset //= 0;
Thanks for the comment, the attached v3 version patch has the changes
for the same.
Regards,
Vignesh