)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":8542,"name":"Prashanth Pai","email":"ppai@redhat.com","username":"pai"},"change_message_id":"2ac4b394a12326d2794b1d404ff7b2df91160786","unresolved":false,"context_lines":[{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Although, it may seem like the right thing to do, this change does come"},{"line_number":18,"context_line":"at a performance penalty. Account and container DBs are not created very"},{"line_number":19,"context_line":"frequently. But objects are! A configurable option is provided to turn"},{"line_number":20,"context_line":"off fsyncing dirs in object-server."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Also, lock_path() inside invalidate_hash() was always creating part of"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":13,"id":"da86d52c_74adb19a","line":19,"updated":"2015-02-12 11:05:29.000000000","message":"Removed the option. Will update commit msg in subsequent patchset,","commit_id":"4c7aae118302508a3a2259ed004780fbc50dc4e0"}],"swift/common/db.py":[{"author":{"_account_id":6198,"name":"Peter Portante","email":"peter.a.portante@gmail.com","username":"peter-a-portante"},"change_message_id":"45196668ebc2c15de563eb549411349914f129e1","unresolved":false,"context_lines":[{"line_number":297,"context_line":"                    # of the system were \"racing\" each other."},{"line_number":298,"context_line":"                    raise DatabaseAlreadyExists(self.db_file)"},{"line_number":299,"context_line":"                renamer(tmp_db_file, self.db_file)"},{"line_number":300,"context_line":"                fsync_dir(os.path.dirname(self.db_file))"},{"line_number":301,"context_line":"            self.conn \u003d get_db_connection(self.db_file, self.timeout)"},{"line_number":302,"context_line":"        else:"},{"line_number":303,"context_line":"            self.conn \u003d conn"}],"source_content_type":"text/x-python","patch_set":5,"id":"baa201ad_bb52e9db","line":300,"updated":"2014-10-15 10:49:08.000000000","message":"Every call to fsync_dir invokes os.path.direname before hand.  Perhaps we could rename fsync_dir to fsync_dirof and have fsync_dirof do the os.path.dirname itself?  Just thinken ...","commit_id":"38fa4a87f0f5a392e17ef7acb89899e83b2b1842"},{"author":{"_account_id":8542,"name":"Prashanth Pai","email":"ppai@redhat.com","username":"pai"},"change_message_id":"ad7af28c8e4bde841554cc50ef2b8d91c3f840db","unresolved":false,"context_lines":[{"line_number":297,"context_line":"                    # of the system were \"racing\" each other."},{"line_number":298,"context_line":"                    raise DatabaseAlreadyExists(self.db_file)"},{"line_number":299,"context_line":"                renamer(tmp_db_file, self.db_file)"},{"line_number":300,"context_line":"                fsync_dir(os.path.dirname(self.db_file))"},{"line_number":301,"context_line":"            self.conn \u003d get_db_connection(self.db_file, self.timeout)"},{"line_number":302,"context_line":"        else:"},{"line_number":303,"context_line":"            self.conn \u003d conn"}],"source_content_type":"text/x-python","patch_set":5,"id":"baa201ad_26e500dc","line":300,"in_reply_to":"baa201ad_bb52e9db","updated":"2014-10-15 11:26:30.000000000","message":"What do u think of this ?\n\n    def fsync_dirof(path):\n        fsync_dir(os.path.dirname(path))","commit_id":"38fa4a87f0f5a392e17ef7acb89899e83b2b1842"},{"author":{"_account_id":6198,"name":"Peter Portante","email":"peter.a.portante@gmail.com","username":"peter-a-portante"},"change_message_id":"45196668ebc2c15de563eb549411349914f129e1","unresolved":false,"context_lines":[{"line_number":338,"context_line":"        quar_path \u003d os.path.join(device_path, \u0027quarantined\u0027,"},{"line_number":339,"context_line":"                                 self.db_type + \u0027s\u0027,"},{"line_number":340,"context_line":"                                 os.path.basename(self.db_dir))"},{"line_number":341,"context_line":"        try:"},{"line_number":342,"context_line":"            renamer(self.db_dir, quar_path)"},{"line_number":343,"context_line":"        except OSError as e:"},{"line_number":344,"context_line":"            if e.errno not in (errno.EEXIST, errno.ENOTEMPTY):"}],"source_content_type":"text/x-python","patch_set":5,"id":"baa201ad_9b94458c","line":341,"updated":"2014-10-15 10:49:08.000000000","message":"I think we have to fsync the parent directory of self.db_dir, too.  Does that make sense?","commit_id":"38fa4a87f0f5a392e17ef7acb89899e83b2b1842"},{"author":{"_account_id":8542,"name":"Prashanth Pai","email":"ppai@redhat.com","username":"pai"},"change_message_id":"ad7af28c8e4bde841554cc50ef2b8d91c3f840db","unresolved":false,"context_lines":[{"line_number":338,"context_line":"        quar_path \u003d os.path.join(device_path, \u0027quarantined\u0027,"},{"line_number":339,"context_line":"                                 self.db_type + \u0027s\u0027,"},{"line_number":340,"context_line":"                                 os.path.basename(self.db_dir))"},{"line_number":341,"context_line":"        try:"},{"line_number":342,"context_line":"            renamer(self.db_dir, quar_path)"},{"line_number":343,"context_line":"        except OSError as e:"},{"line_number":344,"context_line":"            if e.errno not in (errno.EEXIST, errno.ENOTEMPTY):"}],"source_content_type":"text/x-python","patch_set":5,"id":"baa201ad_a64650be","line":341,"in_reply_to":"baa201ad_9b94458c","updated":"2014-10-15 11:26:30.000000000","message":"Yes. But I\u0027m confused what needs to be fsynced in cases like this:\n\n    os.makedirs(\"a/b/c/d\")\n\nIs fsync_dir on \"c\" good enough ? is fsync on each of them is needed, if yes, in what order ?","commit_id":"38fa4a87f0f5a392e17ef7acb89899e83b2b1842"},{"author":{"_account_id":6198,"name":"Peter Portante","email":"peter.a.portante@gmail.com","username":"peter-a-portante"},"change_message_id":"45196668ebc2c15de563eb549411349914f129e1","unresolved":false,"context_lines":[{"line_number":343,"context_line":"        except OSError as e:"},{"line_number":344,"context_line":"            if e.errno not in (errno.EEXIST, errno.ENOTEMPTY):"},{"line_number":345,"context_line":"                raise"},{"line_number":346,"context_line":"            quar_path \u003d \"%s-%s\" % (quar_path, uuid4().hex)"},{"line_number":347,"context_line":"            renamer(self.db_dir, quar_path)"},{"line_number":348,"context_line":"        detail \u003d _(\u0027Quarantined %s to %s due to %s database\u0027) % \\"},{"line_number":349,"context_line":"                  (self.db_dir, quar_path, exc_hint)"}],"source_content_type":"text/x-python","patch_set":5,"id":"baa201ad_fbeab10d","line":346,"updated":"2014-10-15 10:49:08.000000000","message":"Same here...","commit_id":"38fa4a87f0f5a392e17ef7acb89899e83b2b1842"}],"swift/common/db_replicator.py":[{"author":{"_account_id":6198,"name":"Peter Portante","email":"peter.a.portante@gmail.com","username":"peter-a-portante"},"change_message_id":"45196668ebc2c15de563eb549411349914f129e1","unresolved":false,"context_lines":[{"line_number":57,"context_line":"    object_dir \u003d os.path.dirname(object_file)"},{"line_number":58,"context_line":"    quarantine_dir \u003d os.path.abspath("},{"line_number":59,"context_line":"        os.path.join(object_dir, \u0027..\u0027, \u0027..\u0027, \u0027..\u0027, \u0027..\u0027, \u0027quarantined\u0027,"},{"line_number":60,"context_line":"                     server_type + \u0027s\u0027, os.path.basename(object_dir)))"},{"line_number":61,"context_line":"    try:"},{"line_number":62,"context_line":"        renamer(object_dir, quarantine_dir)"},{"line_number":63,"context_line":"    except OSError as e:"}],"source_content_type":"text/x-python","patch_set":5,"id":"baa201ad_5b12dd09","line":60,"updated":"2014-10-15 10:49:08.000000000","message":"Parent of quarantined object_dir needs to be fsync\u0027d, I believe.","commit_id":"38fa4a87f0f5a392e17ef7acb89899e83b2b1842"},{"author":{"_account_id":6198,"name":"Peter Portante","email":"peter.a.portante@gmail.com","username":"peter-a-portante"},"change_message_id":"45196668ebc2c15de563eb549411349914f129e1","unresolved":false,"context_lines":[{"line_number":63,"context_line":"    except OSError as e:"},{"line_number":64,"context_line":"        if e.errno not in (errno.EEXIST, errno.ENOTEMPTY):"},{"line_number":65,"context_line":"            raise"},{"line_number":66,"context_line":"        quarantine_dir \u003d \"%s-%s\" % (quarantine_dir, uuid.uuid4().hex)"},{"line_number":67,"context_line":"        renamer(object_dir, quarantine_dir)"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"baa201ad_fb2d5147","line":66,"updated":"2014-10-15 10:49:08.000000000","message":"Same here...","commit_id":"38fa4a87f0f5a392e17ef7acb89899e83b2b1842"}],"swift/common/utils.py":[{"author":{"_account_id":2622,"name":"Samuel Merritt","email":"spam+launchpad@andcheese.org","username":"torgomatic"},"change_message_id":"dc3725d459f0bcc488cd61640f33bb19de25a3a8","unresolved":false,"context_lines":[{"line_number":792,"context_line":"        except OSError as err:"},{"line_number":793,"context_line":"            if err.errno !\u003d errno.EEXIST or not os.path.isdir(path):"},{"line_number":794,"context_line":"                raise"},{"line_number":795,"context_line":"        fsync_dir(path)  # Should this be done on every dir in hierarchy ?"},{"line_number":796,"context_line":""},{"line_number":797,"context_line":""},{"line_number":798,"context_line":"def renamer(old, new):"}],"source_content_type":"text/x-python","patch_set":4,"id":"baa201ad_36ba0e0b","line":795,"updated":"2014-10-09 23:18:57.000000000","message":"Is path the right thing to fsync here, or is it dirname(path)?\n\nIf my limited understanding is correct, Swift should fsync the parent of any dir that it ends up creating, but I really don\u0027t want to fsync all the way up to the root directory every single time; if nothing else, the wasted syscalls would annoy me to no end.\n\nIf we\u0027re going to take that approach, I think the thing to do is to copy the code from os.makedirs() and add in the fsyncing of the parent dir, but only when that directory was actually created.\n\nI don\u0027t know how necessary it is (really, I don\u0027t) to fsync everything though. Anyone with a better understanding of POSIX or XFS is welcome to chime in here.","commit_id":"6ae57a7a7647b9594641db7f719bc03f29f4e82e"},{"author":{"_account_id":2622,"name":"Samuel Merritt","email":"spam+launchpad@andcheese.org","username":"torgomatic"},"change_message_id":"dc3725d459f0bcc488cd61640f33bb19de25a3a8","unresolved":false,"context_lines":[{"line_number":813,"context_line":"    except OSError:"},{"line_number":814,"context_line":"        mkdirs(dirpath)"},{"line_number":815,"context_line":"        os.rename(old, new)"},{"line_number":816,"context_line":"    fsync_dir(dirpath)"},{"line_number":817,"context_line":""},{"line_number":818,"context_line":""},{"line_number":819,"context_line":"def split_path(path, minsegs\u003d1, maxsegs\u003dNone, rest_with_last\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":4,"id":"baa201ad_5636b2b5","line":816,"updated":"2014-10-09 23:18:57.000000000","message":"I think we should not have fsync_dir() called from within renamer(). There are some users of renamer() that don\u0027t really need the fsync, like all the ones that quarantine things. If the corrupt DB / object doesn\u0027t wind up in the quarantine dir, it doesn\u0027t really matter.","commit_id":"6ae57a7a7647b9594641db7f719bc03f29f4e82e"},{"author":{"_account_id":8542,"name":"Prashanth Pai","email":"ppai@redhat.com","username":"pai"},"change_message_id":"2f8ec4b83e811fa3310c138f758a1b7328c12d88","unresolved":false,"context_lines":[{"line_number":813,"context_line":"    except OSError:"},{"line_number":814,"context_line":"        mkdirs(dirpath)"},{"line_number":815,"context_line":"        os.rename(old, new)"},{"line_number":816,"context_line":"    fsync_dir(dirpath)"},{"line_number":817,"context_line":""},{"line_number":818,"context_line":""},{"line_number":819,"context_line":"def split_path(path, minsegs\u003d1, maxsegs\u003dNone, rest_with_last\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":4,"id":"baa201ad_7b1ec80d","line":816,"in_reply_to":"baa201ad_5636b2b5","updated":"2014-10-15 07:23:27.000000000","message":"Done","commit_id":"6ae57a7a7647b9594641db7f719bc03f29f4e82e"},{"author":{"_account_id":2622,"name":"Samuel Merritt","email":"spam+launchpad@andcheese.org","username":"torgomatic"},"change_message_id":"dc3725d459f0bcc488cd61640f33bb19de25a3a8","unresolved":false,"context_lines":[{"line_number":2429,"context_line":"                with NamedTemporaryFile(dir\u003dos.path.dirname(cache_file),"},{"line_number":2430,"context_line":"                                        delete\u003dFalse) as tf:"},{"line_number":2431,"context_line":"                    tf.write(json.dumps(cache_entry) + \u0027\\n\u0027)"},{"line_number":2432,"context_line":"                renamer(tf.name, cache_file)"},{"line_number":2433,"context_line":"            finally:"},{"line_number":2434,"context_line":"                try:"},{"line_number":2435,"context_line":"                    os.unlink(tf.name)"}],"source_content_type":"text/x-python","patch_set":4,"id":"baa201ad_763bb69c","line":2432,"updated":"2014-10-09 23:18:57.000000000","message":"I don\u0027t think we need this change. Recon cache files don\u0027t have to be super-duper durable. Heck, this function doesn\u0027t even fsync the cache file; it just closes it and hopes.","commit_id":"6ae57a7a7647b9594641db7f719bc03f29f4e82e"},{"author":{"_account_id":8542,"name":"Prashanth Pai","email":"ppai@redhat.com","username":"pai"},"change_message_id":"2f8ec4b83e811fa3310c138f758a1b7328c12d88","unresolved":false,"context_lines":[{"line_number":2429,"context_line":"                with NamedTemporaryFile(dir\u003dos.path.dirname(cache_file),"},{"line_number":2430,"context_line":"                                        delete\u003dFalse) as tf:"},{"line_number":2431,"context_line":"                    tf.write(json.dumps(cache_entry) + \u0027\\n\u0027)"},{"line_number":2432,"context_line":"                renamer(tf.name, cache_file)"},{"line_number":2433,"context_line":"            finally:"},{"line_number":2434,"context_line":"                try:"},{"line_number":2435,"context_line":"                    os.unlink(tf.name)"}],"source_content_type":"text/x-python","patch_set":4,"id":"baa201ad_bb14b0e9","line":2432,"in_reply_to":"baa201ad_763bb69c","updated":"2014-10-15 07:23:27.000000000","message":"Done","commit_id":"6ae57a7a7647b9594641db7f719bc03f29f4e82e"},{"author":{"_account_id":6198,"name":"Peter Portante","email":"peter.a.portante@gmail.com","username":"peter-a-portante"},"change_message_id":"45196668ebc2c15de563eb549411349914f129e1","unresolved":false,"context_lines":[{"line_number":565,"context_line":"        fsync(fd)"},{"line_number":566,"context_line":""},{"line_number":567,"context_line":""},{"line_number":568,"context_line":"def fsync_dir(dirpath):"},{"line_number":569,"context_line":"    \"\"\""},{"line_number":570,"context_line":"    Sync directory entries to disk."},{"line_number":571,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"baa201ad_9be5250b","line":568,"updated":"2014-10-15 10:49:08.000000000","message":"Perhaps rename to fsync_dir to fsync_dirof and have it do the os.path.dirname?","commit_id":"38fa4a87f0f5a392e17ef7acb89899e83b2b1842"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@redhat.com","username":"cschwede"},"change_message_id":"bcec3633763003cde05b7be1ac502a1f6a43b51c","unresolved":false,"context_lines":[{"line_number":586,"context_line":"            os.close(dirfd)"},{"line_number":587,"context_line":""},{"line_number":588,"context_line":""},{"line_number":589,"context_line":"def fsync_dirof(path):"},{"line_number":590,"context_line":"    \"\"\""},{"line_number":591,"context_line":"    Fsync on containing directory of path."},{"line_number":592,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"5a890539_93a1d697","line":589,"updated":"2014-12-01 09:03:41.000000000","message":"This method seems to be unused (except in tests)?","commit_id":"43130b422cbddcbbb7f3f07d146f8bded95a41ad"},{"author":{"_account_id":8542,"name":"Prashanth Pai","email":"ppai@redhat.com","username":"pai"},"change_message_id":"355c90c8c3364a6181c86bd9033a2d3f67bb40f5","unresolved":false,"context_lines":[{"line_number":586,"context_line":"            os.close(dirfd)"},{"line_number":587,"context_line":""},{"line_number":588,"context_line":""},{"line_number":589,"context_line":"def fsync_dirof(path):"},{"line_number":590,"context_line":"    \"\"\""},{"line_number":591,"context_line":"    Fsync on containing directory of path."},{"line_number":592,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"5a890539_881ef90b","line":589,"in_reply_to":"5a890539_93a1d697","updated":"2014-12-01 13:43:45.000000000","message":"Done","commit_id":"43130b422cbddcbbb7f3f07d146f8bded95a41ad"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@redhat.com","username":"cschwede"},"change_message_id":"bcec3633763003cde05b7be1ac502a1f6a43b51c","unresolved":false,"context_lines":[{"line_number":803,"context_line":"                raise"},{"line_number":804,"context_line":""},{"line_number":805,"context_line":""},{"line_number":806,"context_line":"def renamer(old, new, fsync_containing_dir\u003dTrue):"},{"line_number":807,"context_line":"    \"\"\""},{"line_number":808,"context_line":"    Attempt to fix / hide race conditions like empty object directories"},{"line_number":809,"context_line":"    being removed by backend processes during uploads, by retrying."}],"source_content_type":"text/x-python","patch_set":8,"id":"5a890539_53812efa","line":806,"updated":"2014-12-01 09:03:41.000000000","message":"I\u0027m wondering if \"False\" as default for fsync_containing_dir might be a better option? Doing this there is no change to the existing behaviour, and only 5 calls to rename need an additional argument instead of 8.","commit_id":"43130b422cbddcbbb7f3f07d146f8bded95a41ad"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@redhat.com","username":"cschwede"},"change_message_id":"1dfc61eb2a01b845163f1977d2fc5e8008ba908f","unresolved":false,"context_lines":[{"line_number":803,"context_line":"                raise"},{"line_number":804,"context_line":""},{"line_number":805,"context_line":""},{"line_number":806,"context_line":"def renamer(old, new, fsync_containing_dir\u003dTrue):"},{"line_number":807,"context_line":"    \"\"\""},{"line_number":808,"context_line":"    Attempt to fix / hide race conditions like empty object directories"},{"line_number":809,"context_line":"    being removed by backend processes during uploads, by retrying."}],"source_content_type":"text/x-python","patch_set":8,"id":"5a890539_a8c3b537","line":806,"in_reply_to":"5a890539_394c819d","updated":"2014-12-01 13:50:52.000000000","message":"Ah yes, thanks for the hint! Makes sense to me.","commit_id":"43130b422cbddcbbb7f3f07d146f8bded95a41ad"},{"author":{"_account_id":13777,"name":"Nicolas Trangez","email":"ikke@nicolast.be","username":"nicolast"},"change_message_id":"8d06cd07f3856bb56e2bb4807d76305627e95ce5","unresolved":false,"context_lines":[{"line_number":803,"context_line":"                raise"},{"line_number":804,"context_line":""},{"line_number":805,"context_line":""},{"line_number":806,"context_line":"def renamer(old, new, fsync_containing_dir\u003dTrue):"},{"line_number":807,"context_line":"    \"\"\""},{"line_number":808,"context_line":"    Attempt to fix / hide race conditions like empty object directories"},{"line_number":809,"context_line":"    being removed by backend processes during uploads, by retrying."}],"source_content_type":"text/x-python","patch_set":8,"id":"5a890539_394c819d","line":806,"in_reply_to":"5a890539_53812efa","updated":"2014-12-01 10:24:32.000000000","message":"There has been some discussion about this before in this review.\n\nI believe \u0027True\u0027 should be the default (as it is), since that makes the default \u0027safe\u0027/correct, as in: even though \u0027fsync\u0027 might not be required in some cases, it doesn\u0027t do any harm, so forgetting to explicitly disable it (overriding the default) is OK.\nWhen set to \u0027False\u0027 by default, one should make sure to always explicitly override the default in order to be \u0027safe\u0027/correct.\n\nI don\u0027t think the 5 vs. 8 doesn\u0027t play a role here: I\u0027d rather have more places where one consciously decides *not* to \u0027fsync\u0027 and as such override the default, rather than have more cases where one consciously decides to \u0027fsync\u0027, but to forget it in some other places (where it\u0027d be required for correctness).","commit_id":"43130b422cbddcbbb7f3f07d146f8bded95a41ad"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"bb2bb30d5a2f6435403ab9f768b4cd7b609742bd","unresolved":false,"context_lines":[{"line_number":819,"context_line":"    \"\"\""},{"line_number":820,"context_line":"    dirpath \u003d os.path.dirname(new)"},{"line_number":821,"context_line":"    try:"},{"line_number":822,"context_line":"        mkdirs(dirpath)"},{"line_number":823,"context_line":"        os.rename(old, new)"},{"line_number":824,"context_line":"    except OSError:"},{"line_number":825,"context_line":"        mkdirs(dirpath)"}],"source_content_type":"text/x-python","patch_set":8,"id":"5a890539_7f6c4d37","line":822,"updated":"2014-11-19 19:19:37.000000000","message":"If new \u003d /a/b/c/filename then mkdirs *may* create new dirs /a/b and /a/b/c BUT fsync_dir below will only fsync /a/b/c, meaning that /a/b/c may still be lost if failure causes /a/b to be lost.\n\nDon\u0027t we need mkdirs to return all new dirs that were created and then fsync them all ?","commit_id":"43130b422cbddcbbb7f3f07d146f8bded95a41ad"},{"author":{"_account_id":13777,"name":"Nicolas Trangez","email":"ikke@nicolast.be","username":"nicolast"},"change_message_id":"c8f10afad4cfa29c589b5bbf26a21c052a0fed25","unresolved":false,"context_lines":[{"line_number":819,"context_line":"    \"\"\""},{"line_number":820,"context_line":"    dirpath \u003d os.path.dirname(new)"},{"line_number":821,"context_line":"    try:"},{"line_number":822,"context_line":"        mkdirs(dirpath)"},{"line_number":823,"context_line":"        os.rename(old, new)"},{"line_number":824,"context_line":"    except OSError:"},{"line_number":825,"context_line":"        mkdirs(dirpath)"}],"source_content_type":"text/x-python","patch_set":8,"id":"5a890539_acbc896d","line":822,"in_reply_to":"5a890539_0bce67b4","updated":"2014-11-21 12:30:28.000000000","message":"I\u0027d also expect that in a sequence like\n\n- mkdir a\n\n- mkdir a/b\n\n- fsync(a/b)\n\n- fsync(a)\n\nthe last fsync is \u0027free\u0027.","commit_id":"43130b422cbddcbbb7f3f07d146f8bded95a41ad"},{"author":{"_account_id":9625,"name":"Thiago da Silva","email":"thiagodasilva@gmail.com","username":"thiago"},"change_message_id":"98ab110a9f9c03babb8c08c30d5d3e75124e04cc","unresolved":false,"context_lines":[{"line_number":819,"context_line":"    \"\"\""},{"line_number":820,"context_line":"    dirpath \u003d os.path.dirname(new)"},{"line_number":821,"context_line":"    try:"},{"line_number":822,"context_line":"        mkdirs(dirpath)"},{"line_number":823,"context_line":"        os.rename(old, new)"},{"line_number":824,"context_line":"    except OSError:"},{"line_number":825,"context_line":"        mkdirs(dirpath)"}],"source_content_type":"text/x-python","patch_set":8,"id":"5a890539_cc72cb10","line":822,"in_reply_to":"5a890539_69f8e97c","updated":"2014-11-20 21:33:44.000000000","message":"Hi Nicolas, this was more an implementation detail we noticed while testing. We did not look at xfs code, so you should definitely take it with a grain of salt.\n\nwe ran some simple tests on a shell using xfs tools: xfs_io (to fsync) and godown (to crash fs)","commit_id":"43130b422cbddcbbb7f3f07d146f8bded95a41ad"},{"author":{"_account_id":8542,"name":"Prashanth Pai","email":"ppai@redhat.com","username":"pai"},"change_message_id":"5138d61e39007e4cab7cdb351013f408c03da77a","unresolved":false,"context_lines":[{"line_number":819,"context_line":"    \"\"\""},{"line_number":820,"context_line":"    dirpath \u003d os.path.dirname(new)"},{"line_number":821,"context_line":"    try:"},{"line_number":822,"context_line":"        mkdirs(dirpath)"},{"line_number":823,"context_line":"        os.rename(old, new)"},{"line_number":824,"context_line":"    except OSError:"},{"line_number":825,"context_line":"        mkdirs(dirpath)"}],"source_content_type":"text/x-python","patch_set":8,"id":"5a890539_e58fbe2c","line":822,"in_reply_to":"5a890539_7f6c4d37","updated":"2014-11-20 06:05:16.000000000","message":"I had this doubt from the beginning. Unfortunately, I couldn\u0027t find a credible answer :( on what is \"good enough\".","commit_id":"43130b422cbddcbbb7f3f07d146f8bded95a41ad"},{"author":{"_account_id":9625,"name":"Thiago da Silva","email":"thiagodasilva@gmail.com","username":"thiago"},"change_message_id":"3c041cae252670e5e4bbb58363f406b4ebc7f99b","unresolved":false,"context_lines":[{"line_number":819,"context_line":"    \"\"\""},{"line_number":820,"context_line":"    dirpath \u003d os.path.dirname(new)"},{"line_number":821,"context_line":"    try:"},{"line_number":822,"context_line":"        mkdirs(dirpath)"},{"line_number":823,"context_line":"        os.rename(old, new)"},{"line_number":824,"context_line":"    except OSError:"},{"line_number":825,"context_line":"        mkdirs(dirpath)"}],"source_content_type":"text/x-python","patch_set":8,"id":"5a890539_e14dd3c5","line":822,"in_reply_to":"5a890539_7f6c4d37","updated":"2014-11-20 20:30:14.000000000","message":"I talked to Brian Foster, one of our xfs developers here at red hat, about this question and while he couldn\u0027t guarantee 100% without spending time looking at the code, we ran some tests and it is pretty safe to assume that if you only fsync /a/b/c then you would not lose /a/b in case of failure. It was easy to re-create the lost data without the fsync, but once we added the fsync to the containing directory, then the file and directories were written to disk, no data loss.\n\nOur logic is that if you got in this situation where c was fsynced but you lost /a/b then you would have a floating c directory with no links to it, and that would seem more like a filesystem bug. \nBTW: We ran theses tests on both XFS and EXT4 and the results were the same.\n\nHope this helps...","commit_id":"43130b422cbddcbbb7f3f07d146f8bded95a41ad"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"e4f46423c9aba1df850fd0c0067b9df3b085225a","unresolved":false,"context_lines":[{"line_number":819,"context_line":"    \"\"\""},{"line_number":820,"context_line":"    dirpath \u003d os.path.dirname(new)"},{"line_number":821,"context_line":"    try:"},{"line_number":822,"context_line":"        mkdirs(dirpath)"},{"line_number":823,"context_line":"        os.rename(old, new)"},{"line_number":824,"context_line":"    except OSError:"},{"line_number":825,"context_line":"        mkdirs(dirpath)"}],"source_content_type":"text/x-python","patch_set":8,"id":"5a890539_0bce67b4","line":822,"in_reply_to":"5a890539_a606731e","updated":"2014-11-21 11:33:37.000000000","message":"@Thiago interesting, thanks. I\u0027m left with same question that Nicolas asked: is that a POSIX requirement or a nice implementation? I have spent some time reading and not found an answer yet.\n\nIf we took a cautious approach and did fsync on every directory in a newly created branch, I\u0027m wondering if we can assume that any unnecessary/duplicate fsyncs would be no-cost (no IO that is).","commit_id":"43130b422cbddcbbb7f3f07d146f8bded95a41ad"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@redhat.com","username":"cschwede"},"change_message_id":"bcec3633763003cde05b7be1ac502a1f6a43b51c","unresolved":false,"context_lines":[{"line_number":819,"context_line":"    \"\"\""},{"line_number":820,"context_line":"    dirpath \u003d os.path.dirname(new)"},{"line_number":821,"context_line":"    try:"},{"line_number":822,"context_line":"        mkdirs(dirpath)"},{"line_number":823,"context_line":"        os.rename(old, new)"},{"line_number":824,"context_line":"    except OSError:"},{"line_number":825,"context_line":"        mkdirs(dirpath)"}],"source_content_type":"text/x-python","patch_set":8,"id":"5a890539_f3c13a4f","line":822,"in_reply_to":"5a890539_acbc896d","updated":"2014-12-01 09:03:41.000000000","message":"I did a few timing tests and the renamer() method showed an (expected) slowdown with fsync_containing_dir\u003dTrue. On average one call took about 400 microseconds with fsync_containing_dir\u003dTrue, but only 32 microseconds with fsync_containing_dir\u003dFalse (average over 100.000 rename() calls).\n\nHowever, a second fsync_dir on the parent directory took only a few microseconds longer, thus it is nearly \"free\".\n\nI would also add a second fsync_dir call because it takes only a fraction of the overall time.","commit_id":"43130b422cbddcbbb7f3f07d146f8bded95a41ad"},{"author":{"_account_id":13777,"name":"Nicolas Trangez","email":"ikke@nicolast.be","username":"nicolast"},"change_message_id":"697ecb85f1b7a8b7a0909b5a842b0dc839e1ad70","unresolved":false,"context_lines":[{"line_number":819,"context_line":"    \"\"\""},{"line_number":820,"context_line":"    dirpath \u003d os.path.dirname(new)"},{"line_number":821,"context_line":"    try:"},{"line_number":822,"context_line":"        mkdirs(dirpath)"},{"line_number":823,"context_line":"        os.rename(old, new)"},{"line_number":824,"context_line":"    except OSError:"},{"line_number":825,"context_line":"        mkdirs(dirpath)"}],"source_content_type":"text/x-python","patch_set":8,"id":"5a890539_69f8e97c","line":822,"in_reply_to":"5a890539_e14dd3c5","updated":"2014-11-20 20:56:09.000000000","message":"Is that a contract or an unwritten rule/current implementation detail? I seem to remember some changes in ext4 a couple of years ago in behaviour on which apps relied, but which was not \u0027by contract\u0027, and the changes broke apps accordingly.\n\nBTW, care to share your approach to testing this?","commit_id":"43130b422cbddcbbb7f3f07d146f8bded95a41ad"},{"author":{"_account_id":8542,"name":"Prashanth Pai","email":"ppai@redhat.com","username":"pai"},"change_message_id":"31595a4990d18f2501267a7bece5e0f0a12541d6","unresolved":false,"context_lines":[{"line_number":819,"context_line":"    \"\"\""},{"line_number":820,"context_line":"    dirpath \u003d os.path.dirname(new)"},{"line_number":821,"context_line":"    try:"},{"line_number":822,"context_line":"        mkdirs(dirpath)"},{"line_number":823,"context_line":"        os.rename(old, new)"},{"line_number":824,"context_line":"    except OSError:"},{"line_number":825,"context_line":"        mkdirs(dirpath)"}],"source_content_type":"text/x-python","patch_set":8,"id":"5a890539_a606731e","line":822,"in_reply_to":"5a890539_e14dd3c5","updated":"2014-11-21 05:24:00.000000000","message":"That\u0027s great Thiago! Thanks.","commit_id":"43130b422cbddcbbb7f3f07d146f8bded95a41ad"},{"author":{"_account_id":8542,"name":"Prashanth Pai","email":"ppai@redhat.com","username":"pai"},"change_message_id":"e127812d1b041d380ecadf55cc6cdc543e577aec","unresolved":false,"context_lines":[{"line_number":819,"context_line":"    \"\"\""},{"line_number":820,"context_line":"    dirpath \u003d os.path.dirname(new)"},{"line_number":821,"context_line":"    try:"},{"line_number":822,"context_line":"        mkdirs(dirpath)"},{"line_number":823,"context_line":"        os.rename(old, new)"},{"line_number":824,"context_line":"    except OSError:"},{"line_number":825,"context_line":"        mkdirs(dirpath)"}],"source_content_type":"text/x-python","patch_set":8,"id":"5a890539_6ddf1f3b","line":822,"in_reply_to":"5a890539_f3c13a4f","updated":"2014-12-01 13:42:36.000000000","message":"I\u0027m wondering about this case:\n\nDirectory \"a/b/c\" already exists. In Swift: (/\u003cdevice\u003e/objects-\u003csp_index\u003e/\u003cpartition-number\u003e/)\n\n    write(\"/tmp/whatever\")\n    fsync(\"/tmp/whatever\")\n    os.makedirs(\"a/b/c/d/e\")\n    rename(\"/tmp/whatever\", \"a/b/c/d/e/obj.data\")\n    fsync(\"a/b/c/d/e/\")\n\nIs it really required to fsync dirs all the way from e to a ? Fsync is totally not necessary for \"a/b/c\" as it already existed. But after doing a makedirs(), there\u0027s no way to know which subtree() of \"a/b/c/d/e\" needs fsync().\n\nIs it reasonable to fsync only the containing directory and expect the filesystem to take care of the rest ?","commit_id":"43130b422cbddcbbb7f3f07d146f8bded95a41ad"},{"author":{"_account_id":9625,"name":"Thiago da Silva","email":"thiagodasilva@gmail.com","username":"thiago"},"change_message_id":"88e309f7c5f55cfb124d4a517e4cd0cf82330a6f","unresolved":false,"context_lines":[{"line_number":817,"context_line":"        if tail \u003d\u003d os.path.curdir:"},{"line_number":818,"context_line":"            return"},{"line_number":819,"context_line":"    try:"},{"line_number":820,"context_line":"        os.mkdir(path)"},{"line_number":821,"context_line":"    except OSError as e:"},{"line_number":822,"context_line":"        if e.errno !\u003d errno.EEXIST or not os.path.isdir(path):"},{"line_number":823,"context_line":"            raise"}],"source_content_type":"text/x-python","patch_set":10,"id":"3a961159_84b22192","line":820,"updated":"2014-12-11 15:17:21.000000000","message":"how about changing the order, such that you try to create the path first, and only if that fails you try to create the parent (head). Seems like the typical case of LBYL vs. EAFP. Thoughts?","commit_id":"9f0423589e24a2fabeabf396a5a496e0e547faa2"},{"author":{"_account_id":8542,"name":"Prashanth Pai","email":"ppai@redhat.com","username":"pai"},"change_message_id":"c38353b5725dc1a7c420f0f958024b6458f21c86","unresolved":false,"context_lines":[{"line_number":817,"context_line":"        if tail \u003d\u003d os.path.curdir:"},{"line_number":818,"context_line":"            return"},{"line_number":819,"context_line":"    try:"},{"line_number":820,"context_line":"        os.mkdir(path)"},{"line_number":821,"context_line":"    except OSError as e:"},{"line_number":822,"context_line":"        if e.errno !\u003d errno.EEXIST or not os.path.isdir(path):"},{"line_number":823,"context_line":"            raise"}],"source_content_type":"text/x-python","patch_set":10,"id":"3a961159_c997dd8e","line":820,"in_reply_to":"3a961159_84b22192","updated":"2014-12-11 17:10:10.000000000","message":"I did run the swiftonfile\u0027s makedirs version against this on a larger sample set, it looks a little faster. https://gist.github.com/prashanthpai/fa441bf67f5cc36b68b9\n\nI\u0027m still unclear about the OSError handling in renamer(). The old code seems to retry on error to \"Attempt to fix / hide race conditions\"","commit_id":"9f0423589e24a2fabeabf396a5a496e0e547faa2"},{"author":{"_account_id":13777,"name":"Nicolas Trangez","email":"ikke@nicolast.be","username":"nicolast"},"change_message_id":"df41f2f1e8ea9325c5e8fda68ac137426e5994a5","unresolved":false,"context_lines":[{"line_number":817,"context_line":"        if tail \u003d\u003d os.path.curdir:"},{"line_number":818,"context_line":"            return"},{"line_number":819,"context_line":"    try:"},{"line_number":820,"context_line":"        os.mkdir(path)"},{"line_number":821,"context_line":"    except OSError as e:"},{"line_number":822,"context_line":"        if e.errno !\u003d errno.EEXIST or not os.path.isdir(path):"},{"line_number":823,"context_line":"            raise"}],"source_content_type":"text/x-python","patch_set":10,"id":"3a961159_49df6d0d","line":820,"in_reply_to":"3a961159_84b22192","updated":"2014-12-11 17:19:04.000000000","message":"The LBYL vs. EAFP question boils down to VM try/except block handling perf vs. syscall overhead ;-)\nI guess your suggestion to EAFP makes sense, especially because LBYL is a potential case of TOCTOU as well, making `mkdir` fail while we don\u0027t expect it to.\n\nEAFP +1","commit_id":"9f0423589e24a2fabeabf396a5a496e0e547faa2"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7e9b34d089beb97b54bcc68870051683a8193cbf","unresolved":false,"context_lines":[{"line_number":649,"context_line":"    \"\"\""},{"line_number":650,"context_line":"    Sync directory entries to disk."},{"line_number":651,"context_line":""},{"line_number":652,"context_line":"    :param dir: Path to the directory to be synced."},{"line_number":653,"context_line":"    \"\"\""},{"line_number":654,"context_line":"    dirfd \u003d None"},{"line_number":655,"context_line":"    try:"}],"source_content_type":"text/x-python","patch_set":15,"id":"ba7be1f8_c53c0932","line":652,"updated":"2015-03-03 16:56:46.000000000","message":"nit: param is dirpath","commit_id":"6e2ebb282a538e8aa477e90478b9bc477cec4291"},{"author":{"_account_id":8542,"name":"Prashanth Pai","email":"ppai@redhat.com","username":"pai"},"change_message_id":"c7feb862816536c1ea8cfa9efe0c88d9a634c909","unresolved":false,"context_lines":[{"line_number":649,"context_line":"    \"\"\""},{"line_number":650,"context_line":"    Sync directory entries to disk."},{"line_number":651,"context_line":""},{"line_number":652,"context_line":"    :param dir: Path to the directory to be synced."},{"line_number":653,"context_line":"    \"\"\""},{"line_number":654,"context_line":"    dirfd \u003d None"},{"line_number":655,"context_line":"    try:"}],"source_content_type":"text/x-python","patch_set":15,"id":"9a80dd14_0a5edbe9","line":652,"in_reply_to":"ba7be1f8_c53c0932","updated":"2015-03-04 07:25:07.000000000","message":"Done","commit_id":"6e2ebb282a538e8aa477e90478b9bc477cec4291"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7e9b34d089beb97b54bcc68870051683a8193cbf","unresolved":false,"context_lines":[{"line_number":891,"context_line":"    head, tail \u003d os.path.split(path)"},{"line_number":892,"context_line":"    if not tail:"},{"line_number":893,"context_line":"        head, tail \u003d os.path.split(head)"},{"line_number":894,"context_line":"    if head and tail and not os.path.exists(head):"},{"line_number":895,"context_line":"        try:"},{"line_number":896,"context_line":"            count \u003d makedirs_count(head, count)"},{"line_number":897,"context_line":"        except OSError as e:"}],"source_content_type":"text/x-python","patch_set":15,"id":"ba7be1f8_c63890f0","line":894,"updated":"2015-03-03 16:56:46.000000000","message":"do we need \u0027and not os.path.exists(head)\u0027 ? you allow for OSError \u003d\u003d errno.EEXIST, so why not just go ahead and try makedirs_count(head, count) and leave the except clause to handle?","commit_id":"6e2ebb282a538e8aa477e90478b9bc477cec4291"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"93bcf3f34d4cd9b42ddd84ec49d90ac16cd026eb","unresolved":false,"context_lines":[{"line_number":891,"context_line":"    head, tail \u003d os.path.split(path)"},{"line_number":892,"context_line":"    if not tail:"},{"line_number":893,"context_line":"        head, tail \u003d os.path.split(head)"},{"line_number":894,"context_line":"    if head and tail and not os.path.exists(head):"},{"line_number":895,"context_line":"        try:"},{"line_number":896,"context_line":"            count \u003d makedirs_count(head, count)"},{"line_number":897,"context_line":"        except OSError as e:"}],"source_content_type":"text/x-python","patch_set":15,"id":"9a80dd14_29b644dd","line":894,"in_reply_to":"9a80dd14_2dbbb9c5","updated":"2015-03-04 13:24:01.000000000","message":"OK, I see (thanks for the trace!), I think I got confused by line 898 and thinking the first recursion to makedirs_count would raise EEXIST but as you point out it won\u0027t but will continue to recurse.","commit_id":"6e2ebb282a538e8aa477e90478b9bc477cec4291"},{"author":{"_account_id":8542,"name":"Prashanth Pai","email":"ppai@redhat.com","username":"pai"},"change_message_id":"c7feb862816536c1ea8cfa9efe0c88d9a634c909","unresolved":false,"context_lines":[{"line_number":891,"context_line":"    head, tail \u003d os.path.split(path)"},{"line_number":892,"context_line":"    if not tail:"},{"line_number":893,"context_line":"        head, tail \u003d os.path.split(head)"},{"line_number":894,"context_line":"    if head and tail and not os.path.exists(head):"},{"line_number":895,"context_line":"        try:"},{"line_number":896,"context_line":"            count \u003d makedirs_count(head, count)"},{"line_number":897,"context_line":"        except OSError as e:"}],"source_content_type":"text/x-python","patch_set":15,"id":"9a80dd14_2dbbb9c5","line":894,"in_reply_to":"ba7be1f8_c63890f0","updated":"2015-03-04 07:25:07.000000000","message":"The extra check might do more good than harm and makes it efficient (in cases where part of path exists which would be the workload in swift xfs partition). When I trace the flow with example where path \u003d \"/a/b/c/d\" and case where directory \"/a/b/c\" already exists:\n\nhead \u003d \"/a/b/c\" and tail \u003d \"d\". With os.path.exists(\"/a/b/c\") in place, all that is left to do is call mkdir(\"/a/b/c/d\") directly.\n\nWithout os.path.exists(\"/a/b/c\"), it will enter recursive method again needlessly only to discover at some point later (after many failed mkdir) that it existed through EEXIST. You can verify this through strace (http://paste.openstack.org/show/187335/)\n\nHowever, I think, the check for EEXIST in line 898 is redundant. Line 898 would never get a EEXIST  as it\u0027s never raised in line 905 (unless path is a file). I\u0027ll remove EEXIST check at one of the two places.","commit_id":"6e2ebb282a538e8aa477e90478b9bc477cec4291"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7e9b34d089beb97b54bcc68870051683a8193cbf","unresolved":false,"context_lines":[{"line_number":924,"context_line":"    :param fsync: fsync on containing directory of new and also all"},{"line_number":925,"context_line":"                  the newly created directories."},{"line_number":926,"context_line":"    \"\"\""},{"line_number":927,"context_line":"    count \u003d 0"},{"line_number":928,"context_line":"    dirpath \u003d os.path.dirname(new)"},{"line_number":929,"context_line":"    try:"},{"line_number":930,"context_line":"        count \u003d makedirs_count(dirpath)"}],"source_content_type":"text/x-python","patch_set":15,"id":"ba7be1f8_668a8465","line":927,"updated":"2015-03-03 16:56:46.000000000","message":"nit: redundant declaration","commit_id":"6e2ebb282a538e8aa477e90478b9bc477cec4291"},{"author":{"_account_id":8542,"name":"Prashanth Pai","email":"ppai@redhat.com","username":"pai"},"change_message_id":"c7feb862816536c1ea8cfa9efe0c88d9a634c909","unresolved":false,"context_lines":[{"line_number":924,"context_line":"    :param fsync: fsync on containing directory of new and also all"},{"line_number":925,"context_line":"                  the newly created directories."},{"line_number":926,"context_line":"    \"\"\""},{"line_number":927,"context_line":"    count \u003d 0"},{"line_number":928,"context_line":"    dirpath \u003d os.path.dirname(new)"},{"line_number":929,"context_line":"    try:"},{"line_number":930,"context_line":"        count \u003d makedirs_count(dirpath)"}],"source_content_type":"text/x-python","patch_set":15,"id":"9a80dd14_4a58d3fa","line":927,"in_reply_to":"ba7be1f8_668a8465","updated":"2015-03-04 07:25:07.000000000","message":"Done","commit_id":"6e2ebb282a538e8aa477e90478b9bc477cec4291"}],"swift/obj/diskfile.py":[{"author":{"_account_id":6198,"name":"Peter Portante","email":"peter.a.portante@gmail.com","username":"peter-a-portante"},"change_message_id":"45196668ebc2c15de563eb549411349914f129e1","unresolved":false,"context_lines":[{"line_number":166,"context_line":"                  get_data_dir(extract_policy_index(corrupted_file_path)),"},{"line_number":167,"context_line":"                  basename(from_dir))"},{"line_number":168,"context_line":"    invalidate_hash(dirname(from_dir))"},{"line_number":169,"context_line":"    try:"},{"line_number":170,"context_line":"        renamer(from_dir, to_dir)"},{"line_number":171,"context_line":"    except OSError as e:"},{"line_number":172,"context_line":"        if e.errno not in (errno.EEXIST, errno.ENOTEMPTY):"}],"source_content_type":"text/x-python","patch_set":5,"id":"baa201ad_3b80b9f6","line":169,"updated":"2014-10-15 10:49:08.000000000","message":"As in db an db_replicator, I believe this parent of from_dir needs to be fsync\u0027d.","commit_id":"38fa4a87f0f5a392e17ef7acb89899e83b2b1842"},{"author":{"_account_id":6198,"name":"Peter Portante","email":"peter.a.portante@gmail.com","username":"peter-a-portante"},"change_message_id":"45196668ebc2c15de563eb549411349914f129e1","unresolved":false,"context_lines":[{"line_number":171,"context_line":"    except OSError as e:"},{"line_number":172,"context_line":"        if e.errno not in (errno.EEXIST, errno.ENOTEMPTY):"},{"line_number":173,"context_line":"            raise"},{"line_number":174,"context_line":"        to_dir \u003d \"%s-%s\" % (to_dir, uuid.uuid4().hex)"},{"line_number":175,"context_line":"        renamer(from_dir, to_dir)"},{"line_number":176,"context_line":"    return to_dir"},{"line_number":177,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"baa201ad_9b7a8522","line":174,"updated":"2014-10-15 10:49:08.000000000","message":"Same here...","commit_id":"38fa4a87f0f5a392e17ef7acb89899e83b2b1842"},{"author":{"_account_id":8542,"name":"Prashanth Pai","email":"ppai@redhat.com","username":"pai"},"change_message_id":"6bf21650323d305044be104e3815100110b4f34c","unresolved":false,"context_lines":[{"line_number":345,"context_line":"    suffix \u003d basename(suffix_dir)"},{"line_number":346,"context_line":"    partition_dir \u003d dirname(suffix_dir)"},{"line_number":347,"context_line":"    hashes_file \u003d join(partition_dir, HASH_FILE)"},{"line_number":348,"context_line":"    if not os.path.exists(hashes_file):"},{"line_number":349,"context_line":"        return"},{"line_number":350,"context_line":"    with lock_path(partition_dir):"},{"line_number":351,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":12,"id":"1a930d6b_56e3f406","line":348,"updated":"2015-01-22 05:59:22.000000000","message":"a premature mkdirs() which won\u0027t be fsync\u0027d was hidden in lock_path(). This change will avoid that. I hope it\u0027s okay to include that as part of this change although not strictly related. Saves a few sys calls: http://paste.openstack.org/show/160106/","commit_id":"b01622f9adef6570085a1c37accd87e90ad4724e"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7e9b34d089beb97b54bcc68870051683a8193cbf","unresolved":false,"context_lines":[{"line_number":345,"context_line":"    suffix \u003d basename(suffix_dir)"},{"line_number":346,"context_line":"    partition_dir \u003d dirname(suffix_dir)"},{"line_number":347,"context_line":"    hashes_file \u003d join(partition_dir, HASH_FILE)"},{"line_number":348,"context_line":"    if not os.path.exists(hashes_file):"},{"line_number":349,"context_line":"        return"},{"line_number":350,"context_line":"    with lock_path(partition_dir):"},{"line_number":351,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":15,"id":"ba7be1f8_691a57c7","line":348,"updated":"2015-03-03 16:56:46.000000000","message":"i *think* this is ok but it is moving a test outside of the lock scope","commit_id":"6e2ebb282a538e8aa477e90478b9bc477cec4291"}],"test/unit/common/test_db_replicator.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7e9b34d089beb97b54bcc68870051683a8193cbf","unresolved":false,"context_lines":[{"line_number":564,"context_line":"                    \u0027get_repl_missing_table\u0027, True)"},{"line_number":565,"context_line":""},{"line_number":566,"context_line":"        def mock_renamer(was, new, cause_colision\u003dFalse,"},{"line_number":567,"context_line":"                         fsync\u003dFalse):"},{"line_number":568,"context_line":"            if cause_colision and \u0027-\u0027 not in new:"},{"line_number":569,"context_line":"                raise OSError(errno.EEXIST, \"File already exists\")"},{"line_number":570,"context_line":"            self.assertEquals(\u0027/a/b/c/d/e\u0027, was)"}],"source_content_type":"text/x-python","patch_set":15,"id":"ba7be1f8_098abbbe","line":567,"updated":"2015-03-03 16:56:46.000000000","message":"fsync should be third arg","commit_id":"6e2ebb282a538e8aa477e90478b9bc477cec4291"},{"author":{"_account_id":8542,"name":"Prashanth Pai","email":"ppai@redhat.com","username":"pai"},"change_message_id":"c7feb862816536c1ea8cfa9efe0c88d9a634c909","unresolved":false,"context_lines":[{"line_number":564,"context_line":"                    \u0027get_repl_missing_table\u0027, True)"},{"line_number":565,"context_line":""},{"line_number":566,"context_line":"        def mock_renamer(was, new, cause_colision\u003dFalse,"},{"line_number":567,"context_line":"                         fsync\u003dFalse):"},{"line_number":568,"context_line":"            if cause_colision and \u0027-\u0027 not in new:"},{"line_number":569,"context_line":"                raise OSError(errno.EEXIST, \"File already exists\")"},{"line_number":570,"context_line":"            self.assertEquals(\u0027/a/b/c/d/e\u0027, was)"}],"source_content_type":"text/x-python","patch_set":15,"id":"9a80dd14_6a5d97e9","line":567,"in_reply_to":"ba7be1f8_098abbbe","updated":"2015-03-04 07:25:07.000000000","message":"Done","commit_id":"6e2ebb282a538e8aa477e90478b9bc477cec4291"}],"test/unit/common/test_utils.py":[{"author":{"_account_id":13777,"name":"Nicolas Trangez","email":"ikke@nicolast.be","username":"nicolast"},"change_message_id":"73cce8644e62a5a4a3966ca7e2722491775b9db4","unresolved":false,"context_lines":[{"line_number":2771,"context_line":"            self.assertEqual(1, len(logger.get_lines_for_level(\u0027warning\u0027)))"},{"line_number":2772,"context_line":""},{"line_number":2773,"context_line":"        finally:"},{"line_number":2774,"context_line":"            os.close(fd)"},{"line_number":2775,"context_line":"            os.unlink(temppath)"},{"line_number":2776,"context_line":"            os.rmdir(tempdir)"},{"line_number":2777,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"5a890539_0ccfe656","line":2774,"updated":"2014-11-18 09:56:08.000000000","message":"This \u0027finally\u0027 block could (in theory) cause trouble if `mkdtemp` or `mkstemp` failed, hiding the original exception.\n\nConsider using something like\n\n tempdir \u003d mkdtemp(dir\u003d\u0027/tmp\u0027)\n fd \u003d None\n\n try:\n     fd, temppath \u003d tempfile.mkstemp(dir\u003dtempdir)\n     ...\n finally:\n     if fd is not None: # Could be 0, in theory\n         os.close(fd)\n         os.unlink(temppath)\n     os.rmdir(tempdir)\n\nOr, if I\u0027m not mistaken, some `tempfile` things support the context manager protocol, unlinking a created file after execution of the managed block, which would allow to get rid of some finally handling.","commit_id":"786725183c6b833a3ea9822539fa2d99eab89bdf"},{"author":{"_account_id":8542,"name":"Prashanth Pai","email":"ppai@redhat.com","username":"pai"},"change_message_id":"f863f51f6e8973b9f6f0c6d5cc7063798b61d17f","unresolved":false,"context_lines":[{"line_number":2771,"context_line":"            self.assertEqual(1, len(logger.get_lines_for_level(\u0027warning\u0027)))"},{"line_number":2772,"context_line":""},{"line_number":2773,"context_line":"        finally:"},{"line_number":2774,"context_line":"            os.close(fd)"},{"line_number":2775,"context_line":"            os.unlink(temppath)"},{"line_number":2776,"context_line":"            os.rmdir(tempdir)"},{"line_number":2777,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"5a890539_c77e9969","line":2774,"in_reply_to":"5a890539_0ccfe656","updated":"2014-11-18 10:13:56.000000000","message":"If mkdtemp or mkstemp fails, it means there\u0027s something really wrong going on, many unit tests would fail. It\u0027s very very unlikely to happen. But, I\u0027m going to add suggested code anyways as it does no harm.","commit_id":"786725183c6b833a3ea9822539fa2d99eab89bdf"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7e9b34d089beb97b54bcc68870051683a8193cbf","unresolved":false,"context_lines":[{"line_number":2845,"context_line":"            if tempdir:"},{"line_number":2846,"context_line":"                os.rmdir(tempdir)"},{"line_number":2847,"context_line":""},{"line_number":2848,"context_line":"    def test_renamer_with_fsync_dir(self):"},{"line_number":2849,"context_line":"        tempdir \u003d None"},{"line_number":2850,"context_line":"        try:"},{"line_number":2851,"context_line":"            tempdir \u003d mkdtemp(dir\u003d\u0027/tmp\u0027)"}],"source_content_type":"text/x-python","patch_set":15,"id":"ba7be1f8_8c7a39ef","line":2848,"updated":"2015-03-03 16:56:46.000000000","message":"could you add a test that verifies fsync is NOT called when fsync\u003dFalse is passed to renamer?","commit_id":"6e2ebb282a538e8aa477e90478b9bc477cec4291"},{"author":{"_account_id":8542,"name":"Prashanth Pai","email":"ppai@redhat.com","username":"pai"},"change_message_id":"c7feb862816536c1ea8cfa9efe0c88d9a634c909","unresolved":false,"context_lines":[{"line_number":2845,"context_line":"            if tempdir:"},{"line_number":2846,"context_line":"                os.rmdir(tempdir)"},{"line_number":2847,"context_line":""},{"line_number":2848,"context_line":"    def test_renamer_with_fsync_dir(self):"},{"line_number":2849,"context_line":"        tempdir \u003d None"},{"line_number":2850,"context_line":"        try:"},{"line_number":2851,"context_line":"            tempdir \u003d mkdtemp(dir\u003d\u0027/tmp\u0027)"}],"source_content_type":"text/x-python","patch_set":15,"id":"9a80dd14_6a2bd730","line":2848,"in_reply_to":"ba7be1f8_8c7a39ef","updated":"2015-03-04 07:25:07.000000000","message":"Done","commit_id":"6e2ebb282a538e8aa477e90478b9bc477cec4291"}]}