)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d5d1349f8ac646de11fba77ed66c6a0fc1227c99","unresolved":false,"context_lines":[{"line_number":44,"context_line":"A better author than I took a stab at it in the related change."},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"Co-Author: Pavel Kvasnička \u003cpavel.kvasnicka@firma.seznam.cz\u003e"},{"line_number":47,"context_line":"Co-Author: Alistair Coles \u003calistair.coles@hpe.com\u003e"},{"line_number":48,"context_line":"Related-Change-Id: I64cadb1a3feb4d819d545137eecfc295389794f0"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"Change-Id: I8b7a592cd22b7378410f624516db0e127de61d99"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"7a3c09a3_a1012bb1","line":47,"updated":"2017-01-17 19:06:06.000000000","message":"You could remove me - I don\u0027t think I contributed to anything in this patch beyond review comments on https://review.openstack.org/#/c/402043","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d5d1349f8ac646de11fba77ed66c6a0fc1227c99","unresolved":false,"context_lines":[{"line_number":46,"context_line":"Co-Author: Pavel Kvasnička \u003cpavel.kvasnicka@firma.seznam.cz\u003e"},{"line_number":47,"context_line":"Co-Author: Alistair Coles \u003calistair.coles@hpe.com\u003e"},{"line_number":48,"context_line":"Related-Change-Id: I64cadb1a3feb4d819d545137eecfc295389794f0"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"Change-Id: I8b7a592cd22b7378410f624516db0e127de61d99"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"7a3c09a3_0132ff5e","line":49,"updated":"2017-01-17 19:06:06.000000000","message":"no bug to close???","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"a951e67510997b5c19cee63edb8b04b08fa44295","unresolved":false,"context_lines":[{"line_number":46,"context_line":"Co-Author: Pavel Kvasnička \u003cpavel.kvasnicka@firma.seznam.cz\u003e"},{"line_number":47,"context_line":"Co-Author: Alistair Coles \u003calistair.coles@hpe.com\u003e"},{"line_number":48,"context_line":"Related-Change-Id: I64cadb1a3feb4d819d545137eecfc295389794f0"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"Change-Id: I8b7a592cd22b7378410f624516db0e127de61d99"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"7a3c09a3_f9876bb2","line":49,"in_reply_to":"7a3c09a3_0132ff5e","updated":"2017-01-17 20:57:06.000000000","message":"not that i\u0027m aware of - it\u0027s been around for awhile and AFAIK it\u0027s only an optimization not a correctness issue... I\u0027m not inclined to file one; but if one get\u0027s opened for this issue and is resolved with this change I\u0027d be happy to add the appropriate commit annotations.","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"}],"swift/obj/diskfile.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"999c36e31adfc05f0c47b29366bbd3129767122c","unresolved":false,"context_lines":[{"line_number":1039,"context_line":"        modified \u003d False"},{"line_number":1040,"context_line":"        force_rewrite \u003d False"},{"line_number":1041,"context_line":"        hashes \u003d {}"},{"line_number":1042,"context_line":"        mtime \u003d -1"},{"line_number":1043,"context_line":""},{"line_number":1044,"context_line":"        if recalculate is None:"},{"line_number":1045,"context_line":"            recalculate \u003d []"}],"source_content_type":"text/x-python","patch_set":2,"id":"7a3c09a3_c9e8d219","side":"PARENT","line":1042,"updated":"2017-01-13 07:36:14.000000000","message":"mtime is here?","commit_id":"8a69410e0e1fbc9dd2831ad7f26f36caf214ab89"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"f6ca14c19e37896bb52c9b9845a874b462eb3b3b","unresolved":false,"context_lines":[{"line_number":1068,"context_line":"                    mtime \u003d getmtime(hashes_file)"},{"line_number":1069,"context_line":"                except OSError as e:"},{"line_number":1070,"context_line":"                    if e.errno !\u003d errno.ENOENT:"},{"line_number":1071,"context_line":"                        raise"},{"line_number":1072,"context_line":"                else:"},{"line_number":1073,"context_line":"                    hashes \u003d read_hashes(hashes_file) or {}"},{"line_number":1074,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"7a3c09a3_75f177cd","line":1071,"updated":"2017-01-13 01:37:55.000000000","message":"else: mtime is not defined","commit_id":"5fe765d3a635502caad55eddfdae8ce08fdb61b7"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"999c36e31adfc05f0c47b29366bbd3129767122c","unresolved":false,"context_lines":[{"line_number":1068,"context_line":"                    mtime \u003d getmtime(hashes_file)"},{"line_number":1069,"context_line":"                except OSError as e:"},{"line_number":1070,"context_line":"                    if e.errno !\u003d errno.ENOENT:"},{"line_number":1071,"context_line":"                        raise"},{"line_number":1072,"context_line":"                else:"},{"line_number":1073,"context_line":"                    hashes \u003d read_hashes(hashes_file) or {}"},{"line_number":1074,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"7a3c09a3_e9ed8e26","line":1071,"in_reply_to":"7a3c09a3_75f177cd","updated":"2017-01-13 07:36:14.000000000","message":"up top?","commit_id":"5fe765d3a635502caad55eddfdae8ce08fdb61b7"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"4d5f7705554eca43b37f349b9a942a272f3325a3","unresolved":false,"context_lines":[{"line_number":1068,"context_line":"                    mtime \u003d getmtime(hashes_file)"},{"line_number":1069,"context_line":"                except OSError as e:"},{"line_number":1070,"context_line":"                    if e.errno !\u003d errno.ENOENT:"},{"line_number":1071,"context_line":"                        raise"},{"line_number":1072,"context_line":"                else:"},{"line_number":1073,"context_line":"                    hashes \u003d read_hashes(hashes_file) or {}"},{"line_number":1074,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"7a3c09a3_22824801","line":1071,"in_reply_to":"7a3c09a3_e9ed8e26","updated":"2017-01-13 08:33:31.000000000","message":"I see. So mtime \u003d -1, force_rewrite \u003d False, hashes_file does not exist and no suffix is invalidated. That\u0027s correct state.","commit_id":"5fe765d3a635502caad55eddfdae8ce08fdb61b7"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"4d5f7705554eca43b37f349b9a942a272f3325a3","unresolved":false,"context_lines":[{"line_number":1070,"context_line":"                    if e.errno !\u003d errno.ENOENT:"},{"line_number":1071,"context_line":"                        raise"},{"line_number":1072,"context_line":"                else:"},{"line_number":1073,"context_line":"                    hashes \u003d read_hashes(hashes_file) or {}"},{"line_number":1074,"context_line":""},{"line_number":1075,"context_line":"        if do_listdir:"},{"line_number":1076,"context_line":"            for suff in os.listdir(partition_path):"}],"source_content_type":"text/x-python","patch_set":2,"id":"7a3c09a3_4272f4e6","line":1073,"range":{"start_line":1073,"start_character":20,"end_line":1073,"end_character":26},"updated":"2017-01-13 08:33:31.000000000","message":"+1 / We will not rewrite \"better\" *existing* pkl file","commit_id":"5fe765d3a635502caad55eddfdae8ce08fdb61b7"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"f6ca14c19e37896bb52c9b9845a874b462eb3b3b","unresolved":false,"context_lines":[{"line_number":1092,"context_line":"                modified \u003d True"},{"line_number":1093,"context_line":"        if modified:"},{"line_number":1094,"context_line":"            with lock_path(partition_path):"},{"line_number":1095,"context_line":"                if force_rewrite or not exists(hashes_file) or \\"},{"line_number":1096,"context_line":"                        getmtime(hashes_file) \u003d\u003d mtime:"},{"line_number":1097,"context_line":"                    write_pickle("},{"line_number":1098,"context_line":"                        hashes, hashes_file, partition_path, PICKLE_PROTOCOL)"}],"source_content_type":"text/x-python","patch_set":2,"id":"7a3c09a3_58cbd272","line":1095,"range":{"start_line":1095,"start_character":19,"end_line":1095,"end_character":32},"updated":"2017-01-13 01:37:55.000000000","message":"Why force it? When pkl did not exist then it is not good idea to rewrite \"something\". In patch #402043 is patched concurrent write of pkl - there is a minimal chance that one process lists dir and starts calculating hashes. With your patch #418690 is invalidated changed suffix but second call of REPLICATE for the same path will clean invalidations file, lists dir and calculates hashes before the first process. Second process writes correct pkl, first process rewrites them and there is not invalidation entry.","commit_id":"5fe765d3a635502caad55eddfdae8ce08fdb61b7"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"999c36e31adfc05f0c47b29366bbd3129767122c","unresolved":false,"context_lines":[{"line_number":1092,"context_line":"                modified \u003d True"},{"line_number":1093,"context_line":"        if modified:"},{"line_number":1094,"context_line":"            with lock_path(partition_path):"},{"line_number":1095,"context_line":"                if force_rewrite or not exists(hashes_file) or \\"},{"line_number":1096,"context_line":"                        getmtime(hashes_file) \u003d\u003d mtime:"},{"line_number":1097,"context_line":"                    write_pickle("},{"line_number":1098,"context_line":"                        hashes, hashes_file, partition_path, PICKLE_PROTOCOL)"}],"source_content_type":"text/x-python","patch_set":2,"id":"7a3c09a3_e9d4ae48","line":1095,"in_reply_to":"7a3c09a3_58cbd272","updated":"2017-01-13 07:36:14.000000000","message":"I hate force_rewrite, i agree it needs to come out.","commit_id":"5fe765d3a635502caad55eddfdae8ce08fdb61b7"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"648a8ae68dc09a56cce1a9bba07caa9f472799ca","unresolved":false,"context_lines":[{"line_number":59,"context_line":"from swift.common.request_helpers import is_sys_meta"},{"line_number":60,"context_line":"from swift.common.utils import mkdirs, Timestamp, \\"},{"line_number":61,"context_line":"    storage_directory, hash_path, renamer, fallocate, fsync, fdatasync, \\"},{"line_number":62,"context_line":"    fsync_dir, drop_buffer_cache, lock_path, write_pickle, \\"},{"line_number":63,"context_line":"    config_true_value, listdir, split_path, ismount, remove_file, \\"},{"line_number":64,"context_line":"    get_md5_socket, F_SETPIPE_SZ, decode_timestamps, encode_timestamps, \\"},{"line_number":65,"context_line":"    tpool_reraise, MD5_OF_EMPTY_STRING, link_fd_to_path, o_tmpfile_supported, \\"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_3265c81d","line":62,"range":{"start_line":62,"start_character":45,"end_line":62,"end_character":57},"updated":"2017-01-16 20:57:09.000000000","message":"I can imagine to import read_pickle (Your\u0027s read_hashes function) from common.utils","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9c4862a61a7e305dc0466384bd1e217b307f0ad3","unresolved":false,"context_lines":[{"line_number":59,"context_line":"from swift.common.request_helpers import is_sys_meta"},{"line_number":60,"context_line":"from swift.common.utils import mkdirs, Timestamp, \\"},{"line_number":61,"context_line":"    storage_directory, hash_path, renamer, fallocate, fsync, fdatasync, \\"},{"line_number":62,"context_line":"    fsync_dir, drop_buffer_cache, lock_path, write_pickle, \\"},{"line_number":63,"context_line":"    config_true_value, listdir, split_path, ismount, remove_file, \\"},{"line_number":64,"context_line":"    get_md5_socket, F_SETPIPE_SZ, decode_timestamps, encode_timestamps, \\"},{"line_number":65,"context_line":"    tpool_reraise, MD5_OF_EMPTY_STRING, link_fd_to_path, o_tmpfile_supported, \\"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_f8d96123","line":62,"in_reply_to":"7a3c09a3_3265c81d","updated":"2017-01-16 21:24:07.000000000","message":"yeah, when I tried it it seemed like I should teach the updater to use it and I didn\u0027t feel like doing that in this change (there\u0027s some nuanced differences in error handling that made it not so obvious to me)","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d5d1349f8ac646de11fba77ed66c6a0fc1227c99","unresolved":false,"context_lines":[{"line_number":231,"context_line":"    return to_dir"},{"line_number":232,"context_line":""},{"line_number":233,"context_line":""},{"line_number":234,"context_line":"def read_hashes(hashes_file):"},{"line_number":235,"context_line":"    try:"},{"line_number":236,"context_line":"        with open(hashes_file, \u0027rb\u0027) as hashes_fp:"},{"line_number":237,"context_line":"            pickled_hashes \u003d hashes_fp.read()"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_e2e507a4","line":234,"updated":"2017-01-17 19:06:06.000000000","message":"this returns:\n\n  {} if hashes.pkl does not exist (but we only call this function when we expect hashes.pkl to exist\n  {} if hashes.pkl could not be read\n  None if it was read but unpickling failed\n  {hashes} if hashes unpickled successfully","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d5d1349f8ac646de11fba77ed66c6a0fc1227c99","unresolved":false,"context_lines":[{"line_number":236,"context_line":"        with open(hashes_file, \u0027rb\u0027) as hashes_fp:"},{"line_number":237,"context_line":"            pickled_hashes \u003d hashes_fp.read()"},{"line_number":238,"context_line":"    except (IOError, OSError):"},{"line_number":239,"context_line":"        hashes \u003d {}"},{"line_number":240,"context_line":"    else:"},{"line_number":241,"context_line":"        try:"},{"line_number":242,"context_line":"            hashes \u003d pickle.loads(pickled_hashes)"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_5da2409a","line":239,"updated":"2017-01-17 19:06:06.000000000","message":"I don\u0027t think there is any test coverage for this condition - see [1], and I\u0027m not sure what it is intended to catch. We only get here after checking that hashes.pkl exists, so maybe it is unreadable? whatever, do we not want to treat this the same as a corrupt pickle file? or raise the exception? Returning {} could lead to failure to ever calculate the hashes [1]\n\nIn [2] this was changed to hashes\u003dNone and a flag was set to force pkl file to be written. That was consistent with case when the content can\u0027t be unpickled.\n\n[1] https://review.openstack.org/421385\n[2] https://review.openstack.org/#/c/402043","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d5d1349f8ac646de11fba77ed66c6a0fc1227c99","unresolved":false,"context_lines":[{"line_number":263,"context_line":""},{"line_number":264,"context_line":"    if not os.path.exists(hashes_file):"},{"line_number":265,"context_line":"        if os.path.exists(invalidations_file):"},{"line_number":266,"context_line":"            # no hashes at all -\u003e everything\u0027s invalid, so empty the file with"},{"line_number":267,"context_line":"            # the invalid suffixes in it, if it exists"},{"line_number":268,"context_line":"            try:"},{"line_number":269,"context_line":"                with open(invalidations_file, \u0027wb\u0027):"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_38e902e7","line":266,"updated":"2017-01-17 19:06:06.000000000","message":"no test coverage for this clause?","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d5d1349f8ac646de11fba77ed66c6a0fc1227c99","unresolved":false,"context_lines":[{"line_number":274,"context_line":"        return None"},{"line_number":275,"context_line":""},{"line_number":276,"context_line":"    with lock_path(partition_dir):"},{"line_number":277,"context_line":"        hashes \u003d read_hashes(hashes_file)"},{"line_number":278,"context_line":""},{"line_number":279,"context_line":"        modified \u003d False"},{"line_number":280,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_02a2134b","line":277,"updated":"2017-01-17 19:06:06.000000000","message":"hashes could be None if hashes.pkl had corrupt content","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d5d1349f8ac646de11fba77ed66c6a0fc1227c99","unresolved":false,"context_lines":[{"line_number":284,"context_line":"                    if hashes is not None and \\"},{"line_number":285,"context_line":"                            hashes.get(suffix, \u0027\u0027) is not None:"},{"line_number":286,"context_line":"                        hashes[suffix] \u003d None"},{"line_number":287,"context_line":"                        modified \u003d True"},{"line_number":288,"context_line":"        except (IOError, OSError) as e:"},{"line_number":289,"context_line":"            if e.errno !\u003d errno.ENOENT:"},{"line_number":290,"context_line":"                raise"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_c269ebfa","line":287,"updated":"2017-01-17 19:06:06.000000000","message":"modified stays False if hashes\u003d\u003dNone e.g. hashes.pkl had corrupt content","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d5d1349f8ac646de11fba77ed66c6a0fc1227c99","unresolved":false,"context_lines":[{"line_number":290,"context_line":"                raise"},{"line_number":291,"context_line":""},{"line_number":292,"context_line":"        if modified:"},{"line_number":293,"context_line":"            write_pickle(hashes, hashes_file, partition_dir, PICKLE_PROTOCOL)"},{"line_number":294,"context_line":""},{"line_number":295,"context_line":"        # Now that all the invalidations are reflected in hashes.pkl, it\u0027s"},{"line_number":296,"context_line":"        # safe to clear out the invalidations file."}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_a2321f0e","line":293,"updated":"2017-01-17 19:06:06.000000000","message":"corrupt pickle file stays as it was","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"a951e67510997b5c19cee63edb8b04b08fa44295","unresolved":false,"context_lines":[{"line_number":290,"context_line":"                raise"},{"line_number":291,"context_line":""},{"line_number":292,"context_line":"        if modified:"},{"line_number":293,"context_line":"            write_pickle(hashes, hashes_file, partition_dir, PICKLE_PROTOCOL)"},{"line_number":294,"context_line":""},{"line_number":295,"context_line":"        # Now that all the invalidations are reflected in hashes.pkl, it\u0027s"},{"line_number":296,"context_line":"        # safe to clear out the invalidations file."}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_f9f68b8d","line":293,"in_reply_to":"7a3c09a3_a2321f0e","updated":"2017-01-17 20:57:06.000000000","message":"wow, so this was pre-existing - or I botched the method extraction?  was this addressed in PavelK\u0027s fix?","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"adfc835762f398f7bef8113af237b55ae81e0685","unresolved":false,"context_lines":[{"line_number":290,"context_line":"                raise"},{"line_number":291,"context_line":""},{"line_number":292,"context_line":"        if modified:"},{"line_number":293,"context_line":"            write_pickle(hashes, hashes_file, partition_dir, PICKLE_PROTOCOL)"},{"line_number":294,"context_line":""},{"line_number":295,"context_line":"        # Now that all the invalidations are reflected in hashes.pkl, it\u0027s"},{"line_number":296,"context_line":"        # safe to clear out the invalidations file."}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_35aaab85","line":293,"in_reply_to":"7a3c09a3_f9f68b8d","updated":"2017-01-18 16:21:41.000000000","message":"yes, pre-existing and yes Pavel\u0027s patch fixed this [1], but it\u0027s a different issue to the regression addressed here,  I didn\u0027t mean to suggest it should be addressed here.\n\nThe corrupt file case is not a big deal because this method returns None which causes _get_hashes to force a rewrite.\n\nThe related anomaly that Pavel also fixed in passing was different treatment of a hashes.pkl that cannot be read, which can mean that {} is returned and no good hashes.pkl is written.\n\nThese cases are illustrated by these new tests [2]\n\n[1] https://review.openstack.org/#/c/402043/16/swift/obj/diskfile.py@268\n[2] https://review.openstack.org/#/c/421385/1/test/unit/obj/test_diskfile.py@7382","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d5d1349f8ac646de11fba77ed66c6a0fc1227c99","unresolved":false,"context_lines":[{"line_number":1055,"context_line":"            do_listdir \u003d True"},{"line_number":1056,"context_line":"            force_rewrite \u003d True"},{"line_number":1057,"context_line":"        else:"},{"line_number":1058,"context_line":"            if hashes is None:  # no hashes.pkl file; let\u0027s build it"},{"line_number":1059,"context_line":"                do_listdir \u003d True"},{"line_number":1060,"context_line":"                force_rewrite \u003d True"},{"line_number":1061,"context_line":"                hashes \u003d {}"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_f8f05aff","line":1058,"updated":"2017-01-17 19:06:06.000000000","message":"no file or corrupt file, so we force a listdir and rewrite the pickle.\n\nbut if hashes.pkl exists but cannot be read, we get back {} and we don\u0027t do a listdir/rewrite ???","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"0f46fdec977e5636200dffdaf56b893614605168","unresolved":false,"context_lines":[{"line_number":1055,"context_line":"            do_listdir \u003d True"},{"line_number":1056,"context_line":"            force_rewrite \u003d True"},{"line_number":1057,"context_line":"        else:"},{"line_number":1058,"context_line":"            if hashes is None:  # no hashes.pkl file; let\u0027s build it"},{"line_number":1059,"context_line":"                do_listdir \u003d True"},{"line_number":1060,"context_line":"                force_rewrite \u003d True"},{"line_number":1061,"context_line":"                hashes \u003d {}"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_bd484fcd","line":1058,"in_reply_to":"7a3c09a3_f8f05aff","updated":"2017-01-18 13:19:16.000000000","message":"+1 I think by not doing a listdir/rewrite, \"hashes\" will end up having invalid suffixes(consolidate_hashes will add them) and suffixes that are in \"recalculate\" (line#1081). I\u0027m not sure if this is the desired outcome or the use case these different outputs are serving.\n\nAlistair nicely demonstrated this behavior in his tests.","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d5d1349f8ac646de11fba77ed66c6a0fc1227c99","unresolved":false,"context_lines":[{"line_number":1063,"context_line":"                # hashes have been consolidated and hashes.pkl seems to exist;"},{"line_number":1064,"context_line":"                # we take a mtime and read - without the directory lock; if the"},{"line_number":1065,"context_line":"                # file is modified while we\u0027re rehashing invalid suffixes we\u0027ll"},{"line_number":1066,"context_line":"                # abort the operation and retry"},{"line_number":1067,"context_line":"                try:"},{"line_number":1068,"context_line":"                    mtime \u003d getmtime(hashes_file)"},{"line_number":1069,"context_line":"                except OSError as e:"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_a6bce106","line":1066,"updated":"2017-01-17 19:06:06.000000000","message":"ok. so is there a case when we get mtime, then another process writes, then we read the file - we have the latest file from other process, do all the suffix calcs, then throw away the work even though it is up to date :(\n\ncan we avoid that? if we did get mtime and read *with the lock* then we\u0027d know we have the mtime of the file we read, but in taking the lock all we\u0027d be doing is stalling that other process that is then going to then write while we are doing the calcs, so we still throw away the work we did.","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d5d1349f8ac646de11fba77ed66c6a0fc1227c99","unresolved":false,"context_lines":[{"line_number":1070,"context_line":"                    if e.errno !\u003d errno.ENOENT:"},{"line_number":1071,"context_line":"                        raise"},{"line_number":1072,"context_line":"                else:"},{"line_number":1073,"context_line":"                    hashes \u003d read_hashes(hashes_file) or {}"},{"line_number":1074,"context_line":""},{"line_number":1075,"context_line":"        if do_listdir:"},{"line_number":1076,"context_line":"            for suff in os.listdir(partition_path):"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_2b399cf2","line":1073,"range":{"start_line":1073,"start_character":54,"end_line":1073,"end_character":59},"updated":"2017-01-17 19:06:06.000000000","message":"not sure about defaulting to {} here. compare with how the return from consolidate_hashes is treated at line 1058 - if it is None (corrupt file) then we do_listdir and force a rewrite.\n\nSo assume that at line 1053 we get hashes !\u003d None so end up here and then we read_hashes again, but meanwhile some other process corrupted the hashes.pkl, and read_hashes returns None. In contrast to this process discovering a corrupt pkl and forcing do_listdir, we don\u0027t do that here, we just set an empty hashes dict.\n\nArguably that\u0027s a corner case and we\u0027d recover on next pass, but it feels like it would be good to have consistency w.r.t. how the return value of read_hashes is handled.","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"f75fea277908de55c828b4f031c33b4c62c5f92b","unresolved":false,"context_lines":[{"line_number":1070,"context_line":"                    if e.errno !\u003d errno.ENOENT:"},{"line_number":1071,"context_line":"                        raise"},{"line_number":1072,"context_line":"                else:"},{"line_number":1073,"context_line":"                    hashes \u003d read_hashes(hashes_file) or {}"},{"line_number":1074,"context_line":""},{"line_number":1075,"context_line":"        if do_listdir:"},{"line_number":1076,"context_line":"            for suff in os.listdir(partition_path):"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_184aadac","line":1073,"updated":"2017-01-16 21:27:24.000000000","message":"there was some discussion in IRC about just how inefficient this is - hashes_file has a fixed density - it\u0027s small but non-trivial [1].\n\nWe should definitely aim to replace this with a better solution - but it\u0027s hard to see what that is with all the different fixes up in the air [2]\n\n1. https://gist.github.com/clayg/c6a5c41b12c7f0c4075386d3f23d6d81\n2. mainly https://review.openstack.org/#/c/419787/4/swift/obj/diskfile.py","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"0f46fdec977e5636200dffdaf56b893614605168","unresolved":false,"context_lines":[{"line_number":1070,"context_line":"                    if e.errno !\u003d errno.ENOENT:"},{"line_number":1071,"context_line":"                        raise"},{"line_number":1072,"context_line":"                else:"},{"line_number":1073,"context_line":"                    hashes \u003d read_hashes(hashes_file) or {}"},{"line_number":1074,"context_line":""},{"line_number":1075,"context_line":"        if do_listdir:"},{"line_number":1076,"context_line":"            for suff in os.listdir(partition_path):"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_7d28a79a","line":1073,"range":{"start_line":1073,"start_character":54,"end_line":1073,"end_character":59},"in_reply_to":"7a3c09a3_2b399cf2","updated":"2017-01-18 13:19:16.000000000","message":"I think we will also need to resolve on if there is a need to treat hashes\u003d\u003dNone and hashes\u003d\u003d{} differently (refer comment on line#1058).","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"a951e67510997b5c19cee63edb8b04b08fa44295","unresolved":false,"context_lines":[{"line_number":1070,"context_line":"                    if e.errno !\u003d errno.ENOENT:"},{"line_number":1071,"context_line":"                        raise"},{"line_number":1072,"context_line":"                else:"},{"line_number":1073,"context_line":"                    hashes \u003d read_hashes(hashes_file) or {}"},{"line_number":1074,"context_line":""},{"line_number":1075,"context_line":"        if do_listdir:"},{"line_number":1076,"context_line":"            for suff in os.listdir(partition_path):"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_99590791","line":1073,"in_reply_to":"7a3c09a3_2b399cf2","updated":"2017-01-17 20:57:06.000000000","message":"Yeah that seems like a bug - I couldn\u0027t quite keep track of how None and {} are used for which conditions - I\u0027ll try adding a docstring to read_hashes?","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"adfc835762f398f7bef8113af237b55ae81e0685","unresolved":false,"context_lines":[{"line_number":1070,"context_line":"                    if e.errno !\u003d errno.ENOENT:"},{"line_number":1071,"context_line":"                        raise"},{"line_number":1072,"context_line":"                else:"},{"line_number":1073,"context_line":"                    hashes \u003d read_hashes(hashes_file) or {}"},{"line_number":1074,"context_line":""},{"line_number":1075,"context_line":"        if do_listdir:"},{"line_number":1076,"context_line":"            for suff in os.listdir(partition_path):"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_15376fff","line":1073,"in_reply_to":"7a3c09a3_99590791","updated":"2017-01-18 16:21:41.000000000","message":"The obvious (but not necessarily correct ?!) suggestion is to make this:\n\n        else:\n            hashes \u003d read_hashes(hashes_file)\n            if hashes is None:  # no hashes.pkl file; let\u0027s build it\n                do_listdir \u003d True\n                force_rewrite \u003d True\n                hashes \u003d {}\n\nso same as line 1058. It\u0027s strange how read_hashes does not raise an exception but consolidate_hashes might. I think the error handling is just confused in these methods, and we should just decide (a) if we fail to read a good hashes.pkl do we attempt to create it *always* (modulo detecting that another process got there first, and (b) do we always do that inside consolidate_hashes.\n\nBut again, that\u0027s probably for another patch.","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d5d1349f8ac646de11fba77ed66c6a0fc1227c99","unresolved":false,"context_lines":[{"line_number":1093,"context_line":"        if modified:"},{"line_number":1094,"context_line":"            with lock_path(partition_path):"},{"line_number":1095,"context_line":"                if force_rewrite or not exists(hashes_file) or \\"},{"line_number":1096,"context_line":"                        getmtime(hashes_file) \u003d\u003d mtime:"},{"line_number":1097,"context_line":"                    write_pickle("},{"line_number":1098,"context_line":"                        hashes, hashes_file, partition_path, PICKLE_PROTOCOL)"},{"line_number":1099,"context_line":"                    return hashed, hashes"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_b0609bd0","line":1096,"updated":"2017-01-17 19:06:06.000000000","message":"ok, so write the pickle if we modified hashes, and if under lock it still does not exist or is same file that we read before doing hash calcs.\n\nBut force_rewrite looks like it could race - force_rewrite means some error when we read the file, or file did not exist (latter is superfluous). What if another process has written file since then, we\u0027re going to ignore that and write our version?? We didn\u0027t seem to fix this in [1]. Maybe it is now fixed in [2]\n\nanyway, its not the scope of this patch.\n\n[1] https://review.openstack.org/#/c/402043\n[2] https://review.openstack.org/#/c/419787","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"adfc835762f398f7bef8113af237b55ae81e0685","unresolved":false,"context_lines":[{"line_number":1093,"context_line":"        if modified:"},{"line_number":1094,"context_line":"            with lock_path(partition_path):"},{"line_number":1095,"context_line":"                if force_rewrite or not exists(hashes_file) or \\"},{"line_number":1096,"context_line":"                        getmtime(hashes_file) \u003d\u003d mtime:"},{"line_number":1097,"context_line":"                    write_pickle("},{"line_number":1098,"context_line":"                        hashes, hashes_file, partition_path, PICKLE_PROTOCOL)"},{"line_number":1099,"context_line":"                    return hashed, hashes"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_7016014d","line":1096,"in_reply_to":"7a3c09a3_09d3e160","updated":"2017-01-18 16:21:41.000000000","message":"@Mahati - yes, that\u0027s my concern, but it\u0027s an existing behaviour, not new with this patch.","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"a951e67510997b5c19cee63edb8b04b08fa44295","unresolved":false,"context_lines":[{"line_number":1093,"context_line":"        if modified:"},{"line_number":1094,"context_line":"            with lock_path(partition_path):"},{"line_number":1095,"context_line":"                if force_rewrite or not exists(hashes_file) or \\"},{"line_number":1096,"context_line":"                        getmtime(hashes_file) \u003d\u003d mtime:"},{"line_number":1097,"context_line":"                    write_pickle("},{"line_number":1098,"context_line":"                        hashes, hashes_file, partition_path, PICKLE_PROTOCOL)"},{"line_number":1099,"context_line":"                    return hashed, hashes"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_f9d92bf4","line":1096,"in_reply_to":"7a3c09a3_b0609bd0","updated":"2017-01-17 20:57:06.000000000","message":"There enough interactions with this change and the force_rewrite fix that maybe I would work better to put them in series","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"0f46fdec977e5636200dffdaf56b893614605168","unresolved":false,"context_lines":[{"line_number":1093,"context_line":"        if modified:"},{"line_number":1094,"context_line":"            with lock_path(partition_path):"},{"line_number":1095,"context_line":"                if force_rewrite or not exists(hashes_file) or \\"},{"line_number":1096,"context_line":"                        getmtime(hashes_file) \u003d\u003d mtime:"},{"line_number":1097,"context_line":"                    write_pickle("},{"line_number":1098,"context_line":"                        hashes, hashes_file, partition_path, PICKLE_PROTOCOL)"},{"line_number":1099,"context_line":"                    return hashed, hashes"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_09d3e160","line":1096,"in_reply_to":"7a3c09a3_f9d92bf4","updated":"2017-01-18 13:19:16.000000000","message":"what if there was a force_rewrite *and* there is a change in getmtime? We will go ahead and write the pickle with the hashes already calculated with an old mtime?","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b3042838cf0f7b46e5be9ab3f7325787a78ef894","unresolved":false,"context_lines":[{"line_number":246,"context_line":"            # given invalid input depending on the way in which the"},{"line_number":247,"context_line":"            # input is invalid."},{"line_number":248,"context_line":"            hashes \u003d None"},{"line_number":249,"context_line":"    return hashes"},{"line_number":250,"context_line":""},{"line_number":251,"context_line":""},{"line_number":252,"context_line":"def consolidate_hashes(partition_dir):"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a3905b3_80f05bcb","line":249,"updated":"2017-01-24 22:23:06.000000000","message":"while this is good and pure method extraction refactor https://review.openstack.org/#/c/422076/ has made it incredibly obvious to me that we *must not* differentiate behaviors on all these different return values to indicate various problems reading hashes!","commit_id":"ff8844a7dc32ded27a327930b78f79e999a3263b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b3042838cf0f7b46e5be9ab3f7325787a78ef894","unresolved":false,"context_lines":[{"line_number":1060,"context_line":"                    mtime \u003d getmtime(hashes_file)"},{"line_number":1061,"context_line":"                except OSError as e:"},{"line_number":1062,"context_line":"                    if e.errno !\u003d errno.ENOENT:"},{"line_number":1063,"context_line":"                        raise"},{"line_number":1064,"context_line":"                else:"},{"line_number":1065,"context_line":"                    hashes \u003d read_hashes(hashes_file) or {}"},{"line_number":1066,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"5a3905b3_20afa795","line":1063,"updated":"2017-01-24 22:23:06.000000000","message":"Y U NO get_safe_inode!\n\nhttps://review.openstack.org/#/c/419787","commit_id":"ff8844a7dc32ded27a327930b78f79e999a3263b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b3042838cf0f7b46e5be9ab3f7325787a78ef894","unresolved":false,"context_lines":[{"line_number":1062,"context_line":"                    if e.errno !\u003d errno.ENOENT:"},{"line_number":1063,"context_line":"                        raise"},{"line_number":1064,"context_line":"                else:"},{"line_number":1065,"context_line":"                    hashes \u003d read_hashes(hashes_file) or {}"},{"line_number":1066,"context_line":""},{"line_number":1067,"context_line":"        if do_listdir:"},{"line_number":1068,"context_line":"            for suff in os.listdir(partition_path):"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a3905b3_e0075f7f","line":1065,"updated":"2017-01-24 22:23:06.000000000","message":"this normalization certainly it\u0027s heart in the right place - but w/o setting do_listdir + {} it\u0027s not correct\n\nObviously this change is lacking a test because there is no way this can be made to look like correct behavior.  At best it\u0027s not as bad as the performance regression because of some un-related safety mechanism.","commit_id":"ff8844a7dc32ded27a327930b78f79e999a3263b"}],"test/unit/obj/test_diskfile.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d5d1349f8ac646de11fba77ed66c6a0fc1227c99","unresolved":false,"context_lines":[{"line_number":6139,"context_line":""},{"line_number":6140,"context_line":"    @mock.patch(\u0027swift.obj.diskfile.getmtime\u0027)"},{"line_number":6141,"context_line":"    @mock.patch(\u0027swift.obj.diskfile.write_pickle\u0027)"},{"line_number":6142,"context_line":"    def test_get_hashes_consolidates_suffix_rehash_once("},{"line_number":6143,"context_line":"            self, mock_write_pickle, mock_getmtime):"},{"line_number":6144,"context_line":"        non_local \u003d {}"},{"line_number":6145,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"7a3c09a3_a64781ba","line":6142,"updated":"2017-01-17 19:06:06.000000000","message":"ok, fails when run against diskfile.py on master\n\n  Failure\nTraceback (most recent call last):\n  File \"/usr/local/lib/python2.7/dist-packages/mock/mock.py\", line 1305, in patched\n    return func(*args, **keywargs)\n  File \"/home/anc/0dev/swift/test/unit/obj/test_diskfile.py\", line 6193, in test_get_hashes_consolidates_suffix_rehash_once\n    mock_consolidate_hashes.assert_called_once()\n  File \"/usr/local/lib/python2.7/dist-packages/mock/mock.py\", line 915, in assert_called_once\n    raise AssertionError(msg)\nAssertionError: Expected \u0027consolidate_hashes\u0027 to have been called once. Called 2 times.","commit_id":"3c2ca2722d55efbf274ca62f2a75c434f6a69321"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b3042838cf0f7b46e5be9ab3f7325787a78ef894","unresolved":false,"context_lines":[{"line_number":6140,"context_line":"                df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)"},{"line_number":6141,"context_line":"            # each file is opened to read - hashes.pkl is read twice :\u0027("},{"line_number":6142,"context_line":"            expected \u003d {"},{"line_number":6143,"context_line":"                \u0027hashes.pkl\u0027: [\u0027rb\u0027, \u0027rb\u0027],"},{"line_number":6144,"context_line":"                \u0027hashes.invalid\u0027: [\u0027rb\u0027],"},{"line_number":6145,"context_line":"            }"},{"line_number":6146,"context_line":"            self.assertEqual(open_log, expected)"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a3905b3_20f7a7f6","line":6143,"updated":"2017-01-24 22:23:06.000000000","message":"gah!  I can\u0027t take it anymore!  I\u0027m mad at this change now.  I can do better!","commit_id":"ff8844a7dc32ded27a327930b78f79e999a3263b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b3042838cf0f7b46e5be9ab3f7325787a78ef894","unresolved":false,"context_lines":[{"line_number":6342,"context_line":"            except KeyError:"},{"line_number":6343,"context_line":"                # N.B. might raise OSError"},{"line_number":6344,"context_line":"                non_local[\u0027mtime\u0027] \u003d os.path.getmtime(filename)"},{"line_number":6345,"context_line":"            return non_local[\u0027mtime\u0027]"},{"line_number":6346,"context_line":""},{"line_number":6347,"context_line":"        mock_write_pickle.side_effect \u003d mock_write_pickle_def"},{"line_number":6348,"context_line":"        mock_getmtime.side_effect \u003d mock_getmtime_def"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a3905b3_40c3d38a","line":6345,"updated":"2017-01-24 22:23:06.000000000","message":"look at this getmtime mocking!?  It\u0027s crazyness!  https://review.openstack.org/#/c/419787 is going to make all of this go away!  This change should just depend on that one and not be so terrible.","commit_id":"ff8844a7dc32ded27a327930b78f79e999a3263b"}]}