NON-JIRA: Cleaning unused old datastore entities during retention pro… #149
Conversation
radkomateusz
requested review from
marcin-kolda,
przemyslaw-jasinski,
jarekdrabek,
MZatorski and
marek-tabor
Pull Request Test Coverage Report for Build 1196
💛 - Coveralls |
radkomateusz
force-pushed
the
cleanup_not_used_datastore_entities_during_retention_process
branch
from
d02b2bd
to
df069fe
Compare
| self.assertTrue(Backup.get_by_key(backup2.key) is not None) | ||
|
|
||
| @freeze_time("2019-05-16") | ||
| def test_should_not_delete_table_and_backups_entities_if_table_not_exist_for_more_than_threshold_value_and_backups_are_not_deleted_yet( |
There was a problem hiding this comment.
missing test case when only 1 backup is deleted from the group
There was a problem hiding this comment.
there is no situation like removing one entity but leaving another. Everything stays, or everything is deleted (if all backups entities are marked as deleted and source table last seen is above grace deletion period)
| backup1.put() | ||
| backup2 = Backup( | ||
| parent=table.key, | ||
| last_modified=datetime(2017, 02, 1, 16, 30), |
…cess. Process for removing entities about tables not existing for a long time and their backup entities. After one month after removing last backup by retention process, the table entity and backup entities connected with them are deleted. Hardcoded constants for managing retention are now moved to configuration. Documentation and tests updated. Renaming filter name to not contain hardcoded value in class name.
radkomateusz
force-pushed
the
cleanup_not_used_datastore_entities_during_retention_process
branch
from
df069fe
to
b98996d
Compare
|
|
||
| retention_settings: | ||
|
|
||
| # young_old_generation_threshold_in_months - number of months from above retention process will left only one backup older than |
There was a problem hiding this comment.
"number of months from above retention process will left only one backup older than" -> "for all backups older than this number of months, retention process will delete all backups except the most recent one. This doesn't affect backups younger than this threshold"
| @@ -46,6 +46,13 @@ def default_restoration_project_id(self): | |||
| def projects_to_skip(self): | |||
| return self.__project_config['backup_settings'].get('projects_to_skip') | |||
|
|
|||
| @property | |||
| def grace_period_after_source_table_deletion_in_months(self): | |||
| return self.__project_config['retention_settings'].get('grace_period_after_source_table_deletion_in_months') \ | |||
There was a problem hiding this comment.
Maybe we could add some validation for integers larger than one?
| return self.__project_config['retention_settings'].get('grace_period_after_source_table_deletion_in_months') \ | ||
|
|
||
| @property | ||
| def young_old_generation_threshold_in_months(self): |
There was a problem hiding this comment.
I would also print all configuration during start up
| table_key = ndb.Key(urlsafe=url_safe_key) | ||
| backups_entities_to_delete = Backup.get_all_backups_sorted(table_key) | ||
| logging.info("Removing table and backup entites for table:'%s'", table_reference) | ||
| for backup in backups_entities_to_delete: |
There was a problem hiding this comment.
You can delete entities in bulk: https://cloud.google.com/appengine/docs/standard/python/ndb/creating-entities#Python_Deleting_entities_in_bulk
After successful deletion of backup entities, remove table entity
There was a problem hiding this comment.
You could also try to delete all of them within transaction (but there might be some limits for number of entities)
| def test(url_safe_key, existing_backups): | ||
| if existing_backups: | ||
| return False | ||
| if ShouldPerformDatastoreCleanupPredicate.\ |
There was a problem hiding this comment.
You can just return the value from method
| @@ -164,3 +165,89 @@ def test_should_not_perform_retention_if_no_backups(self, delete_table): | |||
|
|
|||
| # then | |||
| delete_table.assert_not_called() | |||
|
|
|||
| @freeze_time("2019-05-16") | |||
| def test_should_delete_both_table_and_backups_entities_if_table_not_exist_for_a_long_time_and_any_not_deleted_backup_exists( | |||
There was a problem hiding this comment.
test_should_delete_both_table_and_backups_entities_if_table_not_exist_for_a_long_time_and_any_not_deleted_backup_exists => test_should_delete_both_table_and_backups_entities_if_table_does_not_exist_for_a_long_time_and_there_is_no_not_deleted_backup
| self.assertTrue(Backup.get_by_key(backup2.key) is None) | ||
|
|
||
| @freeze_time("2019-05-16") | ||
| def test_should_not_delete_table_and_backups_entities_if_table_not_exist_for_less_than_threshold_value_and_backups_are_already_deleted( |
There was a problem hiding this comment.
test_should_not_delete_table_and_backups_entities_if_table_not_exist_for_less_than_threshold_value_and_backups_are_already_deleted => test_should_not_delete_table_and_backup_entities_if_table_does_not_exist_for_less_than_threshold_value_and_backups_are_already_deleted
|
We decided to chose more safety option of optimisation, which doesn't require deleting anything. |
radkomateusz
deleted the
cleanup_not_used_datastore_entities_during_retention_process
branch
…cess.
Process for removing entities about tables not existing for a long time and their backup entities.
After one month after removing last backup by retention process, the table entity and backup entities connected with them are deleted.
Hardcoded constants for managing retention are now moved to configuration.
Documentation and tests updated. Renaming filter name to not contain hardcoded value in class name.