From 2cdd78b8e6fe23a16714b62c7e910cd824155166 Mon Sep 17 00:00:00 2001 From: Sergi Delgado Segura Date: Tue, 10 Dec 2019 13:31:42 +0100 Subject: [PATCH] Simplifies the Cleaner and adds docstrings We were passing some unnecessary parameters to the Cleaner (locator) that could be derived from other data (uuid and appointments). Also standarises the order of the parameters to match the rest of the methods --- pisa/cleaner.py | 52 +++++++++++++++++++++++++++++++++------ pisa/responder.py | 2 +- pisa/watcher.py | 2 +- test/unit/test_cleaner.py | 8 +++--- 4 files changed, 49 insertions(+), 15 deletions(-) diff --git a/pisa/cleaner.py b/pisa/cleaner.py index d30e473..4e2f19a 100644 --- a/pisa/cleaner.py +++ b/pisa/cleaner.py @@ -2,13 +2,27 @@ from pisa.logger import Logger logger = Logger("Cleaner") -# Dictionaries in Python are "passed-by-reference", so no return is needed for the Cleaner" -# https://docs.python.org/3/faq/programming.html#how-do-i-write-a-function-with-output-parameters-call-by-reference - class Cleaner: + """ + The ``Cleaner`` is the class in charge of removing expired / completed data from the tower. + + Mutable objects (like dicts) are passed-by-reference in Python, so no return is needed for the Cleaner. + """ + @staticmethod def delete_expired_appointment(expired_appointments, appointments, locator_uuid_map, db_manager): + """ + Deletes appointments which ``end_time`` has been reached (with no trigger) both from memory + (:mod:`Watcher `) and disk. + + Args: + expired_appointments (list): a list of appointments to be deleted. + appointments (dict): a dictionary containing all the :mod:`Watcher ` appointments. + locator_uuid_map (dict): a ``locator:uuid`` map for the :mod:`Watcher ` appointments. + db_manager (DBManager): a :mod:`DBManager ` instance to interact with the database. + """ + for uuid in expired_appointments: locator = appointments[uuid].locator @@ -26,16 +40,26 @@ class Cleaner: db_manager.delete_watcher_appointment(uuid) @staticmethod - def delete_completed_appointment(locator, uuid, appointments, locator_uuid_map, db_manager): + def delete_completed_appointment(uuid, appointments, locator_uuid_map, db_manager): + """ + Deletes a triggered appointment from memory (:mod:`Watcher `) and flags it as triggered in disk. + + Args: + uuid (str): a unique 16-byte hex-encoded str that identifies the appointment. + appointments (dict): a dictionary containing all the :mod:`Watcher ` appointments. + locator_uuid_map (dict): a ``locator:uuid`` map for the :mod:`Watcher ` appointments. + db_manager (DBManager): a :mod:`DBManager ` instance to interact with the database. + """ + # Delete the appointment appointment = appointments.pop(uuid) # If there was only one appointment that matches the locator we can delete the whole list - if len(locator_uuid_map[locator]) == 1: - locator_uuid_map.pop(locator) + if len(locator_uuid_map[appointment.locator]) == 1: + locator_uuid_map.pop(appointment.locator) else: # Otherwise we just delete the appointment that matches locator:appointment_pos - locator_uuid_map[locator].remove(uuid) + locator_uuid_map[appointment.locator].remove(uuid) # DISCUSS: instead of deleting the appointment, we will mark it as triggered and delete it from both # the watcher's and responder's db after fulfilled @@ -43,7 +67,19 @@ class Cleaner: db_manager.store_watcher_appointment(uuid, appointment.to_json(triggered=True)) @staticmethod - def delete_completed_jobs(jobs, tx_job_map, completed_jobs, height, db_manager): + def delete_completed_jobs(completed_jobs, height, jobs, tx_job_map, db_manager): + """ + Deletes a completed job both from memory (:mod:`Responder `) and disk (from the + :mod:`Responder ` and :mod:`Watcher ` databases). + + Args: + jobs (dict): a dictionary containing all the :mod:`Responder ` jobs. + tx_job_map (dict): a ``penalty_txid:uuid`` map for the :mod:`Responder ` jobs. + completed_jobs (list): a list of completed jobs to be deleted. + height (int): the block height at which the jobs were completed. + db_manager (DBManager): a :mod:`DBManager ` instance to interact with the database. + """ + for uuid, confirmations in completed_jobs: logger.info( "Job completed. Appointment ended after reaching enough confirmations.", diff --git a/pisa/responder.py b/pisa/responder.py index fe59e8c..c8607b9 100644 --- a/pisa/responder.py +++ b/pisa/responder.py @@ -156,7 +156,7 @@ class Responder: txs_to_rebroadcast = self.get_txs_to_rebroadcast(txs) completed_jobs = self.get_completed_jobs(height) - Cleaner.delete_completed_jobs(self.jobs, self.tx_job_map, completed_jobs, height, self.db_manager) + Cleaner.delete_completed_jobs(completed_jobs, height, self.jobs, self.tx_job_map, self.db_manager) self.rebroadcast(txs_to_rebroadcast, block_hash) # NOTCOVERED diff --git a/pisa/watcher.py b/pisa/watcher.py index 83771e6..120f822 100644 --- a/pisa/watcher.py +++ b/pisa/watcher.py @@ -138,7 +138,7 @@ class Watcher: # Delete the appointment and update db Cleaner.delete_completed_appointment( - filtered_match["locator"], uuid, self.appointments, self.locator_uuid_map, self.db_manager + uuid, self.appointments, self.locator_uuid_map, self.db_manager ) # Register the last processed block for the watcher diff --git a/test/unit/test_cleaner.py b/test/unit/test_cleaner.py index 0431e69..c8935a6 100644 --- a/test/unit/test_cleaner.py +++ b/test/unit/test_cleaner.py @@ -96,9 +96,7 @@ def test_delete_completed_appointments(db_manager): uuids = list(appointments.keys()) for uuid in uuids: - Cleaner.delete_completed_appointment( - appointments[uuid].locator, uuid, appointments, locator_uuid_map, db_manager - ) + Cleaner.delete_completed_appointment(uuid, appointments, locator_uuid_map, db_manager) # All appointments should have been deleted assert len(appointments) == 0 @@ -118,7 +116,7 @@ def test_delete_completed_jobs_db_match(db_manager): completed_jobs = [(job, 6) for job in selected_jobs] - Cleaner.delete_completed_jobs(jobs, tx_job_map, completed_jobs, height, db_manager) + Cleaner.delete_completed_jobs(completed_jobs, height, jobs, tx_job_map, db_manager) assert not set(completed_jobs).issubset(jobs.keys()) @@ -156,5 +154,5 @@ def test_delete_completed_jobs_no_db_match(db_manager): completed_jobs = [(job, 6) for job in selected_jobs] # We should be able to delete the correct ones and not fail in the others - Cleaner.delete_completed_jobs(jobs, tx_job_map, completed_jobs, height, db_manager) + Cleaner.delete_completed_jobs(completed_jobs, height, jobs, tx_job_map, db_manager) assert not set(completed_jobs).issubset(jobs.keys())