)]}'
{"octavia/controller/housekeeping/house_keeping.py":[{"author":{"_account_id":11628,"name":"Michael Johnson","email":"johnsomor@gmail.com","username":"johnsom"},"change_message_id":"dfad6497df436e4b905d3ddb83401e7827dee5b2","unresolved":false,"context_lines":[{"line_number":99,"context_line":"        expiring_lbs \u003d self.lb_repo.get_all_deleted_expiring_lb("},{"line_number":100,"context_line":"            session, exp_age\u003dexp_age)"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"        for lb in expiring_lbs:"},{"line_number":103,"context_line":"            LOG.info(\u0027Attempting to delete load balancer id : %s\u0027, lb.id)"},{"line_number":104,"context_line":"            self.lb_repo.delete(session, id\u003dlb.id)"},{"line_number":105,"context_line":"            LOG.info(\u0027Deleted load balancer id : %s\u0027, lb.id)"}],"source_content_type":"text/x-python","patch_set":4,"id":"ffd0ebdf_d2ca044e","line":102,"updated":"2019-01-02 23:06:29.000000000","message":"It seems a bit silly to walk this list.\nConsider logging the list and then doing a bulk delete (one call to the db).","commit_id":"04819c84183c7c6a152cd5fab478bc4b5b7f1422"},{"author":{"_account_id":6469,"name":"Carlos Gonçalves","display_name":"Carlos Goncalves","email":"cgoncalves@redhat.com","username":"cgoncalves"},"change_message_id":"33cba0fd7cbe1dd005b0ec0e62bea10e9040fbc2","unresolved":false,"context_lines":[{"line_number":99,"context_line":"        expiring_lbs \u003d self.lb_repo.get_all_deleted_expiring_lb("},{"line_number":100,"context_line":"            session, exp_age\u003dexp_age)"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"        for lb in expiring_lbs:"},{"line_number":103,"context_line":"            LOG.info(\u0027Attempting to delete load balancer id : %s\u0027, lb.id)"},{"line_number":104,"context_line":"            self.lb_repo.delete(session, id\u003dlb.id)"},{"line_number":105,"context_line":"            LOG.info(\u0027Deleted load balancer id : %s\u0027, lb.id)"}],"source_content_type":"text/x-python","patch_set":4,"id":"9fdfeff1_a00b5122","line":102,"in_reply_to":"9fdfeff1_ed9d6ebb","updated":"2019-02-19 15:34:53.000000000","message":"Three ways I see this could be handled, from simplicity/less optimized to more complex/optimized:\n\n1. Keep as is. lb_repo.delete flushes so we flush for each resource. It also makes another trip to the DB to query per delete.\n\n2. Use lb_repo.delete_batch instead. Today it doesn\u0027t flush, but should at the end...? Flushing at the end reduces number of interactions with DB.\n\n3. Remove get_all_deleted_expiring and for-loop, and create repo.delete_deleted_and_expiring where Query.delete is used. Single DB trip. Saves querying deleted and expiring resources followed by for-each delete.\n\nhttps://docs.sqlalchemy.org/en/latest/orm/query.html#sqlalchemy.orm.query.Query.delete","commit_id":"04819c84183c7c6a152cd5fab478bc4b5b7f1422"},{"author":{"_account_id":11628,"name":"Michael Johnson","email":"johnsomor@gmail.com","username":"johnsom"},"change_message_id":"39d72e2526c3db50114b503fbf093b76daf806d7","unresolved":false,"context_lines":[{"line_number":99,"context_line":"        expiring_lbs \u003d self.lb_repo.get_all_deleted_expiring_lb("},{"line_number":100,"context_line":"            session, exp_age\u003dexp_age)"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"        for lb in expiring_lbs:"},{"line_number":103,"context_line":"            LOG.info(\u0027Attempting to delete load balancer id : %s\u0027, lb.id)"},{"line_number":104,"context_line":"            self.lb_repo.delete(session, id\u003dlb.id)"},{"line_number":105,"context_line":"            LOG.info(\u0027Deleted load balancer id : %s\u0027, lb.id)"}],"source_content_type":"text/x-python","patch_set":4,"id":"9fdfeff1_ed9d6ebb","line":102,"in_reply_to":"bfdaf3ff_e7ced22b","updated":"2019-02-11 20:17:28.000000000","message":"Well, since the loop has no error handling, it\u0027s going to stop at the first failure anyway.\nI\u0027m not a hard -1 on this, just pointing out an opportunity for optimization.","commit_id":"04819c84183c7c6a152cd5fab478bc4b5b7f1422"},{"author":{"_account_id":6469,"name":"Carlos Gonçalves","display_name":"Carlos Goncalves","email":"cgoncalves@redhat.com","username":"cgoncalves"},"change_message_id":"9fcd29dd6685a3f3e9b8b169c4f31232cb55895b","unresolved":false,"context_lines":[{"line_number":99,"context_line":"        expiring_lbs \u003d self.lb_repo.get_all_deleted_expiring_lb("},{"line_number":100,"context_line":"            session, exp_age\u003dexp_age)"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"        for lb in expiring_lbs:"},{"line_number":103,"context_line":"            LOG.info(\u0027Attempting to delete load balancer id : %s\u0027, lb.id)"},{"line_number":104,"context_line":"            self.lb_repo.delete(session, id\u003dlb.id)"},{"line_number":105,"context_line":"            LOG.info(\u0027Deleted load balancer id : %s\u0027, lb.id)"}],"source_content_type":"text/x-python","patch_set":4,"id":"bfdaf3ff_e7ced22b","line":102,"in_reply_to":"ffd0ebdf_d2ca044e","updated":"2019-01-18 14:37:32.000000000","message":"I considered that. Reason I didn\u0027t do it was two fold:\n\n1. simpler code refactor, less changes\n2. what if a subset of expired LBs fail to delete?\n\nThere are caveats for bulk query deletes: https://docs.sqlalchemy.org/en/latest/orm/query.html","commit_id":"04819c84183c7c6a152cd5fab478bc4b5b7f1422"}],"octavia/db/repositories.py":[{"author":{"_account_id":11628,"name":"Michael Johnson","email":"johnsomor@gmail.com","username":"johnsom"},"change_message_id":"dfad6497df436e4b905d3ddb83401e7827dee5b2","unresolved":false,"context_lines":[{"line_number":189,"context_line":"        query \u003d query.options(noload(\u0027*\u0027))"},{"line_number":190,"context_line":"        model_list \u003d query.all()"},{"line_number":191,"context_line":""},{"line_number":192,"context_line":"        data_model_list \u003d [model.to_data_model() for model in model_list]"},{"line_number":193,"context_line":"        return data_model_list"},{"line_number":194,"context_line":""},{"line_number":195,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"ffd0ebdf_b2c18031","line":192,"updated":"2019-01-02 23:06:29.000000000","message":"This really isn\u0027t needed either and is a waste of CPU.","commit_id":"04819c84183c7c6a152cd5fab478bc4b5b7f1422"},{"author":{"_account_id":6469,"name":"Carlos Gonçalves","display_name":"Carlos Goncalves","email":"cgoncalves@redhat.com","username":"cgoncalves"},"change_message_id":"33cba0fd7cbe1dd005b0ec0e62bea10e9040fbc2","unresolved":false,"context_lines":[{"line_number":189,"context_line":"        query \u003d query.options(noload(\u0027*\u0027))"},{"line_number":190,"context_line":"        model_list \u003d query.all()"},{"line_number":191,"context_line":""},{"line_number":192,"context_line":"        data_model_list \u003d [model.to_data_model() for model in model_list]"},{"line_number":193,"context_line":"        return data_model_list"},{"line_number":194,"context_line":""},{"line_number":195,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"9fdfeff1_f5703e0b","line":192,"in_reply_to":"9fdfeff1_2d45b643","updated":"2019-02-19 15:34:53.000000000","message":"Done","commit_id":"04819c84183c7c6a152cd5fab478bc4b5b7f1422"},{"author":{"_account_id":11628,"name":"Michael Johnson","email":"johnsomor@gmail.com","username":"johnsom"},"change_message_id":"3d8fb6f602ba6d6be9b27237374961a93782d4e6","unresolved":false,"context_lines":[{"line_number":189,"context_line":"        query \u003d query.options(noload(\u0027*\u0027))"},{"line_number":190,"context_line":"        model_list \u003d query.all()"},{"line_number":191,"context_line":""},{"line_number":192,"context_line":"        data_model_list \u003d [model.to_data_model() for model in model_list]"},{"line_number":193,"context_line":"        return data_model_list"},{"line_number":194,"context_line":""},{"line_number":195,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"9fdfeff1_2d45b643","line":192,"in_reply_to":"bfdaf3ff_99aaca6e","updated":"2019-02-11 20:15:25.000000000","message":"My point here the code doesn\u0027t need the objects. It could just return the ID list, saving a bunch of memory and CPU.","commit_id":"04819c84183c7c6a152cd5fab478bc4b5b7f1422"},{"author":{"_account_id":6469,"name":"Carlos Gonçalves","display_name":"Carlos Goncalves","email":"cgoncalves@redhat.com","username":"cgoncalves"},"change_message_id":"9fcd29dd6685a3f3e9b8b169c4f31232cb55895b","unresolved":false,"context_lines":[{"line_number":189,"context_line":"        query \u003d query.options(noload(\u0027*\u0027))"},{"line_number":190,"context_line":"        model_list \u003d query.all()"},{"line_number":191,"context_line":""},{"line_number":192,"context_line":"        data_model_list \u003d [model.to_data_model() for model in model_list]"},{"line_number":193,"context_line":"        return data_model_list"},{"line_number":194,"context_line":""},{"line_number":195,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"bfdaf3ff_99aaca6e","line":192,"in_reply_to":"ffd0ebdf_b2c18031","updated":"2019-01-18 14:37:32.000000000","message":"If not translating from db model to data model here, then it would need to be translated in methods get_all_deleted_expiring_amphora and get_all_deleted_expiring_lb","commit_id":"04819c84183c7c6a152cd5fab478bc4b5b7f1422"},{"author":{"_account_id":11628,"name":"Michael Johnson","email":"johnsomor@gmail.com","username":"johnsom"},"change_message_id":"dfad6497df436e4b905d3ddb83401e7827dee5b2","unresolved":false,"context_lines":[{"line_number":792,"context_line":"            session.add(lb)"},{"line_number":793,"context_line":"            return True"},{"line_number":794,"context_line":""},{"line_number":795,"context_line":"    def get_all_deleted_expiring_lb(self, session, exp_age\u003dNone):"},{"line_number":796,"context_line":"        \"\"\"Get all previously deleted load balancers that are now expiring."},{"line_number":797,"context_line":""},{"line_number":798,"context_line":"        :param session: A Sql Alchemy database session."}],"source_content_type":"text/x-python","patch_set":4,"id":"ffd0ebdf_12988c68","line":795,"updated":"2019-01-02 23:06:29.000000000","message":"Is this even needed anymore? Can\u0027t we just remove it and use the method from the base?","commit_id":"04819c84183c7c6a152cd5fab478bc4b5b7f1422"},{"author":{"_account_id":6469,"name":"Carlos Gonçalves","display_name":"Carlos Goncalves","email":"cgoncalves@redhat.com","username":"cgoncalves"},"change_message_id":"33cba0fd7cbe1dd005b0ec0e62bea10e9040fbc2","unresolved":false,"context_lines":[{"line_number":792,"context_line":"            session.add(lb)"},{"line_number":793,"context_line":"            return True"},{"line_number":794,"context_line":""},{"line_number":795,"context_line":"    def get_all_deleted_expiring_lb(self, session, exp_age\u003dNone):"},{"line_number":796,"context_line":"        \"\"\"Get all previously deleted load balancers that are now expiring."},{"line_number":797,"context_line":""},{"line_number":798,"context_line":"        :param session: A Sql Alchemy database session."}],"source_content_type":"text/x-python","patch_set":4,"id":"9fdfeff1_b565d61f","line":795,"in_reply_to":"9fdfeff1_ad2446b0","updated":"2019-02-19 15:34:53.000000000","message":"Done","commit_id":"04819c84183c7c6a152cd5fab478bc4b5b7f1422"},{"author":{"_account_id":11628,"name":"Michael Johnson","email":"johnsomor@gmail.com","username":"johnsom"},"change_message_id":"3d8fb6f602ba6d6be9b27237374961a93782d4e6","unresolved":false,"context_lines":[{"line_number":792,"context_line":"            session.add(lb)"},{"line_number":793,"context_line":"            return True"},{"line_number":794,"context_line":""},{"line_number":795,"context_line":"    def get_all_deleted_expiring_lb(self, session, exp_age\u003dNone):"},{"line_number":796,"context_line":"        \"\"\"Get all previously deleted load balancers that are now expiring."},{"line_number":797,"context_line":""},{"line_number":798,"context_line":"        :param session: A Sql Alchemy database session."}],"source_content_type":"text/x-python","patch_set":4,"id":"9fdfeff1_ad2446b0","line":795,"in_reply_to":"bfdaf3ff_d942923e","updated":"2019-02-11 20:15:25.000000000","message":"No, the class LoadBalancerRepository(BaseRepository): is a child of the BaseRepository. So, as long as you call it via the LoadBalancerRepository it will still get it\u0027s model class, but use the code in the base. It\u0027s how all of our creates/gets work.\nBy adding get_all_deleted_expiring to BaseRepository, all of the object specific repos now have that method.","commit_id":"04819c84183c7c6a152cd5fab478bc4b5b7f1422"},{"author":{"_account_id":10273,"name":"Adam Harwell","email":"flux.adam@gmail.com","username":"rm_you"},"change_message_id":"3f3de1c08f4eb45918624c1faa9dc36ce59f3ff4","unresolved":false,"context_lines":[{"line_number":792,"context_line":"            session.add(lb)"},{"line_number":793,"context_line":"            return True"},{"line_number":794,"context_line":""},{"line_number":795,"context_line":"    def get_all_deleted_expiring_lb(self, session, exp_age\u003dNone):"},{"line_number":796,"context_line":"        \"\"\"Get all previously deleted load balancers that are now expiring."},{"line_number":797,"context_line":""},{"line_number":798,"context_line":"        :param session: A Sql Alchemy database session."}],"source_content_type":"text/x-python","patch_set":4,"id":"dfd5e7cf_f71a7d15","line":795,"in_reply_to":"ffd0ebdf_12988c68","updated":"2019-01-07 20:57:03.000000000","message":"I don\u0027t think so -- will just need to update the code in a few places to clean it up and make it actually use the base method, which is just a bit more work... Not too much though.","commit_id":"04819c84183c7c6a152cd5fab478bc4b5b7f1422"},{"author":{"_account_id":6469,"name":"Carlos Gonçalves","display_name":"Carlos Goncalves","email":"cgoncalves@redhat.com","username":"cgoncalves"},"change_message_id":"9fcd29dd6685a3f3e9b8b169c4f31232cb55895b","unresolved":false,"context_lines":[{"line_number":792,"context_line":"            session.add(lb)"},{"line_number":793,"context_line":"            return True"},{"line_number":794,"context_line":""},{"line_number":795,"context_line":"    def get_all_deleted_expiring_lb(self, session, exp_age\u003dNone):"},{"line_number":796,"context_line":"        \"\"\"Get all previously deleted load balancers that are now expiring."},{"line_number":797,"context_line":""},{"line_number":798,"context_line":"        :param session: A Sql Alchemy database session."}],"source_content_type":"text/x-python","patch_set":4,"id":"bfdaf3ff_d942923e","line":795,"in_reply_to":"ffd0ebdf_12988c68","updated":"2019-01-18 14:37:32.000000000","message":"Yes and no. We\u0027d be violating layered abstraction because now get_all_deleted_expiring would need to receive additional parameter: model_class of desired resource (LB, amphora, ...), and this would need to come from octavia/controller/housekeeping/house_keeping.py which should be agnostic to underlying DB bits.","commit_id":"04819c84183c7c6a152cd5fab478bc4b5b7f1422"},{"author":{"_account_id":11628,"name":"Michael Johnson","email":"johnsomor@gmail.com","username":"johnsom"},"change_message_id":"dfad6497df436e4b905d3ddb83401e7827dee5b2","unresolved":false,"context_lines":[{"line_number":1162,"context_line":"                return db_lb.to_data_model()"},{"line_number":1163,"context_line":"            return None"},{"line_number":1164,"context_line":""},{"line_number":1165,"context_line":"    def get_all_deleted_expiring_amphora(self, session, exp_age\u003dNone):"},{"line_number":1166,"context_line":""},{"line_number":1167,"context_line":"        \"\"\"Get all previously deleted amphora that are now expiring."},{"line_number":1168,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"ffd0ebdf_92ab1c74","line":1165,"updated":"2019-01-02 23:06:29.000000000","message":"Is this needed anymore? Can we just remove it and use the base method?","commit_id":"04819c84183c7c6a152cd5fab478bc4b5b7f1422"},{"author":{"_account_id":6469,"name":"Carlos Gonçalves","display_name":"Carlos Goncalves","email":"cgoncalves@redhat.com","username":"cgoncalves"},"change_message_id":"33cba0fd7cbe1dd005b0ec0e62bea10e9040fbc2","unresolved":false,"context_lines":[{"line_number":1162,"context_line":"                return db_lb.to_data_model()"},{"line_number":1163,"context_line":"            return None"},{"line_number":1164,"context_line":""},{"line_number":1165,"context_line":"    def get_all_deleted_expiring_amphora(self, session, exp_age\u003dNone):"},{"line_number":1166,"context_line":""},{"line_number":1167,"context_line":"        \"\"\"Get all previously deleted amphora that are now expiring."},{"line_number":1168,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"9fdfeff1_7548ce86","line":1165,"in_reply_to":"9fdfeff1_2d3056e8","updated":"2019-02-19 15:34:53.000000000","message":"Done","commit_id":"04819c84183c7c6a152cd5fab478bc4b5b7f1422"},{"author":{"_account_id":11628,"name":"Michael Johnson","email":"johnsomor@gmail.com","username":"johnsom"},"change_message_id":"3d8fb6f602ba6d6be9b27237374961a93782d4e6","unresolved":false,"context_lines":[{"line_number":1162,"context_line":"                return db_lb.to_data_model()"},{"line_number":1163,"context_line":"            return None"},{"line_number":1164,"context_line":""},{"line_number":1165,"context_line":"    def get_all_deleted_expiring_amphora(self, session, exp_age\u003dNone):"},{"line_number":1166,"context_line":""},{"line_number":1167,"context_line":"        \"\"\"Get all previously deleted amphora that are now expiring."},{"line_number":1168,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"9fdfeff1_2d3056e8","line":1165,"in_reply_to":"bfdaf3ff_596222b0","updated":"2019-02-11 20:15:25.000000000","message":"Same as above, I still think it should be removed.","commit_id":"04819c84183c7c6a152cd5fab478bc4b5b7f1422"},{"author":{"_account_id":6469,"name":"Carlos Gonçalves","display_name":"Carlos Goncalves","email":"cgoncalves@redhat.com","username":"cgoncalves"},"change_message_id":"9fcd29dd6685a3f3e9b8b169c4f31232cb55895b","unresolved":false,"context_lines":[{"line_number":1162,"context_line":"                return db_lb.to_data_model()"},{"line_number":1163,"context_line":"            return None"},{"line_number":1164,"context_line":""},{"line_number":1165,"context_line":"    def get_all_deleted_expiring_amphora(self, session, exp_age\u003dNone):"},{"line_number":1166,"context_line":""},{"line_number":1167,"context_line":"        \"\"\"Get all previously deleted amphora that are now expiring."},{"line_number":1168,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"bfdaf3ff_596222b0","line":1165,"in_reply_to":"ffd0ebdf_92ab1c74","updated":"2019-01-18 14:37:32.000000000","message":"See comment above.","commit_id":"04819c84183c7c6a152cd5fab478bc4b5b7f1422"},{"author":{"_account_id":11628,"name":"Michael Johnson","email":"johnsomor@gmail.com","username":"johnsom"},"change_message_id":"12ebf7f7ce398b4f044bdfdc9dab562e78b9062a","unresolved":false,"context_lines":[{"line_number":199,"context_line":"            query \u003d query.filter_by(operating_status\u003dconsts.DELETED)"},{"line_number":200,"context_line":"        # Do not load any relationship"},{"line_number":201,"context_line":"        query \u003d query.options(noload(\u0027*\u0027))"},{"line_number":202,"context_line":"        model_list \u003d query.all()"},{"line_number":203,"context_line":""},{"line_number":204,"context_line":"        id_list \u003d [model.id for model in model_list]"},{"line_number":205,"context_line":"        return id_list"}],"source_content_type":"text/x-python","patch_set":5,"id":"9fdfeff1_7edae64b","line":202,"updated":"2019-02-19 16:29:04.000000000","message":"nit: We could just ask the DB for the ID column here as an optimization.","commit_id":"be552d99a434057c44f784f08f3ebd8ad2844928"}]}