)]}'
{"sahara/service/direct_engine.py":[{"author":{"_account_id":7125,"name":"Alexander Ignatov","email":"aignatov@mirantis.com","username":"aignatov"},"change_message_id":"2006a487297d8712b83b1816cbcc8bb2af7f68c4","unresolved":false,"context_lines":[{"line_number":56,"context_line":""},{"line_number":57,"context_line":"            self._await_active(cluster, instances)"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":"            if not g.check_cluster_exists(cluster):"},{"line_number":60,"context_line":"                LOG.info(g.format_cluster_deleted_message(cluster))"},{"line_number":61,"context_line":"                return"},{"line_number":62,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_c86c99a0","line":59,"updated":"2014-06-10 05:20:23.000000000","message":"Too much copypasted \u0027if not\u0027 code blocks. I think all of them could be extracted to single function. Probably general.py is appropriate place for that.","commit_id":"d429c22a85a8637d805a6f66716688bc3e969180"},{"author":{"_account_id":8411,"name":"Andrew Lazarev","email":"alazarev@mirantis.com","username":"alazarev"},"change_message_id":"4d779df4112c0b2a752ecfa4e14a51b9de3457c8","unresolved":false,"context_lines":[{"line_number":56,"context_line":""},{"line_number":57,"context_line":"            self._await_active(cluster, instances)"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":"            if not g.check_cluster_exists(cluster):"},{"line_number":60,"context_line":"                LOG.info(g.format_cluster_deleted_message(cluster))"},{"line_number":61,"context_line":"                return"},{"line_number":62,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_ee923df5","line":59,"in_reply_to":"1ae5cdf2_c86c99a0","updated":"2014-06-10 06:53:52.000000000","message":"This function is already exists. And its name surprisingly \u0027check_cluster_exists\u0027. :)\n\nWhat do you suggest to put into that function?","commit_id":"d429c22a85a8637d805a6f66716688bc3e969180"},{"author":{"_account_id":8411,"name":"Andrew Lazarev","email":"alazarev@mirantis.com","username":"alazarev"},"change_message_id":"85a5d402b8c21fb708485b73c334e5dd821044d2","unresolved":false,"context_lines":[{"line_number":56,"context_line":""},{"line_number":57,"context_line":"            self._await_active(cluster, instances)"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":"            if not g.check_cluster_exists(cluster):"},{"line_number":60,"context_line":"                LOG.info(g.format_cluster_deleted_message(cluster))"},{"line_number":61,"context_line":"                return"},{"line_number":62,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_79cd15b7","line":59,"in_reply_to":"1ae5cdf2_d91c696d","updated":"2014-06-10 07:07:25.000000000","message":"1. check_cluster_deleted \u003d\u003d not g.check_cluster_exists\n\n2. LOG instance and place of the call are important. They indicate which operation is canceled\n\n3. See how cluster state change is organized","commit_id":"d429c22a85a8637d805a6f66716688bc3e969180"},{"author":{"_account_id":7125,"name":"Alexander Ignatov","email":"aignatov@mirantis.com","username":"aignatov"},"change_message_id":"edae1f4fd0a595de1eeb061e2007dc0fc5eed47f","unresolved":false,"context_lines":[{"line_number":56,"context_line":""},{"line_number":57,"context_line":"            self._await_active(cluster, instances)"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":"            if not g.check_cluster_exists(cluster):"},{"line_number":60,"context_line":"                LOG.info(g.format_cluster_deleted_message(cluster))"},{"line_number":61,"context_line":"                return"},{"line_number":62,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_d91c696d","line":59,"in_reply_to":"1ae5cdf2_ee923df5","updated":"2014-06-10 06:59:25.000000000","message":"You could rename format_cluster_deleted_message and put all code under if statement to it.\n\nformat_cluster_deleted_message  -\u003e check_cluster_deleted for example.","commit_id":"d429c22a85a8637d805a6f66716688bc3e969180"},{"author":{"_account_id":7109,"name":"Dmitry Mescheryakov (dmitryme)","email":"dmescheryakov@mirantis.com","username":"dmitryme"},"change_message_id":"2d6943fe882c6ab99a0ee08ac27777bafcf3babb","unresolved":false,"context_lines":[{"line_number":66,"context_line":""},{"line_number":67,"context_line":"            if not g.check_cluster_exists(cluster):"},{"line_number":68,"context_line":"                LOG.info(g.format_cluster_deleted_message(cluster))"},{"line_number":69,"context_line":"                return"},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"            cluster \u003d conductor.cluster_get(ctx, cluster)"},{"line_number":72,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_5ab6593f","line":69,"updated":"2014-06-11 07:48:16.000000000","message":"For example, can you please explain why we should bother and check if cluster exists there. If cluster does not exist, just some exception will be thrown and caught in except clause, where it will be properly processed.\n\nIMO it makes sense to check if cluster exists only in except clauses and one more place I forgot about last time - infinite loops which will hand forever if cluster is deleted, like there: https://github.com/openstack/sahara/blob/master/sahara/service/engine.py#L99\n\nLets discuss it verbally, I actually had an idea that we should drop error processing / rollback from the engines and leave it only to the very high level - ops.py.","commit_id":"d429c22a85a8637d805a6f66716688bc3e969180"},{"author":{"_account_id":8411,"name":"Andrew Lazarev","email":"alazarev@mirantis.com","username":"alazarev"},"change_message_id":"e9cb3911144bd438c3ae2024b0f1346c6098fb24","unresolved":false,"context_lines":[{"line_number":66,"context_line":""},{"line_number":67,"context_line":"            if not g.check_cluster_exists(cluster):"},{"line_number":68,"context_line":"                LOG.info(g.format_cluster_deleted_message(cluster))"},{"line_number":69,"context_line":"                return"},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"            cluster \u003d conductor.cluster_get(ctx, cluster)"},{"line_number":72,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_3738b63c","line":69,"in_reply_to":"1ae5cdf2_5ab6593f","updated":"2014-06-11 15:16:55.000000000","message":"Exception will be logged on ERROR level. This is not something user expects when deleting cluster.","commit_id":"d429c22a85a8637d805a6f66716688bc3e969180"},{"author":{"_account_id":7109,"name":"Dmitry Mescheryakov (dmitryme)","email":"dmescheryakov@mirantis.com","username":"dmitryme"},"change_message_id":"2d6943fe882c6ab99a0ee08ac27777bafcf3babb","unresolved":false,"context_lines":[{"line_number":83,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":84,"context_line":"                if not g.check_cluster_exists(cluster):"},{"line_number":85,"context_line":"                    LOG.info(g.format_cluster_deleted_message(cluster))"},{"line_number":86,"context_line":"                    return"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"                self._log_operation_exception("},{"line_number":89,"context_line":"                    \"Can\u0027t start cluster \u0027%s\u0027 (reason: %s)\", cluster, ex)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_faad659c","line":86,"updated":"2014-06-11 07:48:16.000000000","message":"Note that \u0027return\u0027 will not work within save_and_reraise context. See the doc for details:\nhttps://github.com/openstack/sahara/blob/master/sahara/openstack/common/excutils.py#L47","commit_id":"d429c22a85a8637d805a6f66716688bc3e969180"},{"author":{"_account_id":8411,"name":"Andrew Lazarev","email":"alazarev@mirantis.com","username":"alazarev"},"change_message_id":"e9cb3911144bd438c3ae2024b0f1346c6098fb24","unresolved":false,"context_lines":[{"line_number":83,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":84,"context_line":"                if not g.check_cluster_exists(cluster):"},{"line_number":85,"context_line":"                    LOG.info(g.format_cluster_deleted_message(cluster))"},{"line_number":86,"context_line":"                    return"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"                self._log_operation_exception("},{"line_number":89,"context_line":"                    \"Can\u0027t start cluster \u0027%s\u0027 (reason: %s)\", cluster, ex)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_b743e6ad","line":86,"in_reply_to":"1ae5cdf2_faad659c","updated":"2014-06-11 15:16:55.000000000","message":"I know, that\u0027s why I prevent exceptions caused by missing cluster.","commit_id":"d429c22a85a8637d805a6f66716688bc3e969180"}]}