)]}'
{"swift/cli/relinker.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1cf261bbdc6003ec0e74d30827722c6a17318969","unresolved":true,"context_lines":[{"line_number":243,"context_line":"                                if e.errno !\u003d errno.ENOENT:"},{"line_number":244,"context_line":"                                    raise"},{"line_number":245,"context_line":""},{"line_number":246,"context_line":"                        # After that, any lock files (replication lock, hashes"},{"line_number":247,"context_line":"                        # lock, etc.) can be removed. It\u0027s important that we"},{"line_number":248,"context_line":"                        # not touch any directories, though -- those could"},{"line_number":249,"context_line":"                        # have new writes"},{"line_number":250,"context_line":"                        for f in os.listdir(partition_path):"}],"source_content_type":"text/x-python","patch_set":1,"id":"0b40e624_62aaed05","line":247,"range":{"start_line":246,"start_character":24,"end_line":247,"end_character":53},"updated":"2021-05-10 15:42:59.000000000","message":"is there any ‘etc’? I wonder if it might be better to stick with the list of known lock files?","commit_id":"5b3a0b78b02975ede829404b72c363e0dc355253"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"db16ee7b426c61d2d9b196de52c9e0b3d7bf8da9","unresolved":true,"context_lines":[{"line_number":243,"context_line":"                                if e.errno !\u003d errno.ENOENT:"},{"line_number":244,"context_line":"                                    raise"},{"line_number":245,"context_line":""},{"line_number":246,"context_line":"                        # After that, any lock files (replication lock, hashes"},{"line_number":247,"context_line":"                        # lock, etc.) can be removed. It\u0027s important that we"},{"line_number":248,"context_line":"                        # not touch any directories, though -- those could"},{"line_number":249,"context_line":"                        # have new writes"},{"line_number":250,"context_line":"                        for f in os.listdir(partition_path):"}],"source_content_type":"text/x-python","patch_set":1,"id":"f5c26f26_2938e9ba","line":247,"range":{"start_line":246,"start_character":24,"end_line":247,"end_character":53},"in_reply_to":"0b40e624_62aaed05","updated":"2021-05-12 00:22:43.000000000","message":"I don\u0027t think so, but at the same time, the listdir is unavoidable, since you can have N replication locks.\n\nOriginally, I was planning on unlinking every non-directory but then I got skittish when it caused test_cleanup_old_part_careful to fail. I figured \"all locks\" should be a decent middle-ground; they are inherently transient things, and if there isn\u0027t anything else in the partition, there\u0027s no reason to keep it around.","commit_id":"5b3a0b78b02975ede829404b72c363e0dc355253"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"dd97c23a2ae84849c03df7d0018bb22e94526a72","unresolved":true,"context_lines":[{"line_number":243,"context_line":"                                if e.errno !\u003d errno.ENOENT:"},{"line_number":244,"context_line":"                                    raise"},{"line_number":245,"context_line":""},{"line_number":246,"context_line":"                        # After that, any lock files (replication lock, hashes"},{"line_number":247,"context_line":"                        # lock, etc.) can be removed. It\u0027s important that we"},{"line_number":248,"context_line":"                        # not touch any directories, though -- those could"},{"line_number":249,"context_line":"                        # have new writes"},{"line_number":250,"context_line":"                        for f in os.listdir(partition_path):"}],"source_content_type":"text/x-python","patch_set":1,"id":"45c078b8_f779948b","line":247,"range":{"start_line":246,"start_character":24,"end_line":247,"end_character":53},"in_reply_to":"c9077e34_06947d25","updated":"2021-05-12 15:16:43.000000000","message":"😮 I need to check my assumptions more often. Yup, it\u0027s just the one replication lock! And looking into the history a bit, there\u0027s only ever been the one replication lock. I definitely *was* thinking about the device-level locks.\n\nIf we\u0027re going to take the replication lock here, we\u0027ve gotta think through the order in which should they be taken. Looks like ssync_receiver is the only thing that takes the replication lock; that\u0027s fairly high-level, then it goes down and opens up diskfiles. Since those could need to be quarantined, it may need to take the partition lock, too -- so I guess I should match that order, to avoid deadlocks?","commit_id":"5b3a0b78b02975ede829404b72c363e0dc355253"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8341b8c78e30d933fe623589c4602c243320b1e8","unresolved":true,"context_lines":[{"line_number":243,"context_line":"                                if e.errno !\u003d errno.ENOENT:"},{"line_number":244,"context_line":"                                    raise"},{"line_number":245,"context_line":""},{"line_number":246,"context_line":"                        # After that, any lock files (replication lock, hashes"},{"line_number":247,"context_line":"                        # lock, etc.) can be removed. It\u0027s important that we"},{"line_number":248,"context_line":"                        # not touch any directories, though -- those could"},{"line_number":249,"context_line":"                        # have new writes"},{"line_number":250,"context_line":"                        for f in os.listdir(partition_path):"}],"source_content_type":"text/x-python","patch_set":1,"id":"c9077e34_06947d25","line":247,"range":{"start_line":246,"start_character":24,"end_line":247,"end_character":53},"in_reply_to":"f5c26f26_2938e9ba","updated":"2021-05-12 11:02:54.000000000","message":"There\u0027s only one .lock-replication file in a partition dir though - the multiple concurrency locks are at the device level:\n\n  ├── node3\n│   ├── sdb3\n│   │   └── objects-1\n│   │       └── 396\n│   │           ├── .lock\n│   │           └── 784\n│   │               └── 633ab34b43784a4a069092fe954ee784\n│   └── sdb7\n│       ├── .lock\n│       ├── .lock-1\n│       └── objects-1\n│           └── 396\n│               ├── .lock\n│               ├── .lock-replication\n│               └── 784\n│                   └── 633ab34b43784a4a069092fe954ee784\n└── node4","commit_id":"5b3a0b78b02975ede829404b72c363e0dc355253"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"4ad53e2d5638e0681e0fd8f58e7e28eee997cc8a","unresolved":true,"context_lines":[{"line_number":250,"context_line":"                        for f in os.listdir(partition_path):"},{"line_number":251,"context_line":"                            if f.startswith(\u0027.lock\u0027):"},{"line_number":252,"context_line":"                                try:"},{"line_number":253,"context_line":"                                    os.unlink(os.path.join(partition_path, f))"},{"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"}],"source_content_type":"text/x-python","patch_set":2,"id":"25b45bf9_977635ee","line":253,"updated":"2021-05-12 07:09:03.000000000","message":"ok, so we lock_path to delete the files, it itself will places a lock file file which we then delete. In essence unlocking it, so something else could come in a lock it in the meantime.. but I guess if that\u0027s the case then we probably don\u0027t want to delete the directory.\n\nBut if there are some other lock in there how do we know it\u0027s not currently being used? Should we attempt a write lock and if so, delete it then? But that would mean we\u0027d need to know what our own lock_path lock is.. though it should just be the .lock.\n\nFrom a quick look at the code it\u0027s mostly the relinker and the rehashing that locks the partition dir. And we did confirm there was no hashes.. so unless something is actaully causing a rehash it should be safe.","commit_id":"9565fb484f075e37f679372010c9baf762171c07"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8341b8c78e30d933fe623589c4602c243320b1e8","unresolved":true,"context_lines":[{"line_number":250,"context_line":"                        for f in os.listdir(partition_path):"},{"line_number":251,"context_line":"                            if f.startswith(\u0027.lock\u0027):"},{"line_number":252,"context_line":"                                try:"},{"line_number":253,"context_line":"                                    os.unlink(os.path.join(partition_path, f))"},{"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"}],"source_content_type":"text/x-python","patch_set":2,"id":"b559a522_a89d669c","line":253,"in_reply_to":"25b45bf9_977635ee","updated":"2021-05-12 11:02:54.000000000","message":"Agree, we should probably take each lock before deleting it. That way we know we\u0027re not deleting some other process\u0027s lock.\n\nI think it is ok to delete the lock we\u0027ve taken if we\u0027re careful what else we do inside the context manager (could be worth a comment to remind us): another process can then immediately take the lock while we\u0027re still in the lock_path context manager, but then os.rmdir will fail.","commit_id":"9565fb484f075e37f679372010c9baf762171c07"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"dd97c23a2ae84849c03df7d0018bb22e94526a72","unresolved":true,"context_lines":[{"line_number":250,"context_line":"                        for f in os.listdir(partition_path):"},{"line_number":251,"context_line":"                            if f.startswith(\u0027.lock\u0027):"},{"line_number":252,"context_line":"                                try:"},{"line_number":253,"context_line":"                                    os.unlink(os.path.join(partition_path, f))"},{"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"}],"source_content_type":"text/x-python","patch_set":2,"id":"61e32b47_f830b2a3","line":253,"in_reply_to":"b559a522_a89d669c","updated":"2021-05-12 15:16:43.000000000","message":"I\u0027m a little worried about B and C writing concurrently after\n\n* A takes the lock\n* B tries to take the lock and blocks\n* A deletes the lock file\n* C takes the (new) lock\n* A releases the (old) lock\n\nLooks like B acquires the *old* lock:\n\n vagrant@saio:~/swift$ lsof -p 464829 | grep /tmp\n python  464829 vagrant    3wW     REG    8,1        0   3763 /tmp/.lock (deleted)\n vagrant@saio:~/swift$ lsof -p 464840 | grep /tmp\n python  464840 vagrant    3wW     REG    8,1        0   7927 /tmp/.lock\n\nI almost want to take the old lock, delete the lock file, then *never release it* to ensure that any blocked processes timeout -- but then I\u0027d worry about the file descriptor leak. I guess the next best thing would be to stat the lock file after acquiring it, compare inodes against an fstat of the open fd, and raise a \"Timeout\" if they differ.","commit_id":"9565fb484f075e37f679372010c9baf762171c07"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"807005aeb0bd4c78c95fe22c369f3133e1d816a4","unresolved":true,"context_lines":[{"line_number":341,"context_line":"            if not hashes:"},{"line_number":342,"context_line":"                try:"},{"line_number":343,"context_line":"                    with lock_path(partition_path, name\u003d\u0027replication\u0027), \\"},{"line_number":344,"context_line":"                            lock_path(partition_path):"},{"line_number":345,"context_line":"                        # Order here is somewhat important for crash-tolerance"},{"line_number":346,"context_line":"                        for f in (\u0027hashes.pkl\u0027, \u0027hashes.invalid\u0027, \u0027.lock\u0027,"},{"line_number":347,"context_line":"                                  \u0027.lock-replication\u0027):"}],"source_content_type":"text/x-python","patch_set":4,"id":"d32e3a06_4db2aec9","line":344,"updated":"2021-05-13 07:01:20.000000000","message":"OK so the idea here, just to make sure I\u0027m on the same page, is to take a replication lock in essence so we know there is no other replication happening, because if somehow there was on the partition then we\u0027d be waiting for the lock. And a .lock that we normally use, this should block the hashes at the same time.\n\nThen we can clean up hashes.pkl, hashes.invalid, then finally clear the hashes .lock and the finally clear the replication lock we have held?\n\nThe lock_path function takes a limit, where you can control how many locks there should be to a particular location.. which is interesting, but I can\u0027t see anywhere we use it. So just taking the single .lock and .lock-replication should be ok. But it\u0027s an interesting feature to remember if we ever want to limit access to places for whatever reason.","commit_id":"b662ad6d81767b9a0cbe368e4ef703ac9e7a2034"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"5b8c75af4bd8424e946d396190f9e07e0c280a12","unresolved":true,"context_lines":[{"line_number":341,"context_line":"            if not hashes:"},{"line_number":342,"context_line":"                try:"},{"line_number":343,"context_line":"                    with lock_path(partition_path, name\u003d\u0027replication\u0027), \\"},{"line_number":344,"context_line":"                            lock_path(partition_path):"},{"line_number":345,"context_line":"                        # Order here is somewhat important for crash-tolerance"},{"line_number":346,"context_line":"                        for f in (\u0027hashes.pkl\u0027, \u0027hashes.invalid\u0027, \u0027.lock\u0027,"},{"line_number":347,"context_line":"                                  \u0027.lock-replication\u0027):"}],"source_content_type":"text/x-python","patch_set":4,"id":"6dfb0c34_b7ba3bfc","line":344,"in_reply_to":"d32e3a06_4db2aec9","updated":"2021-05-13 08:34:18.000000000","message":"limit is used in replication_lock for the device level locks, to allow concurrency, but not at partition level:\n\nhttps://github.com/openstack/swift/blob/7087fb0d7ed6e36cc2d120eb6d8125257c8c7089/swift/obj/diskfile.py#L1361-L1382","commit_id":"b662ad6d81767b9a0cbe368e4ef703ac9e7a2034"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b13c6a99071aae11b66b5b0688bef0f9432cbded","unresolved":true,"context_lines":[{"line_number":340,"context_line":"            # increase)."},{"line_number":341,"context_line":"            if not hashes:"},{"line_number":342,"context_line":"                try:"},{"line_number":343,"context_line":"                    with lock_path(partition_path, name\u003d\u0027replication\u0027), \\"},{"line_number":344,"context_line":"                            lock_path(partition_path):"},{"line_number":345,"context_line":"                        # Order here is somewhat important for crash-tolerance"},{"line_number":346,"context_line":"                        for f in (\u0027hashes.pkl\u0027, \u0027hashes.invalid\u0027, \u0027.lock\u0027,"},{"line_number":347,"context_line":"                                  \u0027.lock-replication\u0027):"}],"source_content_type":"text/x-python","patch_set":5,"id":"afb0fe72_3f65a1cd","line":344,"range":{"start_line":343,"start_character":20,"end_line":344,"end_character":53},"updated":"2021-05-20 12:42:22.000000000","message":"there doesn\u0027t seem to be any test coverage for the locks being taken - I hacked something together in https://review.opendev.org/c/openstack/swift/+/792369 to cover .lock-replication but found it harder to target the plain .lock","commit_id":"3c0f412c046604d09e1f9c0a5fa60d2461984712"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b13c6a99071aae11b66b5b0688bef0f9432cbded","unresolved":true,"context_lines":[{"line_number":354,"context_line":"                    # other process could come along and make new ones -- so"},{"line_number":355,"context_line":"                    # this may well complain that the directory is not empty"},{"line_number":356,"context_line":"                    os.rmdir(partition_path)"},{"line_number":357,"context_line":"                except (OSError, LockTimeout):"},{"line_number":358,"context_line":"                    # Most likely, some data landed in here or we hit an error"},{"line_number":359,"context_line":"                    # above. Let the replicator deal with things; it was worth"},{"line_number":360,"context_line":"                    # a shot."}],"source_content_type":"text/x-python","patch_set":5,"id":"e137f153_345c7985","line":357,"range":{"start_line":357,"start_character":33,"end_line":357,"end_character":45},"updated":"2021-05-20 12:42:22.000000000","message":"good to have","commit_id":"3c0f412c046604d09e1f9c0a5fa60d2461984712"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"5711e164cac59330790e7a5c84c8f22318ea15a1","unresolved":true,"context_lines":[{"line_number":330,"context_line":"                hashes \u003d self.diskfile_mgr.get_hashes("},{"line_number":331,"context_line":"                    device, int(partition), [], self.policy)"},{"line_number":332,"context_line":"            except LockTimeout:"},{"line_number":333,"context_line":"                hashes \u003d 1  # truthy, but invalid"},{"line_number":334,"context_line":"            # In any reasonably-large cluster, we\u0027d expect all old"},{"line_number":335,"context_line":"            # partitions P to be empty after cleanup (i.e., it\u0027s unlikely"},{"line_number":336,"context_line":"            # that there\u0027s another partition Q :\u003d P//2 that also has data"}],"source_content_type":"text/x-python","patch_set":6,"id":"b48e6796_1579e091","line":333,"range":{"start_line":333,"start_character":16,"end_line":333,"end_character":49},"updated":"2021-05-21 09:49:32.000000000","message":"right, let\u0027s not unlink hashes.* if get_hashes failed to run","commit_id":"ae00408f7bc59ffa97e352ec3aa32e879925a4b1"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"5711e164cac59330790e7a5c84c8f22318ea15a1","unresolved":true,"context_lines":[{"line_number":343,"context_line":"            # increase)."},{"line_number":344,"context_line":"            if not hashes:"},{"line_number":345,"context_line":"                try:"},{"line_number":346,"context_line":"                    with self.diskfile_mgr.replication_lock("},{"line_number":347,"context_line":"                            device, self.policy, partition), \\"},{"line_number":348,"context_line":"                        self.diskfile_mgr.partition_lock("},{"line_number":349,"context_line":"                            device, self.policy, partition):"}],"source_content_type":"text/x-python","patch_set":6,"id":"d18a2530_0c16fe99","line":346,"range":{"start_line":346,"start_character":43,"end_line":346,"end_character":59},"updated":"2021-05-21 09:49:32.000000000","message":"ok. this is going to lock the whole device, but only while a partition is rm\u0027d, and the only other process to compete for that lock is ssync which should be inactive during part power increase.","commit_id":"ae00408f7bc59ffa97e352ec3aa32e879925a4b1"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7f1be5dfe7443d4a5dbd7fb930381ccc58186056","unresolved":true,"context_lines":[{"line_number":330,"context_line":"                hashes \u003d self.diskfile_mgr.get_hashes("},{"line_number":331,"context_line":"                    device, int(partition), [], self.policy)"},{"line_number":332,"context_line":"            except LockTimeout:"},{"line_number":333,"context_line":"                hashes \u003d 1  # truthy, but invalid"},{"line_number":334,"context_line":"            # In any reasonably-large cluster, we\u0027d expect all old"},{"line_number":335,"context_line":"            # partitions P to be empty after cleanup (i.e., it\u0027s unlikely"},{"line_number":336,"context_line":"            # that there\u0027s another partition Q :\u003d P//2 that also has data"}],"source_content_type":"text/x-python","patch_set":8,"id":"809562fe_a118f79c","line":333,"range":{"start_line":333,"start_character":16,"end_line":333,"end_character":49},"updated":"2021-05-27 10:06:19.000000000","message":"I still don\u0027t get a test failure if this regresses e.g.\n\n  hashes \u003d None","commit_id":"960f2fea329ab300b3af038f4fe60d45c52f978f"}],"swift/common/utils.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"5711e164cac59330790e7a5c84c8f22318ea15a1","unresolved":true,"context_lines":[{"line_number":2877,"context_line":"    :raises TypeError: if limit is not an int."},{"line_number":2878,"context_line":"    :raises ValueError: if limit is less than 1."},{"line_number":2879,"context_line":"    \"\"\""},{"line_number":2880,"context_line":"    if timeout is None:"},{"line_number":2881,"context_line":"        timeout \u003d DEFAULT_LOCK_TIMEOUT"},{"line_number":2882,"context_line":"    if timeout_class is None:"},{"line_number":2883,"context_line":"        timeout_class \u003d swift.common.exceptions.LockTimeout"},{"line_number":2884,"context_line":"    if limit \u003c 1:"}],"source_content_type":"text/x-python","patch_set":6,"id":"20719966_3a181e97","line":2881,"range":{"start_line":2880,"start_character":4,"end_line":2881,"end_character":38},"updated":"2021-05-21 09:49:32.000000000","message":"I\u0027m curious that you haven\u0027t used DEFAULT_LOCK_TIMEOUT as the kwarg default...but wondering if that has to do with when the kwarg default is initialised w.r.t. monkey patching in the tests? I introduced the exact same constant when trying to knock up a test yesterday but failed to patch it successfully. Maybe this is the cleverness I was missing.","commit_id":"ae00408f7bc59ffa97e352ec3aa32e879925a4b1"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"4e3d6e487b81be3545ca894d2c1c3f4957f2f8e7","unresolved":true,"context_lines":[{"line_number":2877,"context_line":"    :raises TypeError: if limit is not an int."},{"line_number":2878,"context_line":"    :raises ValueError: if limit is less than 1."},{"line_number":2879,"context_line":"    \"\"\""},{"line_number":2880,"context_line":"    if timeout is None:"},{"line_number":2881,"context_line":"        timeout \u003d DEFAULT_LOCK_TIMEOUT"},{"line_number":2882,"context_line":"    if timeout_class is None:"},{"line_number":2883,"context_line":"        timeout_class \u003d swift.common.exceptions.LockTimeout"},{"line_number":2884,"context_line":"    if limit \u003c 1:"}],"source_content_type":"text/x-python","patch_set":6,"id":"2a7cc0ed_d237d4b3","line":2881,"range":{"start_line":2880,"start_character":4,"end_line":2881,"end_character":38},"in_reply_to":"20719966_3a181e97","updated":"2021-05-26 17:59:33.000000000","message":"Yeah, it\u0027s for the sake of monkey-patching. The default args get evaluated when lock_path gets defined. *Maybe* we could do something clever with ctypes or something so we\u0027ve got a pointer to a double that we could manipulate, but that seems not worth the effort.","commit_id":"ae00408f7bc59ffa97e352ec3aa32e879925a4b1"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"5711e164cac59330790e7a5c84c8f22318ea15a1","unresolved":true,"context_lines":[{"line_number":2917,"context_line":"    first)."},{"line_number":2918,"context_line":""},{"line_number":2919,"context_line":"    :param filename: file to be locked"},{"line_number":2920,"context_line":"    :param timeout: timeout (in seconds)"},{"line_number":2921,"context_line":"    :param append: True if file should be opened in append mode"},{"line_number":2922,"context_line":"    :param unlink: True if the file should be unlinked at the end"},{"line_number":2923,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":6,"id":"aca6ea28_cef7c2a3","line":2920,"updated":"2021-05-21 09:49:32.000000000","message":"nit: docstring could be updated with default","commit_id":"ae00408f7bc59ffa97e352ec3aa32e879925a4b1"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"5711e164cac59330790e7a5c84c8f22318ea15a1","unresolved":true,"context_lines":[{"line_number":2964,"context_line":"    time has expired (whichever occurs first)."},{"line_number":2965,"context_line":""},{"line_number":2966,"context_line":"    :param filename: file path of the parent directory to be locked"},{"line_number":2967,"context_line":"    :param timeout: timeout (in seconds)"},{"line_number":2968,"context_line":"    \"\"\""},{"line_number":2969,"context_line":"    return lock_path(os.path.dirname(filename), timeout\u003dtimeout)"},{"line_number":2970,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"882bc168_08c5bac1","line":2967,"updated":"2021-05-21 09:49:32.000000000","message":"nit: docstring could be updated with default","commit_id":"ae00408f7bc59ffa97e352ec3aa32e879925a4b1"}],"test/unit/cli/test_relinker.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1cf261bbdc6003ec0e74d30827722c6a17318969","unresolved":true,"context_lines":[{"line_number":2884,"context_line":"                                 set(os.listdir(self.part_dir)))"},{"line_number":2885,"context_line":"                # unlink a random file, should be empty"},{"line_number":2886,"context_line":"                os.unlink(os.path.join(self.part_dir, \u0027hashes.pkl\u0027))"},{"line_number":2887,"context_line":"                # create an extra ssync replication lock"},{"line_number":2888,"context_line":"                with open(os.path.join(self.part_dir,"},{"line_number":2889,"context_line":"                                       \u0027.lock-replication-123\u0027), \u0027w\u0027):"},{"line_number":2890,"context_line":"                    pass"}],"source_content_type":"text/x-python","patch_set":1,"id":"d805bca1_c8e81eef","line":2887,"range":{"start_line":2887,"start_character":18,"end_line":2887,"end_character":56},"updated":"2021-05-10 15:42:59.000000000","message":"with the current change, we should check some other .lock_blah file also gets removed","commit_id":"5b3a0b78b02975ede829404b72c363e0dc355253"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1cf261bbdc6003ec0e74d30827722c6a17318969","unresolved":true,"context_lines":[{"line_number":2905,"context_line":"            ]))"},{"line_number":2906,"context_line":"        self.assertEqual([True], calls)"},{"line_number":2907,"context_line":"        # old partition can still be cleaned up"},{"line_number":2908,"context_line":"        self.assertFalse(os.path.exists(self.part_dir))"},{"line_number":2909,"context_line":""},{"line_number":2910,"context_line":"    def test_cleanup_reapable(self):"},{"line_number":2911,"context_line":"        # relink a tombstone"}],"source_content_type":"text/x-python","patch_set":1,"id":"af08e2c0_88b5cb76","line":2908,"updated":"2021-05-10 15:42:59.000000000","message":"would be good to have a test that covers a dir preventing the partition removal","commit_id":"5b3a0b78b02975ede829404b72c363e0dc355253"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8341b8c78e30d933fe623589c4602c243320b1e8","unresolved":true,"context_lines":[{"line_number":2901,"context_line":"                os.unlink(os.path.join(self.part_dir, \u0027hashes.pkl\u0027))"},{"line_number":2902,"context_line":"                # create an extra ssync replication lock"},{"line_number":2903,"context_line":"                with open(os.path.join(self.part_dir,"},{"line_number":2904,"context_line":"                                       \u0027.lock-replication-123\u0027), \u0027w\u0027):"},{"line_number":2905,"context_line":"                    pass"},{"line_number":2906,"context_line":"                # or really, any other kind of lock"},{"line_number":2907,"context_line":"                with open(os.path.join(self.part_dir, \u0027.lock-other\u0027), \u0027w\u0027):"}],"source_content_type":"text/x-python","patch_set":2,"id":"3acf76e7_d73f0c9c","line":2904,"range":{"start_line":2904,"start_character":40,"end_line":2904,"end_character":61},"updated":"2021-05-12 11:02:54.000000000","message":"Reading the diskfile replication_lock code I only see one .lock-replication file getting created, but at the device level there could be .lock, .lock-1, .lock-2 etc","commit_id":"9565fb484f075e37f679372010c9baf762171c07"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"5711e164cac59330790e7a5c84c8f22318ea15a1","unresolved":true,"context_lines":[{"line_number":3480,"context_line":"                    \u0027swift.common.utils.DEFAULT_LOCK_TIMEOUT\u0027, 0.1):"},{"line_number":3481,"context_line":"                self.assertEqual(0, relinker.main([\u0027cleanup\u0027, conf_file]))"},{"line_number":3482,"context_line":"        # old partition can\u0027t be cleaned up"},{"line_number":3483,"context_line":"        self.assertTrue(os.path.exists(self.part_dir))"},{"line_number":3484,"context_line":"        self.assertTrue(os.path.exists("},{"line_number":3485,"context_line":"            os.path.join(self.part_dir, \u0027.lock\u0027)))"},{"line_number":3486,"context_line":"        self.assertEqual([], self.logger.get_lines_for_level(\u0027error\u0027))"}],"source_content_type":"text/x-python","patch_set":6,"id":"5ed15187_71ca4364","line":3483,"range":{"start_line":3483,"start_character":8,"end_line":3483,"end_character":54},"updated":"2021-05-21 09:49:32.000000000","message":"this does not fail when I revert the relinker taking the partition lock, because the preceding get_hashes raises LockTimeout so that the relinker does not even attempt to remove the partition dir.\n\nSo, the test *is* verifying the try/except around get_hashes, but not what the test purports to verify.\n\nEven if get_hashes had run, invalidate_hash has also failed due to the lock being held! This is the kind of tangle I ran into trying to construct this test. It probably needs the lock to be taken more surgically than \u0027for the whole relinker cycle\u0027.","commit_id":"ae00408f7bc59ffa97e352ec3aa32e879925a4b1"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"4e3d6e487b81be3545ca894d2c1c3f4957f2f8e7","unresolved":true,"context_lines":[{"line_number":3480,"context_line":"                    \u0027swift.common.utils.DEFAULT_LOCK_TIMEOUT\u0027, 0.1):"},{"line_number":3481,"context_line":"                self.assertEqual(0, relinker.main([\u0027cleanup\u0027, conf_file]))"},{"line_number":3482,"context_line":"        # old partition can\u0027t be cleaned up"},{"line_number":3483,"context_line":"        self.assertTrue(os.path.exists(self.part_dir))"},{"line_number":3484,"context_line":"        self.assertTrue(os.path.exists("},{"line_number":3485,"context_line":"            os.path.join(self.part_dir, \u0027.lock\u0027)))"},{"line_number":3486,"context_line":"        self.assertEqual([], self.logger.get_lines_for_level(\u0027error\u0027))"}],"source_content_type":"text/x-python","patch_set":6,"id":"a7c5605b_9dd63e85","line":3483,"range":{"start_line":3483,"start_character":8,"end_line":3483,"end_character":54},"in_reply_to":"5ed15187_71ca4364","updated":"2021-05-26 17:59:33.000000000","message":"Good point; I\u0027ll see what I can do about making this test more targeted. Still, I\u0027m glad I did it this way as a first pass so I found all the other bits that needed to catch LockTimeouts ;-)","commit_id":"ae00408f7bc59ffa97e352ec3aa32e879925a4b1"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7f1be5dfe7443d4a5dbd7fb930381ccc58186056","unresolved":true,"context_lines":[{"line_number":3478,"context_line":"        def new_hook(*args, **kwargs):"},{"line_number":3479,"context_line":"            # lock taken so relinker should be unable to rehash"},{"line_number":3480,"context_line":"            with utils.lock_path(self.part_dir):"},{"line_number":3481,"context_line":"                return orig_hook_post_partition(*args, **kwargs)"},{"line_number":3482,"context_line":""},{"line_number":3483,"context_line":"        with self._mock_relinker(), \\"},{"line_number":3484,"context_line":"                mock.patch(\u0027swift.common.utils.DEFAULT_LOCK_TIMEOUT\u0027, 0.1), \\"}],"source_content_type":"text/x-python","patch_set":8,"id":"a838de73_4de5d3da","line":3481,"updated":"2021-05-27 10:06:19.000000000","message":"the relinker is unable to rehash, but also unable to take the lock to rm, so this doesn\u0027t verify that the rehash LockTimeout sets hashes to a truthy value - which is why I had to mock get_hashes in https://review.opendev.org/c/openstack/swift/+/792556/1/test/unit/cli/test_relinker.py","commit_id":"960f2fea329ab300b3af038f4fe60d45c52f978f"}]}