)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"c2a9b028eedbe263e9e53cfb85d0068f0d78fa6a","unresolved":true,"context_lines":[{"line_number":9,"context_line":"During an MPU complete, the s3api first PUTs an SLO manifest and then"},{"line_number":10,"context_line":"DELETEs the upload marker in \u003cbucket\u003e+segments. If the proxy\u0027s clock"},{"line_number":11,"context_line":"is slow relative the proxy that created the upload marker, the DELETE"},{"line_number":12,"context_line":"will fail with a 409."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Previously the 409 would be returned to the client, which for some"},{"line_number":15,"context_line":"clients (e.g. aws cli) does not trigger a retry. This patch will"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"3e377c6d_91cf218d","line":12,"updated":"2023-11-22 22:58:47.000000000","message":"interestig; the failure mode only exibits on s3api because \"complete-mpu\" is a multi-stage request.  The SLO manifest create will succeed (maybe sometimes depending on the node clock with a slightly accelerated timestamp) and on the lagging proxy it\u0027s only the upload marker cleanup that fails 409 if it was created with an acceleated timestamp.","commit_id":"0d570c7a1b3a51c4687695dc1b920b8d18edd460"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"fec141912be1ce5a739b33dbbbf78600ec7f7cff","unresolved":true,"context_lines":[{"line_number":9,"context_line":"During an MPU complete, the s3api first PUTs an SLO manifest and then"},{"line_number":10,"context_line":"DELETEs the upload marker in \u003cbucket\u003e+segments. If the proxy\u0027s clock"},{"line_number":11,"context_line":"is slow relative the proxy that created the upload marker, the DELETE"},{"line_number":12,"context_line":"will fail with a 409."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Previously the 409 would be returned to the client, which for some"},{"line_number":15,"context_line":"clients (e.g. aws cli) does not trigger a retry. This patch will"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"cea902aa_2fe080e4","line":12,"in_reply_to":"3e377c6d_91cf218d","updated":"2023-11-27 17:00:39.000000000","message":"Something to keep in mind as we think about ALOs...\n\nHow bad was the time skew between the proxies? How did it get so bad? Was it a particularly small MPU, with just one or two segments or something?","commit_id":"0d570c7a1b3a51c4687695dc1b920b8d18edd460"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"bc34fefe6d5cff6a067c6b443d9394d44724159c","unresolved":false,"context_lines":[{"line_number":9,"context_line":"During an MPU complete, the s3api first PUTs an SLO manifest and then"},{"line_number":10,"context_line":"DELETEs the upload marker in \u003cbucket\u003e+segments. If the proxy\u0027s clock"},{"line_number":11,"context_line":"is slow relative the proxy that created the upload marker, the DELETE"},{"line_number":12,"context_line":"will fail with a 409."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Previously the 409 would be returned to the client, which for some"},{"line_number":15,"context_line":"clients (e.g. aws cli) does not trigger a retry. This patch will"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"d9345a9b_cc813f22","line":12,"in_reply_to":"469d0580_6c2009ca","updated":"2023-11-30 10:06:10.000000000","message":"Done","commit_id":"0d570c7a1b3a51c4687695dc1b920b8d18edd460"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"400958b72afaa5738a5d8442fc7433271e52e47d","unresolved":true,"context_lines":[{"line_number":9,"context_line":"During an MPU complete, the s3api first PUTs an SLO manifest and then"},{"line_number":10,"context_line":"DELETEs the upload marker in \u003cbucket\u003e+segments. If the proxy\u0027s clock"},{"line_number":11,"context_line":"is slow relative the proxy that created the upload marker, the DELETE"},{"line_number":12,"context_line":"will fail with a 409."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Previously the 409 would be returned to the client, which for some"},{"line_number":15,"context_line":"clients (e.g. aws cli) does not trigger a retry. This patch will"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"469d0580_6c2009ca","line":12,"in_reply_to":"cea902aa_2fe080e4","updated":"2023-11-28 10:43:53.000000000","message":"I estimated 5 seconds lag (based on the log timestamps). There were 6 parts uploaded.\n\nThe multi-stage request, or more precisely the use of multiple objects to represent an eventually-single-item-of-state, is problematic, and it would be good to address with ALOs, although I\u0027ve not yet had a great-but-simple idea!","commit_id":"0d570c7a1b3a51c4687695dc1b920b8d18edd460"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"c2a9b028eedbe263e9e53cfb85d0068f0d78fa6a","unresolved":true,"context_lines":[{"line_number":12,"context_line":"will fail with a 409."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Previously the 409 would be returned to the client, which for some"},{"line_number":15,"context_line":"clients (e.g. aws cli) does not trigger a retry. This patch will"},{"line_number":16,"context_line":"convert the 409 to a 503 and also return a \u0027Connection: close\u0027 header"},{"line_number":17,"context_line":"to provoke clients to retry, hopefully to another proxy."},{"line_number":18,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"9fe1e43a_97d71d37","line":15,"updated":"2023-11-22 22:58:47.000000000","message":"the option to just \"ignore\" the 409 and return success seems undesirable because it leaks the upload-marker and will continue to show the MPU as \"in-progress\" even tho the manifest has already been created.  Luckily the \"s3api-mpu-complete\" API is idempotent WRT to the existance of a manifest.","commit_id":"0d570c7a1b3a51c4687695dc1b920b8d18edd460"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"400958b72afaa5738a5d8442fc7433271e52e47d","unresolved":false,"context_lines":[{"line_number":12,"context_line":"will fail with a 409."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Previously the 409 would be returned to the client, which for some"},{"line_number":15,"context_line":"clients (e.g. aws cli) does not trigger a retry. This patch will"},{"line_number":16,"context_line":"convert the 409 to a 503 and also return a \u0027Connection: close\u0027 header"},{"line_number":17,"context_line":"to provoke clients to retry, hopefully to another proxy."},{"line_number":18,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"3f72d1dd_486e082c","line":15,"in_reply_to":"9fe1e43a_97d71d37","updated":"2023-11-28 10:43:53.000000000","message":"Acknowledged","commit_id":"0d570c7a1b3a51c4687695dc1b920b8d18edd460"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"c2a9b028eedbe263e9e53cfb85d0068f0d78fa6a","unresolved":true,"context_lines":[{"line_number":14,"context_line":"Previously the 409 would be returned to the client, which for some"},{"line_number":15,"context_line":"clients (e.g. aws cli) does not trigger a retry. This patch will"},{"line_number":16,"context_line":"convert the 409 to a 503 and also return a \u0027Connection: close\u0027 header"},{"line_number":17,"context_line":"to provoke clients to retry, hopefully to another proxy."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"Change-Id: Ie2e0cb75425e00cff533014af6b6fafad89bff94"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"835cd4b5_fda555a8","line":17,"updated":"2023-11-22 22:58:47.000000000","message":"I wonder if the \"try another proxy\" is as important as \"wait N sec for real-time to catch up with the accelerated clock\" so the DELETE can succeed.  My aws-cli was sleeping for ~0.5-3.0 seconds between each of 4 retries by default.  Just returning 503 managed to fix the issue up to ~10s clock drift in my testing.  Since the request that creates the delete-marker and the one that removes the delete-marker are different requests I don\u0027t think it matters much if proxy that\u0027s hitting the 409 is running behind or the one that created it was running ahead - as long as they wait and retry until the clock drift catches up with the delta.  But maybe it\u0027s a case of better safe than sorry - it *is* a fairly specific case; I would be significantly less comfortable with always sending connection close on 503.","commit_id":"0d570c7a1b3a51c4687695dc1b920b8d18edd460"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"400958b72afaa5738a5d8442fc7433271e52e47d","unresolved":true,"context_lines":[{"line_number":14,"context_line":"Previously the 409 would be returned to the client, which for some"},{"line_number":15,"context_line":"clients (e.g. aws cli) does not trigger a retry. This patch will"},{"line_number":16,"context_line":"convert the 409 to a 503 and also return a \u0027Connection: close\u0027 header"},{"line_number":17,"context_line":"to provoke clients to retry, hopefully to another proxy."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"Change-Id: Ie2e0cb75425e00cff533014af6b6fafad89bff94"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"450fcda1_e80c0ed6","line":17,"in_reply_to":"835cd4b5_fda555a8","updated":"2023-11-28 10:43:53.000000000","message":"What I observed in prod was that the client did retry, but presumably not with enough backoff (this was not aws cli).\n\nYour experiment is interesting though, and more realistic. When I reproduced on vsaio I used an internal client to create an upload marker with a ridiculously large clock skew, so I never saw the client \u0027wait long enough to catch up\u0027.","commit_id":"0d570c7a1b3a51c4687695dc1b920b8d18edd460"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"fec141912be1ce5a739b33dbbbf78600ec7f7cff","unresolved":true,"context_lines":[{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Previously the 409 would be returned to the client, which for some"},{"line_number":15,"context_line":"clients (e.g. aws cli) does not trigger a retry. This patch will"},{"line_number":16,"context_line":"convert the 409 to a 503 and also return a \u0027Connection: close\u0027 header"},{"line_number":17,"context_line":"to provoke clients to retry, hopefully to another proxy."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"Change-Id: Ie2e0cb75425e00cff533014af6b6fafad89bff94"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"f3ba8de0_2430d511","line":16,"range":{"start_line":16,"start_character":44,"end_line":16,"end_character":61},"updated":"2023-11-27 17:00:39.000000000","message":"I remember playing around with this sort of an idea a long time ago... maybe in https://review.opendev.org/c/openstack/swift/+/471993 ?\n\nAssuming the client is well-behaved and *does* close the connection pretty promptly -- the server\u0027s `ESTABLISHED` socket should get cleaned up pretty quickly, yeah?\n\nIf it doesn\u0027t... we\u0027re just waiting for a `keepalive_timeout`, right? And it\u0027s not really any *worse* than skipping the header entirely...\n\nOTOH, [PEP-3333](https://peps.python.org/pep-3333/#the-start-response-callable) says\n\n\u003e Applications and middleware are forbidden from using HTTP/1.1 “hop-by-hop” features or headers, any equivalent features in HTTP/1.0, or any headers that would affect the persistence of the client’s connection to the web server. These features are the exclusive province of the actual web server, and a server or gateway **should** consider it a fatal error for an application to attempt sending them, and raise an error if they are supplied to `start_response()`.\n\nso eventlet could justifiably start bombing out at any time if we do this...\n\nAlso, I saw in the notes of https://bugs.launchpad.net/swift/+bug/1680731 that we might need to watch out for\n\n\u003e if a client *sends* \u0027Connection: keep-alive\u0027 (which is redundant in http 1.1 but not invalid)\n\nas eventlet would add a `Connection: keep-alive` itself; I\u0027m not sure which one would/should be respected if we send back two conflicting `Connection` headers...","commit_id":"b49db1e96fe91548c4085026fae942336cfde9ad"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"bc34fefe6d5cff6a067c6b443d9394d44724159c","unresolved":false,"context_lines":[{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Previously the 409 would be returned to the client, which for some"},{"line_number":15,"context_line":"clients (e.g. aws cli) does not trigger a retry. This patch will"},{"line_number":16,"context_line":"convert the 409 to a 503 and also return a \u0027Connection: close\u0027 header"},{"line_number":17,"context_line":"to provoke clients to retry, hopefully to another proxy."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"Change-Id: Ie2e0cb75425e00cff533014af6b6fafad89bff94"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"0a28df54_65014129","line":16,"range":{"start_line":16,"start_character":44,"end_line":16,"end_character":61},"in_reply_to":"014cca29_2219ba93","updated":"2023-11-30 10:06:10.000000000","message":"Done","commit_id":"b49db1e96fe91548c4085026fae942336cfde9ad"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"400958b72afaa5738a5d8442fc7433271e52e47d","unresolved":true,"context_lines":[{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Previously the 409 would be returned to the client, which for some"},{"line_number":15,"context_line":"clients (e.g. aws cli) does not trigger a retry. This patch will"},{"line_number":16,"context_line":"convert the 409 to a 503 and also return a \u0027Connection: close\u0027 header"},{"line_number":17,"context_line":"to provoke clients to retry, hopefully to another proxy."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"Change-Id: Ie2e0cb75425e00cff533014af6b6fafad89bff94"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"014cca29_2219ba93","line":16,"range":{"start_line":16,"start_character":44,"end_line":16,"end_character":61},"in_reply_to":"f3ba8de0_2430d511","updated":"2023-11-28 10:43:53.000000000","message":"very good points, thank you","commit_id":"b49db1e96fe91548c4085026fae942336cfde9ad"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"c2a9b028eedbe263e9e53cfb85d0068f0d78fa6a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"af139ddd_30992652","updated":"2023-11-22 22:58:47.000000000","message":"409\u003d\u003e503 fix is needed; test seems solid.\n\nI am curious if we might try moving the fix down a layer to avoid the confusing \"DELETE returns BrokenMPU\" comment.\n\n901690: DNM: testing/debugging 409 mpu error | https://review.opendev.org/c/openstack/swift/+/901690\n\n(and maybe knock out a TODO in the process!)\n\nStill I would lean to +2, but I\u0027m not 100% sold on the connection: close behavior\n\nI\u0027m sure someone could also justify that we should have a lp bug in the commit message since it\u0027s client facing.  Sorry I couldn\u0027t be more help today!","commit_id":"0d570c7a1b3a51c4687695dc1b920b8d18edd460"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b5d746f01050845a1257b730cca7cc6c09560c85","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"0fac9192_6004418d","updated":"2023-11-27 17:01:49.000000000","message":"i had some comments i forgot to post","commit_id":"b49db1e96fe91548c4085026fae942336cfde9ad"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"400958b72afaa5738a5d8442fc7433271e52e47d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"4f60e631_b77fc31c","updated":"2023-11-28 10:43:53.000000000","message":"thanks for the feedback! I want to try out Tim\u0027s suggestion to bail out earlier, and ditch the connection:close which almost seems like a regression on reflection :/","commit_id":"b49db1e96fe91548c4085026fae942336cfde9ad"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1fb4dddbb812056cb454a7ac6ad096cbe1c986e3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"a567e7c3_11f3ea92","updated":"2023-11-29 14:41:40.000000000","message":"bug seems serious, fix seems legit\n\nDepending on how much clock skew we\u0027re dealing with and how quickly the client is getting from create-mpu to complete-mpu (or even how many segments need validation) I think the LBYL check might actaully be actively unhelpful.\n\nBut in my testing it made little difference, the first set of 4 retries might fail but a second attempt is going to work even up to rather huge time drift.  If we wanted to get really serious we might include a Retry-After header, but it\u0027s probably not that common of failure mode (which again makes the special case LBYL handling somewhat annoying, but probably not the most terrible thing happening in that already huge method).","commit_id":"63defd1430492c22ff80ab54b10ef61e21066f8b"}],"swift/common/middleware/s3api/controllers/multi_upload.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"6335398c3335218d993af548aa965200473f6adb","unresolved":true,"context_lines":[{"line_number":836,"context_line":"                    # to the proxy that created the upload marker; provoke the"},{"line_number":837,"context_line":"                    # client to retry using a different proxy."},{"line_number":838,"context_line":"                    resp.headers[\u0027Connection\u0027] \u003d \u0027close\u0027"},{"line_number":839,"context_line":"                    raise ServiceUnavailable"},{"line_number":840,"context_line":""},{"line_number":841,"context_line":"                yield _make_complete_body(req, s3_etag, yielded_anything)"},{"line_number":842,"context_line":"            except ErrorResponse as err_resp:"}],"source_content_type":"text/x-python","patch_set":2,"id":"63783ac5_10489448","line":839,"updated":"2023-11-22 16:31:53.000000000","message":"This feels like hoping the client will fix our mess :/ but maybe it is better than master.\n\nWhen we observed this happen in prod, the client did retry, but on the same connection. When I experiment with \u0027aws s3api complete-multipart-upload\u0027 it does not retry a 409, but does with a 503.\n\nA proxy running in the past can create the mpu manifest, but can\u0027t delete the upload marker, nor can it POST some metadata to the marker to flag it as done. So it\u0027s hard to see how it can return success for the MPU complete, despite the manifest being created, because the uploads listing will continue to show the upload as in progress.\n\nThe best I have come up (and it\u0027s not great) would be to write another \u0027upload done-marker\u0027 in the +segments bucket and have the uploads list hide uploads that have also a done-marker object.","commit_id":"30e06836b747c0d998fedcd02dfacb782cdb633f"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8af6446b4baef34b1eb800d11b002f6de8473dd2","unresolved":false,"context_lines":[{"line_number":836,"context_line":"                    # to the proxy that created the upload marker; provoke the"},{"line_number":837,"context_line":"                    # client to retry using a different proxy."},{"line_number":838,"context_line":"                    resp.headers[\u0027Connection\u0027] \u003d \u0027close\u0027"},{"line_number":839,"context_line":"                    raise ServiceUnavailable"},{"line_number":840,"context_line":""},{"line_number":841,"context_line":"                yield _make_complete_body(req, s3_etag, yielded_anything)"},{"line_number":842,"context_line":"            except ErrorResponse as err_resp:"}],"source_content_type":"text/x-python","patch_set":2,"id":"8859fb85_500bd63b","line":839,"in_reply_to":"58eb85e1_f9b7e7e6","updated":"2023-11-28 18:24:21.000000000","message":"Done","commit_id":"30e06836b747c0d998fedcd02dfacb782cdb633f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"c2a9b028eedbe263e9e53cfb85d0068f0d78fa6a","unresolved":true,"context_lines":[{"line_number":836,"context_line":"                    # to the proxy that created the upload marker; provoke the"},{"line_number":837,"context_line":"                    # client to retry using a different proxy."},{"line_number":838,"context_line":"                    resp.headers[\u0027Connection\u0027] \u003d \u0027close\u0027"},{"line_number":839,"context_line":"                    raise ServiceUnavailable"},{"line_number":840,"context_line":""},{"line_number":841,"context_line":"                yield _make_complete_body(req, s3_etag, yielded_anything)"},{"line_number":842,"context_line":"            except ErrorResponse as err_resp:"}],"source_content_type":"text/x-python","patch_set":2,"id":"58eb85e1_f9b7e7e6","line":839,"in_reply_to":"63783ac5_10489448","updated":"2023-11-22 22:58:47.000000000","message":"Part of the bug might be returning BrokenMPU when the marker clean-up fails.\n\nhttps://github.com/NVIDIA/swift/blob/master/swift/common/middleware/s3api/s3request.py#L1436\n\nFailure to cleanup \u003d\u003e ServerError seems reasonable, there\u0027s different ways we could achive that.  e.g. \n\n    diff --git a/swift/common/middleware/s3api/s3request.py b/swift/common/middleware/s3api/s3request.py\n    index 33f507628..23d6406e5 100644\n    --- a/swift/common/middleware/s3api/s3request.py\n    +++ b/swift/common/middleware/s3api/s3request.py\n    @@ -1432,8 +1432,8 @@ class S3Request(swob.Request):\n\t\t     raise SlowDown(status\u003d\u0027429 Slow Down\u0027)\n\t\t raise SlowDown()\n\t     if resp.status_int \u003d\u003d HTTP_CONFLICT:\n    -            # TODO: validate that this actually came up out of SLO\n    -            raise BrokenMPU()\n    +            if self.method \u003d\u003d \u0027GET\u0027:\n    +                raise BrokenMPU()\n     \n\t     raise InternalError(\u0027unexpected status code %d\u0027 % status)\n     \n\ncauses this DELETE call to raise 503, but I didn\u0027t see a log about \"unexpected status\" or anything.","commit_id":"30e06836b747c0d998fedcd02dfacb782cdb633f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"c2a9b028eedbe263e9e53cfb85d0068f0d78fa6a","unresolved":true,"context_lines":[{"line_number":835,"context_line":"                    # marker may occur if this proxy\u0027s clock is slow relative"},{"line_number":836,"context_line":"                    # to the proxy that created the upload marker; provoke the"},{"line_number":837,"context_line":"                    # client to retry using a different proxy."},{"line_number":838,"context_line":"                    resp.headers[\u0027Connection\u0027] \u003d \u0027close\u0027"},{"line_number":839,"context_line":"                    raise ServiceUnavailable"},{"line_number":840,"context_line":""},{"line_number":841,"context_line":"                yield _make_complete_body(req, s3_etag, yielded_anything)"}],"source_content_type":"text/x-python","patch_set":3,"id":"a7bd1b1e_8e38f731","line":838,"updated":"2023-11-22 22:58:47.000000000","message":"s3api clients may be well behaved WRT 503 connections and Connection: close headers.  When testing with aws cli I saw messages like:\n\n     2023-11-22 22:19:02,105 - MainThread - urllib3.connectionpool - DEBUG - Resetting dropped connection: saio\n\n... but only when the Connection: close header was returned\n\nThe logging seems reasonable, as when I tried to verifiy by watching netstat in a loop I saw a persistent connection across retries:\n\n    tcp        0      0 127.0.0.1:36592         127.0.0.1:8080          ESTABLISHED\n\nwhile as with Connection: close they\u0027d come and go","commit_id":"0d570c7a1b3a51c4687695dc1b920b8d18edd460"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"bc34fefe6d5cff6a067c6b443d9394d44724159c","unresolved":false,"context_lines":[{"line_number":835,"context_line":"                    # marker may occur if this proxy\u0027s clock is slow relative"},{"line_number":836,"context_line":"                    # to the proxy that created the upload marker; provoke the"},{"line_number":837,"context_line":"                    # client to retry using a different proxy."},{"line_number":838,"context_line":"                    resp.headers[\u0027Connection\u0027] \u003d \u0027close\u0027"},{"line_number":839,"context_line":"                    raise ServiceUnavailable"},{"line_number":840,"context_line":""},{"line_number":841,"context_line":"                yield _make_complete_body(req, s3_etag, yielded_anything)"}],"source_content_type":"text/x-python","patch_set":3,"id":"2785b8b2_0ff24e1a","line":838,"in_reply_to":"a7bd1b1e_8e38f731","updated":"2023-11-30 10:06:10.000000000","message":"Acknowledged","commit_id":"0d570c7a1b3a51c4687695dc1b920b8d18edd460"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"8538a49da5d8c23b33f389a39e0bec48642efc0b","unresolved":true,"context_lines":[{"line_number":656,"context_line":"        Handles Complete Multipart Upload."},{"line_number":657,"context_line":"        \"\"\""},{"line_number":658,"context_line":"        upload_id \u003d get_param(req, \u0027uploadId\u0027)"},{"line_number":659,"context_line":"        resp \u003d _get_upload_info(req, self.app, upload_id)"},{"line_number":660,"context_line":"        headers \u003d {\u0027Accept\u0027: \u0027application/json\u0027,"},{"line_number":661,"context_line":"                   sysmeta_header(\u0027object\u0027, \u0027upload-id\u0027): upload_id}"},{"line_number":662,"context_line":"        for key, val in resp.headers.items():"}],"source_content_type":"text/x-python","patch_set":4,"id":"8cc4ef66_877c457c","line":659,"updated":"2023-11-27 21:21:12.000000000","message":"It might be better for us to bomb out here, as soon as we see that the upload marker comes from the future...","commit_id":"b49db1e96fe91548c4085026fae942336cfde9ad"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8af6446b4baef34b1eb800d11b002f6de8473dd2","unresolved":false,"context_lines":[{"line_number":656,"context_line":"        Handles Complete Multipart Upload."},{"line_number":657,"context_line":"        \"\"\""},{"line_number":658,"context_line":"        upload_id \u003d get_param(req, \u0027uploadId\u0027)"},{"line_number":659,"context_line":"        resp \u003d _get_upload_info(req, self.app, upload_id)"},{"line_number":660,"context_line":"        headers \u003d {\u0027Accept\u0027: \u0027application/json\u0027,"},{"line_number":661,"context_line":"                   sysmeta_header(\u0027object\u0027, \u0027upload-id\u0027): upload_id}"},{"line_number":662,"context_line":"        for key, val in resp.headers.items():"}],"source_content_type":"text/x-python","patch_set":4,"id":"854e6915_5bd4dbfd","line":659,"in_reply_to":"3bd559fb_ee478255","updated":"2023-11-28 18:24:21.000000000","message":"Done","commit_id":"b49db1e96fe91548c4085026fae942336cfde9ad"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"400958b72afaa5738a5d8442fc7433271e52e47d","unresolved":true,"context_lines":[{"line_number":656,"context_line":"        Handles Complete Multipart Upload."},{"line_number":657,"context_line":"        \"\"\""},{"line_number":658,"context_line":"        upload_id \u003d get_param(req, \u0027uploadId\u0027)"},{"line_number":659,"context_line":"        resp \u003d _get_upload_info(req, self.app, upload_id)"},{"line_number":660,"context_line":"        headers \u003d {\u0027Accept\u0027: \u0027application/json\u0027,"},{"line_number":661,"context_line":"                   sysmeta_header(\u0027object\u0027, \u0027upload-id\u0027): upload_id}"},{"line_number":662,"context_line":"        for key, val in resp.headers.items():"}],"source_content_type":"text/x-python","patch_set":4,"id":"3bd559fb_ee478255","line":659,"in_reply_to":"8cc4ef66_877c457c","updated":"2023-11-28 10:43:53.000000000","message":"that\u0027s a good idea, although we might want to be a little more discerning because _get_upload_info falls back to HEADing the manifest...if the *manifest* already exists, but in the \"future\" then that\u0027s OK.","commit_id":"b49db1e96fe91548c4085026fae942336cfde9ad"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8af6446b4baef34b1eb800d11b002f6de8473dd2","unresolved":true,"context_lines":[{"line_number":669,"context_line":"        \"\"\""},{"line_number":670,"context_line":"        upload_id \u003d get_param(req, \u0027uploadId\u0027)"},{"line_number":671,"context_line":"        resp, is_marker \u003d _get_upload_info(req, self.app, upload_id)"},{"line_number":672,"context_line":"        if (is_marker and"},{"line_number":673,"context_line":"                resp.sw_headers.get(\u0027X-Backend-Timestamp\u0027) \u003e\u003d Timestamp.now()):"},{"line_number":674,"context_line":"            # Somehow the marker was created in the future w.r.t. this thread\u0027s"},{"line_number":675,"context_line":"            # clock. The manifest PUT may succeed but the subsequent marker"}],"source_content_type":"text/x-python","patch_set":5,"id":"46fa3037_139b12cc","line":672,"updated":"2023-11-28 18:24:21.000000000","message":"I\u0027m annoyed by having to return a tuple for the is_marker condition, but if the *manifest* has been created in the future then we\u0027d return 200 at line 765 so I don\u0027t want to turn that outcome into a 503 here.\n\nI could dig into the response to find the request path, and compare that with the marker path, but I opted for the more explicit tuple return.","commit_id":"63defd1430492c22ff80ab54b10ef61e21066f8b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1fb4dddbb812056cb454a7ac6ad096cbe1c986e3","unresolved":true,"context_lines":[{"line_number":669,"context_line":"        \"\"\""},{"line_number":670,"context_line":"        upload_id \u003d get_param(req, \u0027uploadId\u0027)"},{"line_number":671,"context_line":"        resp, is_marker \u003d _get_upload_info(req, self.app, upload_id)"},{"line_number":672,"context_line":"        if (is_marker and"},{"line_number":673,"context_line":"                resp.sw_headers.get(\u0027X-Backend-Timestamp\u0027) \u003e\u003d Timestamp.now()):"},{"line_number":674,"context_line":"            # Somehow the marker was created in the future w.r.t. this thread\u0027s"},{"line_number":675,"context_line":"            # clock. The manifest PUT may succeed but the subsequent marker"}],"source_content_type":"text/x-python","patch_set":5,"id":"f27ec914_19c9e69c","line":672,"in_reply_to":"46fa3037_139b12cc","updated":"2023-11-29 14:41:40.000000000","message":"the return tuple doesn\u0027t bother me that much given how dynamic the method is about fetching the info from either marker or manifest.","commit_id":"63defd1430492c22ff80ab54b10ef61e21066f8b"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"bc34fefe6d5cff6a067c6b443d9394d44724159c","unresolved":false,"context_lines":[{"line_number":669,"context_line":"        \"\"\""},{"line_number":670,"context_line":"        upload_id \u003d get_param(req, \u0027uploadId\u0027)"},{"line_number":671,"context_line":"        resp, is_marker \u003d _get_upload_info(req, self.app, upload_id)"},{"line_number":672,"context_line":"        if (is_marker and"},{"line_number":673,"context_line":"                resp.sw_headers.get(\u0027X-Backend-Timestamp\u0027) \u003e\u003d Timestamp.now()):"},{"line_number":674,"context_line":"            # Somehow the marker was created in the future w.r.t. this thread\u0027s"},{"line_number":675,"context_line":"            # clock. The manifest PUT may succeed but the subsequent marker"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ed1e3e1_0b3029f5","line":672,"in_reply_to":"f27ec914_19c9e69c","updated":"2023-11-30 10:06:10.000000000","message":"Acknowledged","commit_id":"63defd1430492c22ff80ab54b10ef61e21066f8b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1fb4dddbb812056cb454a7ac6ad096cbe1c986e3","unresolved":true,"context_lines":[{"line_number":670,"context_line":"        upload_id \u003d get_param(req, \u0027uploadId\u0027)"},{"line_number":671,"context_line":"        resp, is_marker \u003d _get_upload_info(req, self.app, upload_id)"},{"line_number":672,"context_line":"        if (is_marker and"},{"line_number":673,"context_line":"                resp.sw_headers.get(\u0027X-Backend-Timestamp\u0027) \u003e\u003d Timestamp.now()):"},{"line_number":674,"context_line":"            # Somehow the marker was created in the future w.r.t. this thread\u0027s"},{"line_number":675,"context_line":"            # clock. The manifest PUT may succeed but the subsequent marker"},{"line_number":676,"context_line":"            # DELETE will fail, so don\u0027t attempt either."}],"source_content_type":"text/x-python","patch_set":5,"id":"26ce0ffb_2cd86cbb","line":673,"updated":"2023-11-29 14:41:40.000000000","message":"probably better as\n\n    headers.get(\u0027X-Backend-Timestamp\u0027) \u003e\u003d req.timestamp\n\n... although I suppose that may require req.ensure_x_timestamp(); maybe the sans side-effect approach has merrit.\n    \nFWIW the timestamp has some magic in the `__lt__` and `__eq__` methods that allow @total_ordering to make the type comparison:\n\nhttps://github.com/NVIDIA/swift/blob/master/swift/common/utils/timestamp.py#L230-L252\n\n... but I may still prefer and explicit `Timestamp(headers.get())` for maximum clarity.","commit_id":"63defd1430492c22ff80ab54b10ef61e21066f8b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1fb4dddbb812056cb454a7ac6ad096cbe1c986e3","unresolved":true,"context_lines":[{"line_number":674,"context_line":"            # Somehow the marker was created in the future w.r.t. this thread\u0027s"},{"line_number":675,"context_line":"            # clock. The manifest PUT may succeed but the subsequent marker"},{"line_number":676,"context_line":"            # DELETE will fail, so don\u0027t attempt either."},{"line_number":677,"context_line":"            raise ServiceUnavailable"},{"line_number":678,"context_line":""},{"line_number":679,"context_line":"        headers \u003d {\u0027Accept\u0027: \u0027application/json\u0027,"},{"line_number":680,"context_line":"                   sysmeta_header(\u0027object\u0027, \u0027upload-id\u0027): upload_id}"}],"source_content_type":"text/x-python","patch_set":5,"id":"e59e7295_18b201b6","line":677,"updated":"2023-11-29 14:41:40.000000000","message":"I guess the LBYL here is acceptable, it certainly would be the case that an attempt to DELETE the upload marker would result in a 409 and our best response would be 503.\n\nDoing the segment validation might have the side effect of returning a more descriptive error if there actually is a problem; if we expect the client to retry with the same connection this mostly has the side effect of getting them to burn their retries faster.","commit_id":"63defd1430492c22ff80ab54b10ef61e21066f8b"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"bc34fefe6d5cff6a067c6b443d9394d44724159c","unresolved":true,"context_lines":[{"line_number":674,"context_line":"            # Somehow the marker was created in the future w.r.t. this thread\u0027s"},{"line_number":675,"context_line":"            # clock. The manifest PUT may succeed but the subsequent marker"},{"line_number":676,"context_line":"            # DELETE will fail, so don\u0027t attempt either."},{"line_number":677,"context_line":"            raise ServiceUnavailable"},{"line_number":678,"context_line":""},{"line_number":679,"context_line":"        headers \u003d {\u0027Accept\u0027: \u0027application/json\u0027,"},{"line_number":680,"context_line":"                   sysmeta_header(\u0027object\u0027, \u0027upload-id\u0027): upload_id}"}],"source_content_type":"text/x-python","patch_set":5,"id":"b2d07601_14ca840f","line":677,"in_reply_to":"e59e7295_18b201b6","updated":"2023-11-30 10:06:10.000000000","message":"IIUC the segment validation is done by the SLO PUT and I\u0027m trying to avoid the PUT even happening if the DELETE can\u0027t","commit_id":"63defd1430492c22ff80ab54b10ef61e21066f8b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1fb4dddbb812056cb454a7ac6ad096cbe1c986e3","unresolved":true,"context_lines":[{"line_number":761,"context_line":"            # This header should only already be present if the upload marker"},{"line_number":762,"context_line":"            # has been cleaned up and the current target uses the same"},{"line_number":763,"context_line":"            # upload-id; assuming the segments to use haven\u0027t changed, the work"},{"line_number":764,"context_line":"            # is already done"},{"line_number":765,"context_line":"            return HTTPOk(body\u003d_make_complete_body(req, s3_etag, False),"},{"line_number":766,"context_line":"                          content_type\u003d\u0027application/xml\u0027)"},{"line_number":767,"context_line":"        headers[s3_etag_header] \u003d s3_etag"}],"source_content_type":"text/x-python","patch_set":5,"id":"a0fe16b0_36b99e6b","line":764,"updated":"2023-11-29 14:41:40.000000000","message":"\u003e the work is already done\n\nsans possibly cleaning up a stale upload-marker; which I guess isn\u0027t strictly required but I believe the upload would continue to list as \"in-progress\"","commit_id":"63defd1430492c22ff80ab54b10ef61e21066f8b"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"bc34fefe6d5cff6a067c6b443d9394d44724159c","unresolved":false,"context_lines":[{"line_number":761,"context_line":"            # This header should only already be present if the upload marker"},{"line_number":762,"context_line":"            # has been cleaned up and the current target uses the same"},{"line_number":763,"context_line":"            # upload-id; assuming the segments to use haven\u0027t changed, the work"},{"line_number":764,"context_line":"            # is already done"},{"line_number":765,"context_line":"            return HTTPOk(body\u003d_make_complete_body(req, s3_etag, False),"},{"line_number":766,"context_line":"                          content_type\u003d\u0027application/xml\u0027)"},{"line_number":767,"context_line":"        headers[s3_etag_header] \u003d s3_etag"}],"source_content_type":"text/x-python","patch_set":5,"id":"31b1ec17_a96075ef","line":764,"in_reply_to":"a0fe16b0_36b99e6b","updated":"2023-11-30 10:06:10.000000000","message":"Acknowledged","commit_id":"63defd1430492c22ff80ab54b10ef61e21066f8b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1fb4dddbb812056cb454a7ac6ad096cbe1c986e3","unresolved":true,"context_lines":[{"line_number":848,"context_line":"                    # make sure the marker got cleaned up. If it\u0027s already"},{"line_number":849,"context_line":"                    # gone (e.g., because of concurrent completes or a retried"},{"line_number":850,"context_line":"                    # complete), so much the better."},{"line_number":851,"context_line":"                    pass"},{"line_number":852,"context_line":""},{"line_number":853,"context_line":"                yield _make_complete_body(req, s3_etag, yielded_anything)"},{"line_number":854,"context_line":"            except ErrorResponse as err_resp:"}],"source_content_type":"text/x-python","patch_set":5,"id":"40372221_7086a0f2","line":851,"updated":"2023-11-29 14:41:40.000000000","message":"maybe we could extract marker cleanup to a method and move it to a finally","commit_id":"63defd1430492c22ff80ab54b10ef61e21066f8b"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"bc34fefe6d5cff6a067c6b443d9394d44724159c","unresolved":true,"context_lines":[{"line_number":848,"context_line":"                    # make sure the marker got cleaned up. If it\u0027s already"},{"line_number":849,"context_line":"                    # gone (e.g., because of concurrent completes or a retried"},{"line_number":850,"context_line":"                    # complete), so much the better."},{"line_number":851,"context_line":"                    pass"},{"line_number":852,"context_line":""},{"line_number":853,"context_line":"                yield _make_complete_body(req, s3_etag, yielded_anything)"},{"line_number":854,"context_line":"            except ErrorResponse as err_resp:"}],"source_content_type":"text/x-python","patch_set":5,"id":"54be3c62_3614abe5","line":851,"in_reply_to":"40372221_7086a0f2","updated":"2023-11-30 10:06:10.000000000","message":"+1 I think there\u0027s other useful extractions, maybe a refactor follow up","commit_id":"63defd1430492c22ff80ab54b10ef61e21066f8b"}],"swift/common/middleware/s3api/s3request.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1fb4dddbb812056cb454a7ac6ad096cbe1c986e3","unresolved":true,"context_lines":[{"line_number":1435,"context_line":"            if self.method \u003d\u003d \u0027GET\u0027:"},{"line_number":1436,"context_line":"                raise BrokenMPU()"},{"line_number":1437,"context_line":"            else:"},{"line_number":1438,"context_line":"                raise ServiceUnavailable()"},{"line_number":1439,"context_line":""},{"line_number":1440,"context_line":"        raise InternalError(\u0027unexpected status code %d\u0027 % status)"},{"line_number":1441,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"be6f1d29_a4beee6b","line":1438,"updated":"2023-11-29 14:41:40.000000000","message":"why not just let this fall through to the catch all block?  i.e.\n\n    if conflict \u0026\u0026 GET:\n         raise BrokenMPU()","commit_id":"63defd1430492c22ff80ab54b10ef61e21066f8b"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"bc34fefe6d5cff6a067c6b443d9394d44724159c","unresolved":true,"context_lines":[{"line_number":1435,"context_line":"            if self.method \u003d\u003d \u0027GET\u0027:"},{"line_number":1436,"context_line":"                raise BrokenMPU()"},{"line_number":1437,"context_line":"            else:"},{"line_number":1438,"context_line":"                raise ServiceUnavailable()"},{"line_number":1439,"context_line":""},{"line_number":1440,"context_line":"        raise InternalError(\u0027unexpected status code %d\u0027 % status)"},{"line_number":1441,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"c5e5d9d2_3f6aa27a","line":1438,"in_reply_to":"be6f1d29_a4beee6b","updated":"2023-11-30 10:06:10.000000000","message":"1. ServiceUnavailable seemed more appropriate (particularly with the addition of a RetryAfter header - nice idea)\n\n2. consistency with the LBYL case (ok, so I could have gone for 500 in both cases)","commit_id":"63defd1430492c22ff80ab54b10ef61e21066f8b"}],"test/unit/common/middleware/s3api/test_multi_upload.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"c2a9b028eedbe263e9e53cfb85d0068f0d78fa6a","unresolved":true,"context_lines":[{"line_number":1661,"context_line":"                                     \u0027Date\u0027: self.get_date_header(), },"},{"line_number":1662,"context_line":"                            body\u003dXML)"},{"line_number":1663,"context_line":"        status, headers, body \u003d self.call_s3api(req)"},{"line_number":1664,"context_line":"        self.assertEqual(status.split()[0], \u0027503\u0027)"},{"line_number":1665,"context_line":"        self.assertEqual(\u0027ServiceUnavailable\u0027, self._get_error_code(body))"},{"line_number":1666,"context_line":"        self.assertEqual(\u0027close\u0027, headers.get(\u0027Connection\u0027))"},{"line_number":1667,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"eaa4e95e_756d8b0a","line":1664,"updated":"2023-11-22 22:58:47.000000000","message":"with the fix reverted we can see that we\u0027re returning 409 from the object-server response\n\n    E       AssertionError: \u0027409\u0027 !\u003d \u0027503\u0027\n    E       - 409\n    E       + 503\n\n    swift/test/unit/common/middleware/s3api/test_multi_upload.py:1664: AssertionError\n\nand that\u0027s definately not going to trigger the client backoff/retry behavior we want.  This is a good fix.","commit_id":"0d570c7a1b3a51c4687695dc1b920b8d18edd460"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8af6446b4baef34b1eb800d11b002f6de8473dd2","unresolved":false,"context_lines":[{"line_number":1661,"context_line":"                                     \u0027Date\u0027: self.get_date_header(), },"},{"line_number":1662,"context_line":"                            body\u003dXML)"},{"line_number":1663,"context_line":"        status, headers, body \u003d self.call_s3api(req)"},{"line_number":1664,"context_line":"        self.assertEqual(status.split()[0], \u0027503\u0027)"},{"line_number":1665,"context_line":"        self.assertEqual(\u0027ServiceUnavailable\u0027, self._get_error_code(body))"},{"line_number":1666,"context_line":"        self.assertEqual(\u0027close\u0027, headers.get(\u0027Connection\u0027))"},{"line_number":1667,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"0c02ccae_cf169e4b","line":1664,"in_reply_to":"eaa4e95e_756d8b0a","updated":"2023-11-28 18:24:21.000000000","message":"Acknowledged","commit_id":"0d570c7a1b3a51c4687695dc1b920b8d18edd460"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"c2a9b028eedbe263e9e53cfb85d0068f0d78fa6a","unresolved":true,"context_lines":[{"line_number":1663,"context_line":"        status, headers, body \u003d self.call_s3api(req)"},{"line_number":1664,"context_line":"        self.assertEqual(status.split()[0], \u0027503\u0027)"},{"line_number":1665,"context_line":"        self.assertEqual(\u0027ServiceUnavailable\u0027, self._get_error_code(body))"},{"line_number":1666,"context_line":"        self.assertEqual(\u0027close\u0027, headers.get(\u0027Connection\u0027))"},{"line_number":1667,"context_line":""},{"line_number":1668,"context_line":"    def test_object_multipart_upload_complete_old_content_type(self):"},{"line_number":1669,"context_line":"        self.swift.register_unconditionally("}],"source_content_type":"text/x-python","patch_set":3,"id":"6d63e2a7_2f5aede4","line":1666,"updated":"2023-11-22 22:58:47.000000000","message":"I\u0027m not convincned this is the right thing to do.  The main reason I think I might be-able to get behind it is because it\u0027s a *very* specific 503 condition: so what\u0027s the harm!?  But I\u0027m reminding of import this:\n\nSpecial cases aren\u0027t special enough to break the rules.\nAlthough practicality beats purity.\n\nAre we sure it\u0027s helpful in practice?  Maybe if the upload marker was created only a few ms ago in realtime on an in-sync proxy, but *this* connections clock is behind longer than the normal client retry/backoff/sleep?  IIRC the problem we actually saw was a few upload-markers created ahead of realtime in rare cases causing problems across the fleet.  In which case picking another proxy even won\u0027t help - you have to just wait until you catch up with PUT request x-timestamp before you can delete it (unless you get luck and snag the bad one!)","commit_id":"0d570c7a1b3a51c4687695dc1b920b8d18edd460"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"400958b72afaa5738a5d8442fc7433271e52e47d","unresolved":true,"context_lines":[{"line_number":1663,"context_line":"        status, headers, body \u003d self.call_s3api(req)"},{"line_number":1664,"context_line":"        self.assertEqual(status.split()[0], \u0027503\u0027)"},{"line_number":1665,"context_line":"        self.assertEqual(\u0027ServiceUnavailable\u0027, self._get_error_code(body))"},{"line_number":1666,"context_line":"        self.assertEqual(\u0027close\u0027, headers.get(\u0027Connection\u0027))"},{"line_number":1667,"context_line":""},{"line_number":1668,"context_line":"    def test_object_multipart_upload_complete_old_content_type(self):"},{"line_number":1669,"context_line":"        self.swift.register_unconditionally("}],"source_content_type":"text/x-python","patch_set":3,"id":"afbe331f_76bf8e4b","line":1666,"in_reply_to":"6d63e2a7_2f5aede4","updated":"2023-11-28 10:43:53.000000000","message":"The problem I saw was a single proxy running slow when trying to delete the marker, so trying another proxy would have worked. But you\u0027re right, if it had been single proxy running *fast* when it *created* the marker, then flipping to another proxy would be unlikely to fix anything.","commit_id":"0d570c7a1b3a51c4687695dc1b920b8d18edd460"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8af6446b4baef34b1eb800d11b002f6de8473dd2","unresolved":false,"context_lines":[{"line_number":1663,"context_line":"        status, headers, body \u003d self.call_s3api(req)"},{"line_number":1664,"context_line":"        self.assertEqual(status.split()[0], \u0027503\u0027)"},{"line_number":1665,"context_line":"        self.assertEqual(\u0027ServiceUnavailable\u0027, self._get_error_code(body))"},{"line_number":1666,"context_line":"        self.assertEqual(\u0027close\u0027, headers.get(\u0027Connection\u0027))"},{"line_number":1667,"context_line":""},{"line_number":1668,"context_line":"    def test_object_multipart_upload_complete_old_content_type(self):"},{"line_number":1669,"context_line":"        self.swift.register_unconditionally("}],"source_content_type":"text/x-python","patch_set":3,"id":"bc2a4283_a24a97f5","line":1666,"in_reply_to":"afbe331f_76bf8e4b","updated":"2023-11-28 18:24:21.000000000","message":"Done","commit_id":"0d570c7a1b3a51c4687695dc1b920b8d18edd460"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1fb4dddbb812056cb454a7ac6ad096cbe1c986e3","unresolved":true,"context_lines":[{"line_number":1673,"context_line":"        with patch(\u0027swift.common.middleware.s3api.controllers.multi_upload.\u0027"},{"line_number":1674,"context_line":"                   \u0027Timestamp.now\u0027, return_value\u003dnow_timestamp):"},{"line_number":1675,"context_line":"            status, headers, body \u003d self.call_s3api(req)"},{"line_number":1676,"context_line":"        self.assertEqual(status.split()[0], \u0027503\u0027)"},{"line_number":1677,"context_line":"        self.assertEqual(\u0027ServiceUnavailable\u0027, self._get_error_code(body))"},{"line_number":1678,"context_line":"        self.assertEqual(self.swift.calls, ["},{"line_number":1679,"context_line":"            (\u0027HEAD\u0027, \u0027/v1/AUTH_test\u0027),"}],"source_content_type":"text/x-python","patch_set":5,"id":"2ea1e4df_5ca1239d","line":1676,"updated":"2023-11-29 14:41:40.000000000","message":"ok so everyone that calls this helper expects a 503","commit_id":"63defd1430492c22ff80ab54b10ef61e21066f8b"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"bc34fefe6d5cff6a067c6b443d9394d44724159c","unresolved":false,"context_lines":[{"line_number":1673,"context_line":"        with patch(\u0027swift.common.middleware.s3api.controllers.multi_upload.\u0027"},{"line_number":1674,"context_line":"                   \u0027Timestamp.now\u0027, return_value\u003dnow_timestamp):"},{"line_number":1675,"context_line":"            status, headers, body \u003d self.call_s3api(req)"},{"line_number":1676,"context_line":"        self.assertEqual(status.split()[0], \u0027503\u0027)"},{"line_number":1677,"context_line":"        self.assertEqual(\u0027ServiceUnavailable\u0027, self._get_error_code(body))"},{"line_number":1678,"context_line":"        self.assertEqual(self.swift.calls, ["},{"line_number":1679,"context_line":"            (\u0027HEAD\u0027, \u0027/v1/AUTH_test\u0027),"}],"source_content_type":"text/x-python","patch_set":5,"id":"81772ce8_4fd58343","line":1676,"in_reply_to":"2ea1e4df_5ca1239d","updated":"2023-11-30 10:06:10.000000000","message":"Acknowledged","commit_id":"63defd1430492c22ff80ab54b10ef61e21066f8b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1fb4dddbb812056cb454a7ac6ad096cbe1c986e3","unresolved":true,"context_lines":[{"line_number":1687,"context_line":""},{"line_number":1688,"context_line":"    def test_object_multipart_upload_complete_marker_ts_in_future(self):"},{"line_number":1689,"context_line":"        marker_timestamp \u003d Timestamp.now()"},{"line_number":1690,"context_line":"        now_timestamp \u003d Timestamp(float(marker_timestamp) - 1)"},{"line_number":1691,"context_line":"        self._do_test_object_multipart_upload_complete_marker_in_future("},{"line_number":1692,"context_line":"            marker_timestamp, now_timestamp)"},{"line_number":1693,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"1d975257_96001068","line":1690,"updated":"2023-11-29 14:41:40.000000000","message":"heh, maybe marker_timestamp +\u003d 1 would match the test title better but I guess time\u0027s all relative!","commit_id":"63defd1430492c22ff80ab54b10ef61e21066f8b"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"bc34fefe6d5cff6a067c6b443d9394d44724159c","unresolved":true,"context_lines":[{"line_number":1687,"context_line":""},{"line_number":1688,"context_line":"    def test_object_multipart_upload_complete_marker_ts_in_future(self):"},{"line_number":1689,"context_line":"        marker_timestamp \u003d Timestamp.now()"},{"line_number":1690,"context_line":"        now_timestamp \u003d Timestamp(float(marker_timestamp) - 1)"},{"line_number":1691,"context_line":"        self._do_test_object_multipart_upload_complete_marker_in_future("},{"line_number":1692,"context_line":"            marker_timestamp, now_timestamp)"},{"line_number":1693,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"57230699_2e9eccf8","line":1690,"in_reply_to":"1d975257_96001068","updated":"2023-11-30 10:06:10.000000000","message":"fair point, but it does depend on whose version of the present you\u0027re thinking in :) I guess I was thinking in terms of how I saw it in prod - the marker was in the correct present, the complete req perceived that to be in the future! 😊","commit_id":"63defd1430492c22ff80ab54b10ef61e21066f8b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1fb4dddbb812056cb454a7ac6ad096cbe1c986e3","unresolved":true,"context_lines":[{"line_number":1705,"context_line":"                            body\u003dXML)"},{"line_number":1706,"context_line":"        status, headers, body \u003d self.call_s3api(req)"},{"line_number":1707,"context_line":"        self.assertEqual(status.split()[0], \u0027503\u0027)"},{"line_number":1708,"context_line":"        self.assertEqual(\u0027ServiceUnavailable\u0027, self._get_error_code(body))"},{"line_number":1709,"context_line":""},{"line_number":1710,"context_line":"    def test_object_multipart_upload_complete_old_content_type(self):"},{"line_number":1711,"context_line":"        self.swift.register_unconditionally("}],"source_content_type":"text/x-python","patch_set":5,"id":"a7277e44_9798900c","line":1708,"updated":"2023-11-29 14:41:40.000000000","message":"this would probably be \"sufficient\" even w/o the LBYL check, and fails as you\u0027d expect if i revert the fix in s3request.py\n\n    ___________________________________________________ TestS3ApiMultiUpload.test_object_multipart_upload_complete_409_on_marker_delete ___________________________________________________\n\n    self \u003d \u003ctest.unit.common.middleware.s3api.test_multi_upload.TestS3ApiMultiUpload testMethod\u003dtest_object_multipart_upload_complete_409_on_marker_delete\u003e\n\n        def test_object_multipart_upload_complete_409_on_marker_delete(self):\n            # verify that clock skew preventing an upload marker DELETE results in\n            # a 503 (this would be unexpected because there\u0027s a check for clock\n            # skew before the manifest PUT and marker DELETE)\n            segment_bucket \u003d \u0027/v1/AUTH_test/bucket+segments\u0027\n            self.swift.register(\u0027DELETE\u0027, segment_bucket + \u0027/object/X\u0027,\n                                swob.HTTPConflict, {}, None)\n            req \u003d Request.blank(\u0027/bucket/object?uploadId\u003dX\u0027,\n                                environ\u003d{\u0027REQUEST_METHOD\u0027: \u0027POST\u0027},\n                                headers\u003d{\u0027Authorization\u0027: \u0027AWS test:tester:hmac\u0027,\n                                         \u0027Date\u0027: self.get_date_header(), },\n                                body\u003dXML)\n            status, headers, body \u003d self.call_s3api(req)\n    \u003e       self.assertEqual(status.split()[0], \u0027503\u0027)\n    E       AssertionError: \u0027409\u0027 !\u003d \u0027503\u0027\n    E       - 409\n    E       + 503\n\n    swift/test/unit/common/middleware/s3api/test_multi_upload.py:1707: AssertionError","commit_id":"63defd1430492c22ff80ab54b10ef61e21066f8b"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"bc34fefe6d5cff6a067c6b443d9394d44724159c","unresolved":false,"context_lines":[{"line_number":1705,"context_line":"                            body\u003dXML)"},{"line_number":1706,"context_line":"        status, headers, body \u003d self.call_s3api(req)"},{"line_number":1707,"context_line":"        self.assertEqual(status.split()[0], \u0027503\u0027)"},{"line_number":1708,"context_line":"        self.assertEqual(\u0027ServiceUnavailable\u0027, self._get_error_code(body))"},{"line_number":1709,"context_line":""},{"line_number":1710,"context_line":"    def test_object_multipart_upload_complete_old_content_type(self):"},{"line_number":1711,"context_line":"        self.swift.register_unconditionally("}],"source_content_type":"text/x-python","patch_set":5,"id":"72515438_238dfdaf","line":1708,"in_reply_to":"a7277e44_9798900c","updated":"2023-11-30 10:06:10.000000000","message":"Acknowledged","commit_id":"63defd1430492c22ff80ab54b10ef61e21066f8b"}]}