)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"fd8fba8bea0e311caf87236bdd0cb0aced571850","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"71b0f551_a35f42b5","updated":"2022-07-05 20:03:53.000000000","message":"Hey Carlos - this fix is an improvement; i\u0027ve a suggestion inline that we can discuss and probably round it off with a release note asking folks not to set the two conflicting options for deletion of share servers to True","commit_id":"af7bc925771862873eaeae1d22e9e5f9bd2ea3e2"},{"author":{"_account_id":29632,"name":"Carlos Eduardo","email":"ces.eduardo98@gmail.com","username":"silvacarlos"},"change_message_id":"d133b64e8e287f6ebf71ddbb90d27267c8a40449","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"0d49c9bf_bc134b78","updated":"2022-08-01 21:39:24.000000000","message":"Thank you for the review\nAlso took the opportunity to write up some missing unit tests. Please check the latest changes inline","commit_id":"7b036c42443900bcf887f7953f4df72c52ddd338"},{"author":{"_account_id":36180,"name":"Gireesh Awasthi","display_name":"Gireesh","email":"gawasthi2010@gmail.com","username":"agireesh","status":"NetApp"},"change_message_id":"3a2f5176ed28c195b2626e829b0b37d4b39fb474","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":4,"id":"2dc3fe82_a51db1f3","updated":"2024-09-11 17:58:23.000000000","message":"Thanks for working on this. I have added few comments, also please add the release note for this patch.","commit_id":"7cdc3173973606f8926f2d01ff5f5bdcc3924d74"},{"author":{"_account_id":36180,"name":"Gireesh Awasthi","display_name":"Gireesh","email":"gawasthi2010@gmail.com","username":"agireesh","status":"NetApp"},"change_message_id":"e6704c0306c2bb60f1432c07c647c418b6f04eb0","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"3035dfc8_2aeb4c81","updated":"2024-09-13 06:11:06.000000000","message":"LGTM ..!","commit_id":"3ae5ea796fd2d498a4d01e5dd328bc20c493d4a7"},{"author":{"_account_id":32594,"name":"Ashley Rodriguez","email":"ashrod98@redhat.com","username":"ashrod98"},"change_message_id":"25983c20b3ba05a297b6e74855559547982a266c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"614608d0_fcd50023","updated":"2024-09-12 19:01:35.000000000","message":"LGTM, thanks!","commit_id":"3ae5ea796fd2d498a4d01e5dd328bc20c493d4a7"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"c1ce5741e68d6bc39948987143ed886a748fac11","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"39ff5675_c97c02fb","updated":"2024-09-12 18:47:27.000000000","message":"LGTM; Thank you Carlos!","commit_id":"3ae5ea796fd2d498a4d01e5dd328bc20c493d4a7"}],"manila/share/manager.py":[{"author":{"_account_id":29632,"name":"Carlos Eduardo","email":"ces.eduardo98@gmail.com","username":"silvacarlos"},"change_message_id":"4478745314d8daa80c3a1acc462467ea91f542fb","unresolved":true,"context_lines":[{"line_number":3506,"context_line":"        for server in servers:"},{"line_number":3507,"context_line":"            try:"},{"line_number":3508,"context_line":"                self.delete_share_server(ctxt, server)"},{"line_number":3509,"context_line":"            except Exception:"},{"line_number":3510,"context_line":"                server_id \u003d server[\u0027id\u0027]"},{"line_number":3511,"context_line":"                LOG.exception("},{"line_number":3512,"context_line":"                    f\u0027Failed to clean up share server with id {server_id}\u0027)"},{"line_number":3513,"context_line":""},{"line_number":3514,"context_line":"    @periodic_task.periodic_task("},{"line_number":3515,"context_line":"        spacing\u003dCONF.check_for_expired_shares_in_recycle_bin_interval)"}],"source_content_type":"text/x-python","patch_set":1,"id":"fa65a09e_8934e176","line":3512,"range":{"start_line":3509,"start_character":12,"end_line":3512,"end_character":75},"updated":"2022-05-04 22:05:18.000000000","message":"sounds fairly simple - unsure if we should do more when we have an exception. Thoughts?","commit_id":"af7bc925771862873eaeae1d22e9e5f9bd2ea3e2"},{"author":{"_account_id":29632,"name":"Carlos Eduardo","email":"ces.eduardo98@gmail.com","username":"silvacarlos"},"change_message_id":"d133b64e8e287f6ebf71ddbb90d27267c8a40449","unresolved":false,"context_lines":[{"line_number":3506,"context_line":"        for server in servers:"},{"line_number":3507,"context_line":"            try:"},{"line_number":3508,"context_line":"                self.delete_share_server(ctxt, server)"},{"line_number":3509,"context_line":"            except Exception:"},{"line_number":3510,"context_line":"                server_id \u003d server[\u0027id\u0027]"},{"line_number":3511,"context_line":"                LOG.exception("},{"line_number":3512,"context_line":"                    f\u0027Failed to clean up share server with id {server_id}\u0027)"},{"line_number":3513,"context_line":""},{"line_number":3514,"context_line":"    @periodic_task.periodic_task("},{"line_number":3515,"context_line":"        spacing\u003dCONF.check_for_expired_shares_in_recycle_bin_interval)"}],"source_content_type":"text/x-python","patch_set":1,"id":"ca048fc2_cd5250c1","line":3512,"range":{"start_line":3509,"start_character":12,"end_line":3512,"end_character":75},"in_reply_to":"42e283d9_b8336d0e","updated":"2022-08-01 21:39:24.000000000","message":"Done","commit_id":"af7bc925771862873eaeae1d22e9e5f9bd2ea3e2"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"fd8fba8bea0e311caf87236bdd0cb0aced571850","unresolved":true,"context_lines":[{"line_number":3506,"context_line":"        for server in servers:"},{"line_number":3507,"context_line":"            try:"},{"line_number":3508,"context_line":"                self.delete_share_server(ctxt, server)"},{"line_number":3509,"context_line":"            except Exception:"},{"line_number":3510,"context_line":"                server_id \u003d server[\u0027id\u0027]"},{"line_number":3511,"context_line":"                LOG.exception("},{"line_number":3512,"context_line":"                    f\u0027Failed to clean up share server with id {server_id}\u0027)"},{"line_number":3513,"context_line":""},{"line_number":3514,"context_line":"    @periodic_task.periodic_task("},{"line_number":3515,"context_line":"        spacing\u003dCONF.check_for_expired_shares_in_recycle_bin_interval)"}],"source_content_type":"text/x-python","patch_set":1,"id":"42e283d9_b8336d0e","line":3512,"range":{"start_line":3509,"start_character":12,"end_line":3512,"end_character":75},"in_reply_to":"fa65a09e_8934e176","updated":"2022-07-05 20:03:53.000000000","message":"This is a good fix in itself; but, it probably makes sense for us to improve \"delete_share_server\" to avoid failing hard if a race condition has occurred","commit_id":"af7bc925771862873eaeae1d22e9e5f9bd2ea3e2"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"fd8fba8bea0e311caf87236bdd0cb0aced571850","unresolved":true,"context_lines":[{"line_number":4250,"context_line":"            # of share_server_id field for share. If so, after lock realese"},{"line_number":4251,"context_line":"            # this method starts executing when amount of dependent shares"},{"line_number":4252,"context_line":"            # has been changed."},{"line_number":4253,"context_line":"            server_id \u003d share_server[\u0027id\u0027]"},{"line_number":4254,"context_line":"            shares \u003d self.db.share_instances_get_all_by_share_server("},{"line_number":4255,"context_line":"                context, server_id)"},{"line_number":4256,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"e8517161_ba99b83b","line":4253,"updated":"2022-07-05 20:03:53.000000000","message":"Perhaps make another round trip to the database here and check if the server is still present..","commit_id":"af7bc925771862873eaeae1d22e9e5f9bd2ea3e2"},{"author":{"_account_id":29632,"name":"Carlos Eduardo","email":"ces.eduardo98@gmail.com","username":"silvacarlos"},"change_message_id":"d133b64e8e287f6ebf71ddbb90d27267c8a40449","unresolved":false,"context_lines":[{"line_number":4250,"context_line":"            # of share_server_id field for share. If so, after lock realese"},{"line_number":4251,"context_line":"            # this method starts executing when amount of dependent shares"},{"line_number":4252,"context_line":"            # has been changed."},{"line_number":4253,"context_line":"            server_id \u003d share_server[\u0027id\u0027]"},{"line_number":4254,"context_line":"            shares \u003d self.db.share_instances_get_all_by_share_server("},{"line_number":4255,"context_line":"                context, server_id)"},{"line_number":4256,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"1c69f2ce_ddcad08d","line":4253,"in_reply_to":"e8517161_ba99b83b","updated":"2022-08-01 21:39:24.000000000","message":"Done","commit_id":"af7bc925771862873eaeae1d22e9e5f9bd2ea3e2"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"093b7526485aa9cdb9c12a49749a0991997c8669","unresolved":true,"context_lines":[{"line_number":3508,"context_line":"                self.delete_share_server(ctxt, server)"},{"line_number":3509,"context_line":"            except Exception as e:"},{"line_number":3510,"context_line":"                server_id \u003d server[\u0027id\u0027]"},{"line_number":3511,"context_line":"                LOG.exception("},{"line_number":3512,"context_line":"                    f\u0027Failed to clean up share server with id {server_id}. \u0027"},{"line_number":3513,"context_line":"                    f\u0027Reason: {str(e)}\u0027)"},{"line_number":3514,"context_line":""},{"line_number":3515,"context_line":"    @periodic_task.periodic_task("},{"line_number":3516,"context_line":"        spacing\u003dCONF.check_for_expired_shares_in_recycle_bin_interval)"}],"source_content_type":"text/x-python","patch_set":2,"id":"0c6a2f33_76bc54e6","line":3513,"range":{"start_line":3511,"start_character":16,"end_line":3513,"end_character":40},"updated":"2022-08-31 09:03:14.000000000","message":"if ShareServerNotFound is the exception, we don\u0027t need to log an exception here - it actually needs no action - somethng else already deleted the server we wanted to delete, so its not a problem?","commit_id":"7b036c42443900bcf887f7953f4df72c52ddd338"},{"author":{"_account_id":29632,"name":"Carlos Eduardo","email":"ces.eduardo98@gmail.com","username":"silvacarlos"},"change_message_id":"b4c261ff53e2108a01b1cab3ec78e61081d3e1bb","unresolved":false,"context_lines":[{"line_number":3508,"context_line":"                self.delete_share_server(ctxt, server)"},{"line_number":3509,"context_line":"            except Exception as e:"},{"line_number":3510,"context_line":"                server_id \u003d server[\u0027id\u0027]"},{"line_number":3511,"context_line":"                LOG.exception("},{"line_number":3512,"context_line":"                    f\u0027Failed to clean up share server with id {server_id}. \u0027"},{"line_number":3513,"context_line":"                    f\u0027Reason: {str(e)}\u0027)"},{"line_number":3514,"context_line":""},{"line_number":3515,"context_line":"    @periodic_task.periodic_task("},{"line_number":3516,"context_line":"        spacing\u003dCONF.check_for_expired_shares_in_recycle_bin_interval)"}],"source_content_type":"text/x-python","patch_set":2,"id":"939a335a_7ded8e2d","line":3513,"range":{"start_line":3511,"start_character":16,"end_line":3513,"end_character":40},"in_reply_to":"0c6a2f33_76bc54e6","updated":"2024-09-09 18:06:25.000000000","message":"Done","commit_id":"7b036c42443900bcf887f7953f4df72c52ddd338"},{"author":{"_account_id":36180,"name":"Gireesh Awasthi","display_name":"Gireesh","email":"gawasthi2010@gmail.com","username":"agireesh","status":"NetApp"},"change_message_id":"3a2f5176ed28c195b2626e829b0b37d4b39fb474","unresolved":true,"context_lines":[{"line_number":3766,"context_line":"        for server in servers:"},{"line_number":3767,"context_line":"            try:"},{"line_number":3768,"context_line":"                self.delete_share_server(ctxt, server)"},{"line_number":3769,"context_line":"            except Exception:"},{"line_number":3770,"context_line":"                return"},{"line_number":3771,"context_line":""},{"line_number":3772,"context_line":"    @periodic_task.periodic_task("}],"source_content_type":"text/x-python","patch_set":4,"id":"b5d5de7a_592b88be","line":3769,"range":{"start_line":3769,"start_character":19,"end_line":3769,"end_character":28},"updated":"2024-09-11 17:58:23.000000000","message":"nit: if possible can we except specific exception and then log the message also.","commit_id":"7cdc3173973606f8926f2d01ff5f5bdcc3924d74"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"c1ce5741e68d6bc39948987143ed886a748fac11","unresolved":false,"context_lines":[{"line_number":3766,"context_line":"        for server in servers:"},{"line_number":3767,"context_line":"            try:"},{"line_number":3768,"context_line":"                self.delete_share_server(ctxt, server)"},{"line_number":3769,"context_line":"            except Exception:"},{"line_number":3770,"context_line":"                return"},{"line_number":3771,"context_line":""},{"line_number":3772,"context_line":"    @periodic_task.periodic_task("}],"source_content_type":"text/x-python","patch_set":4,"id":"348e37fa_ca8bf5ec","line":3769,"range":{"start_line":3769,"start_character":19,"end_line":3769,"end_character":28},"in_reply_to":"820a3c27_45ed1d17","updated":"2024-09-12 18:47:27.000000000","message":"Done","commit_id":"7cdc3173973606f8926f2d01ff5f5bdcc3924d74"},{"author":{"_account_id":29632,"name":"Carlos Eduardo","email":"ces.eduardo98@gmail.com","username":"silvacarlos"},"change_message_id":"72011bee3cb74cdadf3b76964d1967400e8aeec2","unresolved":true,"context_lines":[{"line_number":3766,"context_line":"        for server in servers:"},{"line_number":3767,"context_line":"            try:"},{"line_number":3768,"context_line":"                self.delete_share_server(ctxt, server)"},{"line_number":3769,"context_line":"            except Exception:"},{"line_number":3770,"context_line":"                return"},{"line_number":3771,"context_line":""},{"line_number":3772,"context_line":"    @periodic_task.periodic_task("}],"source_content_type":"text/x-python","patch_set":4,"id":"cd352045_68c5fcaf","line":3769,"range":{"start_line":3769,"start_character":19,"end_line":3769,"end_character":28},"in_reply_to":"b5d5de7a_592b88be","updated":"2024-09-11 21:32:12.000000000","message":"So I was doing the logging and gouthamr asked me to removed in a previous comment:\nhttps://review.opendev.org/c/openstack/manila/+/840547/comment/0c6a2f33_76bc54e6/ :) made it specific for share server not found though. I believe that other exceptions would still be worth it raising (ShareServerInUse and others that might be a result of network deallocation). What do you guys think?","commit_id":"7cdc3173973606f8926f2d01ff5f5bdcc3924d74"},{"author":{"_account_id":36180,"name":"Gireesh Awasthi","display_name":"Gireesh","email":"gawasthi2010@gmail.com","username":"agireesh","status":"NetApp"},"change_message_id":"b2b4e35929d754a72752637d3efa831389cd006d","unresolved":true,"context_lines":[{"line_number":3766,"context_line":"        for server in servers:"},{"line_number":3767,"context_line":"            try:"},{"line_number":3768,"context_line":"                self.delete_share_server(ctxt, server)"},{"line_number":3769,"context_line":"            except Exception:"},{"line_number":3770,"context_line":"                return"},{"line_number":3771,"context_line":""},{"line_number":3772,"context_line":"    @periodic_task.periodic_task("}],"source_content_type":"text/x-python","patch_set":4,"id":"820a3c27_45ed1d17","line":3769,"range":{"start_line":3769,"start_character":19,"end_line":3769,"end_character":28},"in_reply_to":"cd352045_68c5fcaf","updated":"2024-09-12 11:59:47.000000000","message":"Yes, there might a chance that other exception can occur, then it is fine not to except specific exception and no need to log the message.","commit_id":"7cdc3173973606f8926f2d01ff5f5bdcc3924d74"},{"author":{"_account_id":36180,"name":"Gireesh Awasthi","display_name":"Gireesh","email":"gawasthi2010@gmail.com","username":"agireesh","status":"NetApp"},"change_message_id":"3a2f5176ed28c195b2626e829b0b37d4b39fb474","unresolved":true,"context_lines":[{"line_number":3767,"context_line":"            try:"},{"line_number":3768,"context_line":"                self.delete_share_server(ctxt, server)"},{"line_number":3769,"context_line":"            except Exception:"},{"line_number":3770,"context_line":"                return"},{"line_number":3771,"context_line":""},{"line_number":3772,"context_line":"    @periodic_task.periodic_task("},{"line_number":3773,"context_line":"        spacing\u003dCONF.check_for_expired_shares_in_recycle_bin_interval)"}],"source_content_type":"text/x-python","patch_set":4,"id":"ed9af18a_45239cc2","line":3770,"range":{"start_line":3770,"start_character":16,"end_line":3770,"end_character":22},"updated":"2024-09-11 17:58:23.000000000","message":"I my opinion, we have to use \u0027continue\u0027 instead of \u0027return\u0027 to make sure we are trying to delete other servers in list. with \u0027return\u0027 if we got the exception with first server, method will return form there and delete will not execute for other list elements.","commit_id":"7cdc3173973606f8926f2d01ff5f5bdcc3924d74"},{"author":{"_account_id":29632,"name":"Carlos Eduardo","email":"ces.eduardo98@gmail.com","username":"silvacarlos"},"change_message_id":"b49633a99aa0bd2cee812faf1ba6d0bed476331d","unresolved":false,"context_lines":[{"line_number":3767,"context_line":"            try:"},{"line_number":3768,"context_line":"                self.delete_share_server(ctxt, server)"},{"line_number":3769,"context_line":"            except Exception:"},{"line_number":3770,"context_line":"                return"},{"line_number":3771,"context_line":""},{"line_number":3772,"context_line":"    @periodic_task.periodic_task("},{"line_number":3773,"context_line":"        spacing\u003dCONF.check_for_expired_shares_in_recycle_bin_interval)"}],"source_content_type":"text/x-python","patch_set":4,"id":"202d3a3d_b9375703","line":3770,"range":{"start_line":3770,"start_character":16,"end_line":3770,"end_character":22},"in_reply_to":"c70b5166_772534cd","updated":"2024-09-11 21:32:39.000000000","message":"Done","commit_id":"7cdc3173973606f8926f2d01ff5f5bdcc3924d74"},{"author":{"_account_id":29632,"name":"Carlos Eduardo","email":"ces.eduardo98@gmail.com","username":"silvacarlos"},"change_message_id":"72011bee3cb74cdadf3b76964d1967400e8aeec2","unresolved":true,"context_lines":[{"line_number":3767,"context_line":"            try:"},{"line_number":3768,"context_line":"                self.delete_share_server(ctxt, server)"},{"line_number":3769,"context_line":"            except Exception:"},{"line_number":3770,"context_line":"                return"},{"line_number":3771,"context_line":""},{"line_number":3772,"context_line":"    @periodic_task.periodic_task("},{"line_number":3773,"context_line":"        spacing\u003dCONF.check_for_expired_shares_in_recycle_bin_interval)"}],"source_content_type":"text/x-python","patch_set":4,"id":"c70b5166_772534cd","line":3770,"range":{"start_line":3770,"start_character":16,"end_line":3770,"end_character":22},"in_reply_to":"ed9af18a_45239cc2","updated":"2024-09-11 21:32:12.000000000","message":"ah, yes! you\u0027re right. For a moment I forgot I was in a for loop. Thanks","commit_id":"7cdc3173973606f8926f2d01ff5f5bdcc3924d74"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"94a33a5df871579c5f630d4f65ab7bf5706de849","unresolved":true,"context_lines":[{"line_number":3763,"context_line":"        servers \u003d self.db.share_server_get_all_unused_deletable(ctxt,"},{"line_number":3764,"context_line":"                                                                self.host,"},{"line_number":3765,"context_line":"                                                                updated_before)"},{"line_number":3766,"context_line":"        for server in servers:"},{"line_number":3767,"context_line":"            try:"},{"line_number":3768,"context_line":"                self.delete_share_server(ctxt, server)"},{"line_number":3769,"context_line":"            except exception.ShareServerNotFound:"},{"line_number":3770,"context_line":"                continue"},{"line_number":3771,"context_line":""},{"line_number":3772,"context_line":"    @periodic_task.periodic_task("},{"line_number":3773,"context_line":"        spacing\u003dCONF.check_for_expired_shares_in_recycle_bin_interval)"}],"source_content_type":"text/x-python","patch_set":5,"id":"55479b68_0fdfedcb","line":3770,"range":{"start_line":3766,"start_character":8,"end_line":3770,"end_character":24},"updated":"2024-09-12 14:17:34.000000000","message":"switching to a specific exception is good; but, the method seems to raise others too; you could log an exception and continue to the next server in those cases too\n\n```suggestion\n        for server in servers:\n            try:\n                self.delete_share_server(ctxt, server)\n            except exception.ShareServerNotFound:\n                continue\n            except Exception:\n                LOG.exception(\"Unable to delete share server %s, will retry in the next run\", server[\u0027id\u0027])\n              \n```\nor something to that effect","commit_id":"1ca3298ef5c3d5e5ca48bf684a3d4b336d1ee8ca"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"c1ce5741e68d6bc39948987143ed886a748fac11","unresolved":false,"context_lines":[{"line_number":3763,"context_line":"        servers \u003d self.db.share_server_get_all_unused_deletable(ctxt,"},{"line_number":3764,"context_line":"                                                                self.host,"},{"line_number":3765,"context_line":"                                                                updated_before)"},{"line_number":3766,"context_line":"        for server in servers:"},{"line_number":3767,"context_line":"            try:"},{"line_number":3768,"context_line":"                self.delete_share_server(ctxt, server)"},{"line_number":3769,"context_line":"            except exception.ShareServerNotFound:"},{"line_number":3770,"context_line":"                continue"},{"line_number":3771,"context_line":""},{"line_number":3772,"context_line":"    @periodic_task.periodic_task("},{"line_number":3773,"context_line":"        spacing\u003dCONF.check_for_expired_shares_in_recycle_bin_interval)"}],"source_content_type":"text/x-python","patch_set":5,"id":"bc65f363_9452e7ac","line":3770,"range":{"start_line":3766,"start_character":8,"end_line":3770,"end_character":24},"in_reply_to":"55479b68_0fdfedcb","updated":"2024-09-12 18:47:27.000000000","message":"Done","commit_id":"1ca3298ef5c3d5e5ca48bf684a3d4b336d1ee8ca"}]}