From d82d74313b27405e2a0a59cb77d539ee5000f5ab Mon Sep 17 00:00:00 2001 From: Zoe Cai Date: Tue, 6 Jul 2021 14:26:41 +1000 Subject: [PATCH 1/8] feat: add configurable leader placement support --- google/cloud/spanner_v1/database.py | 11 +++++ tests/system/test_system.py | 71 +++++++++++++++++++++++++++++ tests/unit/test_client.py | 6 ++- tests/unit/test_database.py | 10 ++++ 4 files changed, 97 insertions(+), 1 deletion(-) diff --git a/google/cloud/spanner_v1/database.py b/google/cloud/spanner_v1/database.py index fae983f334..3d62737e03 100644 --- a/google/cloud/spanner_v1/database.py +++ b/google/cloud/spanner_v1/database.py @@ -144,6 +144,7 @@ def __init__( self._version_retention_period = None self._earliest_version_time = None self._encryption_info = None + self._default_leader = None self.log_commit_stats = False self._logger = logger self._encryption_config = encryption_config @@ -279,6 +280,15 @@ def encryption_info(self): """ return self._encryption_info + @property + def default_leader(self): + """The read-write region which contains the database's leader replicas. + + :rtype: str + :returns: a string representing the read-write region + """ + return self._default_leader + @property def ddl_statements(self): """DDL Statements used to define database schema. @@ -414,6 +424,7 @@ def reload(self): self._earliest_version_time = response.earliest_version_time self._encryption_config = response.encryption_config self._encryption_info = response.encryption_info + self._default_leader = response.default_leader def update_ddl(self, ddl_statements, operation_id=""): """Update DDL for this database. diff --git a/tests/system/test_system.py b/tests/system/test_system.py index ad2b8a9178..bab9eaf447 100644 --- a/tests/system/test_system.py +++ b/tests/system/test_system.py @@ -443,6 +443,46 @@ def test_create_database_pitr_success(self): for result in results: self.assertEqual(result[0], retention_period) + @unittest.skipIf( + USE_EMULATOR, "Default leader setting is not supported by the emulator" + ) + def test_create_database_with_default_leader_success(self): + pool = BurstyPool(labels={"testcase": "create_database_default_leader"}) + temp_db_id = "temp_db" + unique_resource_id("_") + default_leader = "us-east4" + ddl_statements = [ + "ALTER DATABASE {}" + " SET OPTIONS (default_leader = '{}')".format( + temp_db_id, default_leader + ) + ] + temp_db = Config.INSTANCE.database( + temp_db_id, pool=pool, ddl_statements=ddl_statements + ) + operation = temp_db.create() + self.to_delete.append(temp_db) + + # We want to make sure the operation completes. + operation.result(30) # raises on failure / timeout. + + database_ids = [database.name for database in Config.INSTANCE.list_databases()] + self.assertIn(temp_db.name, database_ids) + + temp_db.reload() + self.assertEqual(temp_db.default_reader, default_leader) + + with self.assertRaises(exceptions.InvalidArgument): + temp_db.create() + + with temp_db.snapshot() as snapshot: + results = snapshot.execute_sql( + "SELECT OPTION_VALUE AS default_leader " + "FROM INFORMATION_SCHEMA.DATABASE_OPTIONS " + "WHERE SCHEMA_NAME = '' AND OPTION_NAME = 'default_leader'" + ) + for result in results: + self.assertEqual(result[0], default_leader) + def test_table_not_found(self): temp_db_id = "temp_db" + unique_resource_id("_") @@ -551,6 +591,37 @@ def test_update_database_ddl_pitr_success(self): self.assertEqual(temp_db.version_retention_period, retention_period) self.assertEqual(len(temp_db.ddl_statements), len(ddl_statements)) + @unittest.skipIf( + USE_EMULATOR, "Default leader update is not supported by the emulator" + ) + def test_update_database_ddl_default_leader_success(self): + pool = BurstyPool(labels={"testcase": "update_database_ddl_default_leader"}) + temp_db_id = "temp_db" + unique_resource_id("_") + default_leader = "us-east4" + temp_db = Config.INSTANCE.database(temp_db_id, pool=pool) + create_op = temp_db.create() + self.to_delete.append(temp_db) + + # We want to make sure the operation completes. + create_op.result(240) # raises on failure / timeout. + + self.assertIsNone(temp_db.default_leader) + + ddl_statements = DDL_STATEMENTS + [ + "ALTER DATABASE {}" + " SET OPTIONS (default_leader = '{}')".format( + temp_db_id, default_leader + ) + ] + operation = temp_db.update_ddl(ddl_statements) + + # We want to make sure the operation completes. + operation.result(240) # raises on failure / timeout. + + temp_db.reload() + self.assertEqual(temp_db.default_leader, default_leader) + self.assertEqual(len(temp_db.ddl_statements), len(ddl_statements)) + def test_db_batch_insert_then_db_snapshot_read(self): retry = RetryInstanceState(_has_all_ddl) retry(self._db.reload)() diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index 2777fbc9a0..f3a744a96c 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -40,6 +40,7 @@ class TestClient(unittest.TestCase): PROCESSING_UNITS = 5000 LABELS = {"test": "true"} TIMEOUT_SECONDS = 80 + LEADER_OPTIONS = ["leader1", "leader2"] def _get_target_class(self): from google.cloud import spanner @@ -457,7 +458,9 @@ def test_list_instance_configs(self): instance_config_pbs = ListInstanceConfigsResponse( instance_configs=[ InstanceConfigPB( - name=self.CONFIGURATION_NAME, display_name=self.DISPLAY_NAME + name=self.CONFIGURATION_NAME, + display_name=self.DISPLAY_NAME, + leader_options=self.LEADER_OPTIONS ) ] ) @@ -473,6 +476,7 @@ def test_list_instance_configs(self): self.assertIsInstance(instance_config, InstanceConfigPB) self.assertEqual(instance_config.name, self.CONFIGURATION_NAME) self.assertEqual(instance_config.display_name, self.DISPLAY_NAME) + self.assertEqual(instance_config.leader_options, self.LEADER_OPTIONS) expected_metadata = ( ("google-cloud-resource-prefix", client.project_name), diff --git a/tests/unit/test_database.py b/tests/unit/test_database.py index 05e6f2b422..a4b7aa2425 100644 --- a/tests/unit/test_database.py +++ b/tests/unit/test_database.py @@ -333,6 +333,13 @@ def test_encryption_info(self): ] self.assertEqual(database.encryption_info, encryption_info) + def test_default_leader(self): + instance = _Instance(self.INSTANCE_NAME) + pool = _Pool() + database = self._make_one(self.DATABASE_ID, instance, pool=pool) + default_leader = database._default_leader = "us-east4" + self.assertEqual(database.default_leader, default_leader) + def test_spanner_api_property_w_scopeless_creds(self): client = _Client() @@ -715,6 +722,7 @@ def test_reload_success(self): kms_key_version="kms_key_version", ) ] + default_leader = "us-east4" api = client.database_admin_api = self._make_database_admin_api() api.get_database_ddl.return_value = ddl_pb db_pb = Database( @@ -725,6 +733,7 @@ def test_reload_success(self): earliest_version_time=_datetime_to_pb_timestamp(timestamp), encryption_config=encryption_config, encryption_info=encryption_info, + default_leader=default_leader, ) api.get_database.return_value = db_pb instance = _Instance(self.INSTANCE_NAME, client=client) @@ -740,6 +749,7 @@ def test_reload_success(self): self.assertEqual(database._ddl_statements, tuple(DDL_STATEMENTS)) self.assertEqual(database._encryption_config, encryption_config) self.assertEqual(database._encryption_info, encryption_info) + self.assertEqual(database._default_leader, default_leader) api.get_database_ddl.assert_called_once_with( database=self.DATABASE_NAME, From 35d8e3b0d40532323f0062a5c90c9dd1c514ca29 Mon Sep 17 00:00:00 2001 From: Zoe Cai Date: Wed, 7 Jul 2021 10:48:43 +1000 Subject: [PATCH 2/8] lint --- tests/system/test_system.py | 8 ++------ tests/unit/test_client.py | 2 +- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/tests/system/test_system.py b/tests/system/test_system.py index bab9eaf447..547629f361 100644 --- a/tests/system/test_system.py +++ b/tests/system/test_system.py @@ -452,9 +452,7 @@ def test_create_database_with_default_leader_success(self): default_leader = "us-east4" ddl_statements = [ "ALTER DATABASE {}" - " SET OPTIONS (default_leader = '{}')".format( - temp_db_id, default_leader - ) + " SET OPTIONS (default_leader = '{}')".format(temp_db_id, default_leader) ] temp_db = Config.INSTANCE.database( temp_db_id, pool=pool, ddl_statements=ddl_statements @@ -609,9 +607,7 @@ def test_update_database_ddl_default_leader_success(self): ddl_statements = DDL_STATEMENTS + [ "ALTER DATABASE {}" - " SET OPTIONS (default_leader = '{}')".format( - temp_db_id, default_leader - ) + " SET OPTIONS (default_leader = '{}')".format(temp_db_id, default_leader) ] operation = temp_db.update_ddl(ddl_statements) diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index f3a744a96c..68d8ea6857 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -460,7 +460,7 @@ def test_list_instance_configs(self): InstanceConfigPB( name=self.CONFIGURATION_NAME, display_name=self.DISPLAY_NAME, - leader_options=self.LEADER_OPTIONS + leader_options=self.LEADER_OPTIONS, ) ] ) From da35d3391df94f846cd35866593946812643d498 Mon Sep 17 00:00:00 2001 From: Zoe Cai Date: Thu, 22 Jul 2021 21:20:49 +1000 Subject: [PATCH 3/8] Create multi-regional instances --- tests/system/test_system.py | 39 +++++++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/tests/system/test_system.py b/tests/system/test_system.py index 547629f361..d72d42e0f9 100644 --- a/tests/system/test_system.py +++ b/tests/system/test_system.py @@ -358,11 +358,14 @@ def tearDownClass(cls): cls._db.drop() def setUp(self): + self.instances_to_delete = [] self.to_delete = [] def tearDown(self): for doomed in self.to_delete: doomed.drop() + for instance in self.instances_to_delete: + instance.delete() def test_list_databases(self): # Since `Config.INSTANCE` is newly created in `setUpModule`, the @@ -448,13 +451,28 @@ def test_create_database_pitr_success(self): ) def test_create_database_with_default_leader_success(self): pool = BurstyPool(labels={"testcase": "create_database_default_leader"}) + + # Create a multi-region instance + ALT_INSTANCE_ID = "new" + unique_resource_id("-") + config_name = "{}/instanceConfigs/cloud-devel-global-config".format( + Config.CLIENT.project_name + ) + instance = Config.CLIENT.instance( + instance_id=ALT_INSTANCE_ID, configuration_name=config_name + ) + operation = instance.create() + self.instances_to_delete.append(instance) + operation.result(SPANNER_OPERATION_TIMEOUT_IN_SECONDS) + + print(Config.CLIENT.list_instance_configs) + temp_db_id = "temp_db" + unique_resource_id("_") default_leader = "us-east4" ddl_statements = [ "ALTER DATABASE {}" " SET OPTIONS (default_leader = '{}')".format(temp_db_id, default_leader) ] - temp_db = Config.INSTANCE.database( + temp_db = instance.database( temp_db_id, pool=pool, ddl_statements=ddl_statements ) operation = temp_db.create() @@ -463,11 +481,11 @@ def test_create_database_with_default_leader_success(self): # We want to make sure the operation completes. operation.result(30) # raises on failure / timeout. - database_ids = [database.name for database in Config.INSTANCE.list_databases()] + database_ids = [database.name for database in instance.list_databases()] self.assertIn(temp_db.name, database_ids) temp_db.reload() - self.assertEqual(temp_db.default_reader, default_leader) + self.assertEqual(temp_db.default_leader, default_leader) with self.assertRaises(exceptions.InvalidArgument): temp_db.create() @@ -594,9 +612,22 @@ def test_update_database_ddl_pitr_success(self): ) def test_update_database_ddl_default_leader_success(self): pool = BurstyPool(labels={"testcase": "update_database_ddl_default_leader"}) + + # Create a multi-region instance + ALT_INSTANCE_ID = "new" + unique_resource_id("-") + config_name = "{}/instanceConfigs/cloud-devel-global-config".format( + Config.CLIENT.project_name + ) + instance = Config.CLIENT.instance( + instance_id=ALT_INSTANCE_ID, configuration_name=config_name + ) + operation = instance.create() + self.instances_to_delete.append(instance) + operation.result(SPANNER_OPERATION_TIMEOUT_IN_SECONDS) + temp_db_id = "temp_db" + unique_resource_id("_") default_leader = "us-east4" - temp_db = Config.INSTANCE.database(temp_db_id, pool=pool) + temp_db = instance.database(temp_db_id, pool=pool) create_op = temp_db.create() self.to_delete.append(temp_db) From 8f73d2c14b50d924aa231aa7a7006f0cc7ca427a Mon Sep 17 00:00:00 2001 From: Zoe Cai Date: Thu, 22 Jul 2021 22:22:32 +1000 Subject: [PATCH 4/8] try another print stmt --- tests/system/test_system.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/system/test_system.py b/tests/system/test_system.py index d72d42e0f9..bbfd1908f4 100644 --- a/tests/system/test_system.py +++ b/tests/system/test_system.py @@ -117,6 +117,8 @@ def setUpModule(): retry = RetryErrors(exceptions.ServiceUnavailable) configs = list(retry(Config.CLIENT.list_instance_configs)()) + print("zoe configs") + print(configs) instances = retry(_list_instances)() EXISTING_INSTANCES[:] = instances @@ -464,8 +466,6 @@ def test_create_database_with_default_leader_success(self): self.instances_to_delete.append(instance) operation.result(SPANNER_OPERATION_TIMEOUT_IN_SECONDS) - print(Config.CLIENT.list_instance_configs) - temp_db_id = "temp_db" + unique_resource_id("_") default_leader = "us-east4" ddl_statements = [ From 6bb7b0bea71dc2c45bd0ed5a437621d7e5d7cf48 Mon Sep 17 00:00:00 2001 From: Zoe Cai Date: Thu, 22 Jul 2021 23:38:49 +1000 Subject: [PATCH 5/8] try nam3 --- tests/system/test_system.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/system/test_system.py b/tests/system/test_system.py index bbfd1908f4..0e5dc05121 100644 --- a/tests/system/test_system.py +++ b/tests/system/test_system.py @@ -117,8 +117,6 @@ def setUpModule(): retry = RetryErrors(exceptions.ServiceUnavailable) configs = list(retry(Config.CLIENT.list_instance_configs)()) - print("zoe configs") - print(configs) instances = retry(_list_instances)() EXISTING_INSTANCES[:] = instances @@ -456,7 +454,7 @@ def test_create_database_with_default_leader_success(self): # Create a multi-region instance ALT_INSTANCE_ID = "new" + unique_resource_id("-") - config_name = "{}/instanceConfigs/cloud-devel-global-config".format( + config_name = "{}/instanceConfigs/nam3".format( Config.CLIENT.project_name ) instance = Config.CLIENT.instance( @@ -615,7 +613,7 @@ def test_update_database_ddl_default_leader_success(self): # Create a multi-region instance ALT_INSTANCE_ID = "new" + unique_resource_id("-") - config_name = "{}/instanceConfigs/cloud-devel-global-config".format( + config_name = "{}/instanceConfigs/nam3".format( Config.CLIENT.project_name ) instance = Config.CLIENT.instance( From e939fa7fbf5472f117f7150c5bd54fb0ccdc9810 Mon Sep 17 00:00:00 2001 From: Zoe Cai Date: Fri, 23 Jul 2021 15:27:24 +1000 Subject: [PATCH 6/8] variable for config --- tests/system/test_system.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/system/test_system.py b/tests/system/test_system.py index 0e5dc05121..311c76d849 100644 --- a/tests/system/test_system.py +++ b/tests/system/test_system.py @@ -453,9 +453,10 @@ def test_create_database_with_default_leader_success(self): pool = BurstyPool(labels={"testcase": "create_database_default_leader"}) # Create a multi-region instance + multi_region_config = "nam3" ALT_INSTANCE_ID = "new" + unique_resource_id("-") - config_name = "{}/instanceConfigs/nam3".format( - Config.CLIENT.project_name + config_name = "{}/instanceConfigs/{}".format( + Config.CLIENT.project_name, multi_region_config ) instance = Config.CLIENT.instance( instance_id=ALT_INSTANCE_ID, configuration_name=config_name @@ -612,9 +613,10 @@ def test_update_database_ddl_default_leader_success(self): pool = BurstyPool(labels={"testcase": "update_database_ddl_default_leader"}) # Create a multi-region instance + multi_region_config = "nam3" ALT_INSTANCE_ID = "new" + unique_resource_id("-") - config_name = "{}/instanceConfigs/nam3".format( - Config.CLIENT.project_name + config_name = "{}/instanceConfigs/{}".format( + Config.CLIENT.project_name, multi_region_config ) instance = Config.CLIENT.instance( instance_id=ALT_INSTANCE_ID, configuration_name=config_name From ac3f1f59a1ef5a9e1785d0650f055478bba4c4b9 Mon Sep 17 00:00:00 2001 From: Zoe Cai Date: Thu, 29 Jul 2021 11:32:59 +1000 Subject: [PATCH 7/8] review changes --- tests/system/test_system.py | 50 +++++++++++++------------------------ 1 file changed, 18 insertions(+), 32 deletions(-) diff --git a/tests/system/test_system.py b/tests/system/test_system.py index 311c76d849..7fac78925e 100644 --- a/tests/system/test_system.py +++ b/tests/system/test_system.py @@ -353,9 +353,24 @@ def setUpClass(cls): SPANNER_OPERATION_TIMEOUT_IN_SECONDS ) # raises on failure / timeout. + # Create a multi-region instance + multi_region_config = "nam3" + ALT_INSTANCE_ID = "new" + unique_resource_id("-") + config_name = "{}/instanceConfigs/{}".format( + Config.CLIENT.project_name, multi_region_config + ) + create_time = str(int(time.time())) + labels = {"python-spanner-systests": "true", "created": create_time} + cls._instance = Config.CLIENT.instance( + instance_id=ALT_INSTANCE_ID, configuration_name=config_name, labels=labels + ) + operation = cls._instance.create() + operation.result(SPANNER_OPERATION_TIMEOUT_IN_SECONDS) + @classmethod def tearDownClass(cls): cls._db.drop() + cls._instance.delete() def setUp(self): self.instances_to_delete = [] @@ -452,26 +467,13 @@ def test_create_database_pitr_success(self): def test_create_database_with_default_leader_success(self): pool = BurstyPool(labels={"testcase": "create_database_default_leader"}) - # Create a multi-region instance - multi_region_config = "nam3" - ALT_INSTANCE_ID = "new" + unique_resource_id("-") - config_name = "{}/instanceConfigs/{}".format( - Config.CLIENT.project_name, multi_region_config - ) - instance = Config.CLIENT.instance( - instance_id=ALT_INSTANCE_ID, configuration_name=config_name - ) - operation = instance.create() - self.instances_to_delete.append(instance) - operation.result(SPANNER_OPERATION_TIMEOUT_IN_SECONDS) - temp_db_id = "temp_db" + unique_resource_id("_") default_leader = "us-east4" ddl_statements = [ "ALTER DATABASE {}" " SET OPTIONS (default_leader = '{}')".format(temp_db_id, default_leader) ] - temp_db = instance.database( + temp_db = self._instance.database( temp_db_id, pool=pool, ddl_statements=ddl_statements ) operation = temp_db.create() @@ -480,15 +482,12 @@ def test_create_database_with_default_leader_success(self): # We want to make sure the operation completes. operation.result(30) # raises on failure / timeout. - database_ids = [database.name for database in instance.list_databases()] + database_ids = [database.name for database in self._instance.list_databases()] self.assertIn(temp_db.name, database_ids) temp_db.reload() self.assertEqual(temp_db.default_leader, default_leader) - with self.assertRaises(exceptions.InvalidArgument): - temp_db.create() - with temp_db.snapshot() as snapshot: results = snapshot.execute_sql( "SELECT OPTION_VALUE AS default_leader " @@ -612,22 +611,9 @@ def test_update_database_ddl_pitr_success(self): def test_update_database_ddl_default_leader_success(self): pool = BurstyPool(labels={"testcase": "update_database_ddl_default_leader"}) - # Create a multi-region instance - multi_region_config = "nam3" - ALT_INSTANCE_ID = "new" + unique_resource_id("-") - config_name = "{}/instanceConfigs/{}".format( - Config.CLIENT.project_name, multi_region_config - ) - instance = Config.CLIENT.instance( - instance_id=ALT_INSTANCE_ID, configuration_name=config_name - ) - operation = instance.create() - self.instances_to_delete.append(instance) - operation.result(SPANNER_OPERATION_TIMEOUT_IN_SECONDS) - temp_db_id = "temp_db" + unique_resource_id("_") default_leader = "us-east4" - temp_db = instance.database(temp_db_id, pool=pool) + temp_db = self._instance.database(temp_db_id, pool=pool) create_op = temp_db.create() self.to_delete.append(temp_db) From e61606b0c5fc0b4fc1ac6568d4d1fdae11dba101 Mon Sep 17 00:00:00 2001 From: Zoe Cai Date: Thu, 29 Jul 2021 12:53:16 +1000 Subject: [PATCH 8/8] review changes --- tests/system/test_system.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/system/test_system.py b/tests/system/test_system.py index 7fac78925e..845e79f805 100644 --- a/tests/system/test_system.py +++ b/tests/system/test_system.py @@ -68,6 +68,7 @@ INSTANCE_ID = os.environ.get( "GOOGLE_CLOUD_TESTS_SPANNER_INSTANCE", "google-cloud-python-systest" ) +MULTI_REGION_INSTANCE_ID = "multi-region" + unique_resource_id("-") EXISTING_INSTANCES = [] COUNTERS_TABLE = "counters" COUNTERS_COLUMNS = ("name", "value") @@ -355,14 +356,15 @@ def setUpClass(cls): # Create a multi-region instance multi_region_config = "nam3" - ALT_INSTANCE_ID = "new" + unique_resource_id("-") config_name = "{}/instanceConfigs/{}".format( Config.CLIENT.project_name, multi_region_config ) create_time = str(int(time.time())) labels = {"python-spanner-systests": "true", "created": create_time} cls._instance = Config.CLIENT.instance( - instance_id=ALT_INSTANCE_ID, configuration_name=config_name, labels=labels + instance_id=MULTI_REGION_INSTANCE_ID, + configuration_name=config_name, + labels=labels, ) operation = cls._instance.create() operation.result(SPANNER_OPERATION_TIMEOUT_IN_SECONDS) @@ -373,14 +375,11 @@ def tearDownClass(cls): cls._instance.delete() def setUp(self): - self.instances_to_delete = [] self.to_delete = [] def tearDown(self): for doomed in self.to_delete: doomed.drop() - for instance in self.instances_to_delete: - instance.delete() def test_list_databases(self): # Since `Config.INSTANCE` is newly created in `setUpModule`, the