On Sun, Jun 4, 2023 at 3:51 AM Michael Paquier <mich...@paquier.xyz> wrote: > > On Mon, May 29, 2023 at 07:49:52AM +0530, vignesh C wrote: > > Thanks for the comment, the attached v3 version patch has the changes > > for the same. > > -if (find_in_log( > - $node, $log_offset, > - qr/peer authentication is not supported on this platform/)) > +if ($node->log_contains( > + qr/peer authentication is not supported on this platform/), > + $log_offset,) > > This looks like a typo to me, the log offset is eaten. > > Except of that, I am on board with log_contains(). > > There are two things that bugged me with the refactoring related to > connect_ok and connect_fails: > - check_connect_log_contents() is a name too complicated. While > looking at that I have settled to a simpler log_check(), as we could > perfectly use that for something else than connections as long as we > want to check multiple patterns at once. > - The refactoring of the documentation for the routines of Cluster.pm > became incorrect. For example, the patch does not list anymore > log_like and log_unlike for connect_ok. > > With all that in mind I have hacked a few adjustments in a 0003, > though I agree with the separation between 0001 and 0002. > > 033_replay_tsp_drops and 019_replslot_limit are not new to v16, but > 003_peer.pl and 035_standby_logical_decoding.pl, making the number of > places where find_in_log() exists twice as much. So I would be > tempted to refactor these tests in v16. Perhaps anybody from the RMT > could comment? We've usually been quite flexible with the tests even > in beta. >
Personally, I don't see any problem to do this refactoring for v16. However, sometimes, we do decide to backpatch refactoring in tests to avoid backpatch effort. I am not completely sure if that is the case here. -- With Regards, Amit Kapila.
