)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":30615,"name":"Tushar Trambak Gite","email":"tushargite96@gmail.com","username":"tushargite96"},"change_message_id":"ee8acac6ae965601b8209702b665ba4dc2b7870e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"839ec7e8_0582733a","updated":"2022-03-24 09:59:52.000000000","message":"LGTM","commit_id":"2e49aa3550e9b73dda029dd597c21783ce97049b"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"93c6f84f1d5a702762deabed4a9029bb384c6fde","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"4f308864_6a7feb8f","updated":"2022-04-07 21:27:05.000000000","message":"This change follows the pattern outlined in https://docs.openstack.org/oslo.db/latest/reference/api/oslo_db.sqlalchemy.html#module-oslo_db.sqlalchemy.session","commit_id":"2e49aa3550e9b73dda029dd597c21783ce97049b"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"6eeed038582078a75479663df1af6b183491a7c8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"f48813df_a86953d3","updated":"2022-04-25 08:37:59.000000000","message":"One question inline, and one suggestion which i guess is too late to follow for this patch series but generally mentioning it.","commit_id":"8bf3d8593b2e37664f031c53f36c33b7197a013f"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"a1ef2a6d2cbe18c37b670585d6592013fb7a981a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"5f30b7a7_c7286878","updated":"2022-04-27 07:47:00.000000000","message":"Thanks for the replies, I\u0027ve confirmed that the code is in compliance with the usage defined in oslo.db docs for enginefacade[1]. LGTM.\n\n[1] https://docs.openstack.org/oslo.db/latest/user/usage.html","commit_id":"8bf3d8593b2e37664f031c53f36c33b7197a013f"}],"cinder/db/sqlalchemy/api.py":[{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"6eeed038582078a75479663df1af6b183491a7c8","unresolved":true,"context_lines":[{"line_number":277,"context_line":"    elif read_deleted \u003d\u003d \u0027int_no\u0027:"},{"line_number":278,"context_line":"        query \u003d query.filter_by(deleted\u003d0)"},{"line_number":279,"context_line":"    else:"},{"line_number":280,"context_line":"        msg \u003d _(\"Unrecognized read_deleted value \u0027%s\u0027\")"},{"line_number":281,"context_line":"        raise Exception(msg % read_deleted)"},{"line_number":282,"context_line":""},{"line_number":283,"context_line":"    if project_only and is_user_context(context):"},{"line_number":284,"context_line":"        if model is models.VolumeAttachment:"},{"line_number":285,"context_line":"            # NOTE(dulek): In case of VolumeAttachment, we need to join"},{"line_number":286,"context_line":"            # `project_id` through `volume` relationship."},{"line_number":287,"context_line":"            query \u003d query.filter("},{"line_number":288,"context_line":"                models.Volume.project_id \u003d\u003d context.project_id"},{"line_number":289,"context_line":"            )"},{"line_number":290,"context_line":"        else:"},{"line_number":291,"context_line":"            query \u003d query.filter_by(project_id\u003dcontext.project_id)"},{"line_number":292,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"4559d4e9_df600f21","line":289,"range":{"start_line":280,"start_character":0,"end_line":289,"end_character":13},"updated":"2022-04-25 08:37:59.000000000","message":"Just stating my personal opinion here, these type of formatting changes that are unrelated to the main functionality that the patch is intending to address increases reviewers efforts to check if these changes doesn\u0027t introduce a regression. IMO we shouldn\u0027t be doing these type of changes and keep them for a separate refactoring effort.","commit_id":"8bf3d8593b2e37664f031c53f36c33b7197a013f"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"92079cd37273a494c173e82f0c2fa88f16707e88","unresolved":false,"context_lines":[{"line_number":277,"context_line":"    elif read_deleted \u003d\u003d \u0027int_no\u0027:"},{"line_number":278,"context_line":"        query \u003d query.filter_by(deleted\u003d0)"},{"line_number":279,"context_line":"    else:"},{"line_number":280,"context_line":"        msg \u003d _(\"Unrecognized read_deleted value \u0027%s\u0027\")"},{"line_number":281,"context_line":"        raise Exception(msg % read_deleted)"},{"line_number":282,"context_line":""},{"line_number":283,"context_line":"    if project_only and is_user_context(context):"},{"line_number":284,"context_line":"        if model is models.VolumeAttachment:"},{"line_number":285,"context_line":"            # NOTE(dulek): In case of VolumeAttachment, we need to join"},{"line_number":286,"context_line":"            # `project_id` through `volume` relationship."},{"line_number":287,"context_line":"            query \u003d query.filter("},{"line_number":288,"context_line":"                models.Volume.project_id \u003d\u003d context.project_id"},{"line_number":289,"context_line":"            )"},{"line_number":290,"context_line":"        else:"},{"line_number":291,"context_line":"            query \u003d query.filter_by(project_id\u003dcontext.project_id)"},{"line_number":292,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"c03891f0_069fa58a","line":289,"range":{"start_line":280,"start_character":0,"end_line":289,"end_character":13},"in_reply_to":"4559d4e9_df600f21","updated":"2022-04-26 10:48:34.000000000","message":"I\u0027d normally be inclined to agree, but in this case I wasn\u0027t actually making these changes: black was. I was breaking a lot of formatting during this refactor and rather than manually fix these, I was running each modified function through black. This had the unfortunate side effect of introducing some unnecessary changes but given the time it saved me (this effort already took _days_ to write and test), it seemed a fair thing to do. A possible alternative would have been to get things black-ready before I started making changes, but I\u0027ve done that before and not everyone has liked it (e.g. see Gorka\u0027s comment on [1]). Rock, meet hard place 😄 Hopefully you can understand my desire to avoid incurring unnecessary work even if it made the review experience slightly worse...\n\nAs an aside, I did tell Brian that I was using black to avoid having to manually fix formatting in another review so this isn\u0027t the first time I\u0027ve encountered this. I definitely could have been clearer and called it out in the commit messages.\n\nAs another aside, perhaps there\u0027s a willingness in Cinder to run black over the codebase like Django did recently [2]. There would be short term backport pain, but it\u0027d make these large scale refactors so much easier in the future. You could avoid breaking git blame using a \u0027.git-blame-ignore-revs\u0027 file [3]\n\n[1] https://review.opendev.org/c/openstack/cinder/+/813219\n[2] https://news.ycombinator.com/item?id\u003d30258530\n[3] https://michaelheap.com/git-ignore-rev/","commit_id":"8bf3d8593b2e37664f031c53f36c33b7197a013f"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"a1ef2a6d2cbe18c37b670585d6592013fb7a981a","unresolved":false,"context_lines":[{"line_number":277,"context_line":"    elif read_deleted \u003d\u003d \u0027int_no\u0027:"},{"line_number":278,"context_line":"        query \u003d query.filter_by(deleted\u003d0)"},{"line_number":279,"context_line":"    else:"},{"line_number":280,"context_line":"        msg \u003d _(\"Unrecognized read_deleted value \u0027%s\u0027\")"},{"line_number":281,"context_line":"        raise Exception(msg % read_deleted)"},{"line_number":282,"context_line":""},{"line_number":283,"context_line":"    if project_only and is_user_context(context):"},{"line_number":284,"context_line":"        if model is models.VolumeAttachment:"},{"line_number":285,"context_line":"            # NOTE(dulek): In case of VolumeAttachment, we need to join"},{"line_number":286,"context_line":"            # `project_id` through `volume` relationship."},{"line_number":287,"context_line":"            query \u003d query.filter("},{"line_number":288,"context_line":"                models.Volume.project_id \u003d\u003d context.project_id"},{"line_number":289,"context_line":"            )"},{"line_number":290,"context_line":"        else:"},{"line_number":291,"context_line":"            query \u003d query.filter_by(project_id\u003dcontext.project_id)"},{"line_number":292,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"bb268130_e7c28997","line":289,"range":{"start_line":280,"start_character":0,"end_line":289,"end_character":13},"in_reply_to":"c03891f0_069fa58a","updated":"2022-04-27 07:47:00.000000000","message":"Ack, Personally i don\u0027t like some of black\u0027s formatting choices but i will make sure to not flag them again in the series.","commit_id":"8bf3d8593b2e37664f031c53f36c33b7197a013f"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"6eeed038582078a75479663df1af6b183491a7c8","unresolved":true,"context_lines":[{"line_number":681,"context_line":"        query \u003d query.filter(query_filter)"},{"line_number":682,"context_line":"    return query"},{"line_number":683,"context_line":""},{"line_number":684,"context_line":""},{"line_number":685,"context_line":"def _service_query("},{"line_number":686,"context_line":"    context,"},{"line_number":687,"context_line":"    read_deleted\u003d\u0027no\u0027,"},{"line_number":688,"context_line":"    host\u003dNone,"},{"line_number":689,"context_line":"    cluster_name\u003dNone,"},{"line_number":690,"context_line":"    is_up\u003dNone,"},{"line_number":691,"context_line":"    host_or_cluster\u003dNone,"},{"line_number":692,"context_line":"    backend_match_level\u003dNone,"},{"line_number":693,"context_line":"    disabled\u003dNone,"},{"line_number":694,"context_line":"    frozen\u003dNone,"},{"line_number":695,"context_line":"    **filters,"},{"line_number":696,"context_line":"):"},{"line_number":697,"context_line":"    filters \u003d _clean_filters(filters)"},{"line_number":698,"context_line":"    if filters and not is_valid_model_filters(models.Service, filters):"}],"source_content_type":"text/x-python","patch_set":4,"id":"0d946266_050678f7","line":695,"range":{"start_line":684,"start_character":0,"end_line":695,"end_character":14},"updated":"2022-04-25 08:37:59.000000000","message":"same","commit_id":"8bf3d8593b2e37664f031c53f36c33b7197a013f"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"92079cd37273a494c173e82f0c2fa88f16707e88","unresolved":false,"context_lines":[{"line_number":681,"context_line":"        query \u003d query.filter(query_filter)"},{"line_number":682,"context_line":"    return query"},{"line_number":683,"context_line":""},{"line_number":684,"context_line":""},{"line_number":685,"context_line":"def _service_query("},{"line_number":686,"context_line":"    context,"},{"line_number":687,"context_line":"    read_deleted\u003d\u0027no\u0027,"},{"line_number":688,"context_line":"    host\u003dNone,"},{"line_number":689,"context_line":"    cluster_name\u003dNone,"},{"line_number":690,"context_line":"    is_up\u003dNone,"},{"line_number":691,"context_line":"    host_or_cluster\u003dNone,"},{"line_number":692,"context_line":"    backend_match_level\u003dNone,"},{"line_number":693,"context_line":"    disabled\u003dNone,"},{"line_number":694,"context_line":"    frozen\u003dNone,"},{"line_number":695,"context_line":"    **filters,"},{"line_number":696,"context_line":"):"},{"line_number":697,"context_line":"    filters \u003d _clean_filters(filters)"},{"line_number":698,"context_line":"    if filters and not is_valid_model_filters(models.Service, filters):"}],"source_content_type":"text/x-python","patch_set":4,"id":"b5ef4e5e_06c7deca","line":695,"range":{"start_line":684,"start_character":0,"end_line":695,"end_character":14},"in_reply_to":"0d946266_050678f7","updated":"2022-04-26 10:48:34.000000000","message":"See above","commit_id":"8bf3d8593b2e37664f031c53f36c33b7197a013f"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"6eeed038582078a75479663df1af6b183491a7c8","unresolved":true,"context_lines":[{"line_number":819,"context_line":"    if not CONF.enable_new_services:"},{"line_number":820,"context_line":"        service_ref.disabled \u003d True"},{"line_number":821,"context_line":""},{"line_number":822,"context_line":"    service_ref.save(context.session)"},{"line_number":823,"context_line":"    return service_ref"},{"line_number":824,"context_line":""},{"line_number":825,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"6ecd10e3_7bf3f344","line":822,"range":{"start_line":822,"start_character":21,"end_line":822,"end_character":36},"updated":"2022-04-25 08:37:59.000000000","message":"don\u0027t we need the same kind of backward compatibility logic that we\u0027ve done in model_query?\n\n    if hasattr(context, \u0027session\u0027) and context.session:\n        session \u003d context.session\n\n    if not session:\n        session \u003d get_session()","commit_id":"8bf3d8593b2e37664f031c53f36c33b7197a013f"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"a1ef2a6d2cbe18c37b670585d6592013fb7a981a","unresolved":false,"context_lines":[{"line_number":819,"context_line":"    if not CONF.enable_new_services:"},{"line_number":820,"context_line":"        service_ref.disabled \u003d True"},{"line_number":821,"context_line":""},{"line_number":822,"context_line":"    service_ref.save(context.session)"},{"line_number":823,"context_line":"    return service_ref"},{"line_number":824,"context_line":""},{"line_number":825,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"8f75838f_8af3243c","line":822,"range":{"start_line":822,"start_character":21,"end_line":822,"end_character":36},"in_reply_to":"6e8bb983_6cd08de2","updated":"2022-04-27 07:47:00.000000000","message":"Ack","commit_id":"8bf3d8593b2e37664f031c53f36c33b7197a013f"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"92079cd37273a494c173e82f0c2fa88f16707e88","unresolved":false,"context_lines":[{"line_number":819,"context_line":"    if not CONF.enable_new_services:"},{"line_number":820,"context_line":"        service_ref.disabled \u003d True"},{"line_number":821,"context_line":""},{"line_number":822,"context_line":"    service_ref.save(context.session)"},{"line_number":823,"context_line":"    return service_ref"},{"line_number":824,"context_line":""},{"line_number":825,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"6e8bb983_6cd08de2","line":822,"range":{"start_line":822,"start_character":21,"end_line":822,"end_character":36},"in_reply_to":"6ecd10e3_7bf3f344","updated":"2022-04-26 10:48:34.000000000","message":"No, \u0027context.session\u0027 will always be set in this method because the method is decorated with the engine facade context manager (\u0027main_context_manager.writer\u0027). The only reason we need it in \u0027model_query\u0027 is because that method is still being called by some methods that do not have this decorator. This is the same for any other method that has multiple callers and is the reason I added TODOs for myself in [1]. As soon as all callers of \u0027model_query\u0027 have this decorator, we can remove the logic from \u0027model_query\u0027 and rely on \u0027context.session\u0027 being set. I do that in effectively the last patch of the series at [2]\n\n[1] https://review.opendev.org/c/openstack/cinder/+/830088/4\n[2] https://review.opendev.org/c/openstack/cinder/+/837539/3/cinder/db/sqlalchemy/api.py#299","commit_id":"8bf3d8593b2e37664f031c53f36c33b7197a013f"}]}