On Wed, Nov 25, 2015 at 6:22 AM, Alvaro Herrera <alvhe...@2ndquadrant.com> wrote:
> Michael Paquier wrote: > > > - Manage node information using package/class PostgresNode.pm and have > > RecoveryTest use it. I have actually made PostgresNode bare-bone and > simple > > on purpose: one can initialize the node, append configuration parameters > to > > it and manage it through start/stop/restart (we may want to add reload > and > > promote actually if needed). > > This looks great as a starting point. I think we should make TestLib > depend on PostgresNode instead of the other way around. I will have a > look at that (I realize this means messing with the existing tests). > Makes sense. My thoughts following that is that we should keep a track of the nodes started as an array which is part of TestLib, with PGHOST set once at startup using tempdir_short. That's surely an refactoring patch somewhat independent of the recovery test suite. I would not mind writing something among those lines if needed. > > I have also arrived at the conclusion that it is not really worth > > adding a node status flag in PostgresNode because the port number > > saved there is sufficient when doing free port lookup, and the list of > > nodes used in a recovery test are saved in an array. > > I don't disagree with this in principle, but I think the design that you > get a new PostgresNode object by calling get_free_port is strange. I > think the port lookup code should be part of either TestLib or > PostgresNode, not RecoveryTest. > I'd vote for TestLib. I have written PostgresNode this way to allow users to set up arbitrary port numbers if they'd like to do so. That's more flexible. -- Michael
