From 03b56ae5b90ece76c572ff313e8e7b718aec3848 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Thu, 4 Feb 2016 19:14:22 +0100 Subject: [PATCH] Code refactoring per review by Alex Shulgin. In particular, rename most of the functions that have leader in the name if they can be called in the context where the leader is None. --- patroni/dcs.py | 2 +- patroni/ha.py | 10 +++++----- patroni/postgresql.py | 8 ++++---- tests/test_postgresql.py | 6 +++--- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/patroni/dcs.py b/patroni/dcs.py index 62e5a654..a6b9d856 100644 --- a/patroni/dcs.py +++ b/patroni/dcs.py @@ -112,7 +112,7 @@ class Cluster(namedtuple('Cluster', 'initialize,leader,last_leader_operation,mem return not (self.leader and self.leader.name) def has_member(self, member_name): - return len([m for m in self.members if m.name == member_name]) > 0 + return any(m for m in self.members if m.name == member_name) class AbstractDCS: diff --git a/patroni/ha.py b/patroni/ha.py index 7ceac620..7e78eb77 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -62,7 +62,7 @@ class Ha: pass self.dcs.touch_member(json.dumps(data, separators=(',', ':'))) - def copy_backup_from_leader(self, leader): + def clone(self, leader): if self.state_handler.bootstrap(cluster_initialized=True, current_leader=leader): logger.info('bootstrapped from leader' if leader else 'bootstrapped without leader') else: @@ -73,7 +73,7 @@ class Ha: def bootstrap(self): if not self.cluster.is_unlocked(): # cluster already has leader self._async_executor.schedule('bootstrap from leader') - self._async_executor.run_async(self.copy_backup_from_leader, args=(self.cluster.leader, )) + self._async_executor.run_async(self.clone, args=(self.cluster.leader, )) return 'trying to bootstrap from leader' elif not self.cluster.initialize and not self.patroni.nofailover: # no initialize key if self.dcs.initialize(create_new=True): # race for initialization @@ -93,7 +93,7 @@ class Ha: return 'failed to acquire initialize lock' else: if self.state_handler.can_create_replica_without_leader(): - self._async_executor.run_async(self.copy_backup_from_leader, args=(None, )) + self._async_executor.run_async(self.clone, args=(None, )) return "trying to bootstrap without leader" return 'waiting for leader to bootstrap' @@ -120,7 +120,7 @@ class Ha: node_to_follow = node_to_follow[0] if node_to_follow else self.cluster.leader else: node_to_follow = self.cluster.leader - node_to_follow = None if (node_to_follow and node_to_follow.name) == self.state_handler.name else node_to_follow + node_to_follow = None if node_to_follow and node_to_follow.name == self.state_handler.name else node_to_follow if not self.state_handler.check_recovery_conf(node_to_follow) or recovery: self._async_executor.schedule('changing primary_conninfo and restarting') self._async_executor.run_async(self.state_handler.follow, (node_to_follow, recovery)) @@ -350,7 +350,7 @@ class Ha: def reinitialize(self, cluster): self.state_handler.stop('immediate') self.state_handler.remove_data_directory() - self.copy_backup_from_leader(cluster.leader) + self.clone(cluster.leader) def process_scheduled_action(self): if self.reinitialize_scheduled(): diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 293f7b51..113d8e31 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -219,7 +219,7 @@ class Postgresql: env['PGPASSFILE'] = self.pgpass return env - def sync_from_leader(self, leader): + def sync_replica(self, leader): if leader: r = parseurl(leader.conn_url) env = self.write_pgpass(r) if leader else os.environ.copy() @@ -657,8 +657,8 @@ $$""".format(name, options), name, password, password) # master), or if replicatefrom destination member happens to be the current master if self.role == 'master': slots = [m.name for m in cluster.members if m.name != self.name and - (not cluster.has_member(m.replicatefrom) - if m.replicatefrom and m.replicatefrom != self.name else True)] + (m.replicatefrom is None or m.replicatefrom == self.name or + not cluster.has_member(m.replicatefrom))] else: # only manage slots for replicas that want to replicate from this one slots = [m.name for m in cluster.members if m.replicatefrom == self.name] @@ -711,7 +711,7 @@ $$""".format(name, options), name, password, password) else: raise PostgresException("Could not bootstrap master PostgreSQL") else: - if self.sync_from_leader(current_leader): + if self.sync_replica(current_leader): self.restore_configuration_files() self.write_recovery_conf(current_leader, True) ret = self.start() diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index a82bde99..a88f150c 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -229,8 +229,8 @@ class TestPostgresql(unittest.TestCase): self.p.write_pgpass({'host': 'localhost', 'port': '5432', 'user': 'foo', 'password': 'bar'}) @patch('patroni.postgresql.Postgresql.write_pgpass', MagicMock(return_value=dict())) - def test_sync_from_leader(self): - self.assertTrue(self.p.sync_from_leader(self.leader)) + def test_sync_replica(self): + self.assertTrue(self.p.sync_replica(self.leader)) @patch('subprocess.call', side_effect=Exception("Test")) @patch('patroni.postgresql.Postgresql.write_pgpass', MagicMock(return_value=dict())) @@ -370,7 +370,7 @@ class TestPostgresql(unittest.TestCase): with patch('subprocess.call', Mock(return_value=1)): self.assertRaises(PostgresException, self.p.bootstrap) self.p.bootstrap() - with patch('patroni.postgresql.Postgresql.sync_from_leader', MagicMock(return_value=True)): + with patch('patroni.postgresql.Postgresql.sync_replica', MagicMock(return_value=True)): self.p.bootstrap(self.leader) def test_remove_data_directory(self):