)]}'
{"trove/common/apischema.py":[{"author":{"_account_id":13157,"name":"Zhao Chao","email":"zhaochao1984@gmail.com","username":"zhaochao"},"change_message_id":"2f15fb349b0b29199c62d1a3d3ca92c207b88bfa","unresolved":false,"context_lines":[{"line_number":259,"context_line":"                        \"type\": \"object\","},{"line_number":260,"context_line":"                        \"additionalProperties\": True,"},{"line_number":261,"context_line":"                        \"properties\": {"},{"line_number":262,"context_line":"                            \"num_configsvr\": volume_size,"},{"line_number":263,"context_line":"                            \"num_mongos\": volume_size,"},{"line_number":264,"context_line":"                            \"configsvr_volume_size\": volume_size,"},{"line_number":265,"context_line":"                            \"configsvr_volume_type\": non_empty_string,"},{"line_number":266,"context_line":"                            \"mongos_volume_size\": volume_size,"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_5a666757","line":263,"range":{"start_line":262,"start_character":0,"end_line":263,"end_character":54},"updated":"2018-07-19 04:17:35.000000000","message":"It\u0027s better to add a new scheme for this purpose(and maybe some limitations of the number?).","commit_id":"6dff0a56c78d510d7cebf5e7512cdc6501cac12b"},{"author":{"_account_id":22608,"name":"zhanggang","email":"zhanggang@cmss.chinamobile.com","username":"zhanggang"},"change_message_id":"eef2bab1cccedf7d6e39afc9e502157797323be4","unresolved":false,"context_lines":[{"line_number":259,"context_line":"                        \"type\": \"object\","},{"line_number":260,"context_line":"                        \"additionalProperties\": True,"},{"line_number":261,"context_line":"                        \"properties\": {"},{"line_number":262,"context_line":"                            \"num_configsvr\": volume_size,"},{"line_number":263,"context_line":"                            \"num_mongos\": volume_size,"},{"line_number":264,"context_line":"                            \"configsvr_volume_size\": volume_size,"},{"line_number":265,"context_line":"                            \"configsvr_volume_type\": non_empty_string,"},{"line_number":266,"context_line":"                            \"mongos_volume_size\": volume_size,"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_fe2030fc","line":263,"range":{"start_line":262,"start_character":0,"end_line":263,"end_character":54},"in_reply_to":"5f7c97a3_5a666757","updated":"2018-07-23 05:50:18.000000000","message":"agree, the \"volume_size\" is not suitable. I just set the minimum\u003d1 and not set the limitations of maximum, if we have a widely_used max-value, I could update again.","commit_id":"6dff0a56c78d510d7cebf5e7512cdc6501cac12b"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"261efe14a76bd10389390477184353e7fac345e9","unresolved":false,"context_lines":[{"line_number":77,"context_line":"        configuration_positive_integer]"},{"line_number":78,"context_line":"}"},{"line_number":79,"context_line":""},{"line_number":80,"context_line":"number_of_node \u003d {"},{"line_number":81,"context_line":"    \"oneOf\": ["},{"line_number":82,"context_line":"        {"},{"line_number":83,"context_line":"            \"type\": \"integer\","}],"source_content_type":"text/x-python","patch_set":4,"id":"5f7c97a3_83f19c03","line":80,"updated":"2018-07-23 12:50:15.000000000","message":"number_of_node -\u003e number_of_nodes","commit_id":"84c123d6bdf75ade839beb37b33324315e63d005"}],"trove/common/cfg.py":[{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"174d141b8de069506014bfc8162dd6121a3625bf","unresolved":false,"context_lines":[{"line_number":1127,"context_line":"               help\u003d\u0027Character length of generated passwords.\u0027,"},{"line_number":1128,"context_line":"               deprecated_name\u003d\u0027default_password_length\u0027,"},{"line_number":1129,"context_line":"               deprecated_group\u003d\u0027DEFAULT\u0027),"},{"line_number":1130,"context_line":"    cfg.IntOpt(\u0027query_routers_volume_size\u0027, default\u003d10,"},{"line_number":1131,"context_line":"               help\u003d\u0027Default volume_size for query routers (mongos)\u0027),"},{"line_number":1132,"context_line":"    cfg.IntOpt(\u0027config_servers_volume_size\u0027, default\u003d10,"},{"line_number":1133,"context_line":"               help\u003d\u0027Default volume_size for config_servers\u0027),"}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa0c359_ba05c47c","line":1130,"updated":"2018-02-05 21:03:32.000000000","message":"Whats the unit?","commit_id":"7976d7a0dbdf29f4f57d243d24ad55b2fc4834fa"},{"author":{"_account_id":22608,"name":"zhanggang","email":"zhanggang@cmss.chinamobile.com","username":"zhanggang"},"change_message_id":"a40ec34c11bc2cd16c9965be727faa2fe471fe2b","unresolved":false,"context_lines":[{"line_number":1127,"context_line":"               help\u003d\u0027Character length of generated passwords.\u0027,"},{"line_number":1128,"context_line":"               deprecated_name\u003d\u0027default_password_length\u0027,"},{"line_number":1129,"context_line":"               deprecated_group\u003d\u0027DEFAULT\u0027),"},{"line_number":1130,"context_line":"    cfg.IntOpt(\u0027query_routers_volume_size\u0027, default\u003d10,"},{"line_number":1131,"context_line":"               help\u003d\u0027Default volume_size for query routers (mongos)\u0027),"},{"line_number":1132,"context_line":"    cfg.IntOpt(\u0027config_servers_volume_size\u0027, default\u003d10,"},{"line_number":1133,"context_line":"               help\u003d\u0027Default volume_size for config_servers\u0027),"}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa0c359_1a7631a6","line":1130,"in_reply_to":"3fa0c359_ba05c47c","updated":"2018-02-06 02:09:41.000000000","message":"Done","commit_id":"7976d7a0dbdf29f4f57d243d24ad55b2fc4834fa"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"174d141b8de069506014bfc8162dd6121a3625bf","unresolved":false,"context_lines":[{"line_number":1128,"context_line":"               deprecated_name\u003d\u0027default_password_length\u0027,"},{"line_number":1129,"context_line":"               deprecated_group\u003d\u0027DEFAULT\u0027),"},{"line_number":1130,"context_line":"    cfg.IntOpt(\u0027query_routers_volume_size\u0027, default\u003d10,"},{"line_number":1131,"context_line":"               help\u003d\u0027Default volume_size for query routers (mongos)\u0027),"},{"line_number":1132,"context_line":"    cfg.IntOpt(\u0027config_servers_volume_size\u0027, default\u003d10,"},{"line_number":1133,"context_line":"               help\u003d\u0027Default volume_size for config_servers\u0027),"},{"line_number":1134,"context_line":"]"}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa0c359_da0a1089","line":1131,"updated":"2018-02-05 21:03:32.000000000","message":"Whats the unit?","commit_id":"7976d7a0dbdf29f4f57d243d24ad55b2fc4834fa"},{"author":{"_account_id":22608,"name":"zhanggang","email":"zhanggang@cmss.chinamobile.com","username":"zhanggang"},"change_message_id":"a40ec34c11bc2cd16c9965be727faa2fe471fe2b","unresolved":false,"context_lines":[{"line_number":1128,"context_line":"               deprecated_name\u003d\u0027default_password_length\u0027,"},{"line_number":1129,"context_line":"               deprecated_group\u003d\u0027DEFAULT\u0027),"},{"line_number":1130,"context_line":"    cfg.IntOpt(\u0027query_routers_volume_size\u0027, default\u003d10,"},{"line_number":1131,"context_line":"               help\u003d\u0027Default volume_size for query routers (mongos)\u0027),"},{"line_number":1132,"context_line":"    cfg.IntOpt(\u0027config_servers_volume_size\u0027, default\u003d10,"},{"line_number":1133,"context_line":"               help\u003d\u0027Default volume_size for config_servers\u0027),"},{"line_number":1134,"context_line":"]"}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa0c359_3a7bed8c","line":1131,"in_reply_to":"3fa0c359_da0a1089","updated":"2018-02-06 02:09:41.000000000","message":"Done","commit_id":"7976d7a0dbdf29f4f57d243d24ad55b2fc4834fa"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"261efe14a76bd10389390477184353e7fac345e9","unresolved":false,"context_lines":[{"line_number":1083,"context_line":"    cfg.IntOpt(\u0027query_routers_volume_size\u0027, default\u003d10,"},{"line_number":1084,"context_line":"               help\u003d\u0027Default volume_size (in GB) for query routers (mongos)\u0027),"},{"line_number":1085,"context_line":"    cfg.IntOpt(\u0027config_servers_volume_size\u0027, default\u003d10,"},{"line_number":1086,"context_line":"               help\u003d\u0027Default volume_size (in GB) for config_servers\u0027),"},{"line_number":1087,"context_line":"    cfg.BoolOpt(\u0027cluster_support\u0027, default\u003dTrue,"},{"line_number":1088,"context_line":"                help\u003d\u0027Enable clusters to be created and managed.\u0027),"},{"line_number":1089,"context_line":"    cfg.BoolOpt(\u0027cluster_secure\u0027, default\u003dTrue,"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f7c97a3_43bce406","line":1086,"updated":"2018-07-23 12:50:15.000000000","message":"End help messages with dots.","commit_id":"84c123d6bdf75ade839beb37b33324315e63d005"}],"trove/common/strategies/cluster/experimental/mongodb/api.py":[{"author":{"_account_id":13157,"name":"Zhao Chao","email":"zhaochao1984@gmail.com","username":"zhaochao"},"change_message_id":"2f15fb349b0b29199c62d1a3d3ca92c207b88bfa","unresolved":false,"context_lines":[{"line_number":95,"context_line":"        if extends.get(\u0027configsvr_volume_size\u0027, None):"},{"line_number":96,"context_line":"            configsvr_vsize \u003d int(extends[\u0027configsvr_volume_size\u0027])"},{"line_number":97,"context_line":"        else:"},{"line_number":98,"context_line":"            configsvr_vsize \u003d 10  # default to 10(GB)"},{"line_number":99,"context_line":"        if extends.get(\u0027configsvr_volume_type\u0027, None):"},{"line_number":100,"context_line":"            configsvr_vtype \u003d extends[\u0027configsvr_volume_type\u0027]"},{"line_number":101,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_3a004b35","line":98,"range":{"start_line":98,"start_character":12,"end_line":98,"end_character":53},"updated":"2018-07-19 04:17:35.000000000","message":"What about also giving a configuration for this?","commit_id":"6dff0a56c78d510d7cebf5e7512cdc6501cac12b"},{"author":{"_account_id":22608,"name":"zhanggang","email":"zhanggang@cmss.chinamobile.com","username":"zhanggang"},"change_message_id":"eef2bab1cccedf7d6e39afc9e502157797323be4","unresolved":false,"context_lines":[{"line_number":95,"context_line":"        if extends.get(\u0027configsvr_volume_size\u0027, None):"},{"line_number":96,"context_line":"            configsvr_vsize \u003d int(extends[\u0027configsvr_volume_size\u0027])"},{"line_number":97,"context_line":"        else:"},{"line_number":98,"context_line":"            configsvr_vsize \u003d 10  # default to 10(GB)"},{"line_number":99,"context_line":"        if extends.get(\u0027configsvr_volume_type\u0027, None):"},{"line_number":100,"context_line":"            configsvr_vtype \u003d extends[\u0027configsvr_volume_type\u0027]"},{"line_number":101,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_de17749d","line":98,"range":{"start_line":98,"start_character":12,"end_line":98,"end_character":53},"in_reply_to":"5f7c97a3_3a004b35","updated":"2018-07-23 05:50:18.000000000","message":"Done","commit_id":"6dff0a56c78d510d7cebf5e7512cdc6501cac12b"},{"author":{"_account_id":13157,"name":"Zhao Chao","email":"zhaochao1984@gmail.com","username":"zhaochao"},"change_message_id":"2f15fb349b0b29199c62d1a3d3ca92c207b88bfa","unresolved":false,"context_lines":[{"line_number":103,"context_line":"        if extends.get(\u0027mongos_volume_size\u0027, None):"},{"line_number":104,"context_line":"            mongos_vsize \u003d int(extends[\u0027mongos_volume_size\u0027])"},{"line_number":105,"context_line":"        else:"},{"line_number":106,"context_line":"            mongos_vsize \u003d 10"},{"line_number":107,"context_line":"        if extends.get(\u0027mongos_volume_type\u0027, None):"},{"line_number":108,"context_line":"            mongos_vtype \u003d extends[\u0027mongos_volume_type\u0027]"},{"line_number":109,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_ba48db89","line":106,"range":{"start_line":106,"start_character":12,"end_line":106,"end_character":29},"updated":"2018-07-19 04:17:35.000000000","message":"Same as configsvr_volume_size here, it would be better for an configuration opt than hard-coded numbers.","commit_id":"6dff0a56c78d510d7cebf5e7512cdc6501cac12b"},{"author":{"_account_id":22608,"name":"zhanggang","email":"zhanggang@cmss.chinamobile.com","username":"zhanggang"},"change_message_id":"eef2bab1cccedf7d6e39afc9e502157797323be4","unresolved":false,"context_lines":[{"line_number":103,"context_line":"        if extends.get(\u0027mongos_volume_size\u0027, None):"},{"line_number":104,"context_line":"            mongos_vsize \u003d int(extends[\u0027mongos_volume_size\u0027])"},{"line_number":105,"context_line":"        else:"},{"line_number":106,"context_line":"            mongos_vsize \u003d 10"},{"line_number":107,"context_line":"        if extends.get(\u0027mongos_volume_type\u0027, None):"},{"line_number":108,"context_line":"            mongos_vtype \u003d extends[\u0027mongos_volume_type\u0027]"},{"line_number":109,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_7e72e01a","line":106,"range":{"start_line":106,"start_character":12,"end_line":106,"end_character":29},"in_reply_to":"5f7c97a3_ba48db89","updated":"2018-07-23 05:50:18.000000000","message":"Done","commit_id":"6dff0a56c78d510d7cebf5e7512cdc6501cac12b"},{"author":{"_account_id":13157,"name":"Zhao Chao","email":"zhaochao1984@gmail.com","username":"zhaochao"},"change_message_id":"2f15fb349b0b29199c62d1a3d3ca92c207b88bfa","unresolved":false,"context_lines":[{"line_number":111,"context_line":""},{"line_number":112,"context_line":"        req_volume_size \u003d models.get_required_volume_size("},{"line_number":113,"context_line":"            instances, mongo_conf.volume_support)"},{"line_number":114,"context_line":"        if mongo_conf.volume_support:"},{"line_number":115,"context_line":"            req_volume_size +\u003d (configsvr_vsize * num_configsvr"},{"line_number":116,"context_line":"                                + mongos_vsize * num_mongos)"},{"line_number":117,"context_line":"        deltas \u003d {\u0027instances\u0027: delta_instances, \u0027volumes\u0027: req_volume_size}"},{"line_number":118,"context_line":"        check_quotas(context.tenant, deltas)"},{"line_number":119,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_0617369f","line":116,"range":{"start_line":114,"start_character":0,"end_line":116,"end_character":60},"updated":"2018-07-19 04:17:35.000000000","message":"This changes the previous behavior, but looks resonalbe and correct, i.e. before this patch, creating mongodb cluster may pass quota checking but still failed by the \u0027volumes\u0027 quota as the volume size of configsrv and mongos ware not accounted.\n\nIt would be better to have new unittests for this kind of change.","commit_id":"6dff0a56c78d510d7cebf5e7512cdc6501cac12b"},{"author":{"_account_id":22608,"name":"zhanggang","email":"zhanggang@cmss.chinamobile.com","username":"zhanggang"},"change_message_id":"eef2bab1cccedf7d6e39afc9e502157797323be4","unresolved":false,"context_lines":[{"line_number":111,"context_line":""},{"line_number":112,"context_line":"        req_volume_size \u003d models.get_required_volume_size("},{"line_number":113,"context_line":"            instances, mongo_conf.volume_support)"},{"line_number":114,"context_line":"        if mongo_conf.volume_support:"},{"line_number":115,"context_line":"            req_volume_size +\u003d (configsvr_vsize * num_configsvr"},{"line_number":116,"context_line":"                                + mongos_vsize * num_mongos)"},{"line_number":117,"context_line":"        deltas \u003d {\u0027instances\u0027: delta_instances, \u0027volumes\u0027: req_volume_size}"},{"line_number":118,"context_line":"        check_quotas(context.tenant, deltas)"},{"line_number":119,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_9e77bc08","line":116,"range":{"start_line":114,"start_character":0,"end_line":116,"end_character":60},"in_reply_to":"5f7c97a3_0617369f","updated":"2018-07-23 05:50:18.000000000","message":"A test case has added","commit_id":"6dff0a56c78d510d7cebf5e7512cdc6501cac12b"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"261efe14a76bd10389390477184353e7fac345e9","unresolved":false,"context_lines":[{"line_number":100,"context_line":"            configsvr_vtype \u003d extends[\u0027configsvr_volume_type\u0027]"},{"line_number":101,"context_line":"        else:"},{"line_number":102,"context_line":"            configsvr_vtype \u003d volume_type  # the same as node instance"},{"line_number":103,"context_line":"        if extends.get(\u0027mongos_volume_size\u0027, None):"},{"line_number":104,"context_line":"            mongos_vsize \u003d int(extends[\u0027mongos_volume_size\u0027])"},{"line_number":105,"context_line":"        else:"},{"line_number":106,"context_line":"            mongos_vsize \u003d mongo_conf.query_routers_volume_size"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f7c97a3_e6489eb5","line":103,"updated":"2018-07-23 12:50:15.000000000","message":"Consider replacing if statements with the following one-liners:\n\n mongos_vsize \u003d int(extends.get(\u0027mongos_volume_size\u0027, \n                                mongo_conf.query_routers_volume_size))\n\nNote, however, that this changes the logic of the previous statement. In case \"mongos_volume_size\" property has \"None\" value, now it will be accepted.\n\nAlthough API schema enforces non-None values for extended properties we should discuss whether this policy allows us to define such assumption at this point.\n\nAlso, single-line spaces between if statements would improve readibility a little bit.","commit_id":"84c123d6bdf75ade839beb37b33324315e63d005"},{"author":{"_account_id":22608,"name":"zhanggang","email":"zhanggang@cmss.chinamobile.com","username":"zhanggang"},"change_message_id":"1f5da05ab95f8b058be6b91906e04da73743a105","unresolved":false,"context_lines":[{"line_number":100,"context_line":"            configsvr_vtype \u003d extends[\u0027configsvr_volume_type\u0027]"},{"line_number":101,"context_line":"        else:"},{"line_number":102,"context_line":"            configsvr_vtype \u003d volume_type  # the same as node instance"},{"line_number":103,"context_line":"        if extends.get(\u0027mongos_volume_size\u0027, None):"},{"line_number":104,"context_line":"            mongos_vsize \u003d int(extends[\u0027mongos_volume_size\u0027])"},{"line_number":105,"context_line":"        else:"},{"line_number":106,"context_line":"            mongos_vsize \u003d mongo_conf.query_routers_volume_size"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f7c97a3_1738663a","line":103,"in_reply_to":"5f7c97a3_e6489eb5","updated":"2018-07-23 13:54:14.000000000","message":"Impressive, you have thought it in every  aspects. In my opinion, the limitation of api schema should enough for us to do the replacement you comments above. Keep this unchange for now to seeking for other\u0027s advise.","commit_id":"84c123d6bdf75ade839beb37b33324315e63d005"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"261efe14a76bd10389390477184353e7fac345e9","unresolved":false,"context_lines":[{"line_number":112,"context_line":"        req_volume_size \u003d models.get_required_volume_size("},{"line_number":113,"context_line":"            instances, mongo_conf.volume_support)"},{"line_number":114,"context_line":"        if mongo_conf.volume_support:"},{"line_number":115,"context_line":"            req_volume_size +\u003d (configsvr_vsize * num_configsvr"},{"line_number":116,"context_line":"                                + mongos_vsize * num_mongos)"},{"line_number":117,"context_line":"        deltas \u003d {\u0027instances\u0027: delta_instances, \u0027volumes\u0027: req_volume_size}"},{"line_number":118,"context_line":"        check_quotas(context.tenant, deltas)"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f7c97a3_299c275b","line":115,"updated":"2018-07-23 12:50:15.000000000","message":"You skipped volume size validation (see: cluster.models.validate_volume_size function) that is being executed in get_required_volume_size function:\n\n models.validate_volume_size(mongos_vsize)\n\nAnother approach would be to reuse get_required_volume_size function by changing its signature:\n\n def get_required_volume_size(volume_size, num_instances, volume_enabled)","commit_id":"84c123d6bdf75ade839beb37b33324315e63d005"},{"author":{"_account_id":22608,"name":"zhanggang","email":"zhanggang@cmss.chinamobile.com","username":"zhanggang"},"change_message_id":"1f5da05ab95f8b058be6b91906e04da73743a105","unresolved":false,"context_lines":[{"line_number":112,"context_line":"        req_volume_size \u003d models.get_required_volume_size("},{"line_number":113,"context_line":"            instances, mongo_conf.volume_support)"},{"line_number":114,"context_line":"        if mongo_conf.volume_support:"},{"line_number":115,"context_line":"            req_volume_size +\u003d (configsvr_vsize * num_configsvr"},{"line_number":116,"context_line":"                                + mongos_vsize * num_mongos)"},{"line_number":117,"context_line":"        deltas \u003d {\u0027instances\u0027: delta_instances, \u0027volumes\u0027: req_volume_size}"},{"line_number":118,"context_line":"        check_quotas(context.tenant, deltas)"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f7c97a3_9730f6b9","line":115,"in_reply_to":"5f7c97a3_299c275b","updated":"2018-07-23 13:54:14.000000000","message":"hi Bartosz, thanks review, and yes, I skip do the validation here. Some thoughts below:\n1. get_required_volume_size mainly do the validation that if the size has exceeded the maximum limit, but the CONF.max_accepted_volume_size is not suitable for configsvr_volume_size and mongos size.(maybe too large)\n2. We could use the schema that defined in apischema.py to do the size limitation(already set minimum to 1, leave the maximum to discuss).","commit_id":"84c123d6bdf75ade839beb37b33324315e63d005"},{"author":{"_account_id":22608,"name":"zhanggang","email":"zhanggang@cmss.chinamobile.com","username":"zhanggang"},"change_message_id":"19f0f0a93568f07ed754f8e89d551d74d913023e","unresolved":false,"context_lines":[{"line_number":112,"context_line":"        req_volume_size \u003d models.get_required_volume_size("},{"line_number":113,"context_line":"            instances, mongo_conf.volume_support)"},{"line_number":114,"context_line":"        if mongo_conf.volume_support:"},{"line_number":115,"context_line":"            req_volume_size +\u003d (configsvr_vsize * num_configsvr"},{"line_number":116,"context_line":"                                + mongos_vsize * num_mongos)"},{"line_number":117,"context_line":"        deltas \u003d {\u0027instances\u0027: delta_instances, \u0027volumes\u0027: req_volume_size}"},{"line_number":118,"context_line":"        check_quotas(context.tenant, deltas)"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f7c97a3_b257adf5","line":115,"in_reply_to":"5f7c97a3_5d691dbe","updated":"2018-07-30 10:46:59.000000000","message":"Got your point, but I am not sure it is a good idea to change the interface of get_required_volume_size. As a compromise， do you think it\u0027s a solution to set maximum\u003dCONF.max_accepted_volume_size of volume_size in apischema.py.","commit_id":"84c123d6bdf75ade839beb37b33324315e63d005"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"6ff244d999c531b16dc4c4fe8fa9dbfd40a1078e","unresolved":false,"context_lines":[{"line_number":112,"context_line":"        req_volume_size \u003d models.get_required_volume_size("},{"line_number":113,"context_line":"            instances, mongo_conf.volume_support)"},{"line_number":114,"context_line":"        if mongo_conf.volume_support:"},{"line_number":115,"context_line":"            req_volume_size +\u003d (configsvr_vsize * num_configsvr"},{"line_number":116,"context_line":"                                + mongos_vsize * num_mongos)"},{"line_number":117,"context_line":"        deltas \u003d {\u0027instances\u0027: delta_instances, \u0027volumes\u0027: req_volume_size}"},{"line_number":118,"context_line":"        check_quotas(context.tenant, deltas)"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f7c97a3_5d691dbe","line":115,"in_reply_to":"5f7c97a3_9730f6b9","updated":"2018-07-23 14:46:32.000000000","message":"1. In my opinion, it will be better to perform validation with a larger limit than not to do it at all. I agree that the limits will not be perfectly adequate for instances of routers and config servers, but honestly I\u0027m not convinced that we need to break up into separate limits for each type of server.\n\n2. Limiting volume sizes at API schema level would be problematic. For sure, we can not hardcode this value, because: what if the user needs to change it (for larger or smaller)?. It will be difficult to determine one value that will be appropriate in each case.","commit_id":"84c123d6bdf75ade839beb37b33324315e63d005"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"22d38e3df931329ef31af71cd986ba5edb23f478","unresolved":false,"context_lines":[{"line_number":112,"context_line":"        req_volume_size \u003d models.get_required_volume_size("},{"line_number":113,"context_line":"            instances, mongo_conf.volume_support)"},{"line_number":114,"context_line":"        if mongo_conf.volume_support:"},{"line_number":115,"context_line":"            req_volume_size +\u003d (configsvr_vsize * num_configsvr"},{"line_number":116,"context_line":"                                + mongos_vsize * num_mongos)"},{"line_number":117,"context_line":"        deltas \u003d {\u0027instances\u0027: delta_instances, \u0027volumes\u0027: req_volume_size}"},{"line_number":118,"context_line":"        check_quotas(context.tenant, deltas)"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f7c97a3_eb9a0c5e","line":115,"in_reply_to":"5f7c97a3_b257adf5","updated":"2018-07-30 13:52:40.000000000","message":"I am not sure if apischema is an appropriate module for placing such validation. The schema should only define the attributes of objects and simple validations such as the type of value or the sign of the number (positive or negative). Validation of a specific maximum value collected from config seems to go beyond this scope and in my opinion violates the \"Separation of concerns\" principle.\n\nVolume size validation is already done in the MongoDB clustering strategy, therefore I suggest placing an additional validation there, especially since it is a validation with the same logic. The only problem is that we do not have an instance list, which is required in the signature of the \"get_required_volume_size\".  method - why not to to build it? :)\n\n configsvr \u003d {}\n \n configsvr[\u0027volume_size\u0027] \u003d int(\n     extends.get(\u0027configsvr_volume_size\u0027,\n                 mongo_conf.query_routers_volume_size))\n \n configsvr[\u0027volume_type\u0027] \u003d int(\n     extends.get(\u0027configsvr_volume_type\u0027,\n                 mongo_conf.query_routers_volume_size))\n \n req_volume_size +\u003d num_configsvr * models.get_required_volume_size(\n     [configsvr], mongo_conf.volume_support)\n\nIn this way, we close the validation within one module. In addition, we check not only the maximum value, but also all other fancy things that are checked in the \"get_required_volume_size\" method, for example whether volume support is enabled.","commit_id":"84c123d6bdf75ade839beb37b33324315e63d005"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"368d3f779c9b6c4accdbfc2798647b097abe058e","unresolved":false,"context_lines":[{"line_number":72,"context_line":"            raise exception.ClusterNumInstancesNotSupported(num_instances\u003d3)"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"        mongo_conf \u003d CONF.get(datastore_version.manager)"},{"line_number":75,"context_line":"        extends \u003d extended_properties  # truncated the param for brevity"},{"line_number":76,"context_line":"        num_configsvr \u003d int(extends.get("},{"line_number":77,"context_line":"            \u0027num_configsvr\u0027, mongo_conf.num_config_servers_per_cluster))"},{"line_number":78,"context_line":"        num_mongos \u003d int(extends.get("}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_0ffb66bf","line":75,"updated":"2018-08-03 11:49:36.000000000","message":"I\u0027m hesitant about this alias and would like full \"extended_properties\" name more, especially since most directives that use this variable are stretched in multiple lines so longer variable name would fit nicely. \"extends\" doesn\u0027t hurt though so I don\u0027t insist.","commit_id":"56109c0e6da9adc4f0a3cb8c76c6ba666a003a79"},{"author":{"_account_id":22608,"name":"zhanggang","email":"zhanggang@cmss.chinamobile.com","username":"zhanggang"},"change_message_id":"efb1cb1cd3d446aaa2d0adf6464755f3fb2532ef","unresolved":false,"context_lines":[{"line_number":72,"context_line":"            raise exception.ClusterNumInstancesNotSupported(num_instances\u003d3)"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"        mongo_conf \u003d CONF.get(datastore_version.manager)"},{"line_number":75,"context_line":"        extends \u003d extended_properties  # truncated the param for brevity"},{"line_number":76,"context_line":"        num_configsvr \u003d int(extends.get("},{"line_number":77,"context_line":"            \u0027num_configsvr\u0027, mongo_conf.num_config_servers_per_cluster))"},{"line_number":78,"context_line":"        num_mongos \u003d int(extends.get("}],"source_content_type":"text/x-python","patch_set":7,"id":"3f79a3b5_dabe93fe","line":75,"in_reply_to":"5f7c97a3_0ffb66bf","updated":"2018-08-06 07:11:54.000000000","message":"Done","commit_id":"56109c0e6da9adc4f0a3cb8c76c6ba666a003a79"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"368d3f779c9b6c4accdbfc2798647b097abe058e","unresolved":false,"context_lines":[{"line_number":75,"context_line":"        extends \u003d extended_properties  # truncated the param for brevity"},{"line_number":76,"context_line":"        num_configsvr \u003d int(extends.get("},{"line_number":77,"context_line":"            \u0027num_configsvr\u0027, mongo_conf.num_config_servers_per_cluster))"},{"line_number":78,"context_line":"        num_mongos \u003d int(extends.get("},{"line_number":79,"context_line":"            \u0027num_mongos\u0027, mongo_conf.num_query_routers_per_cluster))"},{"line_number":80,"context_line":"        delta_instances \u003d num_instances + num_configsvr + num_mongos"},{"line_number":81,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_6fdc5a43","line":78,"updated":"2018-08-03 11:49:36.000000000","message":"It got cramped in here. Add single line spacing between extracting each option to improve readability.","commit_id":"56109c0e6da9adc4f0a3cb8c76c6ba666a003a79"},{"author":{"_account_id":22608,"name":"zhanggang","email":"zhanggang@cmss.chinamobile.com","username":"zhanggang"},"change_message_id":"efb1cb1cd3d446aaa2d0adf6464755f3fb2532ef","unresolved":false,"context_lines":[{"line_number":75,"context_line":"        extends \u003d extended_properties  # truncated the param for brevity"},{"line_number":76,"context_line":"        num_configsvr \u003d int(extends.get("},{"line_number":77,"context_line":"            \u0027num_configsvr\u0027, mongo_conf.num_config_servers_per_cluster))"},{"line_number":78,"context_line":"        num_mongos \u003d int(extends.get("},{"line_number":79,"context_line":"            \u0027num_mongos\u0027, mongo_conf.num_query_routers_per_cluster))"},{"line_number":80,"context_line":"        delta_instances \u003d num_instances + num_configsvr + num_mongos"},{"line_number":81,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"3f79a3b5_fabb4f0b","line":78,"in_reply_to":"5f7c97a3_6fdc5a43","updated":"2018-08-06 07:11:54.000000000","message":"Done","commit_id":"56109c0e6da9adc4f0a3cb8c76c6ba666a003a79"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"368d3f779c9b6c4accdbfc2798647b097abe058e","unresolved":false,"context_lines":[{"line_number":91,"context_line":"        configsvr_vsize \u003d int(extends.get("},{"line_number":92,"context_line":"            \u0027configsvr_volume_size\u0027, mongo_conf.config_servers_volume_size))"},{"line_number":93,"context_line":"        configsvr_vtype \u003d extends.get(\u0027configsvr_volume_type\u0027, volume_type)"},{"line_number":94,"context_line":"        mongos_vsize \u003d int(extends.get("},{"line_number":95,"context_line":"            \u0027mongos_volume_size\u0027, mongo_conf.query_routers_volume_size))"},{"line_number":96,"context_line":"        mongos_vtype \u003d extends.get(\u0027mongos_volume_type\u0027, volume_type)"},{"line_number":97,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_8faa166b","line":94,"updated":"2018-08-03 11:49:36.000000000","message":"It got cramped in here. Add single line spacing between extracting each option to improve readability.","commit_id":"56109c0e6da9adc4f0a3cb8c76c6ba666a003a79"},{"author":{"_account_id":22608,"name":"zhanggang","email":"zhanggang@cmss.chinamobile.com","username":"zhanggang"},"change_message_id":"efb1cb1cd3d446aaa2d0adf6464755f3fb2532ef","unresolved":false,"context_lines":[{"line_number":91,"context_line":"        configsvr_vsize \u003d int(extends.get("},{"line_number":92,"context_line":"            \u0027configsvr_volume_size\u0027, mongo_conf.config_servers_volume_size))"},{"line_number":93,"context_line":"        configsvr_vtype \u003d extends.get(\u0027configsvr_volume_type\u0027, volume_type)"},{"line_number":94,"context_line":"        mongos_vsize \u003d int(extends.get("},{"line_number":95,"context_line":"            \u0027mongos_volume_size\u0027, mongo_conf.query_routers_volume_size))"},{"line_number":96,"context_line":"        mongos_vtype \u003d extends.get(\u0027mongos_volume_type\u0027, volume_type)"},{"line_number":97,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"3f79a3b5_9a36dbb4","line":94,"in_reply_to":"5f7c97a3_8faa166b","updated":"2018-08-06 07:11:54.000000000","message":"Done","commit_id":"56109c0e6da9adc4f0a3cb8c76c6ba666a003a79"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"368d3f779c9b6c4accdbfc2798647b097abe058e","unresolved":false,"context_lines":[{"line_number":99,"context_line":"        mongos \u003d {\u0027volume_size\u0027: mongos_vsize}"},{"line_number":100,"context_line":"        req_volume_size \u003d models.get_required_volume_size("},{"line_number":101,"context_line":"            instances, mongo_conf.volume_support)"},{"line_number":102,"context_line":"        # In case get_required_volume_size return None"},{"line_number":103,"context_line":"        if mongo_conf.volume_support:"},{"line_number":104,"context_line":"            req_configsvr_vsize \u003d models.get_required_volume_size("},{"line_number":105,"context_line":"                [configsvr], mongo_conf.volume_support) * num_configsvr"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_e2d0812d","line":102,"updated":"2018-08-03 11:49:36.000000000","message":"In that case maybe it would be a better idea to check the value of \"req_volume_size\" variable?\n\n if req_volume_size is not None:\n     ...\n\nIt gets value from \"get_required_volume_size\" which already has mongo_conf.volume_support condition. In my opinion we should depend on the return value of \"get_required_volume_size\" rather than duplicating the condition from its implementation.","commit_id":"56109c0e6da9adc4f0a3cb8c76c6ba666a003a79"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"368d3f779c9b6c4accdbfc2798647b097abe058e","unresolved":false,"context_lines":[{"line_number":100,"context_line":"        req_volume_size \u003d models.get_required_volume_size("},{"line_number":101,"context_line":"            instances, mongo_conf.volume_support)"},{"line_number":102,"context_line":"        # In case get_required_volume_size return None"},{"line_number":103,"context_line":"        if mongo_conf.volume_support:"},{"line_number":104,"context_line":"            req_configsvr_vsize \u003d models.get_required_volume_size("},{"line_number":105,"context_line":"                [configsvr], mongo_conf.volume_support) * num_configsvr"},{"line_number":106,"context_line":"            req_mongos_vsize \u003d models.get_required_volume_size("}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_827c2d23","line":103,"updated":"2018-08-03 11:49:36.000000000","message":"Also, it would be appropriate if we had unit test that would cover this case: all instances with ephemeral storage and volume_support disabled so that \"get_required_volume_size\" returned \"None\" value for req_volume_size.","commit_id":"56109c0e6da9adc4f0a3cb8c76c6ba666a003a79"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"cdf4d9f3b003fd30a8253b5e67ee810edaa9a154","unresolved":false,"context_lines":[{"line_number":100,"context_line":"        req_volume_size \u003d models.get_required_volume_size("},{"line_number":101,"context_line":"            instances, mongo_conf.volume_support)"},{"line_number":102,"context_line":"        # In case get_required_volume_size return None"},{"line_number":103,"context_line":"        if mongo_conf.volume_support:"},{"line_number":104,"context_line":"            req_configsvr_vsize \u003d models.get_required_volume_size("},{"line_number":105,"context_line":"                [configsvr], mongo_conf.volume_support) * num_configsvr"},{"line_number":106,"context_line":"            req_mongos_vsize \u003d models.get_required_volume_size("}],"source_content_type":"text/x-python","patch_set":7,"id":"3f79a3b5_6d0a2827","line":103,"in_reply_to":"3f79a3b5_5acb63a4","updated":"2018-08-06 11:24:31.000000000","message":"Agreed.","commit_id":"56109c0e6da9adc4f0a3cb8c76c6ba666a003a79"},{"author":{"_account_id":22608,"name":"zhanggang","email":"zhanggang@cmss.chinamobile.com","username":"zhanggang"},"change_message_id":"efb1cb1cd3d446aaa2d0adf6464755f3fb2532ef","unresolved":false,"context_lines":[{"line_number":100,"context_line":"        req_volume_size \u003d models.get_required_volume_size("},{"line_number":101,"context_line":"            instances, mongo_conf.volume_support)"},{"line_number":102,"context_line":"        # In case get_required_volume_size return None"},{"line_number":103,"context_line":"        if mongo_conf.volume_support:"},{"line_number":104,"context_line":"            req_configsvr_vsize \u003d models.get_required_volume_size("},{"line_number":105,"context_line":"                [configsvr], mongo_conf.volume_support) * num_configsvr"},{"line_number":106,"context_line":"            req_mongos_vsize \u003d models.get_required_volume_size("}],"source_content_type":"text/x-python","patch_set":7,"id":"3f79a3b5_5acb63a4","line":103,"in_reply_to":"5f7c97a3_827c2d23","updated":"2018-08-06 07:11:54.000000000","message":"About this point, I don\u0027t quite agree with you, I\u0027m afraid.\nThe two test case[1][2] have tested two \"if block\" in get_required_volume_size. The rest code in this method is only a \"return None\", since \"None\" is not calculated by an expression(or a piece of code), so in my opinion, it\u0027s unnecessary to add a test case to match this situation.\n\nhttps://github.com/openstack/trove/blob/master/trove/tests/unittests/cluster/test_mongodb_cluster.py#L155\nhttps://github.com/openstack/trove/blob/master/trove/tests/unittests/cluster/test_mongodb_cluster.py#L178","commit_id":"56109c0e6da9adc4f0a3cb8c76c6ba666a003a79"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"cdf4d9f3b003fd30a8253b5e67ee810edaa9a154","unresolved":false,"context_lines":[{"line_number":103,"context_line":"        req_volume_size \u003d models.get_required_volume_size("},{"line_number":104,"context_line":"            instances, mongo_conf.volume_support)"},{"line_number":105,"context_line":"        # In case get_required_volume_size return None"},{"line_number":106,"context_line":"        if req_volume_size:"},{"line_number":107,"context_line":"            configsvr \u003d {\u0027volume_size\u0027: configsvr_vsize}"},{"line_number":108,"context_line":"            mongos \u003d {\u0027volume_size\u0027: mongos_vsize}"},{"line_number":109,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_fbbb510b","line":106,"updated":"2018-08-06 11:24:31.000000000","message":"In case \"req_volume_size\" equals \"0\" this condition will not pass. It must be:\n\n if req_volume_size is not None\n\nNow, we can remove the comment above since the code speaks for itself :)","commit_id":"531e3ff85e3bee2083d8dacea67e8e7ddb8d7c95"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"e0ca285d8c527a8ee2b703ca7d4e1493d6894ba0","unresolved":false,"context_lines":[{"line_number":103,"context_line":"        req_volume_size \u003d models.get_required_volume_size("},{"line_number":104,"context_line":"            instances, mongo_conf.volume_support)"},{"line_number":105,"context_line":"        # In case get_required_volume_size return None"},{"line_number":106,"context_line":"        if req_volume_size:"},{"line_number":107,"context_line":"            configsvr \u003d {\u0027volume_size\u0027: configsvr_vsize}"},{"line_number":108,"context_line":"            mongos \u003d {\u0027volume_size\u0027: mongos_vsize}"},{"line_number":109,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_971229b1","line":106,"in_reply_to":"3f79a3b5_1cc2c034","updated":"2018-08-06 15:14:43.000000000","message":"Zhao Chao, very good point. Do you mean, something like this:\n\n configsvr \u003d {\u0027volume_size\u0027: configsvr_vsize}\n mongos \u003d {\u0027volume_size\u0027: mongos_vsize}\n \n all_instances \u003d instances\n all_instances.extend([configsrv for _ in xrange(num_configsrv)])\n all_instances.extend([mongos for _ in xrange(num_mongos)])\n \n req_volume_size \u003d models.get_required_volume_size(\n    all_instances, mongo_conf.volume_support)","commit_id":"531e3ff85e3bee2083d8dacea67e8e7ddb8d7c95"},{"author":{"_account_id":22608,"name":"zhanggang","email":"zhanggang@cmss.chinamobile.com","username":"zhanggang"},"change_message_id":"fabea35c3e5efcdc1c72b96e9b0d6fbec71928a0","unresolved":false,"context_lines":[{"line_number":103,"context_line":"        req_volume_size \u003d models.get_required_volume_size("},{"line_number":104,"context_line":"            instances, mongo_conf.volume_support)"},{"line_number":105,"context_line":"        # In case get_required_volume_size return None"},{"line_number":106,"context_line":"        if req_volume_size:"},{"line_number":107,"context_line":"            configsvr \u003d {\u0027volume_size\u0027: configsvr_vsize}"},{"line_number":108,"context_line":"            mongos \u003d {\u0027volume_size\u0027: mongos_vsize}"},{"line_number":109,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_f3836355","line":106,"in_reply_to":"3f79a3b5_3fc68469","updated":"2018-08-07 07:33:28.000000000","message":"Done","commit_id":"531e3ff85e3bee2083d8dacea67e8e7ddb8d7c95"},{"author":{"_account_id":13157,"name":"Zhao Chao","email":"zhaochao1984@gmail.com","username":"zhaochao"},"change_message_id":"f785ab5bb24ac51a74226b79829d57cd68296b74","unresolved":false,"context_lines":[{"line_number":103,"context_line":"        req_volume_size \u003d models.get_required_volume_size("},{"line_number":104,"context_line":"            instances, mongo_conf.volume_support)"},{"line_number":105,"context_line":"        # In case get_required_volume_size return None"},{"line_number":106,"context_line":"        if req_volume_size:"},{"line_number":107,"context_line":"            configsvr \u003d {\u0027volume_size\u0027: configsvr_vsize}"},{"line_number":108,"context_line":"            mongos \u003d {\u0027volume_size\u0027: mongos_vsize}"},{"line_number":109,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_1fc5c83c","line":106,"in_reply_to":"3f79a3b5_5f3b40d0","updated":"2018-08-07 02:40:18.000000000","message":"Yes, looking thought instance/models.py and taskmanager/models.py, it looks like it makes no sense with volume_size\u003d0. However we also accept a string pattern for volume_size, so the pattern should also be updated or a new patter created for this(this kind of change could be fine with an seperate patch).","commit_id":"531e3ff85e3bee2083d8dacea67e8e7ddb8d7c95"},{"author":{"_account_id":22608,"name":"zhanggang","email":"zhanggang@cmss.chinamobile.com","username":"zhanggang"},"change_message_id":"bd7e3441b104e51d867c60127b0263d5e737a452","unresolved":false,"context_lines":[{"line_number":103,"context_line":"        req_volume_size \u003d models.get_required_volume_size("},{"line_number":104,"context_line":"            instances, mongo_conf.volume_support)"},{"line_number":105,"context_line":"        # In case get_required_volume_size return None"},{"line_number":106,"context_line":"        if req_volume_size:"},{"line_number":107,"context_line":"            configsvr \u003d {\u0027volume_size\u0027: configsvr_vsize}"},{"line_number":108,"context_line":"            mongos \u003d {\u0027volume_size\u0027: mongos_vsize}"},{"line_number":109,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_5f3b40d0","line":106,"in_reply_to":"3f79a3b5_971229b1","updated":"2018-08-07 01:58:37.000000000","message":"Agree, consider the minimum of volume_size is set to 0 in apischema. I just wonder why we set the minimum to 0, it seems set to 1 is more reasonable.","commit_id":"531e3ff85e3bee2083d8dacea67e8e7ddb8d7c95"},{"author":{"_account_id":13157,"name":"Zhao Chao","email":"zhaochao1984@gmail.com","username":"zhaochao"},"change_message_id":"f785ab5bb24ac51a74226b79829d57cd68296b74","unresolved":false,"context_lines":[{"line_number":103,"context_line":"        req_volume_size \u003d models.get_required_volume_size("},{"line_number":104,"context_line":"            instances, mongo_conf.volume_support)"},{"line_number":105,"context_line":"        # In case get_required_volume_size return None"},{"line_number":106,"context_line":"        if req_volume_size:"},{"line_number":107,"context_line":"            configsvr \u003d {\u0027volume_size\u0027: configsvr_vsize}"},{"line_number":108,"context_line":"            mongos \u003d {\u0027volume_size\u0027: mongos_vsize}"},{"line_number":109,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_3fc68469","line":106,"in_reply_to":"3f79a3b5_971229b1","updated":"2018-08-07 02:40:18.000000000","message":"Yes, something like:\n\n    all_instances \u003d (instances\n                     + [{\u0027volume_size\u0027: configsvr_vsize}] * num_configsrv\n                     + [{\u0027volume_size\u0027: mongos_vsize}] * num_mongos)\n    req_volume_size \u003d models.get_required_volume_size(...)\n\nshould also work.","commit_id":"531e3ff85e3bee2083d8dacea67e8e7ddb8d7c95"},{"author":{"_account_id":13157,"name":"Zhao Chao","email":"zhaochao1984@gmail.com","username":"zhaochao"},"change_message_id":"e45611c2829e3dccf07a37793c99d32ac3b434dc","unresolved":false,"context_lines":[{"line_number":103,"context_line":"        req_volume_size \u003d models.get_required_volume_size("},{"line_number":104,"context_line":"            instances, mongo_conf.volume_support)"},{"line_number":105,"context_line":"        # In case get_required_volume_size return None"},{"line_number":106,"context_line":"        if req_volume_size:"},{"line_number":107,"context_line":"            configsvr \u003d {\u0027volume_size\u0027: configsvr_vsize}"},{"line_number":108,"context_line":"            mongos \u003d {\u0027volume_size\u0027: mongos_vsize}"},{"line_number":109,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_1cc2c034","line":106,"in_reply_to":"3f79a3b5_fbbb510b","updated":"2018-08-06 14:43:15.000000000","message":"Agree.\n\nAnd one more thought here: is it better to construct a temporary list of instances, then we could only call get_required_volume_size once.","commit_id":"531e3ff85e3bee2083d8dacea67e8e7ddb8d7c95"}],"trove/tests/unittests/cluster/test_cluster.py":[{"author":{"_account_id":13157,"name":"Zhao Chao","email":"zhaochao1984@gmail.com","username":"zhaochao"},"change_message_id":"2f15fb349b0b29199c62d1a3d3ca92c207b88bfa","unresolved":false,"context_lines":[{"line_number":81,"context_line":"                          self.datastore,"},{"line_number":82,"context_line":"                          self.datastore_version,"},{"line_number":83,"context_line":"                          [],"},{"line_number":84,"context_line":"                          {}, None, None)"},{"line_number":85,"context_line":""},{"line_number":86,"context_line":"    @patch.object(remote, \u0027create_nova_client\u0027)"},{"line_number":87,"context_line":"    def test_create_unequal_flavors(self, mock_client):"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_863826ed","line":84,"range":{"start_line":84,"start_character":26,"end_line":84,"end_character":28},"updated":"2018-07-19 04:17:35.000000000","message":"Seems not necessary change?","commit_id":"6dff0a56c78d510d7cebf5e7512cdc6501cac12b"},{"author":{"_account_id":22608,"name":"zhanggang","email":"zhanggang@cmss.chinamobile.com","username":"zhanggang"},"change_message_id":"eef2bab1cccedf7d6e39afc9e502157797323be4","unresolved":false,"context_lines":[{"line_number":81,"context_line":"                          self.datastore,"},{"line_number":82,"context_line":"                          self.datastore_version,"},{"line_number":83,"context_line":"                          [],"},{"line_number":84,"context_line":"                          {}, None, None)"},{"line_number":85,"context_line":""},{"line_number":86,"context_line":"    @patch.object(remote, \u0027create_nova_client\u0027)"},{"line_number":87,"context_line":"    def test_create_unequal_flavors(self, mock_client):"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_e112d155","line":84,"range":{"start_line":84,"start_character":26,"end_line":84,"end_character":28},"in_reply_to":"5f7c97a3_863826ed","updated":"2018-07-23 05:50:18.000000000","message":"yes, This exception raised before use the extended_properties. Done","commit_id":"6dff0a56c78d510d7cebf5e7512cdc6501cac12b"},{"author":{"_account_id":13157,"name":"Zhao Chao","email":"zhaochao1984@gmail.com","username":"zhaochao"},"change_message_id":"e45611c2829e3dccf07a37793c99d32ac3b434dc","unresolved":false,"context_lines":[{"line_number":94,"context_line":"                          self.datastore,"},{"line_number":95,"context_line":"                          self.datastore_version,"},{"line_number":96,"context_line":"                          instances,"},{"line_number":97,"context_line":"                          {}, None, None)"},{"line_number":98,"context_line":""},{"line_number":99,"context_line":"    @patch.object(remote, \u0027create_nova_client\u0027)"},{"line_number":100,"context_line":"    def test_create_unequal_volumes(self,"}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_fcaa6417","line":97,"range":{"start_line":97,"start_character":26,"end_line":97,"end_character":28},"updated":"2018-08-06 14:43:15.000000000","message":"Unnecessary change, see my comment in PS3.","commit_id":"531e3ff85e3bee2083d8dacea67e8e7ddb8d7c95"},{"author":{"_account_id":22608,"name":"zhanggang","email":"zhanggang@cmss.chinamobile.com","username":"zhanggang"},"change_message_id":"4ae193f864aff7c09cea43adc2f5ea32c2629cc4","unresolved":false,"context_lines":[{"line_number":94,"context_line":"                          self.datastore,"},{"line_number":95,"context_line":"                          self.datastore_version,"},{"line_number":96,"context_line":"                          instances,"},{"line_number":97,"context_line":"                          {}, None, None)"},{"line_number":98,"context_line":""},{"line_number":99,"context_line":"    @patch.object(remote, \u0027create_nova_client\u0027)"},{"line_number":100,"context_line":"    def test_create_unequal_volumes(self,"}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_49e14a9a","line":97,"range":{"start_line":97,"start_character":26,"end_line":97,"end_character":28},"in_reply_to":"3f79a3b5_9369cf26","updated":"2018-08-07 09:48:41.000000000","message":"Just take a brief look at the code, seems no more need to update except the two file you mentioned, will update them under this patch.","commit_id":"531e3ff85e3bee2083d8dacea67e8e7ddb8d7c95"},{"author":{"_account_id":22608,"name":"zhanggang","email":"zhanggang@cmss.chinamobile.com","username":"zhanggang"},"change_message_id":"4138bf31dffadcd74e5dcfb432e58c28e60798e5","unresolved":false,"context_lines":[{"line_number":94,"context_line":"                          self.datastore,"},{"line_number":95,"context_line":"                          self.datastore_version,"},{"line_number":96,"context_line":"                          instances,"},{"line_number":97,"context_line":"                          {}, None, None)"},{"line_number":98,"context_line":""},{"line_number":99,"context_line":"    @patch.object(remote, \u0027create_nova_client\u0027)"},{"line_number":100,"context_line":"    def test_create_unequal_volumes(self,"}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_ffd6cc4f","line":97,"range":{"start_line":97,"start_character":26,"end_line":97,"end_character":28},"in_reply_to":"3f79a3b5_fcaa6417","updated":"2018-08-07 07:24:39.000000000","message":"Since I use extended_properties.get at the very beginning of method create, set to None will cause a \"AttributeError: \u0027NoneType\u0027 object has no attribute \u0027get\u0027\" error by executing this unittest. \nAnd this parameters is passed by method create \n under cluster/service.py[1], and default value is \"{}\", so maybe set to \"{}\" is a safe value.\n\n[1] https://github.com/openstack/trove/blob/master/trove/cluster/service.py#L174","commit_id":"531e3ff85e3bee2083d8dacea67e8e7ddb8d7c95"},{"author":{"_account_id":13157,"name":"Zhao Chao","email":"zhaochao1984@gmail.com","username":"zhaochao"},"change_message_id":"9af052e4db200228df2bdced3e42ec2cec2bddd8","unresolved":false,"context_lines":[{"line_number":94,"context_line":"                          self.datastore,"},{"line_number":95,"context_line":"                          self.datastore_version,"},{"line_number":96,"context_line":"                          instances,"},{"line_number":97,"context_line":"                          {}, None, None)"},{"line_number":98,"context_line":""},{"line_number":99,"context_line":"    @patch.object(remote, \u0027create_nova_client\u0027)"},{"line_number":100,"context_line":"    def test_create_unequal_volumes(self,"}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_9369cf26","line":97,"range":{"start_line":97,"start_character":26,"end_line":97,"end_character":28},"in_reply_to":"3f79a3b5_ffd6cc4f","updated":"2018-08-07 08:30:39.000000000","message":"Oh, I see, this makes sense, in fact the \"extended_properties\" argument of Cluster.create shouldn\u0027t get the value \"None\".\n\nWould you also update the other places in the unittests where Cluster.create is called? Please refer to the original patch of introducing this argument, https://review.openstack.org/#/c/206931/, it looks like only test_cluster.py and test_vertica_cluster.py need to be updated.(of course, this part could also be splitted to another separate patch).","commit_id":"531e3ff85e3bee2083d8dacea67e8e7ddb8d7c95"},{"author":{"_account_id":13157,"name":"Zhao Chao","email":"zhaochao1984@gmail.com","username":"zhaochao"},"change_message_id":"e45611c2829e3dccf07a37793c99d32ac3b434dc","unresolved":false,"context_lines":[{"line_number":110,"context_line":"                          self.datastore,"},{"line_number":111,"context_line":"                          self.datastore_version,"},{"line_number":112,"context_line":"                          instances,"},{"line_number":113,"context_line":"                          {}, None, None)"},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"    @patch.object(remote, \u0027create_nova_client\u0027)"},{"line_number":116,"context_line":"    def test_create_storage_not_specified(self,"}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_7c3ad4e7","line":113,"range":{"start_line":113,"start_character":26,"end_line":113,"end_character":28},"updated":"2018-08-06 14:43:15.000000000","message":"Ditto.","commit_id":"531e3ff85e3bee2083d8dacea67e8e7ddb8d7c95"},{"author":{"_account_id":13157,"name":"Zhao Chao","email":"zhaochao1984@gmail.com","username":"zhaochao"},"change_message_id":"e45611c2829e3dccf07a37793c99d32ac3b434dc","unresolved":false,"context_lines":[{"line_number":139,"context_line":"                          self.datastore,"},{"line_number":140,"context_line":"                          self.datastore_version,"},{"line_number":141,"context_line":"                          instances,"},{"line_number":142,"context_line":"                          {}, None, None)"},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"    @patch(\u0027trove.cluster.models.LOG\u0027)"},{"line_number":145,"context_line":"    def test_delete_bad_task_status(self, mock_logging):"}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_9c3fb0d5","line":142,"range":{"start_line":142,"start_character":26,"end_line":142,"end_character":28},"updated":"2018-08-06 14:43:15.000000000","message":"Ditto.","commit_id":"531e3ff85e3bee2083d8dacea67e8e7ddb8d7c95"}],"trove/tests/unittests/cluster/test_mongodb_cluster.py":[{"author":{"_account_id":13157,"name":"Zhao Chao","email":"zhaochao1984@gmail.com","username":"zhaochao"},"change_message_id":"2f15fb349b0b29199c62d1a3d3ca92c207b88bfa","unresolved":false,"context_lines":[{"line_number":250,"context_line":"                            self.datastore_version,"},{"line_number":251,"context_line":"                            self.instances,"},{"line_number":252,"context_line":"                            extended_properties, None, None)"},{"line_number":253,"context_line":"        configsvr_vsize_count \u003d ["},{"line_number":254,"context_line":"            arg.count(11) for arg, _ in"},{"line_number":255,"context_line":"            mock_ins_create.call_args_list].count(1)"},{"line_number":256,"context_line":"        self.assertEqual(5, configsvr_vsize_count)"},{"line_number":257,"context_line":""},{"line_number":258,"context_line":"        configsvr_vtype_count \u003d ["},{"line_number":259,"context_line":"            kw.get(\u0027volume_type\u0027) for _, kw in"},{"line_number":260,"context_line":"            mock_ins_create.call_args_list].count(\"foo_type\")"},{"line_number":261,"context_line":"        self.assertEqual(5, configsvr_vtype_count)"},{"line_number":262,"context_line":""},{"line_number":263,"context_line":"        mongos_vsize_count \u003d ["},{"line_number":264,"context_line":"            arg.count(12) for arg, _ in"},{"line_number":265,"context_line":"            mock_ins_create.call_args_list].count(1)"},{"line_number":266,"context_line":"        self.assertEqual(7, mongos_vsize_count)"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"        mongos_vtype_count \u003d ["},{"line_number":269,"context_line":"            kw.get(\u0027volume_type\u0027) for _, kw in"},{"line_number":270,"context_line":"            mock_ins_create.call_args_list].count(\"bar_type\")"},{"line_number":271,"context_line":"        self.assertEqual(7, mongos_vtype_count)"},{"line_number":272,"context_line":""},{"line_number":273,"context_line":"    @mock.patch.object(task_api, \u0027load\u0027)"},{"line_number":274,"context_line":"    @mock.patch.object(inst_models.Instance, \u0027create\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_f6a17ef7","line":271,"range":{"start_line":253,"start_character":0,"end_line":271,"end_character":47},"updated":"2018-07-19 04:17:35.000000000","message":"As for creating an specific type of instance, the arguments of instance create are always passed together, so could it be better to have a form like this:\n\n    volume_args_list \u003d [\n        (args[8], kw[\u0027volume_type\u0027]) for args, kw in\n        mock_ins_create.call_args_list\n    ]\n    self.assertEqual(5, volume_args_list.count((11, \"foo_type\")))\n    self.assertEqual(7, volume_args_list.count((12, \"bar_type\")))","commit_id":"6dff0a56c78d510d7cebf5e7512cdc6501cac12b"},{"author":{"_account_id":22608,"name":"zhanggang","email":"zhanggang@cmss.chinamobile.com","username":"zhanggang"},"change_message_id":"eef2bab1cccedf7d6e39afc9e502157797323be4","unresolved":false,"context_lines":[{"line_number":250,"context_line":"                            self.datastore_version,"},{"line_number":251,"context_line":"                            self.instances,"},{"line_number":252,"context_line":"                            extended_properties, None, None)"},{"line_number":253,"context_line":"        configsvr_vsize_count \u003d ["},{"line_number":254,"context_line":"            arg.count(11) for arg, _ in"},{"line_number":255,"context_line":"            mock_ins_create.call_args_list].count(1)"},{"line_number":256,"context_line":"        self.assertEqual(5, configsvr_vsize_count)"},{"line_number":257,"context_line":""},{"line_number":258,"context_line":"        configsvr_vtype_count \u003d ["},{"line_number":259,"context_line":"            kw.get(\u0027volume_type\u0027) for _, kw in"},{"line_number":260,"context_line":"            mock_ins_create.call_args_list].count(\"foo_type\")"},{"line_number":261,"context_line":"        self.assertEqual(5, configsvr_vtype_count)"},{"line_number":262,"context_line":""},{"line_number":263,"context_line":"        mongos_vsize_count \u003d ["},{"line_number":264,"context_line":"            arg.count(12) for arg, _ in"},{"line_number":265,"context_line":"            mock_ins_create.call_args_list].count(1)"},{"line_number":266,"context_line":"        self.assertEqual(7, mongos_vsize_count)"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"        mongos_vtype_count \u003d ["},{"line_number":269,"context_line":"            kw.get(\u0027volume_type\u0027) for _, kw in"},{"line_number":270,"context_line":"            mock_ins_create.call_args_list].count(\"bar_type\")"},{"line_number":271,"context_line":"        self.assertEqual(7, mongos_vtype_count)"},{"line_number":272,"context_line":""},{"line_number":273,"context_line":"    @mock.patch.object(task_api, \u0027load\u0027)"},{"line_number":274,"context_line":"    @mock.patch.object(inst_models.Instance, \u0027create\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_e187315b","line":271,"range":{"start_line":253,"start_character":0,"end_line":271,"end_character":47},"in_reply_to":"5f7c97a3_f6a17ef7","updated":"2018-07-23 05:50:18.000000000","message":"Thanks a lot, this is much better than my origin code.","commit_id":"6dff0a56c78d510d7cebf5e7512cdc6501cac12b"}]}