)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"f0be8678128418a36b45c1969da6a67d5f0cf916","unresolved":false,"context_lines":[{"line_number":9,"context_line":"1) Optimize write IO when REPLICATE called on object server."},{"line_number":10,"context_line":"Invalidations file is not necessary to open when it is empty."},{"line_number":11,"context_line":"2) Hashes was recalculated twice when invalidations file was not empty,"},{"line_number":12,"context_line":"so pkl modification time is returned after hashes consolidation."},{"line_number":13,"context_line":"3) When pkl file doesn\u0027t exist, new hashes are stored only when another"},{"line_number":14,"context_line":"concurrent process doesn\u0027t create it. Suffix invalidation is not lost"},{"line_number":15,"context_line":"when object is created between path listing and new pkl created."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":10,"id":"3a71b18c_2dff2c02","line":12,"updated":"2016-12-09 17:42:37.000000000","message":"whoa whoa whoa - hold the phone - hashes are *recalculated twice*?!\n\nLike every time there\u0027s a suffix in the invalidations files we listdir all down through the partition tree and calculate the hash for the suffix *twice* - that\u0027s seriously not good?","commit_id":"bead9178d2cd83ff11ba3ccb0768e552c8a6a521"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"ec6cc0f25bc6cfa8b1eea7f74087074014e2ec0d","unresolved":false,"context_lines":[{"line_number":9,"context_line":"1) Optimize write IO when REPLICATE called on object server."},{"line_number":10,"context_line":"Invalidations file is not necessary to open when it is empty."},{"line_number":11,"context_line":"2) Hashes was recalculated twice when invalidations file was not empty,"},{"line_number":12,"context_line":"so pkl modification time is returned after hashes consolidation."},{"line_number":13,"context_line":"3) When pkl file doesn\u0027t exist, new hashes are stored only when another"},{"line_number":14,"context_line":"concurrent process doesn\u0027t create it. Suffix invalidation is not lost"},{"line_number":15,"context_line":"when object is created between path listing and new pkl created."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":10,"id":"3a71b18c_9e7916ff","line":12,"in_reply_to":"3a71b18c_2dff2c02","updated":"2016-12-12 11:46:05.000000000","message":"Yes, when existing suffix was invalidated, changed pkl was stored and suffix path was listed and hashes calculated. Because pkl mtime was changed, it is not stored and recalculated again.","commit_id":"bead9178d2cd83ff11ba3ccb0768e552c8a6a521"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"88c555700c2be6e2008af099a27154324207db67","unresolved":false,"context_lines":[{"line_number":5,"context_line":"CommitDate: 2016-12-12 12:41:53 +0100"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Optimize hash calculation when suffix hash invalidated"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"1) Optimize write IO when REPLICATE called on object server."},{"line_number":10,"context_line":"Invalidations file is not necessary to open when it is empty."},{"line_number":11,"context_line":"2) Hashes was recalculated twice when invalidations file was not empty,"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":11,"id":"1a6eadb0_497d3ae6","line":8,"updated":"2016-12-14 18:08:36.000000000","message":"I found it hard to distinguish between statements describing old behaviour vs new behaviour with this patch. Could you make it obvious what is the old (bad) behaviour vs how things have been fixed. Thanks. I think that might help others.","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"525185270f0dff319e6ff62d04c659abfcaa09da","unresolved":false,"context_lines":[{"line_number":9,"context_line":"1) Optimize write IO when REPLICATE called on object server."},{"line_number":10,"context_line":"Invalidations file is not necessary to open when it is empty."},{"line_number":11,"context_line":"2) Hashes was recalculated twice when invalidations file was not empty,"},{"line_number":12,"context_line":"so pkl modification time is returned after hashes consolidation."},{"line_number":13,"context_line":"3) When pkl file doesn\u0027t exist, new hashes are stored only when another"},{"line_number":14,"context_line":"concurrent process doesn\u0027t create it. Suffix invalidation is not lost"},{"line_number":15,"context_line":"when object is created between path listing and new pkl created."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":11,"id":"1a6eadb0_d20965df","line":12,"updated":"2016-12-12 18:09:13.000000000","message":":\u0027(\n\nYeah, so that mtime check is all about \"has it been modified since I started working\" - if you get the mtime before you collapse the invalid hashes the answer is always yes\n\nIt seems pretty obvious to me after in hindsight looking at the original code [1]\n\nThis is pretty annoying.\n\n1. https://github.com/openstack/swift/blob/bd93d44bb46350f716f1dfb66fb43c2ab4ad8c50/swift/obj/diskfile.py","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b586889e2a1f16be7657633655925bf71d589d80","unresolved":false,"context_lines":[{"line_number":18,"context_line":"concurrent process doesn\u0027t create it. Suffix invalidation is not lost"},{"line_number":19,"context_line":"when object is created between path listing and new pkl created."},{"line_number":20,"context_line":"Suffixes changed after hashes consolidation was not invalidated before,"},{"line_number":21,"context_line":"so new pkl file is created to enable hashes invalidation."},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"This regression was introduced in 2.7.0"},{"line_number":24,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":14,"id":"1a6eadb0_5c7d08b0","line":21,"updated":"2016-12-20 18:00:58.000000000","message":"@Pavel I filed a bug report [1] for this last issue because I wanted to keep a record of my understanding of how it occurs in more detail than a commit message allows.\n\nCould you please check my understanding on the bug report and correct me if necessary?\n\nAlso, it\u0027s worth adding a Closes-Bug tag here\n\n[1] https://bugs.launchpad.net/swift/+bug/1651530","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"17fbde937570f2dc34f8aea5a3717cf0ad602117","unresolved":false,"context_lines":[{"line_number":18,"context_line":"concurrent process doesn\u0027t create it. Suffix invalidation is not lost"},{"line_number":19,"context_line":"when object is created between path listing and new pkl created."},{"line_number":20,"context_line":"Suffixes changed after hashes consolidation was not invalidated before,"},{"line_number":21,"context_line":"so new pkl file is created to enable hashes invalidation."},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"This regression was introduced in 2.7.0"},{"line_number":24,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":14,"id":"da4df55a_859304a3","line":21,"in_reply_to":"1a6eadb0_5c7d08b0","updated":"2016-12-22 12:56:37.000000000","message":"The bug report seems clear for me.","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"}],"swift/obj/diskfile.py":[{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"28ee2c7ca1cfeae72a4c675f35a2dac7f57b15cf","unresolved":false,"context_lines":[{"line_number":276,"context_line":"        try:"},{"line_number":277,"context_line":"            with open(invalidations_file, \u0027rb\u0027) as inv_fh:"},{"line_number":278,"context_line":"                for line in inv_fh:"},{"line_number":279,"context_line":"                    modified \u003d True"},{"line_number":280,"context_line":"                    suffix \u003d line.strip()"},{"line_number":281,"context_line":"                    if hashes is not None and \\"},{"line_number":282,"context_line":"                            hashes.get(suffix, \u0027\u0027) is not None:"}],"source_content_type":"text/x-python","patch_set":4,"id":"3a71b18c_86db21ed","line":279,"range":{"start_line":279,"start_character":20,"end_line":279,"end_character":28},"updated":"2016-12-01 16:00:07.000000000","message":"Update pkl mtime to be sure that get_hashes stores the last valid hashes\n * empty pkl file is created to check that another process updates it","commit_id":"701143e4f51fafa6bc15a6221ee101ba2349c0ba"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"28ee2c7ca1cfeae72a4c675f35a2dac7f57b15cf","unresolved":false,"context_lines":[{"line_number":290,"context_line":""},{"line_number":291,"context_line":"            # Now that all the invalidations are reflected in hashes.pkl, it\u0027s"},{"line_number":292,"context_line":"            # safe to clear out the invalidations file."},{"line_number":293,"context_line":"            try:"},{"line_number":294,"context_line":"                with open(invalidations_file, \u0027wb\u0027) as inv_fh:"},{"line_number":295,"context_line":"                    pass"},{"line_number":296,"context_line":"            except OSError as e:"}],"source_content_type":"text/x-python","patch_set":4,"id":"3a71b18c_267ecde4","line":293,"range":{"start_line":293,"start_character":12,"end_line":293,"end_character":15},"updated":"2016-12-01 16:00:07.000000000","message":"No write when invalidation file is empty.\n\n * swift 2.7.0 brings 1 wrtie io per REPLICATE request","commit_id":"701143e4f51fafa6bc15a6221ee101ba2349c0ba"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"28ee2c7ca1cfeae72a4c675f35a2dac7f57b15cf","unresolved":false,"context_lines":[{"line_number":298,"context_line":"                    raise"},{"line_number":299,"context_line":""},{"line_number":300,"context_line":"        try:"},{"line_number":301,"context_line":"            mtime \u003d getmtime(hashes_file)"},{"line_number":302,"context_line":"        except OSError as e:"},{"line_number":303,"context_line":"            if e.errno !\u003d errno.ENOENT:"},{"line_number":304,"context_line":"                raise"}],"source_content_type":"text/x-python","patch_set":4,"id":"3a71b18c_c15503c7","line":301,"range":{"start_line":301,"start_character":12,"end_line":301,"end_character":17},"updated":"2016-12-01 16:00:07.000000000","message":"return pkl mtime - _get_hashes has actual mtime and doesn\u0027t repeat hash calculation","commit_id":"701143e4f51fafa6bc15a6221ee101ba2349c0ba"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"2ffc869e15eac6d04ac59b03d7430f014712ddba","unresolved":false,"context_lines":[{"line_number":298,"context_line":"                    raise"},{"line_number":299,"context_line":""},{"line_number":300,"context_line":"        try:"},{"line_number":301,"context_line":"            mtime \u003d getmtime(hashes_file)"},{"line_number":302,"context_line":"        except OSError as e:"},{"line_number":303,"context_line":"            if e.errno !\u003d errno.ENOENT:"},{"line_number":304,"context_line":"                raise"}],"source_content_type":"text/x-python","patch_set":4,"id":"3a71b18c_c92165a1","line":301,"range":{"start_line":301,"start_character":12,"end_line":301,"end_character":17},"in_reply_to":"3a71b18c_29f740f6","updated":"2016-12-02 18:23:06.000000000","message":"Done","commit_id":"701143e4f51fafa6bc15a6221ee101ba2349c0ba"},{"author":{"_account_id":16896,"name":"Ondřej Nový","email":"novy@ondrej.org","username":"onovy"},"change_message_id":"c2b1f95484963f846a9c225d22ef0d1f1e3c6571","unresolved":false,"context_lines":[{"line_number":298,"context_line":"                    raise"},{"line_number":299,"context_line":""},{"line_number":300,"context_line":"        try:"},{"line_number":301,"context_line":"            mtime \u003d getmtime(hashes_file)"},{"line_number":302,"context_line":"        except OSError as e:"},{"line_number":303,"context_line":"            if e.errno !\u003d errno.ENOENT:"},{"line_number":304,"context_line":"                raise"}],"source_content_type":"text/x-python","patch_set":4,"id":"3a71b18c_29f740f6","line":301,"range":{"start_line":301,"start_character":12,"end_line":301,"end_character":17},"in_reply_to":"3a71b18c_c15503c7","updated":"2016-12-02 16:11:49.000000000","message":"when exception occurs, mtime is not defained for return","commit_id":"701143e4f51fafa6bc15a6221ee101ba2349c0ba"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"28ee2c7ca1cfeae72a4c675f35a2dac7f57b15cf","unresolved":false,"context_lines":[{"line_number":1059,"context_line":"        else:"},{"line_number":1060,"context_line":"            if hashes is None:  # no hashes.pkl file; let\u0027s build it"},{"line_number":1061,"context_line":"                do_listdir \u003d True"},{"line_number":1062,"context_line":"                force_rewrite \u003d True"},{"line_number":1063,"context_line":"                hashes \u003d {}"},{"line_number":1064,"context_line":""},{"line_number":1065,"context_line":"        if do_listdir:"}],"source_content_type":"text/x-python","patch_set":4,"id":"3a71b18c_018ffb23","line":1062,"range":{"start_line":1062,"start_character":16,"end_line":1062,"end_character":36},"updated":"2016-12-01 16:00:07.000000000","message":"TODO: we don\u0027t use force_rewrite because another process can update pkl file","commit_id":"701143e4f51fafa6bc15a6221ee101ba2349c0ba"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"2ffc869e15eac6d04ac59b03d7430f014712ddba","unresolved":false,"context_lines":[{"line_number":1059,"context_line":"        else:"},{"line_number":1060,"context_line":"            if hashes is None:  # no hashes.pkl file; let\u0027s build it"},{"line_number":1061,"context_line":"                do_listdir \u003d True"},{"line_number":1062,"context_line":"                force_rewrite \u003d True"},{"line_number":1063,"context_line":"                hashes \u003d {}"},{"line_number":1064,"context_line":""},{"line_number":1065,"context_line":"        if do_listdir:"}],"source_content_type":"text/x-python","patch_set":4,"id":"3a71b18c_293739e4","line":1062,"range":{"start_line":1062,"start_character":16,"end_line":1062,"end_character":36},"in_reply_to":"3a71b18c_018ffb23","updated":"2016-12-02 18:23:06.000000000","message":"Done\n\nNew pkl file is created to compare mtime and be sure that other process doesn\u0027t store pkl with suffixes whose current process didn\u0027t saw.","commit_id":"701143e4f51fafa6bc15a6221ee101ba2349c0ba"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"f0be8678128418a36b45c1969da6a67d5f0cf916","unresolved":false,"context_lines":[{"line_number":244,"context_line":"    \"\"\""},{"line_number":245,"context_line":"    hashes_file \u003d join(partition_dir, HASH_FILE)"},{"line_number":246,"context_line":"    invalidations_file \u003d join(partition_dir, HASH_INVALIDATIONS_FILE)"},{"line_number":247,"context_line":"    inv_not_empty \u003d False"},{"line_number":248,"context_line":""},{"line_number":249,"context_line":"    with lock_path(partition_dir):"},{"line_number":250,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":10,"id":"3a71b18c_72b01fde","line":247,"updated":"2016-12-09 17:42:37.000000000","message":"nit: I strongly prefer variable names don\u0027t have negatives in the name [1] (i.e. \u0027failed\u0027 instead of \u0027not_success)\u0027\n\nperhaps `found_invalid_entry` ?\n\n1. google for \"how not to name boolean variables\" or something similar for related information","commit_id":"bead9178d2cd83ff11ba3ccb0768e552c8a6a521"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"ec6cc0f25bc6cfa8b1eea7f74087074014e2ec0d","unresolved":false,"context_lines":[{"line_number":244,"context_line":"    \"\"\""},{"line_number":245,"context_line":"    hashes_file \u003d join(partition_dir, HASH_FILE)"},{"line_number":246,"context_line":"    invalidations_file \u003d join(partition_dir, HASH_INVALIDATIONS_FILE)"},{"line_number":247,"context_line":"    inv_not_empty \u003d False"},{"line_number":248,"context_line":""},{"line_number":249,"context_line":"    with lock_path(partition_dir):"},{"line_number":250,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":10,"id":"3a71b18c_7de8bde2","line":247,"in_reply_to":"3a71b18c_72b01fde","updated":"2016-12-12 11:46:05.000000000","message":"Done, thanks for suggestion.","commit_id":"bead9178d2cd83ff11ba3ccb0768e552c8a6a521"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"f0be8678128418a36b45c1969da6a67d5f0cf916","unresolved":false,"context_lines":[{"line_number":1044,"context_line":"            recalculate \u003d []"},{"line_number":1045,"context_line":""},{"line_number":1046,"context_line":"        try:"},{"line_number":1047,"context_line":"            hashes, mtime \u003d self.consolidate_hashes(partition_path)"},{"line_number":1048,"context_line":"        except Exception:"},{"line_number":1049,"context_line":"            do_listdir \u003d True"},{"line_number":1050,"context_line":"            force_rewrite \u003d True"}],"source_content_type":"text/x-python","patch_set":10,"id":"3a71b18c_f2c42f41","line":1047,"updated":"2016-12-09 17:42:37.000000000","message":"i grepped around and this is the only call site I saw","commit_id":"bead9178d2cd83ff11ba3ccb0768e552c8a6a521"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"ec6cc0f25bc6cfa8b1eea7f74087074014e2ec0d","unresolved":false,"context_lines":[{"line_number":1044,"context_line":"            recalculate \u003d []"},{"line_number":1045,"context_line":""},{"line_number":1046,"context_line":"        try:"},{"line_number":1047,"context_line":"            hashes, mtime \u003d self.consolidate_hashes(partition_path)"},{"line_number":1048,"context_line":"        except Exception:"},{"line_number":1049,"context_line":"            do_listdir \u003d True"},{"line_number":1050,"context_line":"            force_rewrite \u003d True"}],"source_content_type":"text/x-python","patch_set":10,"id":"3a71b18c_d93728fc","line":1047,"in_reply_to":"3a71b18c_f2c42f41","updated":"2016-12-12 11:46:05.000000000","message":"You are right. mtime is read when patition directory is locked - so mtime corresponds to the hashes.","commit_id":"bead9178d2cd83ff11ba3ccb0768e552c8a6a521"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"88c555700c2be6e2008af099a27154324207db67","unresolved":false,"context_lines":[{"line_number":1082,"context_line":"        if modified:"},{"line_number":1083,"context_line":"            with lock_path(partition_path):"},{"line_number":1084,"context_line":"                if force_rewrite or not exists(hashes_file) or \\"},{"line_number":1085,"context_line":"                        getmtime(hashes_file) \u003d\u003d mtime:"},{"line_number":1086,"context_line":"                    write_pickle("},{"line_number":1087,"context_line":"                        hashes, hashes_file, partition_path, PICKLE_PROTOCOL)"},{"line_number":1088,"context_line":"                    return hashed, hashes"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_2503cc10","side":"PARENT","line":1085,"updated":"2016-12-14 18:08:36.000000000","message":"force_rewrite\u003d\u003dTrue IFF exception in consolidate_hashes or hashes is None (no hashes.pkl)\n\nso in normal execution when hashes.pkl does exist but gets rewritten in consolidate_hashes then none of these conditions is true so we drop through to line 1089 and needlessly recurse into this function a second time :/","commit_id":"e67a57fb17d576239f8865910c9d56361263e0a9"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"88c555700c2be6e2008af099a27154324207db67","unresolved":false,"context_lines":[{"line_number":251,"context_line":"            with open(hashes_file, \u0027rb\u0027) as hashes_fp:"},{"line_number":252,"context_line":"                pickled_hashes \u003d hashes_fp.read()"},{"line_number":253,"context_line":"        except (IOError, OSError):"},{"line_number":254,"context_line":"            hashes \u003d None"},{"line_number":255,"context_line":"        else:"},{"line_number":256,"context_line":"            try:"},{"line_number":257,"context_line":"                hashes \u003d pickle.loads(pickled_hashes)"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_8e9d985b","line":254,"updated":"2016-12-14 18:08:36.000000000","message":"we need to init a dict for the invalidations to be written to in the loop below","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8cb4efa6cb6d478bb1f491c8e9acc512b0e9ea33","unresolved":false,"context_lines":[{"line_number":251,"context_line":"            with open(hashes_file, \u0027rb\u0027) as hashes_fp:"},{"line_number":252,"context_line":"                pickled_hashes \u003d hashes_fp.read()"},{"line_number":253,"context_line":"        except (IOError, OSError):"},{"line_number":254,"context_line":"            hashes \u003d None"},{"line_number":255,"context_line":"        else:"},{"line_number":256,"context_line":"            try:"},{"line_number":257,"context_line":"                hashes \u003d pickle.loads(pickled_hashes)"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_56768ed7","line":254,"in_reply_to":"1a6eadb0_64512636","updated":"2016-12-15 15:10:18.000000000","message":"ok, got it\n\nso hashes\u003d\u003dNone --\u003e failed to read valid hashes (either no file, unable to read file or bad pickled data)\n\nand then later hashes\u003d\u003dNone --\u003e write a hashes.pkl","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"5830153008d1b02b8ffaeb23e453fd527bc9ed5a","unresolved":false,"context_lines":[{"line_number":251,"context_line":"            with open(hashes_file, \u0027rb\u0027) as hashes_fp:"},{"line_number":252,"context_line":"                pickled_hashes \u003d hashes_fp.read()"},{"line_number":253,"context_line":"        except (IOError, OSError):"},{"line_number":254,"context_line":"            hashes \u003d None"},{"line_number":255,"context_line":"        else:"},{"line_number":256,"context_line":"            try:"},{"line_number":257,"context_line":"                hashes \u003d pickle.loads(pickled_hashes)"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_64512636","line":254,"in_reply_to":"1a6eadb0_8e9d985b","updated":"2016-12-15 08:17:52.000000000","message":"Nope. We need to distinguish between path without hashes (empty path without files) and path without pkl file.\n\nWhen pkl not exists or is corrupted (see line 262) None is returned and do_listdir is set to list path for new suffixes.","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"88c555700c2be6e2008af099a27154324207db67","unresolved":false,"context_lines":[{"line_number":263,"context_line":""},{"line_number":264,"context_line":"        modified \u003d False"},{"line_number":265,"context_line":"        try:"},{"line_number":266,"context_line":"            with open(invalidations_file, \u0027rb\u0027) as inv_fh:"},{"line_number":267,"context_line":"                for line in inv_fh:"},{"line_number":268,"context_line":"                    found_invalid_entry \u003d True"},{"line_number":269,"context_line":"                    suffix \u003d line.strip()"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_6eb11ce1","line":266,"updated":"2016-12-14 18:08:36.000000000","message":"if hashes is None the only thing done in this loop is to set found_invalid_entry\u003dTrue which we probably don\u0027t want to do because that causes the invalidation file to be emptied at line 285 when in fact none of the invalidations have been written to hashes.pkl!","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"5830153008d1b02b8ffaeb23e453fd527bc9ed5a","unresolved":false,"context_lines":[{"line_number":263,"context_line":""},{"line_number":264,"context_line":"        modified \u003d False"},{"line_number":265,"context_line":"        try:"},{"line_number":266,"context_line":"            with open(invalidations_file, \u0027rb\u0027) as inv_fh:"},{"line_number":267,"context_line":"                for line in inv_fh:"},{"line_number":268,"context_line":"                    found_invalid_entry \u003d True"},{"line_number":269,"context_line":"                    suffix \u003d line.strip()"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_ef398fd8","line":266,"in_reply_to":"1a6eadb0_6eb11ce1","updated":"2016-12-15 08:17:52.000000000","message":"Invalidations file probably doesn\u0027t exists when hashes are None so this loop is cheap.\n\nI don\u0027t care about invalidated suffixes when hashes are None because all suffixes are calculated now. You can see line 1052 that causes path listing and I just need to clear invalidations file if exists.","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8cb4efa6cb6d478bb1f491c8e9acc512b0e9ea33","unresolved":false,"context_lines":[{"line_number":263,"context_line":""},{"line_number":264,"context_line":"        modified \u003d False"},{"line_number":265,"context_line":"        try:"},{"line_number":266,"context_line":"            with open(invalidations_file, \u0027rb\u0027) as inv_fh:"},{"line_number":267,"context_line":"                for line in inv_fh:"},{"line_number":268,"context_line":"                    found_invalid_entry \u003d True"},{"line_number":269,"context_line":"                    suffix \u003d line.strip()"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_762d4aa7","line":266,"in_reply_to":"1a6eadb0_ef398fd8","updated":"2016-12-15 15:10:18.000000000","message":"The loop *may* be cheap, (not sure about the file open, it\u0027s just once per partition) but enclosing with \"if hashes is not None\" is even cheaper and IMHO makes the code more intuitive.\n\nThen at line 281 change to:\n\nif hashes is None or found_invalid_entry","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"88c555700c2be6e2008af099a27154324207db67","unresolved":false,"context_lines":[{"line_number":275,"context_line":"            if e.errno !\u003d errno.ENOENT:"},{"line_number":276,"context_line":"                raise"},{"line_number":277,"context_line":""},{"line_number":278,"context_line":"        if hashes is None or modified:"},{"line_number":279,"context_line":"            write_pickle(hashes, hashes_file, partition_dir, PICKLE_PROTOCOL)"},{"line_number":280,"context_line":""},{"line_number":281,"context_line":"        if found_invalid_entry:"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_8eb4b8d1","line":278,"updated":"2016-12-14 18:08:36.000000000","message":"why write None to the pickle file?","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8cb4efa6cb6d478bb1f491c8e9acc512b0e9ea33","unresolved":false,"context_lines":[{"line_number":275,"context_line":"            if e.errno !\u003d errno.ENOENT:"},{"line_number":276,"context_line":"                raise"},{"line_number":277,"context_line":""},{"line_number":278,"context_line":"        if hashes is None or modified:"},{"line_number":279,"context_line":"            write_pickle(hashes, hashes_file, partition_dir, PICKLE_PROTOCOL)"},{"line_number":280,"context_line":""},{"line_number":281,"context_line":"        if found_invalid_entry:"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_3c7e5119","line":278,"in_reply_to":"1a6eadb0_04fb1248","updated":"2016-12-15 15:10:18.000000000","message":"ok, thanks.\n\nI missed before the significance of None being *persisted* here in hashes.pkl as a record that the hashes.pkl was previously non-existent or corrupt, so even if process fails after this point, the next process will know to do a listdir. That takes care of my concern about deleting invalidations that weren\u0027t written to the hashes.pkl.","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"5830153008d1b02b8ffaeb23e453fd527bc9ed5a","unresolved":false,"context_lines":[{"line_number":275,"context_line":"            if e.errno !\u003d errno.ENOENT:"},{"line_number":276,"context_line":"                raise"},{"line_number":277,"context_line":""},{"line_number":278,"context_line":"        if hashes is None or modified:"},{"line_number":279,"context_line":"            write_pickle(hashes, hashes_file, partition_dir, PICKLE_PROTOCOL)"},{"line_number":280,"context_line":""},{"line_number":281,"context_line":"        if found_invalid_entry:"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_04fb1248","line":278,"in_reply_to":"1a6eadb0_8eb4b8d1","updated":"2016-12-15 08:17:52.000000000","message":"To be sure that another process doesn\u0027t create pkl file too.\n\nWe create pkl file with pickeled None value and return pkl mtime. When calculated hashes are ready to be stored (see line 1077) I\u0027m sure that there is not better pkl created by other process.\n\nWhen pkl exists invalidate_hash stores changed suffixes - but suffixes are not invalidated by the previous code between path listing and new pkl set up.\n\nWe store also None value to notice that pkl is empty and another concurrent proces should do listdir.","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"88c555700c2be6e2008af099a27154324207db67","unresolved":false,"context_lines":[{"line_number":282,"context_line":"            # Now that all the invalidations are reflected in hashes.pkl, it\u0027s"},{"line_number":283,"context_line":"            # safe to clear out the invalidations file."},{"line_number":284,"context_line":"            try:"},{"line_number":285,"context_line":"                with open(invalidations_file, \u0027wb\u0027) as inv_fh:"},{"line_number":286,"context_line":"                    pass"},{"line_number":287,"context_line":"            except OSError as e:"},{"line_number":288,"context_line":"                if e.errno !\u003d errno.ENOENT:"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_eec52c3c","line":285,"updated":"2016-12-14 18:08:36.000000000","message":"but they may not be written to hashes.pkl!!\n\nIf hashes.pkl did not exist or was invalid, then the only place we now have the invalidations is in the returned dict. Fortunately this is later written to hashes.pkl in _get_hashes, but that does not feel at all safe, and not consistent with the docstring (i.e. this method writes hashes.pkl if it already existed but otherwise not).","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8cb4efa6cb6d478bb1f491c8e9acc512b0e9ea33","unresolved":false,"context_lines":[{"line_number":282,"context_line":"            # Now that all the invalidations are reflected in hashes.pkl, it\u0027s"},{"line_number":283,"context_line":"            # safe to clear out the invalidations file."},{"line_number":284,"context_line":"            try:"},{"line_number":285,"context_line":"                with open(invalidations_file, \u0027wb\u0027) as inv_fh:"},{"line_number":286,"context_line":"                    pass"},{"line_number":287,"context_line":"            except OSError as e:"},{"line_number":288,"context_line":"                if e.errno !\u003d errno.ENOENT:"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_1c01956a","line":285,"in_reply_to":"1a6eadb0_0fe6cb43","updated":"2016-12-15 15:10:18.000000000","message":"OK I get it now, sorry! hashes\u003d\u003dNone creates a hashes.pkl with None which is sufficient to trigger a future listdir *even if this process fails*.\n\nI still think the docstring could be clearer, see comment on next patchset.","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"5830153008d1b02b8ffaeb23e453fd527bc9ed5a","unresolved":false,"context_lines":[{"line_number":282,"context_line":"            # Now that all the invalidations are reflected in hashes.pkl, it\u0027s"},{"line_number":283,"context_line":"            # safe to clear out the invalidations file."},{"line_number":284,"context_line":"            try:"},{"line_number":285,"context_line":"                with open(invalidations_file, \u0027wb\u0027) as inv_fh:"},{"line_number":286,"context_line":"                    pass"},{"line_number":287,"context_line":"            except OSError as e:"},{"line_number":288,"context_line":"                if e.errno !\u003d errno.ENOENT:"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_0fe6cb43","line":285,"in_reply_to":"1a6eadb0_eec52c3c","updated":"2016-12-15 08:17:52.000000000","message":"When hashes.pkl did not exist or was invalid then hashes \u003d None.\n\nI don\u0027t understand what is not save and not according to docstring - where is written that pkl is not created when it doesn\u0027t exist?","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"88c555700c2be6e2008af099a27154324207db67","unresolved":false,"context_lines":[{"line_number":291,"context_line":"        try:"},{"line_number":292,"context_line":"            mtime \u003d getmtime(hashes_file)"},{"line_number":293,"context_line":"        except OSError as e:"},{"line_number":294,"context_line":"            if e.errno !\u003d errno.ENOENT:"},{"line_number":295,"context_line":"                raise"},{"line_number":296,"context_line":"            mtime \u003d -1"},{"line_number":297,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_86ff4666","line":294,"updated":"2016-12-14 18:08:36.000000000","message":"why ignore ENOENT? at this point the hashes.pkl either existed or should have been written under lock_path so when could ENOENT be OK?","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"5830153008d1b02b8ffaeb23e453fd527bc9ed5a","unresolved":false,"context_lines":[{"line_number":291,"context_line":"        try:"},{"line_number":292,"context_line":"            mtime \u003d getmtime(hashes_file)"},{"line_number":293,"context_line":"        except OSError as e:"},{"line_number":294,"context_line":"            if e.errno !\u003d errno.ENOENT:"},{"line_number":295,"context_line":"                raise"},{"line_number":296,"context_line":"            mtime \u003d -1"},{"line_number":297,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_2f51c7d8","line":294,"in_reply_to":"1a6eadb0_86ff4666","updated":"2016-12-15 08:17:52.000000000","message":"You are right.","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"88c555700c2be6e2008af099a27154324207db67","unresolved":false,"context_lines":[{"line_number":1026,"context_line":"        :param partition_path: absolute path of partition to get hashes for"},{"line_number":1027,"context_line":"        :param recalculate: list of suffixes which should be recalculated when"},{"line_number":1028,"context_line":"                            got"},{"line_number":1029,"context_line":"        :param do_listdir: force existence check for new hashes in the"},{"line_number":1030,"context_line":"                           partition"},{"line_number":1031,"context_line":"        :param reclaim_age: age at which to remove tombstones"},{"line_number":1032,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_10e2879f","line":1029,"updated":"2016-12-14 18:08:36.000000000","message":"not sure this change is necessary or correct - do_listdir causes suffix hashes for all suffix dirs to be set to None which in turn causes the existence of all suffixes to be checked, not just new ones.\n\nI do think the description could mention \"recalculate\" e.g. \n\n  force recalculation of hashes for all suffix dirs in the partition","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"5830153008d1b02b8ffaeb23e453fd527bc9ed5a","unresolved":false,"context_lines":[{"line_number":1026,"context_line":"        :param partition_path: absolute path of partition to get hashes for"},{"line_number":1027,"context_line":"        :param recalculate: list of suffixes which should be recalculated when"},{"line_number":1028,"context_line":"                            got"},{"line_number":1029,"context_line":"        :param do_listdir: force existence check for new hashes in the"},{"line_number":1030,"context_line":"                           partition"},{"line_number":1031,"context_line":"        :param reclaim_age: age at which to remove tombstones"},{"line_number":1032,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_afb797f3","line":1029,"in_reply_to":"1a6eadb0_10e2879f","updated":"2016-12-15 08:17:52.000000000","message":"Check once more what happens when do_listdir is True. \"hashes.setdefault(suff, None)\" creates keys for NEW suffixes found in partition_path. See setdefault docs to realize that existing dict keys are not changed!\n\nThere is no param to recalculate all suffixes. We need to set do_listdir to add new suffixes and set recalculate \u003d (list of existing suffixes) to recalculate them.\n\n\nP.S.: You can await another fix from me when we find that some hashes are not correct in our storage. (And they are incorrect in our dev environs.)","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8cb4efa6cb6d478bb1f491c8e9acc512b0e9ea33","unresolved":false,"context_lines":[{"line_number":1026,"context_line":"        :param partition_path: absolute path of partition to get hashes for"},{"line_number":1027,"context_line":"        :param recalculate: list of suffixes which should be recalculated when"},{"line_number":1028,"context_line":"                            got"},{"line_number":1029,"context_line":"        :param do_listdir: force existence check for new hashes in the"},{"line_number":1030,"context_line":"                           partition"},{"line_number":1031,"context_line":"        :param reclaim_age: age at which to remove tombstones"},{"line_number":1032,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_90a2e9ac","line":1029,"in_reply_to":"1a6eadb0_afb797f3","updated":"2016-12-15 15:10:18.000000000","message":"yes, you\u0027re right re setdefault","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"88c555700c2be6e2008af099a27154324207db67","unresolved":false,"context_lines":[{"line_number":1047,"context_line":"            hashes, mtime \u003d self.consolidate_hashes(partition_path)"},{"line_number":1048,"context_line":"        except Exception:"},{"line_number":1049,"context_line":"            do_listdir \u003d True"},{"line_number":1050,"context_line":"            force_rewrite \u003d True"},{"line_number":1051,"context_line":"        else:"},{"line_number":1052,"context_line":"            if hashes is None:  # no hashes.pkl file; let\u0027s build it"},{"line_number":1053,"context_line":"                do_listdir \u003d True"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_9aba366d","line":1050,"updated":"2016-12-14 18:08:36.000000000","message":"(see below) force_rewrite is not needed, if exception occurs the mtime remains at -1","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"5830153008d1b02b8ffaeb23e453fd527bc9ed5a","unresolved":false,"context_lines":[{"line_number":1047,"context_line":"            hashes, mtime \u003d self.consolidate_hashes(partition_path)"},{"line_number":1048,"context_line":"        except Exception:"},{"line_number":1049,"context_line":"            do_listdir \u003d True"},{"line_number":1050,"context_line":"            force_rewrite \u003d True"},{"line_number":1051,"context_line":"        else:"},{"line_number":1052,"context_line":"            if hashes is None:  # no hashes.pkl file; let\u0027s build it"},{"line_number":1053,"context_line":"                do_listdir \u003d True"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_af38773e","line":1050,"in_reply_to":"1a6eadb0_9aba366d","updated":"2016-12-15 08:17:52.000000000","message":"Done","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"88c555700c2be6e2008af099a27154324207db67","unresolved":false,"context_lines":[{"line_number":1073,"context_line":"                modified \u003d True"},{"line_number":1074,"context_line":"        if modified:"},{"line_number":1075,"context_line":"            with lock_path(partition_path):"},{"line_number":1076,"context_line":"                if force_rewrite or not exists(hashes_file) or \\"},{"line_number":1077,"context_line":"                        getmtime(hashes_file) \u003d\u003d mtime:"},{"line_number":1078,"context_line":"                    write_pickle("},{"line_number":1079,"context_line":"                        hashes, hashes_file, partition_path, PICKLE_PROTOCOL)"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_5af25e70","line":1076,"updated":"2016-12-14 18:08:36.000000000","message":"I think force_rewrite is not necessary, the condition \"mtime \u003d\u003d -1\" is the equivalent.\n\nThe logic here is: if this thread recalculated any suffix then write hashes.pkl if either this thread failed to read it (mtime \u003d\u003d -1 not exists(hashes_file)) or if it has not been written since it was last read/written by this thread","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"5830153008d1b02b8ffaeb23e453fd527bc9ed5a","unresolved":false,"context_lines":[{"line_number":1073,"context_line":"                modified \u003d True"},{"line_number":1074,"context_line":"        if modified:"},{"line_number":1075,"context_line":"            with lock_path(partition_path):"},{"line_number":1076,"context_line":"                if force_rewrite or not exists(hashes_file) or \\"},{"line_number":1077,"context_line":"                        getmtime(hashes_file) \u003d\u003d mtime:"},{"line_number":1078,"context_line":"                    write_pickle("},{"line_number":1079,"context_line":"                        hashes, hashes_file, partition_path, PICKLE_PROTOCOL)"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_0f488bcb","line":1076,"in_reply_to":"1a6eadb0_5af25e70","updated":"2016-12-15 08:17:52.000000000","message":"Done\n\nI left force_rewrite there for not existing pkl file but I see that it is not neccessary.","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"88c555700c2be6e2008af099a27154324207db67","unresolved":false,"context_lines":[{"line_number":1074,"context_line":"        if modified:"},{"line_number":1075,"context_line":"            with lock_path(partition_path):"},{"line_number":1076,"context_line":"                if force_rewrite or not exists(hashes_file) or \\"},{"line_number":1077,"context_line":"                        getmtime(hashes_file) \u003d\u003d mtime:"},{"line_number":1078,"context_line":"                    write_pickle("},{"line_number":1079,"context_line":"                        hashes, hashes_file, partition_path, PICKLE_PROTOCOL)"},{"line_number":1080,"context_line":"                    return hashed, hashes"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_e06462e6","line":1077,"updated":"2016-12-14 18:08:36.000000000","message":"ok, mtime is now the time at which THIS thread read or possibly wrote the hashes.pkl so this condition is true UNLESS someone else wrote the file.\n\nBUT it is also true if this thread wrote the pickle in consolidate_hashes and there must be some conditions in which we now write the pickle twice during get_hashes? Can that be avoided? IDK","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"5830153008d1b02b8ffaeb23e453fd527bc9ed5a","unresolved":false,"context_lines":[{"line_number":1074,"context_line":"        if modified:"},{"line_number":1075,"context_line":"            with lock_path(partition_path):"},{"line_number":1076,"context_line":"                if force_rewrite or not exists(hashes_file) or \\"},{"line_number":1077,"context_line":"                        getmtime(hashes_file) \u003d\u003d mtime:"},{"line_number":1078,"context_line":"                    write_pickle("},{"line_number":1079,"context_line":"                        hashes, hashes_file, partition_path, PICKLE_PROTOCOL)"},{"line_number":1080,"context_line":"                    return hashed, hashes"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_ea4a5d25","line":1077,"in_reply_to":"1a6eadb0_e06462e6","updated":"2016-12-15 08:17:52.000000000","message":"I thought about it. Empty pkl file is created to enable invalidated suffixes store by invalidate_hash function.","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"88c555700c2be6e2008af099a27154324207db67","unresolved":false,"context_lines":[{"line_number":1078,"context_line":"                    write_pickle("},{"line_number":1079,"context_line":"                        hashes, hashes_file, partition_path, PICKLE_PROTOCOL)"},{"line_number":1080,"context_line":"                    return hashed, hashes"},{"line_number":1081,"context_line":"            return self._get_hashes(partition_path, recalculate, do_listdir,"},{"line_number":1082,"context_line":"                                    reclaim_age)"},{"line_number":1083,"context_line":"        else:"},{"line_number":1084,"context_line":"            return hashed, hashes"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a6eadb0_bd21ac4f","line":1081,"updated":"2016-12-14 18:08:36.000000000","message":"re-enter IFF this thread recalculated suffixes but some other thread has modified hashes.pkl since this thread last did.","commit_id":"034169db822e317ba6d3bf9479664aa77170c733"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d23d7020631bfc9aed4f898605641d267fc84b6f","unresolved":false,"context_lines":[{"line_number":234,"context_line":"def consolidate_hashes(partition_dir):"},{"line_number":235,"context_line":"    \"\"\""},{"line_number":236,"context_line":"    Take what\u0027s in hashes.pkl and hashes.invalid, combine them, write the"},{"line_number":237,"context_line":"    result back to hashes.pkl, and clear out hashes.invalid."},{"line_number":238,"context_line":""},{"line_number":239,"context_line":"    :param suffix_dir: absolute path to partition dir containing hashes.pkl"},{"line_number":240,"context_line":"                       and hashes.invalid"}],"source_content_type":"text/x-python","patch_set":13,"id":"1a6eadb0_7018ed75","line":237,"updated":"2016-12-15 15:09:43.000000000","message":"The problem I had with code not matching the docstring is the implication in docstring that he content of hashes.invalid would be merged with an empty hashes.pkl and written back to hashes.pkl. That does not happen, which is fine, but I think it would be helpful to add a sentence to the docstring:\n\n\"If a valid hashes dict cannot be read from hashes.pkl then a None value is written to hashes.pkl to indicate that all hashes should be recalculated, and hashes.invalid is cleared.\"","commit_id":"5c291c421ef5a51f38dc75e12f11b958a6c978fc"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"664abc6394a62339c63adfd1d60ccbd973183030","unresolved":false,"context_lines":[{"line_number":234,"context_line":"def consolidate_hashes(partition_dir):"},{"line_number":235,"context_line":"    \"\"\""},{"line_number":236,"context_line":"    Take what\u0027s in hashes.pkl and hashes.invalid, combine them, write the"},{"line_number":237,"context_line":"    result back to hashes.pkl, and clear out hashes.invalid."},{"line_number":238,"context_line":""},{"line_number":239,"context_line":"    :param suffix_dir: absolute path to partition dir containing hashes.pkl"},{"line_number":240,"context_line":"                       and hashes.invalid"}],"source_content_type":"text/x-python","patch_set":13,"id":"1a6eadb0_3fdb9950","line":237,"in_reply_to":"1a6eadb0_7018ed75","updated":"2016-12-15 17:01:10.000000000","message":"Done, thanks for wording","commit_id":"5c291c421ef5a51f38dc75e12f11b958a6c978fc"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d23d7020631bfc9aed4f898605641d267fc84b6f","unresolved":false,"context_lines":[{"line_number":239,"context_line":"    :param suffix_dir: absolute path to partition dir containing hashes.pkl"},{"line_number":240,"context_line":"                       and hashes.invalid"},{"line_number":241,"context_line":""},{"line_number":242,"context_line":"    :returns: tuple of (the hashes or None if there\u0027s no hashes.pkl,"},{"line_number":243,"context_line":"                        hashes.pkl last modification time)"},{"line_number":244,"context_line":"    \"\"\""},{"line_number":245,"context_line":"    hashes_file \u003d join(partition_dir, HASH_FILE)"}],"source_content_type":"text/x-python","patch_set":13,"id":"1a6eadb0_909ca9c1","line":242,"updated":"2016-12-15 15:09:43.000000000","message":"please change \"there\u0027s no\" to \"there was no valid\"\n\nreason: there\u0027s is typically read as present tense \"there is\", so the implication is that on return there may be no hashes.pkl, but in fact there should always be a hashes.pkl on return. \n\n(This wording played a small part in my previous misunderstanding.)","commit_id":"5c291c421ef5a51f38dc75e12f11b958a6c978fc"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"664abc6394a62339c63adfd1d60ccbd973183030","unresolved":false,"context_lines":[{"line_number":239,"context_line":"    :param suffix_dir: absolute path to partition dir containing hashes.pkl"},{"line_number":240,"context_line":"                       and hashes.invalid"},{"line_number":241,"context_line":""},{"line_number":242,"context_line":"    :returns: tuple of (the hashes or None if there\u0027s no hashes.pkl,"},{"line_number":243,"context_line":"                        hashes.pkl last modification time)"},{"line_number":244,"context_line":"    \"\"\""},{"line_number":245,"context_line":"    hashes_file \u003d join(partition_dir, HASH_FILE)"}],"source_content_type":"text/x-python","patch_set":13,"id":"1a6eadb0_19335dac","line":242,"in_reply_to":"1a6eadb0_909ca9c1","updated":"2016-12-15 17:01:10.000000000","message":"Done","commit_id":"5c291c421ef5a51f38dc75e12f11b958a6c978fc"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d23d7020631bfc9aed4f898605641d267fc84b6f","unresolved":false,"context_lines":[{"line_number":259,"context_line":"                # pickle.loads() can raise a wide variety of exceptions when"},{"line_number":260,"context_line":"                # given invalid input depending on the way in which the"},{"line_number":261,"context_line":"                # input is invalid."},{"line_number":262,"context_line":"                hashes \u003d None"},{"line_number":263,"context_line":""},{"line_number":264,"context_line":"        modified \u003d False"},{"line_number":265,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":13,"id":"1a6eadb0_bc00a102","line":262,"updated":"2016-12-15 15:09:43.000000000","message":"ok. so hashes\u003d\u003dNone --\u003e failed to read valid hashes (either no file, unable to read file or bad pickled data) OR read None from hashes.pkl\n\nand then later hashes\u003d\u003dNone --\u003e write a hashes.pkl\n\nThis means we may rewrite None to existing hashes.pkl that already has None even when there has been no modification. However, that anomaly is probably OK because we don\u0027t ever expect to find hashes.pkl with None in normal operation, only is a process fails later in _get_hashes. Normally, if a hashes.pkl is written with None then hashes\u003d\u003dNone would then trigger a do_listdir and a hashes dict to be pickled in _get_hashes.","commit_id":"5c291c421ef5a51f38dc75e12f11b958a6c978fc"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"664abc6394a62339c63adfd1d60ccbd973183030","unresolved":false,"context_lines":[{"line_number":259,"context_line":"                # pickle.loads() can raise a wide variety of exceptions when"},{"line_number":260,"context_line":"                # given invalid input depending on the way in which the"},{"line_number":261,"context_line":"                # input is invalid."},{"line_number":262,"context_line":"                hashes \u003d None"},{"line_number":263,"context_line":""},{"line_number":264,"context_line":"        modified \u003d False"},{"line_number":265,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":13,"id":"1a6eadb0_8228d6e3","line":262,"in_reply_to":"1a6eadb0_bc00a102","updated":"2016-12-15 17:01:10.000000000","message":"But I can see infinite loop when two processes called _get_hashes at the same time. First one stores None, second one stores None and updates mtime. First process can see changed mtime and repeats hash consolidation that updates mtime. Second process can see changed pkl and loops.\n\nIt is good idea not to write pkl when it is not changed. Thanks.","commit_id":"5c291c421ef5a51f38dc75e12f11b958a6c978fc"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d23d7020631bfc9aed4f898605641d267fc84b6f","unresolved":false,"context_lines":[{"line_number":262,"context_line":"                hashes \u003d None"},{"line_number":263,"context_line":""},{"line_number":264,"context_line":"        modified \u003d False"},{"line_number":265,"context_line":"        try:"},{"line_number":266,"context_line":"            with open(invalidations_file, \u0027rb\u0027) as inv_fh:"},{"line_number":267,"context_line":"                for line in inv_fh:"},{"line_number":268,"context_line":"                    found_invalid_entry \u003d True"}],"source_content_type":"text/x-python","patch_set":13,"id":"1a6eadb0_35cd7f85","line":265,"updated":"2016-12-15 15:09:43.000000000","message":"The loop *may* be cheap, (not sure about the file open, it\u0027s just once per partition) but enclosing with \"if hashes is not None\" is even cheaper and IMHO makes the code more intuitive.\n\nThen at line 281 change to:\n\nif hashes is None or found_invalid_entry","commit_id":"5c291c421ef5a51f38dc75e12f11b958a6c978fc"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"664abc6394a62339c63adfd1d60ccbd973183030","unresolved":false,"context_lines":[{"line_number":262,"context_line":"                hashes \u003d None"},{"line_number":263,"context_line":""},{"line_number":264,"context_line":"        modified \u003d False"},{"line_number":265,"context_line":"        try:"},{"line_number":266,"context_line":"            with open(invalidations_file, \u0027rb\u0027) as inv_fh:"},{"line_number":267,"context_line":"                for line in inv_fh:"},{"line_number":268,"context_line":"                    found_invalid_entry \u003d True"}],"source_content_type":"text/x-python","patch_set":13,"id":"1a6eadb0_82229650","line":265,"in_reply_to":"1a6eadb0_35cd7f85","updated":"2016-12-15 17:01:10.000000000","message":"Done","commit_id":"5c291c421ef5a51f38dc75e12f11b958a6c978fc"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d23d7020631bfc9aed4f898605641d267fc84b6f","unresolved":false,"context_lines":[{"line_number":1066,"context_line":"                modified \u003d True"},{"line_number":1067,"context_line":"        if modified:"},{"line_number":1068,"context_line":"            with lock_path(partition_path):"},{"line_number":1069,"context_line":"                if not exists(hashes_file) or getmtime(hashes_file) \u003d\u003d mtime:"},{"line_number":1070,"context_line":"                    write_pickle("},{"line_number":1071,"context_line":"                        hashes, hashes_file, partition_path, PICKLE_PROTOCOL)"},{"line_number":1072,"context_line":"                    return hashed, hashes"}],"source_content_type":"text/x-python","patch_set":13,"id":"1a6eadb0_15740366","line":1069,"updated":"2016-12-15 15:09:43.000000000","message":"this can result in infinite recursion if consolidate_hashes raises an Exception while there is a hashes.pkl.\n\nMy comment on patchset 11 was intended to result in:\n\n  if mtime \u003d\u003d -1 or not exists(hashes_file) or getmtime(hashes_file) \u003d\u003d mtime\n\nthere is a patch and test for this here if you\u0027d like to include it:\n\n[1] https://gist.github.com/alistairncoles/0f63b9bbf2c589a12b3110b6db66df24","commit_id":"5c291c421ef5a51f38dc75e12f11b958a6c978fc"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"664abc6394a62339c63adfd1d60ccbd973183030","unresolved":false,"context_lines":[{"line_number":1066,"context_line":"                modified \u003d True"},{"line_number":1067,"context_line":"        if modified:"},{"line_number":1068,"context_line":"            with lock_path(partition_path):"},{"line_number":1069,"context_line":"                if not exists(hashes_file) or getmtime(hashes_file) \u003d\u003d mtime:"},{"line_number":1070,"context_line":"                    write_pickle("},{"line_number":1071,"context_line":"                        hashes, hashes_file, partition_path, PICKLE_PROTOCOL)"},{"line_number":1072,"context_line":"                    return hashed, hashes"}],"source_content_type":"text/x-python","patch_set":13,"id":"1a6eadb0_5c2303a7","line":1069,"in_reply_to":"1a6eadb0_15740366","updated":"2016-12-15 17:01:10.000000000","message":"Got it.\nTraceback will appear in log when an exception is returned from consolidate_hashes because that error was not visible.","commit_id":"5c291c421ef5a51f38dc75e12f11b958a6c978fc"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b586889e2a1f16be7657633655925bf71d589d80","unresolved":false,"context_lines":[{"line_number":280,"context_line":"                if e.errno !\u003d errno.ENOENT:"},{"line_number":281,"context_line":"                    raise"},{"line_number":282,"context_line":""},{"line_number":283,"context_line":"        if write_pkl:"},{"line_number":284,"context_line":"            write_pickle(hashes, hashes_file, partition_dir, PICKLE_PROTOCOL)"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"        if found_invalid_entry or \\"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a6eadb0_4d9b9472","line":283,"updated":"2016-12-20 18:00:58.000000000","message":"this is better, thanks!","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b586889e2a1f16be7657633655925bf71d589d80","unresolved":false,"context_lines":[{"line_number":284,"context_line":"            write_pickle(hashes, hashes_file, partition_dir, PICKLE_PROTOCOL)"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"        if found_invalid_entry or \\"},{"line_number":287,"context_line":"                hashes is None and os.path.exists(invalidations_file):"},{"line_number":288,"context_line":""},{"line_number":289,"context_line":"            # Now that all the invalidations are reflected in hashes.pkl, it\u0027s"},{"line_number":290,"context_line":"            # safe to clear out the invalidations file."}],"source_content_type":"text/x-python","patch_set":14,"id":"1a6eadb0_1ee628b4","line":287,"updated":"2016-12-20 18:00:58.000000000","message":"nit: alternative, not a big deal: https://gist.github.com/57dfaf9f6e5188d8dfa0828d3b7ee259","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"17fbde937570f2dc34f8aea5a3717cf0ad602117","unresolved":false,"context_lines":[{"line_number":284,"context_line":"            write_pickle(hashes, hashes_file, partition_dir, PICKLE_PROTOCOL)"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"        if found_invalid_entry or \\"},{"line_number":287,"context_line":"                hashes is None and os.path.exists(invalidations_file):"},{"line_number":288,"context_line":""},{"line_number":289,"context_line":"            # Now that all the invalidations are reflected in hashes.pkl, it\u0027s"},{"line_number":290,"context_line":"            # safe to clear out the invalidations file."}],"source_content_type":"text/x-python","patch_set":14,"id":"da4df55a_256ed8c9","line":287,"in_reply_to":"1a6eadb0_1ee628b4","updated":"2016-12-22 12:56:37.000000000","message":"Done","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d2bc566b5bedb3078c27524eafe86c48c2c4f2f5","unresolved":false,"context_lines":[{"line_number":254,"context_line":"            except OSError as e:"},{"line_number":255,"context_line":"                if e.errno !\u003d errno.ENOENT:"},{"line_number":256,"context_line":"                    raise"},{"line_number":257,"context_line":"        return None"},{"line_number":258,"context_line":""},{"line_number":259,"context_line":"    with lock_path(partition_dir):"},{"line_number":260,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_43498b5f","side":"PARENT","line":257,"updated":"2017-01-12 00:53:35.000000000","message":"BOOM!  this is it - I get it now - *this* is the fix for the race!\n\nI *told* you it\u0027d be a nice state the invariant explicitly:\n\n    it\u0027s impossible to return from this function without the hashes_file existing\n\nif before this function returns a concurrent process determines the hashes_file does not exist - you can be sure this process will return a empty hash file that will force a listdir\n\nif before this function returns a concurrent process determines the hashes_file does exist - you can be sure it will wait for this code to release the directory lock before appending to the invalidate file\n\nIf the hashes.pkl was allowed to not exist - even tho we set force_rewrite and all that - there was this big window *while hashing* that an invalidate_hash call could fall into the \u0027if not hash exists\u0027 block - but it can\u0027t do that now - it\u0027ll just append to the invalids file - which is 100% correct\n\nwhoot!","commit_id":"09fa9dddd72f4aeebd2576c517f3b4d7988a7fa1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"126089e73d8944d6a35dc60907a61bf5210b5b72","unresolved":false,"context_lines":[{"line_number":254,"context_line":"            except OSError as e:"},{"line_number":255,"context_line":"                if e.errno !\u003d errno.ENOENT:"},{"line_number":256,"context_line":"                    raise"},{"line_number":257,"context_line":"        return None"},{"line_number":258,"context_line":""},{"line_number":259,"context_line":"    with lock_path(partition_dir):"},{"line_number":260,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":16,"id":"7a3c09a3_2487bb10","side":"PARENT","line":257,"in_reply_to":"7a3c09a3_781a4e19","updated":"2017-01-13 04:28:11.000000000","message":"starting without a hashes.pkl?  Yeah, since it\u0027s created under the directory lock below only one will create it - the other discovers it\u0027s there and takes an mtime - they both rehash - whoever finishes first gets to write/update (because getmtime matches) - the other has to recalc.  I believe this is correct.  I think master is broken because of force_rewrite - I said I love dropping that state!\n\nI just meant *this* (removing this return path w/o a hashes.pkl on disk) is why it\u0027s ok for invalidate_hash to not serialize an invalidate message to disk if there is no hashes.pkl - the invariant that this function returns with a hashes.pkl on-disk is why we can be sure invalidate_hashes will write down invalidation messages while get_hashes is doing the lock-free rehash.  It\u0027s pretty convoluted the way it works out - IMHO it\u0027d be better for invalidate_hash to just always write down the invalidation messages.  But for me at least - understanding that this function must return with a hashes.pkl on disk even (even if the whole directory was empty to start, and the hashes.pkl is an empty shell) was key to understanding why invalidate_hashes won\u0027t loose a message while get_hashes is rehashing.","commit_id":"09fa9dddd72f4aeebd2576c517f3b4d7988a7fa1"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"66be93b28ebd3f13db11ad14c6258025199faf53","unresolved":false,"context_lines":[{"line_number":254,"context_line":"            except OSError as e:"},{"line_number":255,"context_line":"                if e.errno !\u003d errno.ENOENT:"},{"line_number":256,"context_line":"                    raise"},{"line_number":257,"context_line":"        return None"},{"line_number":258,"context_line":""},{"line_number":259,"context_line":"    with lock_path(partition_dir):"},{"line_number":260,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":16,"id":"7a3c09a3_781a4e19","side":"PARENT","line":257,"in_reply_to":"ba5201f7_43498b5f","updated":"2017-01-13 01:56:24.000000000","message":"Not sure that it\u0027s correct.\n\nImagine two calls of REPLICATE of the same partition.","commit_id":"09fa9dddd72f4aeebd2576c517f3b4d7988a7fa1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"8a285c82869e2cf3f9db4cf3e0f2c1bc939e6d75","unresolved":false,"context_lines":[{"line_number":1037,"context_line":"        hashed \u003d 0"},{"line_number":1038,"context_line":"        hashes_file \u003d join(partition_path, HASH_FILE)"},{"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":""}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_e01fe705","side":"PARENT","line":1040,"updated":"2017-01-11 06:07:25.000000000","message":"Love getting rid of this extra state!","commit_id":"09fa9dddd72f4aeebd2576c517f3b4d7988a7fa1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"126089e73d8944d6a35dc60907a61bf5210b5b72","unresolved":false,"context_lines":[{"line_number":1037,"context_line":"        hashed \u003d 0"},{"line_number":1038,"context_line":"        hashes_file \u003d join(partition_path, HASH_FILE)"},{"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":""}],"source_content_type":"text/x-python","patch_set":16,"id":"7a3c09a3_7f6230da","side":"PARENT","line":1040,"in_reply_to":"ba5201f7_e01fe705","updated":"2017-01-13 04:28:11.000000000","message":"I said I love it!  I mean it!  This is the root of so much evil - I can\u0027t believe I forgot about it!","commit_id":"09fa9dddd72f4aeebd2576c517f3b4d7988a7fa1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"8a285c82869e2cf3f9db4cf3e0f2c1bc939e6d75","unresolved":false,"context_lines":[{"line_number":1048,"context_line":"            mtime \u003d getmtime(hashes_file)"},{"line_number":1049,"context_line":"        except OSError as e:"},{"line_number":1050,"context_line":"            if e.errno !\u003d errno.ENOENT:"},{"line_number":1051,"context_line":"                raise"},{"line_number":1052,"context_line":""},{"line_number":1053,"context_line":"        try:"},{"line_number":1054,"context_line":"            hashes \u003d self.consolidate_hashes(partition_path)"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_40d6a128","side":"PARENT","line":1051,"updated":"2017-01-11 06:07:25.000000000","message":"This refactoring is taking on a lot of great stuff - but for the immediate issue why not just move this mtime block under consolidate_hashes since that seems to be the main issue?","commit_id":"09fa9dddd72f4aeebd2576c517f3b4d7988a7fa1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"126089e73d8944d6a35dc60907a61bf5210b5b72","unresolved":false,"context_lines":[{"line_number":1048,"context_line":"            mtime \u003d getmtime(hashes_file)"},{"line_number":1049,"context_line":"        except OSError as e:"},{"line_number":1050,"context_line":"            if e.errno !\u003d errno.ENOENT:"},{"line_number":1051,"context_line":"                raise"},{"line_number":1052,"context_line":""},{"line_number":1053,"context_line":"        try:"},{"line_number":1054,"context_line":"            hashes \u003d self.consolidate_hashes(partition_path)"}],"source_content_type":"text/x-python","patch_set":16,"id":"7a3c09a3_440cc742","side":"PARENT","line":1051,"in_reply_to":"7a3c09a3_2b7cee1f","updated":"2017-01-13 04:28:11.000000000","message":"Yes, this became clear as soon as I pulled out the fix for just the double-rehash into a separate change - this comment was old/out-dated/naive.\n\nConsidering the common on-disk state of a hashes.pkl with some suffixes in the invalidates file...\n\n\"Only\" moving the mtime call on the other-side of the consolidate_hashes function leaves a very small window where you could return one set of hashes from consolidate, and getmtime in get_hashes off a different set of consolidated invalidated on-disk hashes written by another process.\n\nThis is *super* obvious when you pull this change out independently - but I could not see it until I did it in https://review.openstack.org/#/c/418691/\n\nI agree the cleanest way to fix avoid that race was to move the getmtime under the directory lock after the invalidations are written back (you *do* want to getmtime after the writeback - you just to be sure it was *your* writeback) - but the refactor (moving mtime into conslidate_hashes) bumped into too many other bugs - so I realized that for the sanity of other reviewers I needed to step back and look for a solution that was more obvious - which lead me to the re-read solution.\n\nObviously the issue is even more nuanced when you consider different starting disk states that create interactions with other bugs.\n\nAnd I don\u0027t consider it a permanent solution anyway - just an obvious fix for the current specific double-rehash issue - with a change that we can easily build on - refactors are great - but not under the gun of fixing a critical performance regression.\n\nI think if we can fix some of the other orthogonal issues - well have some breathing room for the refactor - maybe we can optimize out the re-read and fix the race in the same change.  But I wouldn\u0027t be opposed starting with the more obvious re-read trick (essentially conslidate_hashes under directory lock and then from that point on act like the pre invalidations file code) - land that - and then go back purely as a refactor to optimize out the re-read by moving the getmtime under the directory lock in conslidate_hashes!","commit_id":"09fa9dddd72f4aeebd2576c517f3b4d7988a7fa1"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"66be93b28ebd3f13db11ad14c6258025199faf53","unresolved":false,"context_lines":[{"line_number":1048,"context_line":"            mtime \u003d getmtime(hashes_file)"},{"line_number":1049,"context_line":"        except OSError as e:"},{"line_number":1050,"context_line":"            if e.errno !\u003d errno.ENOENT:"},{"line_number":1051,"context_line":"                raise"},{"line_number":1052,"context_line":""},{"line_number":1053,"context_line":"        try:"},{"line_number":1054,"context_line":"            hashes \u003d self.consolidate_hashes(partition_path)"}],"source_content_type":"text/x-python","patch_set":16,"id":"7a3c09a3_2b7cee1f","side":"PARENT","line":1051,"in_reply_to":"ba5201f7_40d6a128","updated":"2017-01-13 01:56:24.000000000","message":"Because the path is not locked when we return from hashes consolidation function. Another process can change pkl file before I read mtime and that another process can add an invalidated suffix that this process will never find and newer pkl can be rewrited by old hashes.\n\nfirst                           | second\n--------------------------------|-----------------------------------------------------------\nget hashes (creates pkl)        |\n                                | get hashes (clears not-empty invalidations file)\n                                | ...\n                                | write pkl\nget mtime                       |\nrecalculate invalidated suffix  |\nwrite pkl                       |","commit_id":"09fa9dddd72f4aeebd2576c517f3b4d7988a7fa1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"8a285c82869e2cf3f9db4cf3e0f2c1bc939e6d75","unresolved":false,"context_lines":[{"line_number":1084,"context_line":"                if force_rewrite or not exists(hashes_file) or \\"},{"line_number":1085,"context_line":"                        getmtime(hashes_file) \u003d\u003d mtime:"},{"line_number":1086,"context_line":"                    write_pickle("},{"line_number":1087,"context_line":"                        hashes, hashes_file, partition_path, PICKLE_PROTOCOL)"},{"line_number":1088,"context_line":"                    return hashed, hashes"},{"line_number":1089,"context_line":"            return self._get_hashes(partition_path, recalculate, do_listdir,"},{"line_number":1090,"context_line":"                                    reclaim_age)"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_ed96de3c","side":"PARENT","line":1087,"updated":"2017-01-11 06:07:25.000000000","message":"so this method does an nice atomic write/rename under lock; but I guess for the invalidations file we just append/truncate - but we still need to lock?","commit_id":"09fa9dddd72f4aeebd2576c517f3b4d7988a7fa1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"8a285c82869e2cf3f9db4cf3e0f2c1bc939e6d75","unresolved":false,"context_lines":[{"line_number":236,"context_line":"    Take what\u0027s in hashes.pkl and hashes.invalid, combine them, write the"},{"line_number":237,"context_line":"    result back to hashes.pkl, and clear out hashes.invalid."},{"line_number":238,"context_line":"    If a valid hashes dict cannot be read from hashes.pkl then a None value is"},{"line_number":239,"context_line":"    written to hashes.pkl to indicate that all hashes should be recalculated,"},{"line_number":240,"context_line":"    and hashes.invalid is cleared."},{"line_number":241,"context_line":""},{"line_number":242,"context_line":"    :param suffix_dir: absolute path to partition dir containing hashes.pkl"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_c24c80ca","line":239,"updated":"2017-01-11 06:07:25.000000000","message":"how many different ways to we have to indicate that all hashes need to be recalculated?  Isn\u0027t there something in the exception handling that calls this fella that can also require a full rehash?  Maybe we could leverage that by raising an ERROR if the ondisk file is garbage instead of translating that to None and then *also* catching unexpected errors from the caller and translating *that* to None (empty dict?) as well?","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"8a285c82869e2cf3f9db4cf3e0f2c1bc939e6d75","unresolved":false,"context_lines":[{"line_number":250,"context_line":"    clear_invalidations_file \u003d False"},{"line_number":251,"context_line":"    write_pkl \u003d False"},{"line_number":252,"context_line":""},{"line_number":253,"context_line":"    with lock_path(partition_dir):"},{"line_number":254,"context_line":"        try:"},{"line_number":255,"context_line":"            with open(hashes_file, \u0027rb\u0027) as hashes_fp:"},{"line_number":256,"context_line":"                pickled_hashes \u003d hashes_fp.read()"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_6032c5a8","line":253,"updated":"2017-01-11 06:07:25.000000000","message":"so this whole read/ consolidate / truncate / write-back pattern is under the lock.","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"8a285c82869e2cf3f9db4cf3e0f2c1bc939e6d75","unresolved":false,"context_lines":[{"line_number":268,"context_line":"                write_pkl \u003d True"},{"line_number":269,"context_line":""},{"line_number":270,"context_line":"        if hashes is None:"},{"line_number":271,"context_line":"            clear_invalidations_file \u003d os.path.exists(invalidations_file)"},{"line_number":272,"context_line":"        else:"},{"line_number":273,"context_line":"            try:"},{"line_number":274,"context_line":"                with open(invalidations_file, \u0027rb\u0027) as inv_fh:"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_a5b9ea30","line":271,"updated":"2017-01-11 06:07:25.000000000","message":"I don\u0027t see the point of doing the stat here, seems racy.  Maybe not racy if updates to the file use the lock_path - then it\u0027s probably just useless.","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"8a285c82869e2cf3f9db4cf3e0f2c1bc939e6d75","unresolved":false,"context_lines":[{"line_number":273,"context_line":"            try:"},{"line_number":274,"context_line":"                with open(invalidations_file, \u0027rb\u0027) as inv_fh:"},{"line_number":275,"context_line":"                    for line in inv_fh:"},{"line_number":276,"context_line":"                        clear_invalidations_file \u003d True"},{"line_number":277,"context_line":"                        suffix \u003d line.strip()"},{"line_number":278,"context_line":"                        if hashes.get(suffix, \u0027\u0027) is not None:"},{"line_number":279,"context_line":"                            hashes[suffix] \u003d None"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_0262083e","line":276,"updated":"2017-01-11 06:07:25.000000000","message":"I think this would look better in an else block of the exception handler - indicating we\u0027ve successfully processed the whole file\n\nif we somehow IOError half-way through isn\u0027t it more correct to *not* truncate?","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b21c64735bf94ae4b1c02ae4840c76ee3cbb35c1","unresolved":false,"context_lines":[{"line_number":273,"context_line":"            try:"},{"line_number":274,"context_line":"                with open(invalidations_file, \u0027rb\u0027) as inv_fh:"},{"line_number":275,"context_line":"                    for line in inv_fh:"},{"line_number":276,"context_line":"                        clear_invalidations_file \u003d True"},{"line_number":277,"context_line":"                        suffix \u003d line.strip()"},{"line_number":278,"context_line":"                        if hashes.get(suffix, \u0027\u0027) is not None:"},{"line_number":279,"context_line":"                            hashes[suffix] \u003d None"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_7983cdcc","line":276,"in_reply_to":"ba5201f7_0262083e","updated":"2017-01-11 10:50:02.000000000","message":"anything other than ENOENT gets re-raised so no truncation.\n\nI think the intent of setting the flag here is to avoid attempting truncation if no lines were read (i.e the file exists but is already empty)","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d2bc566b5bedb3078c27524eafe86c48c2c4f2f5","unresolved":false,"context_lines":[{"line_number":273,"context_line":"            try:"},{"line_number":274,"context_line":"                with open(invalidations_file, \u0027rb\u0027) as inv_fh:"},{"line_number":275,"context_line":"                    for line in inv_fh:"},{"line_number":276,"context_line":"                        clear_invalidations_file \u003d True"},{"line_number":277,"context_line":"                        suffix \u003d line.strip()"},{"line_number":278,"context_line":"                        if hashes.get(suffix, \u0027\u0027) is not None:"},{"line_number":279,"context_line":"                            hashes[suffix] \u003d None"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_18dc5b72","line":276,"in_reply_to":"ba5201f7_7983cdcc","updated":"2017-01-12 00:53:35.000000000","message":"I agree, I think i must have figured that out at some point:\n\nhttps://review.openstack.org/#/c/418692/","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"8a285c82869e2cf3f9db4cf3e0f2c1bc939e6d75","unresolved":false,"context_lines":[{"line_number":289,"context_line":"        # safe to clear out the invalidations file."},{"line_number":290,"context_line":"        if clear_invalidations_file:"},{"line_number":291,"context_line":"            with open(invalidations_file, \u0027wb\u0027) as inv_fh:"},{"line_number":292,"context_line":"                pass"},{"line_number":293,"context_line":""},{"line_number":294,"context_line":"        mtime \u003d getmtime(hashes_file)"},{"line_number":295,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_c5b6ee3c","line":292,"updated":"2017-01-11 06:07:25.000000000","message":"this is idempotent regardless of the existence of the file, and seems always the right thing to do when hashes is None (so useless to stat the file?)\n\nthis is all under the above lock","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d2bc566b5bedb3078c27524eafe86c48c2c4f2f5","unresolved":false,"context_lines":[{"line_number":289,"context_line":"        # safe to clear out the invalidations file."},{"line_number":290,"context_line":"        if clear_invalidations_file:"},{"line_number":291,"context_line":"            with open(invalidations_file, \u0027wb\u0027) as inv_fh:"},{"line_number":292,"context_line":"                pass"},{"line_number":293,"context_line":""},{"line_number":294,"context_line":"        mtime \u003d getmtime(hashes_file)"},{"line_number":295,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_f897978d","line":292,"in_reply_to":"ba5201f7_59ca891b","updated":"2017-01-12 00:53:35.000000000","message":"I think you can\u0027t get ENOENT when you open with \u0027w\u0027 unless the directory doesn\u0027t exist?","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b21c64735bf94ae4b1c02ae4840c76ee3cbb35c1","unresolved":false,"context_lines":[{"line_number":289,"context_line":"        # safe to clear out the invalidations file."},{"line_number":290,"context_line":"        if clear_invalidations_file:"},{"line_number":291,"context_line":"            with open(invalidations_file, \u0027wb\u0027) as inv_fh:"},{"line_number":292,"context_line":"                pass"},{"line_number":293,"context_line":""},{"line_number":294,"context_line":"        mtime \u003d getmtime(hashes_file)"},{"line_number":295,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_59ca891b","line":292,"in_reply_to":"ba5201f7_c5b6ee3c","updated":"2017-01-11 10:50:02.000000000","message":"are you thinking surround in try/except in case file does not exist and then ditch the existence check at line 271? seems reasonable.","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"8a285c82869e2cf3f9db4cf3e0f2c1bc939e6d75","unresolved":false,"context_lines":[{"line_number":291,"context_line":"            with open(invalidations_file, \u0027wb\u0027) as inv_fh:"},{"line_number":292,"context_line":"                pass"},{"line_number":293,"context_line":""},{"line_number":294,"context_line":"        mtime \u003d getmtime(hashes_file)"},{"line_number":295,"context_line":""},{"line_number":296,"context_line":"        return hashes, mtime"},{"line_number":297,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_c0d02322","line":294,"updated":"2017-01-11 06:07:25.000000000","message":"so it\u0027s impossible to return from this function without the hashes_file existing?\n\nIf so, that would be a nice invariant to call out in the doc string","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"8a285c82869e2cf3f9db4cf3e0f2c1bc939e6d75","unresolved":false,"context_lines":[{"line_number":308,"context_line":"    partition_dir \u003d dirname(suffix_dir)"},{"line_number":309,"context_line":"    hashes_file \u003d join(partition_dir, HASH_FILE)"},{"line_number":310,"context_line":"    if not os.path.exists(hashes_file):"},{"line_number":311,"context_line":"        return"},{"line_number":312,"context_line":""},{"line_number":313,"context_line":"    invalidations_file \u003d join(partition_dir, HASH_INVALIDATIONS_FILE)"},{"line_number":314,"context_line":"    with lock_path(partition_dir):"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_0076593d","line":311,"updated":"2017-01-11 06:07:25.000000000","message":"Why not remove this guard return?  Seems to be the root of the \"mark partition invalid while partitions are being hashed for the first time race\"\n\n1) hashes_file not exist is not a common state; they get created pretty quickly if you have the replicators running.\n\n2) What\u0027s wrong with writing to the invalidations_file - new and old code are both happy to do a full rehash if the pkl doesn\u0027t exist - and if gets written in behind the new consolidated pkl - it\u0027ll get cleaned up the next time.\n\nThe main thing is just that we need truncate and append under lock - which seems to be happening.","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"126089e73d8944d6a35dc60907a61bf5210b5b72","unresolved":false,"context_lines":[{"line_number":308,"context_line":"    partition_dir \u003d dirname(suffix_dir)"},{"line_number":309,"context_line":"    hashes_file \u003d join(partition_dir, HASH_FILE)"},{"line_number":310,"context_line":"    if not os.path.exists(hashes_file):"},{"line_number":311,"context_line":"        return"},{"line_number":312,"context_line":""},{"line_number":313,"context_line":"    invalidations_file \u003d join(partition_dir, HASH_INVALIDATIONS_FILE)"},{"line_number":314,"context_line":"    with lock_path(partition_dir):"}],"source_content_type":"text/x-python","patch_set":16,"id":"7a3c09a3_243a5b37","line":311,"in_reply_to":"7a3c09a3_ab9a9ec3","updated":"2017-01-13 04:28:11.000000000","message":"\u003e When a new hashes.pkl is created to be sure that another concurrent process doesn\u0027t rewrite new created hashes with invalid hashes, then change of invalidate_hash is not necessary.\n\nI am not sure I understand this statement.  I *assume* you must mean the bug with force_rewrite - which yeah is insanely stupid and should be fixed.\n\nI think you\u0027re saying \"the fix in this patch ensures that an invalidation message will not be lost without changing the invalidate_hash function\" - and I agree - but I have the option that changing this function is a more directly/clear/obvious way to fix the specific bug of loosing invalidate messages while a single process is recalculating.\n\nTwo processes stomping on each other because force_rewrite is making a decision about the fake of hashes.pkl based on some on-disk state it took lock-free before it started the rehash is wrong in a separate way.\n\nAt least my brain forces me to keep them separate.  And hey - if this function was more obvious/correct/robust - at least we wouldn\u0027t loosing the invalidation message that came in the meanwhile ;)","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"66be93b28ebd3f13db11ad14c6258025199faf53","unresolved":false,"context_lines":[{"line_number":308,"context_line":"    partition_dir \u003d dirname(suffix_dir)"},{"line_number":309,"context_line":"    hashes_file \u003d join(partition_dir, HASH_FILE)"},{"line_number":310,"context_line":"    if not os.path.exists(hashes_file):"},{"line_number":311,"context_line":"        return"},{"line_number":312,"context_line":""},{"line_number":313,"context_line":"    invalidations_file \u003d join(partition_dir, HASH_INVALIDATIONS_FILE)"},{"line_number":314,"context_line":"    with lock_path(partition_dir):"}],"source_content_type":"text/x-python","patch_set":16,"id":"7a3c09a3_ab9a9ec3","line":311,"in_reply_to":"ba5201f7_0076593d","updated":"2017-01-13 01:56:24.000000000","message":"You are right. But...\n\nWhen a new hashes.pkl is created to be sure that another concurrent process doesn\u0027t rewrite new created hashes with invalid hashes, then change of invalidate_hash is not necessary.","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"8a285c82869e2cf3f9db4cf3e0f2c1bc939e6d75","unresolved":false,"context_lines":[{"line_number":1071,"context_line":"                modified \u003d True"},{"line_number":1072,"context_line":"        if modified:"},{"line_number":1073,"context_line":"            with lock_path(partition_path):"},{"line_number":1074,"context_line":"                if mtime \u003d\u003d -1 or not exists(hashes_file) or \\"},{"line_number":1075,"context_line":"                        getmtime(hashes_file) \u003d\u003d mtime:"},{"line_number":1076,"context_line":""},{"line_number":1077,"context_line":"                    write_pickle("}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_80db7b4e","line":1074,"updated":"2017-01-11 06:07:25.000000000","message":"so the mtime \u003d -1 always indicates an error was raised from consolidate hashes?  Like I don\u0027t think consolidate hashes can not return -1.  So this is just keeping track of the read error.\n\nWhy check if hashes_file exists if we always create it when we call consolidate hashes?  The only place I can imagine we might remove it is when we delete/revert a handoff partition.  The stat call is probably not worth it and a race anyway - just catch/ignore ENOENT from getmtime","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"}],"test/unit/obj/test_diskfile.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"33e8ed59de57d68fd4989bf25c4e3d0e0098da5d","unresolved":false,"context_lines":[{"line_number":6066,"context_line":"            inv_mtime \u003d os.path.getmtime(inv_file)"},{"line_number":6067,"context_line":"            df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)"},{"line_number":6068,"context_line":"            # invalidation file not changed"},{"line_number":6069,"context_line":"            self.assertEqual(inv_mtime, os.path.getmtime(inv_file))"},{"line_number":6070,"context_line":""},{"line_number":6071,"context_line":"    @mock.patch(\u0027swift.obj.diskfile.getmtime\u0027)"},{"line_number":6072,"context_line":"    @mock.patch(\u0027swift.obj.diskfile.write_pickle\u0027)"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a6eadb0_c106671b","line":6069,"updated":"2016-12-21 15:45:08.000000000","message":"+1. Fails with diskfile.py on master branch because hashes.invalid was written even when empty and not changed:\n\n  Failure\nTraceback (most recent call last):\n  File \"/home/anc/0dev/swift/test/unit/obj/test_diskfile.py\", line 6069, in test_invalidate_hash_file_not_changed\n    self.assertEqual(inv_mtime, os.path.getmtime(inv_file))\n  File \"/home/anc/0dev/swift/test/unit/obj/test_diskfile.py\", line 5701, in assertEqual\n    unittest.TestCase.assertEqual(self, *args)\n  File \"/usr/lib/python2.7/contextlib.py\", line 35, in __exit__\n    self.gen.throw(type, value, traceback)\n  File \"/home/anc/0dev/swift/test/unit/obj/test_diskfile.py\", line 5697, in policy_in_message\n    raise AssertionError(str(err) + policy_trailer)\nAssertionError: 1482326969.8035803 !\u003d 1482326969.811581\n\n... for policy ECStoragePolicy(0, [\u0027ec\u0027], is_default\u003dTrue, is_deprecated\u003dFalse, policy_type\u003d\u0027erasure_coding\u0027), EC config(ec_type\u003djerasure_rs_vand, ec_segment_size\u003d4096, ec_ndata\u003d10, ec_nparity\u003d4)","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"33e8ed59de57d68fd4989bf25c4e3d0e0098da5d","unresolved":false,"context_lines":[{"line_number":6078,"context_line":"            non_local[\u0027mtime\u0027] +\u003d 1"},{"line_number":6079,"context_line":"            write_pickle(*args, **kwargs)"},{"line_number":6080,"context_line":""},{"line_number":6081,"context_line":"        def mock_getmtime_def(filename):"},{"line_number":6082,"context_line":"            return non_local[\u0027mtime\u0027]"},{"line_number":6083,"context_line":""},{"line_number":6084,"context_line":"        mock_write_pickle.side_effect \u003d mock_write_pickle_def"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a6eadb0_2de38aa5","line":6081,"updated":"2016-12-21 15:45:08.000000000","message":"why is getmtime being mocked? is it to be absolutely sure that each write_pickle results in a different mtime? if so then please add a comment to explain why this is needed. For me the test passes without the mocking.","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"17fbde937570f2dc34f8aea5a3717cf0ad602117","unresolved":false,"context_lines":[{"line_number":6078,"context_line":"            non_local[\u0027mtime\u0027] +\u003d 1"},{"line_number":6079,"context_line":"            write_pickle(*args, **kwargs)"},{"line_number":6080,"context_line":""},{"line_number":6081,"context_line":"        def mock_getmtime_def(filename):"},{"line_number":6082,"context_line":"            return non_local[\u0027mtime\u0027]"},{"line_number":6083,"context_line":""},{"line_number":6084,"context_line":"        mock_write_pickle.side_effect \u003d mock_write_pickle_def"}],"source_content_type":"text/x-python","patch_set":14,"id":"da4df55a_653f0026","line":6081,"in_reply_to":"1a6eadb0_2de38aa5","updated":"2016-12-22 12:56:37.000000000","message":"For me not. With LXC virtualization getmtime() returns time with step 0.01 s. That\u0027s the reason why time is mocked.","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"33e8ed59de57d68fd4989bf25c4e3d0e0098da5d","unresolved":false,"context_lines":[{"line_number":6103,"context_line":"                mock_consolidate_hashes.reset_mock()"},{"line_number":6104,"context_line":"                # updates pkl file"},{"line_number":6105,"context_line":"                df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)"},{"line_number":6106,"context_line":"                mock_consolidate_hashes.assert_called_once()"},{"line_number":6107,"context_line":""},{"line_number":6108,"context_line":"    def test_invalidates_hashes_of_new_partition(self):"},{"line_number":6109,"context_line":"        for policy in self.iter_policies():"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a6eadb0_ed97d28d","line":6106,"updated":"2016-12-21 15:45:08.000000000","message":"+1 Fails on master diskfile.py because when consolidate_hashes created hashes.pkl then _get_hashes detected a change in mtime and re-entered.\n\n  Failure\nTraceback (most recent call last):\n  File \"/home/anc/0dev/swift/test/unit/obj/test_diskfile.py\", line 6110, in test_get_hashes_consolidates_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":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"33e8ed59de57d68fd4989bf25c4e3d0e0098da5d","unresolved":false,"context_lines":[{"line_number":6117,"context_line":"            suffix2 \u003d os.path.basename(os.path.dirname(df2._datadir))"},{"line_number":6118,"context_line":"            non_local \u003d {\u0027df2touched\u0027: False}"},{"line_number":6119,"context_line":""},{"line_number":6120,"context_line":"            self.assertNotEqual(suffix, suffix2)"},{"line_number":6121,"context_line":"            df.delete(self.ts())"},{"line_number":6122,"context_line":""},{"line_number":6123,"context_line":"            def mock_listdir(*args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a6eadb0_a8da2849","line":6120,"updated":"2016-12-21 15:45:08.000000000","message":"this may not be true if you happen to have test machine with /etc/swift/swift.conf and hash suffix/prefix that causes /a/b/o to hash to same suffix as /a/c/o. \n\nThis needs the same trick as used before at line 6244 - that code could be made into a simple helper function to return a df in a different suffix.","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"17fbde937570f2dc34f8aea5a3717cf0ad602117","unresolved":false,"context_lines":[{"line_number":6117,"context_line":"            suffix2 \u003d os.path.basename(os.path.dirname(df2._datadir))"},{"line_number":6118,"context_line":"            non_local \u003d {\u0027df2touched\u0027: False}"},{"line_number":6119,"context_line":""},{"line_number":6120,"context_line":"            self.assertNotEqual(suffix, suffix2)"},{"line_number":6121,"context_line":"            df.delete(self.ts())"},{"line_number":6122,"context_line":""},{"line_number":6123,"context_line":"            def mock_listdir(*args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":14,"id":"da4df55a_68f8b57d","line":6120,"in_reply_to":"1a6eadb0_a8da2849","updated":"2016-12-22 12:56:37.000000000","message":"Done","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"33e8ed59de57d68fd4989bf25c4e3d0e0098da5d","unresolved":false,"context_lines":[{"line_number":6120,"context_line":"            self.assertNotEqual(suffix, suffix2)"},{"line_number":6121,"context_line":"            df.delete(self.ts())"},{"line_number":6122,"context_line":""},{"line_number":6123,"context_line":"            def mock_listdir(*args, **kwargs):"},{"line_number":6124,"context_line":"                result \u003d orig_listdir(*args, **kwargs)"},{"line_number":6125,"context_line":"                if not non_local[\u0027df2touched\u0027]:"},{"line_number":6126,"context_line":"                    non_local[\u0027df2touched\u0027] \u003d True"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a6eadb0_28ab7876","line":6123,"updated":"2016-12-21 15:45:08.000000000","message":"I think it would be worth a comment to explain that this is simulating an invalidation occuring in another process while get_hashes is executing.","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"17fbde937570f2dc34f8aea5a3717cf0ad602117","unresolved":false,"context_lines":[{"line_number":6120,"context_line":"            self.assertNotEqual(suffix, suffix2)"},{"line_number":6121,"context_line":"            df.delete(self.ts())"},{"line_number":6122,"context_line":""},{"line_number":6123,"context_line":"            def mock_listdir(*args, **kwargs):"},{"line_number":6124,"context_line":"                result \u003d orig_listdir(*args, **kwargs)"},{"line_number":6125,"context_line":"                if not non_local[\u0027df2touched\u0027]:"},{"line_number":6126,"context_line":"                    non_local[\u0027df2touched\u0027] \u003d True"}],"source_content_type":"text/x-python","patch_set":14,"id":"da4df55a_b2812392","line":6123,"in_reply_to":"1a6eadb0_28ab7876","updated":"2016-12-22 12:56:37.000000000","message":"Of course. I\u0027m lost in test_diskfile.py file and I hope my comments will help.","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"33e8ed59de57d68fd4989bf25c4e3d0e0098da5d","unresolved":false,"context_lines":[{"line_number":6138,"context_line":"            # updates pkl file"},{"line_number":6139,"context_line":"            hashes \u003d df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)"},{"line_number":6140,"context_line":"            self.assertIn(suffix, hashes)"},{"line_number":6141,"context_line":"            self.assertIn(suffix2, hashes)"},{"line_number":6142,"context_line":""},{"line_number":6143,"context_line":"    @mock.patch(\u0027swift.obj.diskfile.getmtime\u0027)"},{"line_number":6144,"context_line":"    @mock.patch(\u0027swift.obj.diskfile.write_pickle\u0027)"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a6eadb0_48ed14b6","line":6141,"updated":"2016-12-21 15:45:08.000000000","message":"+1 Fails with diskfile.py on master because second invalidation is lost:\n\n  Failure\nTraceback (most recent call last):\n  File \"/home/anc/0dev/swift/test/unit/obj/test_diskfile.py\", line 6141, in test_invalidates_hashes_of_new_partition\n    self.assertIn(suffix2, hashes)\nAssertionError: \u0027204\u0027 not found in {\u0027bbe\u0027: {None: \u002795bf0bd147a0342a320a03bfb15472b0\u0027}}","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"33e8ed59de57d68fd4989bf25c4e3d0e0098da5d","unresolved":false,"context_lines":[{"line_number":6144,"context_line":"    @mock.patch(\u0027swift.obj.diskfile.write_pickle\u0027)"},{"line_number":6145,"context_line":"    def test_contains_hashes_of_new_partition(self, mock_write_pickle,"},{"line_number":6146,"context_line":"                                              mock_getmtime):"},{"line_number":6147,"context_line":"        non_local \u003d {\u0027mtime\u0027: time()}"},{"line_number":6148,"context_line":""},{"line_number":6149,"context_line":"        def mock_write_pickle_def(*args, **kwargs):"},{"line_number":6150,"context_line":"            non_local[\u0027mtime\u0027] +\u003d 1"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a6eadb0_431b37ca","line":6147,"updated":"2016-12-21 15:45:08.000000000","message":"strictly, should this not be set until mock_write_pickle_def is called, and mock_getmtime_def raise an OSError if mtime\u003d\u003dNone? That would better simulate any call to getmtime before a hashes.pkl exists.\n\nSame in test_get_hashes_consolidates_once","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"17fbde937570f2dc34f8aea5a3717cf0ad602117","unresolved":false,"context_lines":[{"line_number":6144,"context_line":"    @mock.patch(\u0027swift.obj.diskfile.write_pickle\u0027)"},{"line_number":6145,"context_line":"    def test_contains_hashes_of_new_partition(self, mock_write_pickle,"},{"line_number":6146,"context_line":"                                              mock_getmtime):"},{"line_number":6147,"context_line":"        non_local \u003d {\u0027mtime\u0027: time()}"},{"line_number":6148,"context_line":""},{"line_number":6149,"context_line":"        def mock_write_pickle_def(*args, **kwargs):"},{"line_number":6150,"context_line":"            non_local[\u0027mtime\u0027] +\u003d 1"}],"source_content_type":"text/x-python","patch_set":14,"id":"da4df55a_b372047b","line":6147,"in_reply_to":"1a6eadb0_431b37ca","updated":"2016-12-22 12:56:37.000000000","message":"Done\n\nFixed until someone use getmtime on another file.","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"33e8ed59de57d68fd4989bf25c4e3d0e0098da5d","unresolved":false,"context_lines":[{"line_number":6151,"context_line":"            write_pickle(*args, **kwargs)"},{"line_number":6152,"context_line":""},{"line_number":6153,"context_line":"        def mock_getmtime_def(filename):"},{"line_number":6154,"context_line":"            return non_local[\u0027mtime\u0027]"},{"line_number":6155,"context_line":""},{"line_number":6156,"context_line":"        mock_write_pickle.side_effect \u003d mock_write_pickle_def"},{"line_number":6157,"context_line":"        mock_getmtime.side_effect \u003d mock_getmtime_def"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a6eadb0_2318fb1d","line":6154,"updated":"2016-12-21 15:45:08.000000000","message":"ok, in this test its more obvious why getmtime is mocked, because the test is simulating another process touching the file.","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"33e8ed59de57d68fd4989bf25c4e3d0e0098da5d","unresolved":false,"context_lines":[{"line_number":6167,"context_line":"            suffix2 \u003d os.path.basename(os.path.dirname(df2._datadir))"},{"line_number":6168,"context_line":"            non_local[\u0027df2touched\u0027] \u003d False"},{"line_number":6169,"context_line":""},{"line_number":6170,"context_line":"            self.assertNotEqual(suffix, suffix2)"},{"line_number":6171,"context_line":"            df.delete(self.ts())"},{"line_number":6172,"context_line":""},{"line_number":6173,"context_line":"            def mock_listdir(*args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a6eadb0_a33ecb88","line":6170,"updated":"2016-12-21 15:45:08.000000000","message":"same as line 6120, needs to be guaranteed different suffixes","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"17fbde937570f2dc34f8aea5a3717cf0ad602117","unresolved":false,"context_lines":[{"line_number":6167,"context_line":"            suffix2 \u003d os.path.basename(os.path.dirname(df2._datadir))"},{"line_number":6168,"context_line":"            non_local[\u0027df2touched\u0027] \u003d False"},{"line_number":6169,"context_line":""},{"line_number":6170,"context_line":"            self.assertNotEqual(suffix, suffix2)"},{"line_number":6171,"context_line":"            df.delete(self.ts())"},{"line_number":6172,"context_line":""},{"line_number":6173,"context_line":"            def mock_listdir(*args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":14,"id":"da4df55a_ed5d8aed","line":6170,"in_reply_to":"1a6eadb0_a33ecb88","updated":"2016-12-22 12:56:37.000000000","message":"Done","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"33e8ed59de57d68fd4989bf25c4e3d0e0098da5d","unresolved":false,"context_lines":[{"line_number":6181,"context_line":""},{"line_number":6182,"context_line":"            with mock.patch(\u0027swift.obj.diskfile.os.listdir\u0027,"},{"line_number":6183,"context_line":"                            mock_listdir):"},{"line_number":6184,"context_line":"                # creates pkl file and repeats lising when pkl modified"},{"line_number":6185,"context_line":"                hashes \u003d df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)"},{"line_number":6186,"context_line":""},{"line_number":6187,"context_line":"            self.assertIn(suffix, hashes)"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a6eadb0_034a1f26","line":6184,"updated":"2016-12-21 15:45:08.000000000","message":"nit s/lising/listing/","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":23449,"name":"Pavel Kvasnička","email":"pavel.kvasnicka@firma.seznam.cz","username":"PavelK"},"change_message_id":"17fbde937570f2dc34f8aea5a3717cf0ad602117","unresolved":false,"context_lines":[{"line_number":6181,"context_line":""},{"line_number":6182,"context_line":"            with mock.patch(\u0027swift.obj.diskfile.os.listdir\u0027,"},{"line_number":6183,"context_line":"                            mock_listdir):"},{"line_number":6184,"context_line":"                # creates pkl file and repeats lising when pkl modified"},{"line_number":6185,"context_line":"                hashes \u003d df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)"},{"line_number":6186,"context_line":""},{"line_number":6187,"context_line":"            self.assertIn(suffix, hashes)"}],"source_content_type":"text/x-python","patch_set":14,"id":"da4df55a_ed04aac7","line":6184,"in_reply_to":"1a6eadb0_034a1f26","updated":"2016-12-22 12:56:37.000000000","message":"Done","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"33e8ed59de57d68fd4989bf25c4e3d0e0098da5d","unresolved":false,"context_lines":[{"line_number":6185,"context_line":"                hashes \u003d df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)"},{"line_number":6186,"context_line":""},{"line_number":6187,"context_line":"            self.assertIn(suffix, hashes)"},{"line_number":6188,"context_line":"            self.assertIn(suffix2, hashes)"},{"line_number":6189,"context_line":""},{"line_number":6190,"context_line":"    def test_invalidate_hash_consolidation(self):"},{"line_number":6191,"context_line":"        def assert_consolidation(suffixes):"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a6eadb0_8388ef72","line":6188,"updated":"2016-12-21 15:45:08.000000000","message":"+1 fails with diskfile.py on master for same reason as previous test - the second invalidation gets lost:\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 6188, in test_contains_hashes_of_new_partition\n    self.assertIn(suffix2, hashes)\nAssertionError: \u0027204\u0027 not found in {\u0027bbe\u0027: {None: \u0027e4c017dd8448135dea65f9584eef79c9\u0027}}","commit_id":"011e86d76c1f37ed54753d3045785f514522f0c3"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"9ca23df3f70f1f6987ed12cfd0f5a6be23dc085d","unresolved":false,"context_lines":[{"line_number":5711,"context_line":"            i +\u003d 1"},{"line_number":5712,"context_line":"            suffix_dir2 \u003d os.path.dirname(df2._datadir)"},{"line_number":5713,"context_line":"            if suffix_dir !\u003d suffix_dir2:"},{"line_number":5714,"context_line":"                return df2"},{"line_number":5715,"context_line":""},{"line_number":5716,"context_line":"    def check_cleanup_ondisk_files(self, policy, input_files, output_files):"},{"line_number":5717,"context_line":"        orig_unlink \u003d os.unlink"}],"source_content_type":"text/x-python","patch_set":16,"id":"da4df55a_b729e28b","line":5714,"updated":"2016-12-23 12:19:07.000000000","message":"nice","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b21c64735bf94ae4b1c02ae4840c76ee3cbb35c1","unresolved":false,"context_lines":[{"line_number":5711,"context_line":"            i +\u003d 1"},{"line_number":5712,"context_line":"            suffix_dir2 \u003d os.path.dirname(df2._datadir)"},{"line_number":5713,"context_line":"            if suffix_dir !\u003d suffix_dir2:"},{"line_number":5714,"context_line":"                return df2"},{"line_number":5715,"context_line":""},{"line_number":5716,"context_line":"    def check_cleanup_ondisk_files(self, policy, input_files, output_files):"},{"line_number":5717,"context_line":"        orig_unlink \u003d os.unlink"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_5c02770a","line":5714,"in_reply_to":"ba5201f7_a09bcd64","updated":"2017-01-11 10:50:02.000000000","message":"AFAICT unit tests pick up any hash path *fix you may have in /etc/swift/swift.conf ( :/ ) so this is about avoiding an unlikely pathological case where your path hashing results in the first choice landing in same suffix.\n\npossibly overkill but no harm in being sure the suffixes differ.","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"8a285c82869e2cf3f9db4cf3e0f2c1bc939e6d75","unresolved":false,"context_lines":[{"line_number":5711,"context_line":"            i +\u003d 1"},{"line_number":5712,"context_line":"            suffix_dir2 \u003d os.path.dirname(df2._datadir)"},{"line_number":5713,"context_line":"            if suffix_dir !\u003d suffix_dir2:"},{"line_number":5714,"context_line":"                return df2"},{"line_number":5715,"context_line":""},{"line_number":5716,"context_line":"    def check_cleanup_ondisk_files(self, policy, input_files, output_files):"},{"line_number":5717,"context_line":"        orig_unlink \u003d os.unlink"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_a09bcd64","line":5714,"in_reply_to":"da4df55a_b729e28b","updated":"2017-01-11 06:07:25.000000000","message":"curious the cardinality on this iteration in practice?\n\n\tdiff --git a/test/unit/obj/test_diskfile.py b/test/unit/obj/test_diskfile.py\n\tindex c1ac441..9f85054 100644\n\t--- a/test/unit/obj/test_diskfile.py\n\t+++ b/test/unit/obj/test_diskfile.py\n\t@@ -5711,6 +5711,9 @@ class TestSuffixHashes(unittest.TestCase):\n\t\t\t\t i +\u003d 1\n\t\t\t\t suffix_dir2 \u003d os.path.dirname(df2._datadir)\n\t\t\t\t if suffix_dir !\u003d suffix_dir2:\n\t+                print \u0027*\u0027 * 50\n\t+                print \u0027count was: %s\u0027 % i\n\t+                print \u0027*\u0027 * 50\n\t\t\t\t\t return df2\n\t \n\t\t def check_cleanup_ondisk_files(self, policy, input_files, output_files):\n\nheh, looks like it\u0027s basically always the first try ;)\n\nwhich makes sense","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"8a285c82869e2cf3f9db4cf3e0f2c1bc939e6d75","unresolved":false,"context_lines":[{"line_number":6057,"context_line":"            hashes \u003d df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)"},{"line_number":6058,"context_line":"            self.assertIn(suffix, hashes)  # sanity"},{"line_number":6059,"context_line":""},{"line_number":6060,"context_line":"    def test_invalidate_hash_file_not_changed(self):"},{"line_number":6061,"context_line":"        for policy in self.iter_policies():"},{"line_number":6062,"context_line":"            df_mgr \u003d self.df_router[policy]"},{"line_number":6063,"context_line":"            part_path \u003d os.path.join(self.devices, \u0027sda1\u0027,"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_6ae93f1a","line":6060,"updated":"2017-01-11 06:07:25.000000000","message":"nit: test_invalidate_hash_file_not_truncated_when_empty","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"8a285c82869e2cf3f9db4cf3e0f2c1bc939e6d75","unresolved":false,"context_lines":[{"line_number":6076,"context_line":"            df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)"},{"line_number":6077,"context_line":""},{"line_number":6078,"context_line":"            self.assertTrue(os.path.exists(inv_file))"},{"line_number":6079,"context_line":"            inv_mtime \u003d os.path.getmtime(inv_file)"},{"line_number":6080,"context_line":"            df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)"},{"line_number":6081,"context_line":"            # invalidation file not changed"},{"line_number":6082,"context_line":"            self.assertEqual(inv_mtime, os.path.getmtime(inv_file))"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_ea728f7b","line":6079,"updated":"2017-01-11 06:07:25.000000000","message":"So I found this test will still sometimes pass even if the file is *definitely* modified just because of the low resolution of getmtime (as hinted with some other mocks!)\n\n\tdiff --git a/swift/obj/diskfile.py b/swift/obj/diskfile.py\n\tindex cc6b844..c95423d 100644\n\t--- a/swift/obj/diskfile.py\n\t+++ b/swift/obj/diskfile.py\n\t@@ -287,7 +287,7 @@ def consolidate_hashes(partition_dir):\n\t \n\t\t\t # Now that all the invalidations are reflected in hashes.pkl, it\u0027s\n\t\t\t # safe to clear out the invalidations file.\n\t-        if clear_invalidations_file:\n\t+        if True or clear_invalidations_file:\n\t\t\t\t with open(invalidations_file, \u0027wb\u0027) as inv_fh:\n\t\t\t\t\t pass\n\t \n\nI wonder if there is a better proxy than mtime for \"did this file get modified\" - perhaps it would be more sensible to check for a call to open.\n\nFWIW I don\u0027t see how it could case sporadic failure as written - so it\u0027s maybe fine.","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"8a285c82869e2cf3f9db4cf3e0f2c1bc939e6d75","unresolved":false,"context_lines":[{"line_number":6079,"context_line":"            inv_mtime \u003d os.path.getmtime(inv_file)"},{"line_number":6080,"context_line":"            df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)"},{"line_number":6081,"context_line":"            # invalidation file not changed"},{"line_number":6082,"context_line":"            self.assertEqual(inv_mtime, os.path.getmtime(inv_file))"},{"line_number":6083,"context_line":""},{"line_number":6084,"context_line":"    @mock.patch(\u0027swift.obj.diskfile.getmtime\u0027)"},{"line_number":6085,"context_line":"    @mock.patch(\u0027swift.obj.diskfile.write_pickle\u0027)"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_6ad05f54","line":6082,"updated":"2017-01-11 06:07:25.000000000","message":"I think it\u0027d be equally interesting to validate the hashes_file does not change:\n\n\tdiff --git a/test/unit/obj/test_diskfile.py b/test/unit/obj/test_diskfile.py\n\tindex c1ac441..a386365 100644\n\t--- a/test/unit/obj/test_diskfile.py\n\t+++ b/test/unit/obj/test_diskfile.py\n\t@@ -6064,6 +6064,8 @@ class TestSuffixHashes(unittest.TestCase):\n\t\t\t\t\t\t\t\t\t\t  diskfile.get_data_dir(policy), \u00270\u0027)\n\t\t\t\t inv_file \u003d os.path.join(\n\t\t\t\t\t part_path, diskfile.HASH_INVALIDATIONS_FILE)\n\t+            hash_file \u003d os.path.join(\n\t+                part_path, diskfile.HASH_FILE)\n\t \n\t\t\t\t hashes \u003d df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)\n\t\t\t\t self.assertEqual(hashes, {})\n\t@@ -6077,8 +6079,12 @@ class TestSuffixHashes(unittest.TestCase):\n\t \n\t\t\t\t self.assertTrue(os.path.exists(inv_file))\n\t\t\t\t inv_mtime \u003d os.path.getmtime(inv_file)\n\t+            hash_mtime \u003d os.path.getmtime(hash_file)\n\t+            sleep(1e-2)\n\t+            # no new suffixes get invalided\n\t\t\t\t df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)\n\t-            # invalidation file not changed\n\t+            # ... so no write iop - invalidation file not changed\n\t+            self.assertEqual(hash_mtime, os.path.getmtime(hash_file))\n\t\t\t\t self.assertEqual(inv_mtime, os.path.getmtime(inv_file))\n\t \n\t\t @mock.patch(\u0027swift.obj.diskfile.getmtime\u0027)","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"9ca23df3f70f1f6987ed12cfd0f5a6be23dc085d","unresolved":false,"context_lines":[{"line_number":6089,"context_line":""},{"line_number":6090,"context_line":"        def mock_write_pickle_def(*args, **kwargs):"},{"line_number":6091,"context_line":"            # getmtime is mocked for LXC virtualization where file"},{"line_number":6092,"context_line":"            # modification time is changed with step 0.01 s"},{"line_number":6093,"context_line":"            if \u0027mtime\u0027 not in non_local:"},{"line_number":6094,"context_line":"                non_local[\u0027mtime\u0027] \u003d time()"},{"line_number":6095,"context_line":"            non_local[\u0027mtime\u0027] +\u003d 1"}],"source_content_type":"text/x-python","patch_set":16,"id":"da4df55a_9774de9c","line":6092,"updated":"2016-12-23 12:19:07.000000000","message":"ok, makes sense","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"8a285c82869e2cf3f9db4cf3e0f2c1bc939e6d75","unresolved":false,"context_lines":[{"line_number":6114,"context_line":"                                   ) as mock_consolidate_hashes:"},{"line_number":6115,"context_line":"                # creates pkl file"},{"line_number":6116,"context_line":"                df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)"},{"line_number":6117,"context_line":"                mock_consolidate_hashes.assert_called_once()"},{"line_number":6118,"context_line":"                # second object in path"},{"line_number":6119,"context_line":"                df2 \u003d self.get_different_suffix_df(df, df_mgr,"},{"line_number":6120,"context_line":"                                                   \u0027sda1\u0027, \u00270\u0027, \u0027a\u0027, \u0027c\u0027,"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_004e59c0","line":6117,"updated":"2017-01-11 06:07:25.000000000","message":"so mock the function with a side effect of itself you can assert it was called - wfm\n\nBut I\u0027ve never been a fan of assert_called_once() - I feel like I\u0027m too likely to type `assert_called_onec()` and have it *also* not raise an AssertionError ;)\n\n    self.assertEqual([mock.call()], my_mock.call_args_list)\n\n^ more verbose but also definitely does what you think and will not silently let you mess up","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b21c64735bf94ae4b1c02ae4840c76ee3cbb35c1","unresolved":false,"context_lines":[{"line_number":6114,"context_line":"                                   ) as mock_consolidate_hashes:"},{"line_number":6115,"context_line":"                # creates pkl file"},{"line_number":6116,"context_line":"                df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)"},{"line_number":6117,"context_line":"                mock_consolidate_hashes.assert_called_once()"},{"line_number":6118,"context_line":"                # second object in path"},{"line_number":6119,"context_line":"                df2 \u003d self.get_different_suffix_df(df, df_mgr,"},{"line_number":6120,"context_line":"                                                   \u0027sda1\u0027, \u00270\u0027, \u0027a\u0027, \u0027c\u0027,"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_7c99fb6d","line":6117,"in_reply_to":"ba5201f7_004e59c0","updated":"2017-01-11 10:50:02.000000000","message":"yeah, I spent time on this because IIRC older versions of mock (like I had installed on my dev machine) don\u0027t have assert_called_once !! But of course the test passed, I just didn\u0027t recognise the method.\n\nIt\u0027s a problem of the mock assertion methods - any typo can go un-noticed. And seemingly valid methods may not really exist! [1][2]\n\nBut IDK, we use other mock assertion methods (e.g. assert_called_with) so if it is a valid assertion method then is it fair to ding usage?\n\nmock should have some mode that disables automocking of anything starting assert_ to prevent user burning themselves!\n\n[1] https://engineeringblog.yelp.com/2015/02/assert_called_once-threat-or-menace.html\n[2] https://bugs.launchpad.net/neutron/+bug/1300265","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"8a285c82869e2cf3f9db4cf3e0f2c1bc939e6d75","unresolved":false,"context_lines":[{"line_number":6123,"context_line":"                mock_consolidate_hashes.reset_mock()"},{"line_number":6124,"context_line":"                # updates pkl file"},{"line_number":6125,"context_line":"                df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)"},{"line_number":6126,"context_line":"                mock_consolidate_hashes.assert_called_once()"},{"line_number":6127,"context_line":""},{"line_number":6128,"context_line":"    def test_invalidates_hashes_of_new_partition(self):"},{"line_number":6129,"context_line":"        # a suffix can be changed or created by second process when new pkl"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_ea960f93","line":6126,"updated":"2017-01-11 06:07:25.000000000","message":"I think it would at lot stronger assertion to check that _hash_suffix was called only once with the suffix_dir2\n\n    diff --git a/test/unit/obj/test_diskfile.py b/test/unit/obj/test_diskfile.py\n    index c1ac441..8f8dbc8 100644\n    --- a/test/unit/obj/test_diskfile.py\n    +++ b/test/unit/obj/test_diskfile.py\n    @@ -6108,22 +6108,32 @@ class TestSuffixHashes(unittest.TestCase):\n                 df \u003d df_mgr.get_diskfile(\u0027sda1\u0027, \u00270\u0027, \u0027a\u0027, \u0027c\u0027, \u0027o\u0027,\n                                          policy\u003dpolicy)\n                 df.delete(self.ts())\n    +            suffix_dir \u003d os.path.dirname(df._datadir)\n     \n                 with mock.patch.object(df_mgr, \u0027consolidate_hashes\u0027,\n                                        side_effect\u003ddf_mgr.consolidate_hashes\n    -                                   ) as mock_consolidate_hashes:\n    +                                   ) as mock_consolidate_hashes, \\\n    +                    mock.patch.object(df_mgr, \u0027_hash_suffix\u0027,\n    +                                      side_effect\u003ddf_mgr._hash_suffix\n    +                                      ) as mock_hash_suffix:\n                     # creates pkl file\n                     df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)\n                     mock_consolidate_hashes.assert_called_once()\n    +                self.assertEqual([mock.call(suffix_dir, df_mgr.reclaim_age)],\n    +                                 mock_hash_suffix.call_args_list)\n                     # second object in path\n                     df2 \u003d self.get_different_suffix_df(df, df_mgr,\n                                                        \u0027sda1\u0027, \u00270\u0027, \u0027a\u0027, \u0027c\u0027,\n                                                        policy)\n                     df2.delete(self.ts())\n    +                suffix_dir2 \u003d os.path.dirname(df2._datadir)\n                     mock_consolidate_hashes.reset_mock()\n    +                mock_hash_suffix.reset_mock()\n                     # updates pkl file\n                     df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)\n                     mock_consolidate_hashes.assert_called_once()\n    +                self.assertEqual([mock.call(suffix_dir2, df_mgr.reclaim_age)],\n    +                                 mock_hash_suffix.call_args_list)\n     \n         def test_invalidates_hashes_of_new_partition(self):\n             # a suffix can be changed or created by second process when new pkl","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d2bc566b5bedb3078c27524eafe86c48c2c4f2f5","unresolved":false,"context_lines":[{"line_number":6147,"context_line":"                if not non_local[\u0027df2touched\u0027]:"},{"line_number":6148,"context_line":"                    non_local[\u0027df2touched\u0027] \u003d True"},{"line_number":6149,"context_line":"                    # other process creates new suffix"},{"line_number":6150,"context_line":"                    df2.delete(self.ts())"},{"line_number":6151,"context_line":"                return result"},{"line_number":6152,"context_line":""},{"line_number":6153,"context_line":"            with mock.patch(\u0027swift.obj.diskfile.os.listdir\u0027,"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba5201f7_e80b3e22","line":6150,"updated":"2017-01-12 00:53:35.000000000","message":"wow this is *so* twisted!?\n\nfirst of all - if you capture calls you can see that this operation causes the call into this mock to recurse (the listdir in cleanup_hash_dir)\n\nsecond of all - the invalid hash call doesn\u0027t hit the \u0027if not hash exist\u0027 block - it writes to the invalidation file.\n\nand last - while the second suffix exists at this point - the reason the calling code behaves like it does not is because the results of listdir were captured above","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"9ca23df3f70f1f6987ed12cfd0f5a6be23dc085d","unresolved":false,"context_lines":[{"line_number":6208,"context_line":"                    non_local[\u0027df2touched\u0027] \u003d True"},{"line_number":6209,"context_line":"                    df2.delete(self.ts())"},{"line_number":6210,"context_line":"                    # simulate pkl update by other process - mtime is updated"},{"line_number":6211,"context_line":"                    self.assertIn(\u0027mtime\u0027, non_local, \"hashes.pkl must exist\")"},{"line_number":6212,"context_line":"                    non_local[\u0027mtime\u0027] +\u003d 1"},{"line_number":6213,"context_line":"                return result"},{"line_number":6214,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"da4df55a_97e93e1d","line":6211,"updated":"2016-12-23 12:19:07.000000000","message":"yes, good sanity check","commit_id":"36db77ace5f75594ad7ae789f14b49117c5b9c4c"}]}