)]}'
{"manila/share/manager.py":[{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"4da10b058d5d38862259a63899e7bfa388396bf1","unresolved":true,"context_lines":[{"line_number":2567,"context_line":"        def qualified_replica(r):"},{"line_number":2568,"context_line":"            return (share_utils.extract_host(r[\u0027host\u0027]) \u003d\u003d"},{"line_number":2569,"context_line":"                    share_utils.extract_host(self.host) and"},{"line_number":2570,"context_line":"                    r[\u0027replica_state\u0027] !\u003d constants.REPLICA_STATE_ACTIVE)"},{"line_number":2571,"context_line":""},{"line_number":2572,"context_line":"        replicas \u003d list(filter(lambda x: qualified_replica(x), replicas))"},{"line_number":2573,"context_line":"        for replica in replicas:"}],"source_content_type":"text/x-python","patch_set":2,"id":"a99fc741_c4c88ba9","line":2570,"range":{"start_line":2570,"start_character":0,"end_line":2570,"end_character":73},"updated":"2021-04-16 11:23:10.000000000","message":"Yes, I think that is safe to remove the active ones here. You will avoid another db call later, but I can\u0027t see how it would fail since there is a validation inside \u0027_share_replica_update\u0027.","commit_id":"7412add382798ae2720460451fda5cad10ed3535"},{"author":{"_account_id":18816,"name":"Maurice Escher","display_name":"carthaca","email":"maurice.escher@sap.com","username":"mapocace"},"change_message_id":"4247cbac0bea7f032733d531639384163df18a70","unresolved":true,"context_lines":[{"line_number":2567,"context_line":"        def qualified_replica(r):"},{"line_number":2568,"context_line":"            return (share_utils.extract_host(r[\u0027host\u0027]) \u003d\u003d"},{"line_number":2569,"context_line":"                    share_utils.extract_host(self.host) and"},{"line_number":2570,"context_line":"                    r[\u0027replica_state\u0027] !\u003d constants.REPLICA_STATE_ACTIVE)"},{"line_number":2571,"context_line":""},{"line_number":2572,"context_line":"        replicas \u003d list(filter(lambda x: qualified_replica(x), replicas))"},{"line_number":2573,"context_line":"        for replica in replicas:"}],"source_content_type":"text/x-python","patch_set":2,"id":"bdbbb481_bf500440","line":2570,"range":{"start_line":2570,"start_character":0,"end_line":2570,"end_character":73},"in_reply_to":"44566810_3fb1123e","updated":"2021-04-16 13:04:29.000000000","message":"I see, I got confused, thanks for double-checking","commit_id":"7412add382798ae2720460451fda5cad10ed3535"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"d27597ad5891684beb4db78a1ecd67a83ee98c7c","unresolved":true,"context_lines":[{"line_number":2567,"context_line":"        def qualified_replica(r):"},{"line_number":2568,"context_line":"            return (share_utils.extract_host(r[\u0027host\u0027]) \u003d\u003d"},{"line_number":2569,"context_line":"                    share_utils.extract_host(self.host) and"},{"line_number":2570,"context_line":"                    r[\u0027replica_state\u0027] !\u003d constants.REPLICA_STATE_ACTIVE)"},{"line_number":2571,"context_line":""},{"line_number":2572,"context_line":"        replicas \u003d list(filter(lambda x: qualified_replica(x), replicas))"},{"line_number":2573,"context_line":"        for replica in replicas:"}],"source_content_type":"text/x-python","patch_set":2,"id":"44566810_3fb1123e","line":2570,"range":{"start_line":2570,"start_character":0,"end_line":2570,"end_character":73},"in_reply_to":"6b05378f_c66b2140","updated":"2021-04-16 12:37:06.000000000","message":"I can see your problem, but I don\u0027t understand how *only* filter the \u0027active\u0027 share instances will prevent that issue to happen, since the replica with \u0027replica_state\u0027 set to \u0027error\u0027 will still be called for \u0027_share_replica_update\u0027. Don\u0027t you need to do some validations in L2618?","commit_id":"7412add382798ae2720460451fda5cad10ed3535"},{"author":{"_account_id":18816,"name":"Maurice Escher","display_name":"carthaca","email":"maurice.escher@sap.com","username":"mapocace"},"change_message_id":"fd35c34d4f94577b0566d2d2950cd7b4caac361f","unresolved":true,"context_lines":[{"line_number":2567,"context_line":"        def qualified_replica(r):"},{"line_number":2568,"context_line":"            return (share_utils.extract_host(r[\u0027host\u0027]) \u003d\u003d"},{"line_number":2569,"context_line":"                    share_utils.extract_host(self.host) and"},{"line_number":2570,"context_line":"                    r[\u0027replica_state\u0027] !\u003d constants.REPLICA_STATE_ACTIVE)"},{"line_number":2571,"context_line":""},{"line_number":2572,"context_line":"        replicas \u003d list(filter(lambda x: qualified_replica(x), replicas))"},{"line_number":2573,"context_line":"        for replica in replicas:"}],"source_content_type":"text/x-python","patch_set":2,"id":"6b05378f_c66b2140","line":2570,"range":{"start_line":2570,"start_character":0,"end_line":2570,"end_character":73},"in_reply_to":"a99fc741_c4c88ba9","updated":"2021-04-16 11:59:12.000000000","message":"Think of a share without an active replica, e.g. where the only related share instance has replica_state\u003derror","commit_id":"7412add382798ae2720460451fda5cad10ed3535"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"4da10b058d5d38862259a63899e7bfa388396bf1","unresolved":true,"context_lines":[{"line_number":2596,"context_line":""},{"line_number":2597,"context_line":"        # We don\u0027t poll for replicas that are busy in some operation,"},{"line_number":2598,"context_line":"        # or if they are the \u0027active\u0027 instance."},{"line_number":2599,"context_line":"        if (share_replica[\u0027status\u0027] in constants.TRANSITIONAL_STATUSES"},{"line_number":2600,"context_line":"            or share_replica[\u0027replica_state\u0027] \u003d\u003d"},{"line_number":2601,"context_line":"                constants.REPLICA_STATE_ACTIVE):"},{"line_number":2602,"context_line":"            return"},{"line_number":2603,"context_line":""},{"line_number":2604,"context_line":"        share_server \u003d self._get_share_server(context, share_replica)"},{"line_number":2605,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"f5d343a1_ee26bb53","line":2602,"range":{"start_line":2599,"start_character":0,"end_line":2602,"end_character":18},"updated":"2021-04-16 11:23:10.000000000","message":"There is a validation here.","commit_id":"7412add382798ae2720460451fda5cad10ed3535"},{"author":{"_account_id":18816,"name":"Maurice Escher","display_name":"carthaca","email":"maurice.escher@sap.com","username":"mapocace"},"change_message_id":"fd35c34d4f94577b0566d2d2950cd7b4caac361f","unresolved":true,"context_lines":[{"line_number":2596,"context_line":""},{"line_number":2597,"context_line":"        # We don\u0027t poll for replicas that are busy in some operation,"},{"line_number":2598,"context_line":"        # or if they are the \u0027active\u0027 instance."},{"line_number":2599,"context_line":"        if (share_replica[\u0027status\u0027] in constants.TRANSITIONAL_STATUSES"},{"line_number":2600,"context_line":"            or share_replica[\u0027replica_state\u0027] \u003d\u003d"},{"line_number":2601,"context_line":"                constants.REPLICA_STATE_ACTIVE):"},{"line_number":2602,"context_line":"            return"},{"line_number":2603,"context_line":""},{"line_number":2604,"context_line":"        share_server \u003d self._get_share_server(context, share_replica)"},{"line_number":2605,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"8d87e995_c1a18422","line":2602,"range":{"start_line":2599,"start_character":0,"end_line":2602,"end_character":18},"in_reply_to":"f5d343a1_ee26bb53","updated":"2021-04-16 11:59:12.000000000","message":"Yes, and I think intentionally we don\u0027t want to remove non-active replicas in error states at this place, because the method is trying to fix these.","commit_id":"7412add382798ae2720460451fda5cad10ed3535"},{"author":{"_account_id":18816,"name":"Maurice Escher","display_name":"carthaca","email":"maurice.escher@sap.com","username":"mapocace"},"change_message_id":"fd35c34d4f94577b0566d2d2950cd7b4caac361f","unresolved":true,"context_lines":[{"line_number":2617,"context_line":""},{"line_number":2618,"context_line":"        _active_replica \u003d [x for x in replica_list"},{"line_number":2619,"context_line":"                           if x[\u0027replica_state\u0027] \u003d\u003d"},{"line_number":2620,"context_line":"                           constants.REPLICA_STATE_ACTIVE][0]"},{"line_number":2621,"context_line":""},{"line_number":2622,"context_line":"        # Get snapshots for the share."},{"line_number":2623,"context_line":"        share_snapshots \u003d self.db.share_snapshot_get_all_for_share("}],"source_content_type":"text/x-python","patch_set":2,"id":"b9723044_718fcdb9","line":2620,"updated":"2021-04-16 11:59:12.000000000","message":"My first attempt of fixing was adding a guard here to avoid an IndexError. But I came to the conclusion that the place with filtering at periodic_share_replica_update() is better, because it is\n- earlier and therefore avoids unnecessary db calls\n- has a more limited scope (only affects periodic_share_replica_update). I could not judge if returning here (or raising a different more meaningful error) would be actually the right thing to do in case of this method being called from other places, e.g. by ShareReplicationController.resync().\n\n... thinking about it while writing: if I trigger an share-replica-resync on my share with it\u0027s only share instance in error state, I\u0027ll maybe get the IndexError, too. But this is another edge case and a really odd thing to do. I could not quickly reproduce, because my example replica did also not have a host set (and there is a validation for that in share.api.update_share_replica())","commit_id":"7412add382798ae2720460451fda5cad10ed3535"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"d27597ad5891684beb4db78a1ecd67a83ee98c7c","unresolved":true,"context_lines":[{"line_number":2617,"context_line":""},{"line_number":2618,"context_line":"        _active_replica \u003d [x for x in replica_list"},{"line_number":2619,"context_line":"                           if x[\u0027replica_state\u0027] \u003d\u003d"},{"line_number":2620,"context_line":"                           constants.REPLICA_STATE_ACTIVE][0]"},{"line_number":2621,"context_line":""},{"line_number":2622,"context_line":"        # Get snapshots for the share."},{"line_number":2623,"context_line":"        share_snapshots \u003d self.db.share_snapshot_get_all_for_share("}],"source_content_type":"text/x-python","patch_set":2,"id":"4e0aa871_f97dc251","line":2620,"in_reply_to":"b9723044_718fcdb9","updated":"2021-04-16 12:37:06.000000000","message":"If we have a replica that doesn\u0027t have a \u0027active\u0027 data source, shouldn\u0027t we set this replica to error? This method was created to update replica states, and it expect replicas in \u0027sync\u0027, \u0027out_of_sync\u0027 or in \u0027error\u0027 status.","commit_id":"7412add382798ae2720460451fda5cad10ed3535"}]}