)]}'
{"senlin/db/sqlalchemy/api.py":[{"author":{"_account_id":22998,"name":"XueFengLiu","email":"liu.xuefeng1@zte.com.cn","username":"sgfeng"},"change_message_id":"fc772f46e3dc84b9ba641b10411cd9195b574959","unresolved":false,"context_lines":[{"line_number":1433,"context_line":"        if svc_ids:"},{"line_number":1434,"context_line":"            q_reg \u003d q_reg.filter("},{"line_number":1435,"context_line":"                models.HealthRegistry.engine_id.notin_(svc_ids))"},{"line_number":1436,"context_line":"        q_reg.update({\u0027engine_id\u0027: engine_id}, synchronize_session\u003dFalse)"},{"line_number":1437,"context_line":"        result \u003d q_reg.all()"},{"line_number":1438,"context_line":"        return result"},{"line_number":1439,"context_line":""},{"line_number":1440,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"da36d5c6_55ac6fed","side":"PARENT","line":1437,"range":{"start_line":1436,"start_character":0,"end_line":1437,"end_character":28},"updated":"2017-02-18 05:20:12.000000000","message":"if svc_ids: true.\nThen here update first, then q_reg.all() with filter will always return [], this is a very hidden bug which will cause we can\u0027t claim.","commit_id":"ff8e6f1eb68e351d94d30c8eaadb2142c8056b61"},{"author":{"_account_id":8246,"name":"Qiming Teng","email":"tengqm@outlook.com","username":"tengqm"},"change_message_id":"a4cd8fabf94dbd70c4d1419c7ed7b65b2016bb58","unresolved":false,"context_lines":[{"line_number":1427,"context_line":""},{"line_number":1428,"context_line":"def registry_claim(context, engine_id):"},{"line_number":1429,"context_line":"    with session_for_write() as session:"},{"line_number":1430,"context_line":"        result \u003d []"},{"line_number":1431,"context_line":"        engines \u003d session.query(models.Service).all()"},{"line_number":1432,"context_line":"        alive_ids \u003d [e.id for e in engines if not utils.is_service_dead(e)]"},{"line_number":1433,"context_line":"        q_reg \u003d session.query(models.HealthRegistry)"},{"line_number":1434,"context_line":"        if alive_ids:"},{"line_number":1435,"context_line":"            q_reg \u003d q_reg.filter("},{"line_number":1436,"context_line":"                models.HealthRegistry.engine_id.notin_(alive_ids))"},{"line_number":1437,"context_line":"            # 1.handle filtered results"},{"line_number":1438,"context_line":"            result \u003d q_reg.all()"},{"line_number":1439,"context_line":"            for r in result:"},{"line_number":1440,"context_line":"                r.engine_id \u003d engine_id"},{"line_number":1441,"context_line":""},{"line_number":1442,"context_line":"            # 2.update HealthRegistry in db"},{"line_number":1443,"context_line":"            q_reg.update({\u0027engine_id\u0027: engine_id}, synchronize_session\u003dFalse)"},{"line_number":1444,"context_line":"        # TODO(AnyOne):In current version, registry_claim is before create self"},{"line_number":1445,"context_line":"        # service record. It\u0027s no matter, becasue we have called registry_claim"},{"line_number":1446,"context_line":"        # periodically. But maybe we\u0027d better change this order."}],"source_content_type":"text/x-python","patch_set":2,"id":"da36d5c6_a811ceb1","line":1443,"range":{"start_line":1430,"start_character":0,"end_line":1443,"end_character":77},"updated":"2017-02-18 10:08:39.000000000","message":"This logic can be optimized.\n\nThe original logic can be easily translated into a simple SQL, but the revised version cannot.\n\nWhy don\u0027t we try setting \u0027synchronize_session\u003dTrue\u0027 if the returned result doesn\u0027t contain the engine id?\n\nAlso, why do we want the resulted registry entries contain the updated engine_id if we are not using them?","commit_id":"8561ff9e404c81b576038c932dbd860afeba25bf"},{"author":{"_account_id":22998,"name":"XueFengLiu","email":"liu.xuefeng1@zte.com.cn","username":"sgfeng"},"change_message_id":"82b6618b8430871f5f88c7712f5b3dc952742a90","unresolved":false,"context_lines":[{"line_number":1427,"context_line":""},{"line_number":1428,"context_line":"def registry_claim(context, engine_id):"},{"line_number":1429,"context_line":"    with session_for_write() as session:"},{"line_number":1430,"context_line":"        result \u003d []"},{"line_number":1431,"context_line":"        engines \u003d session.query(models.Service).all()"},{"line_number":1432,"context_line":"        alive_ids \u003d [e.id for e in engines if not utils.is_service_dead(e)]"},{"line_number":1433,"context_line":"        q_reg \u003d session.query(models.HealthRegistry)"},{"line_number":1434,"context_line":"        if alive_ids:"},{"line_number":1435,"context_line":"            q_reg \u003d q_reg.filter("},{"line_number":1436,"context_line":"                models.HealthRegistry.engine_id.notin_(alive_ids))"},{"line_number":1437,"context_line":"            # 1.handle filtered results"},{"line_number":1438,"context_line":"            result \u003d q_reg.all()"},{"line_number":1439,"context_line":"            for r in result:"},{"line_number":1440,"context_line":"                r.engine_id \u003d engine_id"},{"line_number":1441,"context_line":""},{"line_number":1442,"context_line":"            # 2.update HealthRegistry in db"},{"line_number":1443,"context_line":"            q_reg.update({\u0027engine_id\u0027: engine_id}, synchronize_session\u003dFalse)"},{"line_number":1444,"context_line":"        # TODO(AnyOne):In current version, registry_claim is before create self"},{"line_number":1445,"context_line":"        # service record. It\u0027s no matter, becasue we have called registry_claim"},{"line_number":1446,"context_line":"        # periodically. But maybe we\u0027d better change this order."}],"source_content_type":"text/x-python","patch_set":2,"id":"da36d5c6_cbd43c49","line":1443,"range":{"start_line":1430,"start_character":0,"end_line":1443,"end_character":77},"in_reply_to":"da36d5c6_a811ceb1","updated":"2017-02-18 12:45:24.000000000","message":"Yes, We can remove line1439-1440. This is my first version and when I add unit test, I add these two lines, I think maybe it\u0027s better in that time.\nYou mean use \u0027fetch\u0027? Test when use \u0027fetch\u0027, following error will happened:\nDetachedInstanceError: Instance \u003cHealthRegistry at 0x7f4386426050\u003e is not bound to a Session; attribute refresh operation cannot proceed","commit_id":"8561ff9e404c81b576038c932dbd860afeba25bf"},{"author":{"_account_id":8246,"name":"Qiming Teng","email":"tengqm@outlook.com","username":"tengqm"},"change_message_id":"fc179b9c0372e0f88a4055838e3daa78ce702039","unresolved":false,"context_lines":[{"line_number":1433,"context_line":"        if svc_ids:"},{"line_number":1434,"context_line":"            q_reg \u003d q_reg.filter("},{"line_number":1435,"context_line":"                models.HealthRegistry.engine_id.notin_(svc_ids))"},{"line_number":1436,"context_line":"        q_reg.update({\u0027engine_id\u0027: engine_id}, synchronize_session\u003dFalse)"},{"line_number":1437,"context_line":"        result \u003d q_reg.all()"},{"line_number":1438,"context_line":"        return result"},{"line_number":1439,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"da36d5c6_86623bd7","side":"PARENT","line":1436,"range":{"start_line":1436,"start_character":8,"end_line":1436,"end_character":73},"updated":"2017-02-18 14:07:45.000000000","message":"can we just indent this line to solve the problem?","commit_id":"ff8e6f1eb68e351d94d30c8eaadb2142c8056b61"},{"author":{"_account_id":22998,"name":"XueFengLiu","email":"liu.xuefeng1@zte.com.cn","username":"sgfeng"},"change_message_id":"62630b7ea8517dbaeca2706fd2832ccab0ec2ce4","unresolved":false,"context_lines":[{"line_number":1433,"context_line":"        if svc_ids:"},{"line_number":1434,"context_line":"            q_reg \u003d q_reg.filter("},{"line_number":1435,"context_line":"                models.HealthRegistry.engine_id.notin_(svc_ids))"},{"line_number":1436,"context_line":"        q_reg.update({\u0027engine_id\u0027: engine_id}, synchronize_session\u003dFalse)"},{"line_number":1437,"context_line":"        result \u003d q_reg.all()"},{"line_number":1438,"context_line":"        return result"},{"line_number":1439,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"da36d5c6_46d123fb","side":"PARENT","line":1436,"range":{"start_line":1436,"start_character":8,"end_line":1436,"end_character":73},"in_reply_to":"da36d5c6_2687cf10","updated":"2017-02-18 15:09:44.000000000","message":"Here is TODO point out in patch 2\n# TODO(AnyOne):In current version, registry_claim is before create self service record. It\u0027s no matter, because we have called registry_claim periodically. But maybe we\u0027d better change this order.","commit_id":"ff8e6f1eb68e351d94d30c8eaadb2142c8056b61"},{"author":{"_account_id":8246,"name":"Qiming Teng","email":"tengqm@outlook.com","username":"tengqm"},"change_message_id":"3a3e8d7d2db8a8899da9bda88e7307979c066b8e","unresolved":false,"context_lines":[{"line_number":1433,"context_line":"        if svc_ids:"},{"line_number":1434,"context_line":"            q_reg \u003d q_reg.filter("},{"line_number":1435,"context_line":"                models.HealthRegistry.engine_id.notin_(svc_ids))"},{"line_number":1436,"context_line":"        q_reg.update({\u0027engine_id\u0027: engine_id}, synchronize_session\u003dFalse)"},{"line_number":1437,"context_line":"        result \u003d q_reg.all()"},{"line_number":1438,"context_line":"        return result"},{"line_number":1439,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"da36d5c6_d7e13fd2","side":"PARENT","line":1436,"range":{"start_line":1436,"start_character":8,"end_line":1436,"end_character":73},"in_reply_to":"da36d5c6_46d123fb","updated":"2017-02-19 02:15:50.000000000","message":"that TODO is an imaginative situation that will never happen.","commit_id":"ff8e6f1eb68e351d94d30c8eaadb2142c8056b61"},{"author":{"_account_id":22998,"name":"XueFengLiu","email":"liu.xuefeng1@zte.com.cn","username":"sgfeng"},"change_message_id":"1efad9961e3cd30e13a5c8245737614a858e321c","unresolved":false,"context_lines":[{"line_number":1433,"context_line":"        if svc_ids:"},{"line_number":1434,"context_line":"            q_reg \u003d q_reg.filter("},{"line_number":1435,"context_line":"                models.HealthRegistry.engine_id.notin_(svc_ids))"},{"line_number":1436,"context_line":"        q_reg.update({\u0027engine_id\u0027: engine_id}, synchronize_session\u003dFalse)"},{"line_number":1437,"context_line":"        result \u003d q_reg.all()"},{"line_number":1438,"context_line":"        return result"},{"line_number":1439,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"da36d5c6_86907b97","side":"PARENT","line":1436,"range":{"start_line":1436,"start_character":8,"end_line":1436,"end_character":73},"in_reply_to":"da36d5c6_86623bd7","updated":"2017-02-18 14:15:39.000000000","message":"Cannot.\nIf we only indent line 1436, we need suppose we create current engine service before calling registry_claim.","commit_id":"ff8e6f1eb68e351d94d30c8eaadb2142c8056b61"},{"author":{"_account_id":8246,"name":"Qiming Teng","email":"tengqm@outlook.com","username":"tengqm"},"change_message_id":"fabe864d6a3c52ea4902a510a00328c2cb6d9854","unresolved":false,"context_lines":[{"line_number":1433,"context_line":"        if svc_ids:"},{"line_number":1434,"context_line":"            q_reg \u003d q_reg.filter("},{"line_number":1435,"context_line":"                models.HealthRegistry.engine_id.notin_(svc_ids))"},{"line_number":1436,"context_line":"        q_reg.update({\u0027engine_id\u0027: engine_id}, synchronize_session\u003dFalse)"},{"line_number":1437,"context_line":"        result \u003d q_reg.all()"},{"line_number":1438,"context_line":"        return result"},{"line_number":1439,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"da36d5c6_2687cf10","side":"PARENT","line":1436,"range":{"start_line":1436,"start_character":8,"end_line":1436,"end_character":73},"in_reply_to":"da36d5c6_86907b97","updated":"2017-02-18 14:50:38.000000000","message":"why that is not the case?\n\nis there a situation where there is not \"current engine service\" but some others calling registry_claim with that engine_id?","commit_id":"ff8e6f1eb68e351d94d30c8eaadb2142c8056b61"},{"author":{"_account_id":22998,"name":"XueFengLiu","email":"liu.xuefeng1@zte.com.cn","username":"sgfeng"},"change_message_id":"911efb0beeeddd0e17eea7403925ce188f6b8644","unresolved":false,"context_lines":[{"line_number":1433,"context_line":"        if svc_ids:"},{"line_number":1434,"context_line":"            q_reg \u003d q_reg.filter("},{"line_number":1435,"context_line":"                models.HealthRegistry.engine_id.notin_(svc_ids))"},{"line_number":1436,"context_line":"        q_reg.update({\u0027engine_id\u0027: engine_id}, synchronize_session\u003dFalse)"},{"line_number":1437,"context_line":"        result \u003d q_reg.all()"},{"line_number":1438,"context_line":"        return result"},{"line_number":1439,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"da36d5c6_d15dc6ed","side":"PARENT","line":1436,"range":{"start_line":1436,"start_character":8,"end_line":1436,"end_character":73},"in_reply_to":"da36d5c6_b7e20a9e","updated":"2017-02-20 02:13:27.000000000","message":"OK.","commit_id":"ff8e6f1eb68e351d94d30c8eaadb2142c8056b61"},{"author":{"_account_id":22998,"name":"XueFengLiu","email":"liu.xuefeng1@zte.com.cn","username":"sgfeng"},"change_message_id":"4f3737e08e9e95fca5fd9746ae0db65a8a580486","unresolved":false,"context_lines":[{"line_number":1433,"context_line":"        if svc_ids:"},{"line_number":1434,"context_line":"            q_reg \u003d q_reg.filter("},{"line_number":1435,"context_line":"                models.HealthRegistry.engine_id.notin_(svc_ids))"},{"line_number":1436,"context_line":"        q_reg.update({\u0027engine_id\u0027: engine_id}, synchronize_session\u003dFalse)"},{"line_number":1437,"context_line":"        result \u003d q_reg.all()"},{"line_number":1438,"context_line":"        return result"},{"line_number":1439,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"da36d5c6_f7e723aa","side":"PARENT","line":1436,"range":{"start_line":1436,"start_character":8,"end_line":1436,"end_character":73},"in_reply_to":"da36d5c6_d7e13fd2","updated":"2017-02-19 07:32:58.000000000","message":"QiMing\nI think we have two problem:\n1.About create service record before first registry_claim, not urgent.\n2.About claim api, we assume we create service record before firstregistry_claim. And even if we ensured this assumption. line1436-line1437 still have hidden bug.You can do a test:)\n\nAnd about my change, do you mean I add a result\u003d[] which you don\u0027t want?","commit_id":"ff8e6f1eb68e351d94d30c8eaadb2142c8056b61"},{"author":{"_account_id":8246,"name":"Qiming Teng","email":"tengqm@outlook.com","username":"tengqm"},"change_message_id":"2d18a94b144bb9c0896f9bb50839945fa7b73a84","unresolved":false,"context_lines":[{"line_number":1433,"context_line":"        if svc_ids:"},{"line_number":1434,"context_line":"            q_reg \u003d q_reg.filter("},{"line_number":1435,"context_line":"                models.HealthRegistry.engine_id.notin_(svc_ids))"},{"line_number":1436,"context_line":"        q_reg.update({\u0027engine_id\u0027: engine_id}, synchronize_session\u003dFalse)"},{"line_number":1437,"context_line":"        result \u003d q_reg.all()"},{"line_number":1438,"context_line":"        return result"},{"line_number":1439,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"da36d5c6_b7e20a9e","side":"PARENT","line":1436,"range":{"start_line":1436,"start_character":8,"end_line":1436,"end_character":73},"in_reply_to":"da36d5c6_f7e723aa","updated":"2017-02-20 01:46:00.000000000","message":"My suggestion is we fix root cause of a problem first, then solve the derivative ones.","commit_id":"ff8e6f1eb68e351d94d30c8eaadb2142c8056b61"},{"author":{"_account_id":8246,"name":"Qiming Teng","email":"tengqm@outlook.com","username":"tengqm"},"change_message_id":"b58fb9f960685f8f7ed3c157391f119b981010cb","unresolved":false,"context_lines":[{"line_number":1436,"context_line":"        # get result then do update"},{"line_number":1437,"context_line":"        result \u003d q_reg.all()"},{"line_number":1438,"context_line":""},{"line_number":1439,"context_line":"        q_reg.update({\u0027engine_id\u0027: engine_id}, synchronize_session\u003dFalse)"},{"line_number":1440,"context_line":""},{"line_number":1441,"context_line":"        return result"},{"line_number":1442,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"da36d5c6_039b8fca","line":1439,"range":{"start_line":1439,"start_character":14,"end_line":1439,"end_character":20},"updated":"2017-02-20 05:28:50.000000000","message":"looks okay now.\n\nThe key issue here seems related to the return value from sqlalchemy.Query.update (ref: http://docs.sqlalchemy.org/en/latest/orm/query.html#sqlalchemy.orm.query.Query.update ) which returns only the number of rows updated.\n\nFetching the matched records first is the right way to go.","commit_id":"ca88defda24d192722c1f1c7eff4c7ac7093d8a5"},{"author":{"_account_id":22998,"name":"XueFengLiu","email":"liu.xuefeng1@zte.com.cn","username":"sgfeng"},"change_message_id":"76b346f5713923954361aa91d5dea2c5828f7ee2","unresolved":false,"context_lines":[{"line_number":1436,"context_line":"        # get result then do update"},{"line_number":1437,"context_line":"        result \u003d q_reg.all()"},{"line_number":1438,"context_line":""},{"line_number":1439,"context_line":"        q_reg.update({\u0027engine_id\u0027: engine_id}, synchronize_session\u003dFalse)"},{"line_number":1440,"context_line":""},{"line_number":1441,"context_line":"        return result"},{"line_number":1442,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"da36d5c6_a3ac83ee","line":1439,"range":{"start_line":1439,"start_character":14,"end_line":1439,"end_character":20},"in_reply_to":"da36d5c6_039b8fca","updated":"2017-02-20 05:29:56.000000000","message":"Yes.QiMing.","commit_id":"ca88defda24d192722c1f1c7eff4c7ac7093d8a5"}]}