)]}'
{"swift/common/db_replicator.py":[{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"21e05747f43e0da4e80962d99b4ed102e528f6e5","unresolved":false,"context_lines":[{"line_number":106,"context_line":"                    if os.path.exists(object_file):"},{"line_number":107,"context_line":"                        yield (partition, object_file, node_id)"},{"line_number":108,"context_line":"                    else:"},{"line_number":109,"context_line":"                        os.rmdir(hash_dir)"},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"    its \u003d [walk_datadir(datadir, node_id) for datadir, node_id in datadirs]"},{"line_number":112,"context_line":"    while its:"}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_662a55e9","line":109,"updated":"2016-07-23 20:39:37.000000000","message":"Would it be better to error on the cautious side, and only remove the hash_dir if it is empty?  Let\u0027s say, there is a change later to object_file names (ok, probably never happen) and the backwards compatibility logic has an error.  We\u0027d risk client data being removed in that case.  It\u0027s highly unlikely, but it seems easy enough to error on the side of caution here and only remove if empty.","commit_id":"82efb8647e09ac593bef5f0ef995a043a1ab7707"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"0ac797fd7f2f009a4a9b6a271f957777b1744d5c","unresolved":false,"context_lines":[{"line_number":106,"context_line":"                    if os.path.exists(object_file):"},{"line_number":107,"context_line":"                        yield (partition, object_file, node_id)"},{"line_number":108,"context_line":"                    else:"},{"line_number":109,"context_line":"                        os.rmdir(hash_dir)"},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"    its \u003d [walk_datadir(datadir, node_id) for datadir, node_id in datadirs]"},{"line_number":112,"context_line":"    while its:"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a95cdbc_08183474","line":109,"in_reply_to":"1a95cdbc_84e0ba2a","updated":"2016-10-10 11:10:42.000000000","message":"Thanks Janie. I think I\u0027m gonna add up the try block I\u0027ve mentioned earlier in this patch itself and will also modify the test.\n\nI looked at your patch, two things that occurred to me:\n1) I don\u0027t think we need a separate test for try block change, since it has a lot of duplicate code.\n2) Also in your linked patch, the final assertion is not quite related to the tempfile or the try/catch. I left a comment on the patch.","commit_id":"82efb8647e09ac593bef5f0ef995a043a1ab7707"},{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"5244be9c0fbbdc1155e6536f1a8369ae59c79178","unresolved":false,"context_lines":[{"line_number":106,"context_line":"                    if os.path.exists(object_file):"},{"line_number":107,"context_line":"                        yield (partition, object_file, node_id)"},{"line_number":108,"context_line":"                    else:"},{"line_number":109,"context_line":"                        os.rmdir(hash_dir)"},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"    its \u003d [walk_datadir(datadir, node_id) for datadir, node_id in datadirs]"},{"line_number":112,"context_line":"    while its:"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a95cdbc_84e0ba2a","line":109,"in_reply_to":"1a95cdbc_9e5035d6","updated":"2016-10-10 04:42:47.000000000","message":"I uploaded a patch that you could squash into here if you like.  https://review.openstack.org/384284","commit_id":"82efb8647e09ac593bef5f0ef995a043a1ab7707"},{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"8fcde9674f651b6343c31da0d626eeda98f3f398","unresolved":false,"context_lines":[{"line_number":106,"context_line":"                    if os.path.exists(object_file):"},{"line_number":107,"context_line":"                        yield (partition, object_file, node_id)"},{"line_number":108,"context_line":"                    else:"},{"line_number":109,"context_line":"                        os.rmdir(hash_dir)"},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"    its \u003d [walk_datadir(datadir, node_id) for datadir, node_id in datadirs]"},{"line_number":112,"context_line":"    while its:"}],"source_content_type":"text/x-python","patch_set":2,"id":"bacf61ea_7c1d0166","line":109,"in_reply_to":"bacf61ea_2ef993c9","updated":"2016-07-27 15:19:04.000000000","message":"Correct, this is when there is no file with the expected object name.  Again, it might be over cautious, but I am just thinking that if there *were* a file there that wasn\u0027t the expected object file name, maybe we should keep it there just in case.","commit_id":"82efb8647e09ac593bef5f0ef995a043a1ab7707"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"3ea5368e72f03185e224d35cbbfc339c5030b2c1","unresolved":false,"context_lines":[{"line_number":106,"context_line":"                    if os.path.exists(object_file):"},{"line_number":107,"context_line":"                        yield (partition, object_file, node_id)"},{"line_number":108,"context_line":"                    else:"},{"line_number":109,"context_line":"                        os.rmdir(hash_dir)"},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"    its \u003d [walk_datadir(datadir, node_id) for datadir, node_id in datadirs]"},{"line_number":112,"context_line":"    while its:"}],"source_content_type":"text/x-python","patch_set":2,"id":"bacf61ea_8a4e1d6d","line":109,"in_reply_to":"bacf61ea_7c1d0166","updated":"2016-08-04 17:01:21.000000000","message":"hmm, I\u0027m not sure if it applies here, since we\u0027re scouting files with a specific name (line 105). I can\u0027t yet think of a scenario where there could be a file with different name :\\ Will think more about this.","commit_id":"82efb8647e09ac593bef5f0ef995a043a1ab7707"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9dab8f242edad30467bfc3811b7bf576c79e7c7f","unresolved":false,"context_lines":[{"line_number":106,"context_line":"                    if os.path.exists(object_file):"},{"line_number":107,"context_line":"                        yield (partition, object_file, node_id)"},{"line_number":108,"context_line":"                    else:"},{"line_number":109,"context_line":"                        os.rmdir(hash_dir)"},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"    its \u003d [walk_datadir(datadir, node_id) for datadir, node_id in datadirs]"},{"line_number":112,"context_line":"    while its:"}],"source_content_type":"text/x-python","patch_set":2,"id":"bacf61ea_ea935959","line":109,"in_reply_to":"bacf61ea_8a4e1d6d","updated":"2016-08-04 17:13:27.000000000","message":"the \"bug\" as it were was if there was no .db file, but the directory was not empty this rmdir call would raise ENOTEMPTY and blow up the loop :\\\n\nWe could probably prove that with a unittest....","commit_id":"82efb8647e09ac593bef5f0ef995a043a1ab7707"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"cae685e03df9134eeafc49903a76fb8f58a3bc52","unresolved":false,"context_lines":[{"line_number":106,"context_line":"                    if os.path.exists(object_file):"},{"line_number":107,"context_line":"                        yield (partition, object_file, node_id)"},{"line_number":108,"context_line":"                    else:"},{"line_number":109,"context_line":"                        os.rmdir(hash_dir)"},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"    its \u003d [walk_datadir(datadir, node_id) for datadir, node_id in datadirs]"},{"line_number":112,"context_line":"    while its:"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a95cdbc_9e5035d6","line":109,"in_reply_to":"bacf61ea_ea935959","updated":"2016-10-07 13:49:24.000000000","message":"We currently mock any such error (OSError etc) in unit tests. But i\u0027m not sure if it\u0027d be correct for this case since we\u0027re not catching the error in the code i.e. in db_replicator.py.\n\nBut maybe I could wrap the rmdir calls with a try catch block and then write some unit tests for those calls.","commit_id":"82efb8647e09ac593bef5f0ef995a043a1ab7707"},{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"72a510138f1400655ea2d48fc18c652c970d2e70","unresolved":false,"context_lines":[{"line_number":106,"context_line":"                    if os.path.exists(object_file):"},{"line_number":107,"context_line":"                        yield (partition, object_file, node_id)"},{"line_number":108,"context_line":"                    else:"},{"line_number":109,"context_line":"                        os.rmdir(hash_dir)"},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"    its \u003d [walk_datadir(datadir, node_id) for datadir, node_id in datadirs]"},{"line_number":112,"context_line":"    while its:"}],"source_content_type":"text/x-python","patch_set":2,"id":"bacf61ea_5049725f","line":109,"in_reply_to":"bacf61ea_ea935959","updated":"2016-08-04 17:41:19.000000000","message":"Yes, I had wondered about what would happen - if the remove would just go through or not.  Agreed, it\u0027s be good if we had a test asserting the behavior we expect in a case where the directory is not empty, but does not have the specific file we are seeking.","commit_id":"82efb8647e09ac593bef5f0ef995a043a1ab7707"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"04c7b0802011bba805031a9773ba2685990d3747","unresolved":false,"context_lines":[{"line_number":106,"context_line":"                    if os.path.exists(object_file):"},{"line_number":107,"context_line":"                        yield (partition, object_file, node_id)"},{"line_number":108,"context_line":"                    else:"},{"line_number":109,"context_line":"                        os.rmdir(hash_dir)"},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"    its \u003d [walk_datadir(datadir, node_id) for datadir, node_id in datadirs]"},{"line_number":112,"context_line":"    while its:"}],"source_content_type":"text/x-python","patch_set":2,"id":"bacf61ea_2ef993c9","line":109,"in_reply_to":"dada55a8_662a55e9","updated":"2016-07-27 14:29:44.000000000","message":"The else case here is when there are is no object file, so the scenario you have pointed would not occur.","commit_id":"82efb8647e09ac593bef5f0ef995a043a1ab7707"},{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"307f0c71b9f8b23b6c3e4939cc93ba2ab9d65fab","unresolved":false,"context_lines":[{"line_number":109,"context_line":"                        try:"},{"line_number":110,"context_line":"                            os.rmdir(hash_dir)"},{"line_number":111,"context_line":"                        except OSError as e:"},{"line_number":112,"context_line":"                            if e.errno is errno.ENOTEMPTY:"},{"line_number":113,"context_line":"                                raise"},{"line_number":114,"context_line":"            # remove empty partitions after the above directory walk"},{"line_number":115,"context_line":"            if not suffixes:"}],"source_content_type":"text/x-python","patch_set":6,"id":"7a77a97e_65afad94","line":112,"updated":"2016-11-16 03:49:59.000000000","message":"Ah, I think I didn\u0027t thoroughly explain where I was headed with these suggestions.  I do think the raise should only happen if the error is something other than being non-empty.  See my comment in the test code for an explanation.","commit_id":"c1127328ea96ef576be3f2fb2443009a610189e9"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"27ebc43ea4ad26bbee77082533bf036b7618f176","unresolved":false,"context_lines":[{"line_number":109,"context_line":"                        try:"},{"line_number":110,"context_line":"                            os.rmdir(hash_dir)"},{"line_number":111,"context_line":"                        except OSError as e:"},{"line_number":112,"context_line":"                            if e.errno is errno.ENOTEMPTY:"},{"line_number":113,"context_line":"                                raise"},{"line_number":114,"context_line":"            # remove empty partitions after the above directory walk"},{"line_number":115,"context_line":"            if not suffixes:"}],"source_content_type":"text/x-python","patch_set":6,"id":"7a77a97e_48f488f1","line":112,"in_reply_to":"7a77a97e_65afad94","updated":"2016-11-16 04:41:25.000000000","message":"Yeah, I\u0027ve seen this being done in other parts of the code as well. makes sense, will change it back.","commit_id":"c1127328ea96ef576be3f2fb2443009a610189e9"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"68580e280928cfa17557548533f463cea5821831","unresolved":false,"context_lines":[{"line_number":97,"context_line":"                for hsh in hashes:"},{"line_number":98,"context_line":"                    hash_dir \u003d os.path.join(suff_dir, hsh)"},{"line_number":99,"context_line":"                    if not os.path.isdir(hash_dir):"},{"line_number":100,"context_line":"                        continue"},{"line_number":101,"context_line":"                    object_file \u003d os.path.join(hash_dir, hsh + \u0027.db\u0027)"},{"line_number":102,"context_line":"                    if os.path.exists(object_file):"},{"line_number":103,"context_line":"                        yield (partition, object_file, node_id)"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a74a57a_0a7c3471","side":"PARENT","line":100,"updated":"2016-12-01 06:30:28.000000000","message":"this continue is problematic for me (really the whole LBYL isdir calls are suboptimal IMHO)\n\nif hash_dir is a file - it seems contrary to the stated goa of the change which is to clean up a mess left behind be garbage under the data path - i think if we merged this with the closes bug tag we\u0027d have to open a new bug to track cleaning out empty suffix dirs that have only a file in them.","commit_id":"d7920324f63d27587fe4648b43d2394a35f025d7"},{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"b9f23ed78db8d78cfd1eb1643ca83c627a7905ae","unresolved":false,"context_lines":[{"line_number":97,"context_line":"                for hsh in hashes:"},{"line_number":98,"context_line":"                    hash_dir \u003d os.path.join(suff_dir, hsh)"},{"line_number":99,"context_line":"                    if not os.path.isdir(hash_dir):"},{"line_number":100,"context_line":"                        continue"},{"line_number":101,"context_line":"                    object_file \u003d os.path.join(hash_dir, hsh + \u0027.db\u0027)"},{"line_number":102,"context_line":"                    if os.path.exists(object_file):"},{"line_number":103,"context_line":"                        yield (partition, object_file, node_id)"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_953fb724","side":"PARENT","line":100,"in_reply_to":"3a71b18c_3fbfa4d1","updated":"2016-12-01 20:20:48.000000000","message":"@Tim we added the try/catch around a different rmdir call for this reason when checking for .db files.  If there is a file present that doesn\u0027t end in .db, then it and its directory will remain, but there will not be an error raised.  I expect we could follow that pattern here as well.","commit_id":"d7920324f63d27587fe4648b43d2394a35f025d7"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"89ecc663151b970e477c81f8ce9a164f8a4d5b35","unresolved":false,"context_lines":[{"line_number":97,"context_line":"                for hsh in hashes:"},{"line_number":98,"context_line":"                    hash_dir \u003d os.path.join(suff_dir, hsh)"},{"line_number":99,"context_line":"                    if not os.path.isdir(hash_dir):"},{"line_number":100,"context_line":"                        continue"},{"line_number":101,"context_line":"                    object_file \u003d os.path.join(hash_dir, hsh + \u0027.db\u0027)"},{"line_number":102,"context_line":"                    if os.path.exists(object_file):"},{"line_number":103,"context_line":"                        yield (partition, object_file, node_id)"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_3fbfa4d1","side":"PARENT","line":100,"in_reply_to":"5a74a57a_0a7c3471","updated":"2016-12-01 19:59:48.000000000","message":"But what are we *supposed* to do with some unexpected file? An empty directory should be safe to delete; I agree there. But some arbitrary file? I have no idea what that means or whether it\u0027s important.","commit_id":"d7920324f63d27587fe4648b43d2394a35f025d7"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"68580e280928cfa17557548533f463cea5821831","unresolved":false,"context_lines":[{"line_number":96,"context_line":"                    continue"},{"line_number":97,"context_line":"                hashes \u003d os.listdir(suff_dir)"},{"line_number":98,"context_line":"                if not hashes:"},{"line_number":99,"context_line":"                    os.rmdir(suff_dir)"},{"line_number":100,"context_line":"                    continue"},{"line_number":101,"context_line":"                for hsh in hashes:"},{"line_number":102,"context_line":"                    hash_dir \u003d os.path.join(suff_dir, hsh)"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a74a57a_250cc841","line":99,"updated":"2016-12-01 06:30:28.000000000","message":"it seems like in a race any one of these calls to rmdir could raise an ENOTEMPTY","commit_id":"321bb9145f9e2e184994e5d0cf78c62b6043112c"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"89ecc663151b970e477c81f8ce9a164f8a4d5b35","unresolved":false,"context_lines":[{"line_number":96,"context_line":"                    continue"},{"line_number":97,"context_line":"                hashes \u003d os.listdir(suff_dir)"},{"line_number":98,"context_line":"                if not hashes:"},{"line_number":99,"context_line":"                    os.rmdir(suff_dir)"},{"line_number":100,"context_line":"                    continue"},{"line_number":101,"context_line":"                for hsh in hashes:"},{"line_number":102,"context_line":"                    hash_dir \u003d os.path.join(suff_dir, hsh)"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_0d59e55a","line":99,"in_reply_to":"5a74a57a_250cc841","updated":"2016-12-01 19:59:48.000000000","message":"This seems like a fairly big deal to me. Am I missing something? A concurrent container PUT kinda makes me want to -1 -- it\u0027s a traceback waiting to happen.\n\nWhat about the flip side of the race? Tolerating an ENOTEMPTY here addresses the\n\n* container-server calls mkdirs(hash_dir)\n* replicator calls listdir(suff_dir)\n* replicator calls listdir(hash_dir)\n* container-server creates .lock file and finishes acquiring its lock\n* replicator calls rmdir(hash_dir)\n\ncase, but what happens in the container-server if things get ordered as\n\n* container-server calls mkdirs(hash_dir)\n* replicator calls listdir(suff_dir)\n* replicator calls listdir(hash_dir)\n* replicator calls rmdir(hash_dir)\n* container-server creates .lock file\n\n?","commit_id":"321bb9145f9e2e184994e5d0cf78c62b6043112c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"68580e280928cfa17557548533f463cea5821831","unresolved":false,"context_lines":[{"line_number":113,"context_line":"                                raise"},{"line_number":114,"context_line":"            # remove empty partitions after the above directory walk"},{"line_number":115,"context_line":"            if not suffixes:"},{"line_number":116,"context_line":"                os.rmdir(part_dir)"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"    its \u003d [walk_datadir(datadir, node_id) for datadir, node_id in datadirs]"},{"line_number":119,"context_line":"    while its:"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a74a57a_c565d41d","line":116,"updated":"2016-12-01 06:30:28.000000000","message":"this code is baffling to me\n\ndo we *change* suffixes between line 89 and 115 (!?)\n\nwhy would the results here be any different than the check at line 90 (!?)\n\nif not suffixes, we should not reach this line (!?)","commit_id":"321bb9145f9e2e184994e5d0cf78c62b6043112c"},{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"b9f23ed78db8d78cfd1eb1643ca83c627a7905ae","unresolved":false,"context_lines":[{"line_number":113,"context_line":"                                raise"},{"line_number":114,"context_line":"            # remove empty partitions after the above directory walk"},{"line_number":115,"context_line":"            if not suffixes:"},{"line_number":116,"context_line":"                os.rmdir(part_dir)"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"    its \u003d [walk_datadir(datadir, node_id) for datadir, node_id in datadirs]"},{"line_number":119,"context_line":"    while its:"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_9502573d","line":116,"in_reply_to":"3a71b18c_9f4b582b","updated":"2016-12-01 20:20:48.000000000","message":"+1 on letting the next time through do clean up.","commit_id":"321bb9145f9e2e184994e5d0cf78c62b6043112c"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"89ecc663151b970e477c81f8ce9a164f8a4d5b35","unresolved":false,"context_lines":[{"line_number":113,"context_line":"                                raise"},{"line_number":114,"context_line":"            # remove empty partitions after the above directory walk"},{"line_number":115,"context_line":"            if not suffixes:"},{"line_number":116,"context_line":"                os.rmdir(part_dir)"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"    its \u003d [walk_datadir(datadir, node_id) for datadir, node_id in datadirs]"},{"line_number":119,"context_line":"    while its:"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_9f4b582b","line":116,"in_reply_to":"5a74a57a_88b92c31","updated":"2016-12-01 19:59:48.000000000","message":"Mahati, since we don\u0027t go list the directory again, the suffixes var will still be the same. Maybe it\u0027s worth pulling out\n\n def rmdir_if_empty(dir_name):\n     try:\n         os.rmdir(dir_name)\n     except OSError as e:\n         if e.errno !\u003d errno.ENOTEMPTY:\n             raise\n\nas a separate helper, then calling rmdir_if_empty(part_dir) here? Or we could follow Clay\u0027s suggestion in the follow-up; leave it alone for now, and expect that the next time through it will get cleaned up.","commit_id":"321bb9145f9e2e184994e5d0cf78c62b6043112c"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"4b7dc24d1ed8b58061b3ea2a36e70ca2d72aff75","unresolved":false,"context_lines":[{"line_number":113,"context_line":"                                raise"},{"line_number":114,"context_line":"            # remove empty partitions after the above directory walk"},{"line_number":115,"context_line":"            if not suffixes:"},{"line_number":116,"context_line":"                os.rmdir(part_dir)"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"    its \u003d [walk_datadir(datadir, node_id) for datadir, node_id in datadirs]"},{"line_number":119,"context_line":"    while its:"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a74a57a_88b92c31","line":116,"in_reply_to":"5a74a57a_c565d41d","updated":"2016-12-01 07:00:26.000000000","message":"I wanted to get reviews on this part.\n\nSo after line#99 above, we might end up with an empty suffix directory again, hence this check made sense, correct me if I\u0027m missing something.","commit_id":"321bb9145f9e2e184994e5d0cf78c62b6043112c"}],"test/unit/common/test_db_replicator.py":[{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"56b74e7ae60cd77fa65c67ca59d32953d0747493","unresolved":false,"context_lines":[{"line_number":1112,"context_line":"        def _rmdir(arg):"},{"line_number":1113,"context_line":"            rmdir_calls.append(arg)"},{"line_number":1114,"context_line":"            if arg \u003d\u003d dir_with_existing_file:"},{"line_number":1115,"context_line":"                orig_rmdir(tempdir)"},{"line_number":1116,"context_line":""},{"line_number":1117,"context_line":"        orig_listdir \u003d db_replicator.os.listdir"},{"line_number":1118,"context_line":"        orig_isdir \u003d db_replicator.os.path.isdir"}],"source_content_type":"text/x-python","patch_set":5,"id":"1a95cdbc_b3f07a68","line":1115,"updated":"2016-10-10 18:51:08.000000000","message":"adding \"self.fail(\u0027The rmdir did not raise an exception as expected\u0027)\" shows that the rmdir used in db_replicator did not remove the non-empty directory.  Also, it proves that we did exercise the non-empty directory case.  Without this,  future us might remove the writing of the existing file and then miss that this case may no longer being tested.  Especially with no comment about why we are writing the file to tempdir.","commit_id":"d5cad63d182ef140d3d131bda29894a3fb8650ce"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"0031a5597e23f1a97eaeb4dc8ec829724488df5e","unresolved":false,"context_lines":[{"line_number":1112,"context_line":"        def _rmdir(arg):"},{"line_number":1113,"context_line":"            rmdir_calls.append(arg)"},{"line_number":1114,"context_line":"            if arg \u003d\u003d dir_with_existing_file:"},{"line_number":1115,"context_line":"                orig_rmdir(tempdir)"},{"line_number":1116,"context_line":""},{"line_number":1117,"context_line":"        orig_listdir \u003d db_replicator.os.listdir"},{"line_number":1118,"context_line":"        orig_isdir \u003d db_replicator.os.path.isdir"}],"source_content_type":"text/x-python","patch_set":5,"id":"7a77a97e_71d94821","line":1115,"in_reply_to":"1a95cdbc_b3f07a68","updated":"2016-11-15 16:58:14.000000000","message":"If you observe, code never reaches beyond this line - you can attempt this by adding a dummy at line#1116 self.assertEqual(\u0027a\u0027, \u0027b\u0027) and the test still passes.\n\nThis is because it never reaches the next line as the code in db_replicator is not catching the correct error. I have corrected this in my next patch.","commit_id":"d5cad63d182ef140d3d131bda29894a3fb8650ce"},{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"5183abfe4839d816b4ba0f79edf0a96500bd171c","unresolved":false,"context_lines":[{"line_number":1112,"context_line":"        def _rmdir(arg):"},{"line_number":1113,"context_line":"            rmdir_calls.append(arg)"},{"line_number":1114,"context_line":"            if arg \u003d\u003d dir_with_existing_file:"},{"line_number":1115,"context_line":"                orig_rmdir(tempdir)"},{"line_number":1116,"context_line":""},{"line_number":1117,"context_line":"        orig_listdir \u003d db_replicator.os.listdir"},{"line_number":1118,"context_line":"        orig_isdir \u003d db_replicator.os.path.isdir"}],"source_content_type":"text/x-python","patch_set":5,"id":"7a77a97e_05b7d9d8","line":1115,"in_reply_to":"7a77a97e_71d94821","updated":"2016-11-16 03:53:09.000000000","message":"Yes, the code should never reach line 1115. that is why I am suggesting that we add the self.fail for the test if it does reach that line.","commit_id":"d5cad63d182ef140d3d131bda29894a3fb8650ce"},{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"56b74e7ae60cd77fa65c67ca59d32953d0747493","unresolved":false,"context_lines":[{"line_number":1174,"context_line":"                shuffle_calls, [[\u00276789\u0027, \u00279876\u0027],"},{"line_number":1175,"context_line":"                                [\u00276789\u0027, \u00279876\u0027]])"},{"line_number":1176,"context_line":""},{"line_number":1177,"context_line":"            # Shows that we called removed the directories with no db files"},{"line_number":1178,"context_line":"            # and directories with no hashes"},{"line_number":1179,"context_line":"            self.assertEqual("},{"line_number":1180,"context_line":"                rmdir_calls, [\u0027/srv/node/sda/containers/6789/jkl\u0027,"}],"source_content_type":"text/x-python","patch_set":5,"id":"1a95cdbc_b3beba62","line":1177,"updated":"2016-10-10 18:51:08.000000000","message":"instead of \"we called removed the directories\" which has a typo, maybe just say \"we called rmdir\"","commit_id":"d5cad63d182ef140d3d131bda29894a3fb8650ce"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"0031a5597e23f1a97eaeb4dc8ec829724488df5e","unresolved":false,"context_lines":[{"line_number":1174,"context_line":"                shuffle_calls, [[\u00276789\u0027, \u00279876\u0027],"},{"line_number":1175,"context_line":"                                [\u00276789\u0027, \u00279876\u0027]])"},{"line_number":1176,"context_line":""},{"line_number":1177,"context_line":"            # Shows that we called removed the directories with no db files"},{"line_number":1178,"context_line":"            # and directories with no hashes"},{"line_number":1179,"context_line":"            self.assertEqual("},{"line_number":1180,"context_line":"                rmdir_calls, [\u0027/srv/node/sda/containers/6789/jkl\u0027,"}],"source_content_type":"text/x-python","patch_set":5,"id":"7a77a97e_f1039871","line":1177,"in_reply_to":"1a95cdbc_b3beba62","updated":"2016-11-15 16:58:14.000000000","message":"Done","commit_id":"d5cad63d182ef140d3d131bda29894a3fb8650ce"},{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"307f0c71b9f8b23b6c3e4939cc93ba2ab9d65fab","unresolved":false,"context_lines":[{"line_number":1115,"context_line":"                # use db_replicator.os.rmdir to delete a directory with an"},{"line_number":1116,"context_line":"                # existing file; assert it raised OSError"},{"line_number":1117,"context_line":"                with self.assertRaises(OSError):"},{"line_number":1118,"context_line":"                    orig_rmdir(tempdir)"},{"line_number":1119,"context_line":""},{"line_number":1120,"context_line":"        orig_listdir \u003d db_replicator.os.listdir"},{"line_number":1121,"context_line":"        orig_isdir \u003d db_replicator.os.path.isdir"}],"source_content_type":"text/x-python","patch_set":6,"id":"7a77a97e_e5c49d60","line":1118,"updated":"2016-11-16 03:49:59.000000000","message":"actually, this not the behavior that I was suggesting.  I would suggest that if there is an existing file, then there is no raised error.  That is why I suggested that we fail the test if an error is raised.  Rather, the code should just not remove the non-emtpy directory.","commit_id":"c1127328ea96ef576be3f2fb2443009a610189e9"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"27ebc43ea4ad26bbee77082533bf036b7618f176","unresolved":false,"context_lines":[{"line_number":1115,"context_line":"                # use db_replicator.os.rmdir to delete a directory with an"},{"line_number":1116,"context_line":"                # existing file; assert it raised OSError"},{"line_number":1117,"context_line":"                with self.assertRaises(OSError):"},{"line_number":1118,"context_line":"                    orig_rmdir(tempdir)"},{"line_number":1119,"context_line":""},{"line_number":1120,"context_line":"        orig_listdir \u003d db_replicator.os.listdir"},{"line_number":1121,"context_line":"        orig_isdir \u003d db_replicator.os.path.isdir"}],"source_content_type":"text/x-python","patch_set":6,"id":"7a77a97e_68cea4b5","line":1118,"in_reply_to":"7a77a97e_e5c49d60","updated":"2016-11-16 04:41:25.000000000","message":"Right, but self.fail would never reach I suppose since it always is being passed on when there is a directory with a file, but still I get what you\u0027re saying - *if* at all. Will revert this too.","commit_id":"c1127328ea96ef576be3f2fb2443009a610189e9"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"68580e280928cfa17557548533f463cea5821831","unresolved":false,"context_lines":[{"line_number":1058,"context_line":"        shuffle_calls \u003d []"},{"line_number":1059,"context_line":"        rmdir_calls \u003d []"},{"line_number":1060,"context_line":""},{"line_number":1061,"context_line":"        existing_file \u003d os.path.join(tempdir, \u0027a_file_exists\u0027)"},{"line_number":1062,"context_line":"        open(existing_file, \u0027a\u0027).close()"},{"line_number":1063,"context_line":"        dir_with_no_obj_file \u003d \u0027/srv/node/sdb/containers/9876/xyz/\u0027 \\"},{"line_number":1064,"context_line":"                               \u002711111111111111111111111111111xyz\u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a74a57a_85bf5ca9","line":1061,"updated":"2016-12-01 06:30:28.000000000","message":"seems strange to go to all the trouble to mock out all these dir calls when we have a perfectly reasonable tempdir right here...\n\nI don\u0027t think I fully understand the extent to which this check is trying assert behavior in this case.\n\nFrom the description of the check in the test name, creating a tempdir with an empty suffix and hash dir rolling roundrobin_datadirs over it and observing the final state of tempdir would be sufficient.\n\nProbably this whole approach was just cribbed from test_roundrobin_datadirs - which is ... maybe not a great pattern to proliferate.","commit_id":"321bb9145f9e2e184994e5d0cf78c62b6043112c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"68580e280928cfa17557548533f463cea5821831","unresolved":false,"context_lines":[{"line_number":1127,"context_line":"            db_replicator.os.listdir \u003d _listdir"},{"line_number":1128,"context_line":"            db_replicator.os.path.isdir \u003d _isdir"},{"line_number":1129,"context_line":"            db_replicator.os.path.exists \u003d _exists"},{"line_number":1130,"context_line":"            db_replicator.random.shuffle \u003d _shuffle"},{"line_number":1131,"context_line":"            db_replicator.os.rmdir \u003d _rmdir"},{"line_number":1132,"context_line":""},{"line_number":1133,"context_line":"            datadirs \u003d [(\u0027/srv/node/sda/containers\u0027, 1),"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a74a57a_4577043b","line":1130,"updated":"2016-12-01 06:30:28.000000000","message":"this would look better with mock.patch instead of old school manual patching, but I can see you\u0027re just cribbing from test_roundrobin_datadirs\n\nhttps://gist.github.com/clayg/01efe0017b8da68978d75a6a39c8bb78","commit_id":"321bb9145f9e2e184994e5d0cf78c62b6043112c"}]}