)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"5f1e59d78520aa078b1452a34df76ed1ea8020dc","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Container-Sync to perform HEAD before PUT object on remote"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Author: oshritf \u003coshritf@il.ibm.com\u003e"},{"line_number":10,"context_line":""},{"line_number":11,"context_line":"This change adds a remote HEAD object request before each call to"},{"line_number":12,"context_line":"sync_row."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"5a5ae5dd_8877ce01","line":9,"updated":"2016-02-03 16:20:13.000000000","message":"not necessary, authorship is automatically recorded in git","commit_id":"e7e380ffe5705c7370d3dad5044d3613a8b00477"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"5f1e59d78520aa078b1452a34df76ed1ea8020dc","unresolved":false,"context_lines":[{"line_number":11,"context_line":"This change adds a remote HEAD object request before each call to"},{"line_number":12,"context_line":"sync_row."},{"line_number":13,"context_line":"The check if the object exists on the remote eliminates both"},{"line_number":14,"context_line":"heavy and unneeded remote put and local get object calls"},{"line_number":15,"context_line":"PUT occurs when remote holds an old object or in case object does not"},{"line_number":16,"context_line":"exists"},{"line_number":17,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"5a5ae5dd_c854e655","line":14,"updated":"2016-02-03 16:20:13.000000000","message":"nit I think a period \u0027.\u0027 is missing at end of the line ^^","commit_id":"e7e380ffe5705c7370d3dad5044d3613a8b00477"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":13,"context_line":"object to be transferred on the wire several times (depending on the replication factor)"},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"An alternative to HEAD is to do a conditional PUT (using, 100-continue)."},{"line_number":16,"context_line":"However, this change is more involved and requires upgrade of both the client and server side clusters to work."},{"line_number":17,"context_line":"In the Tokyo design summit it was decided to start with the HEAD approach."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"Change-Id: I60d982dd2cc79a0f13b0924507cd03d7f9c9d70b"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":12,"id":"fa0719c6_b32b0830","line":16,"updated":"2016-03-21 20:15:17.000000000","message":"nit: commit message is easier to read if line wrapped at around 70","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":18,"context_line":""},{"line_number":19,"context_line":"Change-Id: I60d982dd2cc79a0f13b0924507cd03d7f9c9d70b"},{"line_number":20,"context_line":"Closes-Bug: #1277223"},{"line_number":21,"context_line":"Closes-Bug: #1491908"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":12,"id":"fa0719c6_932c044c","line":21,"updated":"2016-03-21 20:15:17.000000000","message":"Its not clear to me how this patch fixes this bug - if indeed it is a bug. This patch does not change the 202 response from proxy when seeing 409s from backend.","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"}],"swift/common/internal_client.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b190a481278c57848759864dc41224bf277f9e2c","unresolved":false,"context_lines":[{"line_number":770,"context_line":"        conn \u003d urllib2.urlopen(req, timeout\u003dtimeout)"},{"line_number":771,"context_line":"        body \u003d conn.read()"},{"line_number":772,"context_line":"        try:"},{"line_number":773,"context_line":"            body_headers \u003d conn.info()"},{"line_number":774,"context_line":"            body_data \u003d json.loads(body)"},{"line_number":775,"context_line":"        except ValueError:"},{"line_number":776,"context_line":"            body_data \u003d None"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a5ae5dd_7287e561","line":773,"updated":"2016-02-03 16:17:45.000000000","message":"move this outside of the try clause, it does not raise a ValueError.\n\nIn fact, the variable body_headers is not needed, just return conn.info() at line 802. conn.info() is also used at line 796.","commit_id":"e7e380ffe5705c7370d3dad5044d3613a8b00477"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":799,"context_line":"                    trans_stop - trans_start,"},{"line_number":800,"context_line":"                    additional_info"},{"line_number":801,"context_line":"                )))"},{"line_number":802,"context_line":"        return [info, body_data]"},{"line_number":803,"context_line":""},{"line_number":804,"context_line":"    def retry_request(self, method, **kwargs):"},{"line_number":805,"context_line":"        retries \u003d kwargs.pop(\u0027retries\u0027, self.retries)"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa0719c6_84796096","line":802,"updated":"2016-03-21 20:15:17.000000000","message":"OK, it looks like none of the existing callers into internal client care about the first return value","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"}],"swift/container/sync.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b190a481278c57848759864dc41224bf277f9e2c","unresolved":false,"context_lines":[{"line_number":412,"context_line":"    def _object_in_remote_container(self, row, sync_to, user_key,"},{"line_number":413,"context_line":"                                    broker, realm, realm_key):"},{"line_number":414,"context_line":"        \"\"\""},{"line_number":415,"context_line":"        Performs head object on remote to eliminates extra remote put and"},{"line_number":416,"context_line":"        local get object calls"},{"line_number":417,"context_line":"        :param row: The updated row in the local database triggering the sync"},{"line_number":418,"context_line":"                    update."}],"source_content_type":"text/x-python","patch_set":5,"id":"5a5ae5dd_1249d631","line":415,"updated":"2016-02-03 16:17:45.000000000","message":"s/eliminates/eliminate/","commit_id":"e7e380ffe5705c7370d3dad5044d3613a8b00477"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b190a481278c57848759864dc41224bf277f9e2c","unresolved":false,"context_lines":[{"line_number":413,"context_line":"                                    broker, realm, realm_key):"},{"line_number":414,"context_line":"        \"\"\""},{"line_number":415,"context_line":"        Performs head object on remote to eliminates extra remote put and"},{"line_number":416,"context_line":"        local get object calls"},{"line_number":417,"context_line":"        :param row: The updated row in the local database triggering the sync"},{"line_number":418,"context_line":"                    update."},{"line_number":419,"context_line":"        :param sync_to: The URL to the remote container."}],"source_content_type":"text/x-python","patch_set":5,"id":"5a5ae5dd_7205aac0","line":416,"updated":"2016-02-03 16:17:45.000000000","message":"nit: usual to have an empty line before first param definition","commit_id":"e7e380ffe5705c7370d3dad5044d3613a8b00477"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b190a481278c57848759864dc41224bf277f9e2c","unresolved":false,"context_lines":[{"line_number":420,"context_line":"        :param user_key: The X-Container-Sync-Key to use when sending requests"},{"line_number":421,"context_line":"                         to the other container."},{"line_number":422,"context_line":"        :param broker: The local container database broker."},{"line_number":423,"context_line":"        :param info: The get_info result from the local container database"},{"line_number":424,"context_line":"                     broker."},{"line_number":425,"context_line":"        :param realm: The realm from self.realms_conf, if there is one."},{"line_number":426,"context_line":"            If None, fallback to using the older allowed_sync_hosts"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a5ae5dd_32265269","line":423,"updated":"2016-02-03 16:17:45.000000000","message":"param info not in method signature","commit_id":"e7e380ffe5705c7370d3dad5044d3613a8b00477"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b190a481278c57848759864dc41224bf277f9e2c","unresolved":false,"context_lines":[{"line_number":449,"context_line":"                                         proxy\u003dself.select_http_proxy(),"},{"line_number":450,"context_line":"                                         logger\u003dself.logger,"},{"line_number":451,"context_line":"                                         retries\u003d0)"},{"line_number":452,"context_line":"            # Compare with remote copy - if local is newer update remote"},{"line_number":453,"context_line":"            remote_ts \u003d metadata.get(\u0027x-timestamp\u0027, 0)"},{"line_number":454,"context_line":"            if Timestamp(row[\u0027created_at\u0027]) \u003c\u003d Timestamp(remote_ts):"},{"line_number":455,"context_line":"                return True"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a5ae5dd_32fe526c","line":452,"updated":"2016-02-03 16:17:45.000000000","message":"alternatively, use an if-modified-since header so that the remote end does the time comparison, with the (small) optimisation of not shipping back all the object metadata when the remote object is up to date - just get a 304 response status.","commit_id":"e7e380ffe5705c7370d3dad5044d3613a8b00477"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b190a481278c57848759864dc41224bf277f9e2c","unresolved":false,"context_lines":[{"line_number":451,"context_line":"                                         retries\u003d0)"},{"line_number":452,"context_line":"            # Compare with remote copy - if local is newer update remote"},{"line_number":453,"context_line":"            remote_ts \u003d metadata.get(\u0027x-timestamp\u0027, 0)"},{"line_number":454,"context_line":"            if Timestamp(row[\u0027created_at\u0027]) \u003c\u003d Timestamp(remote_ts):"},{"line_number":455,"context_line":"                return True"},{"line_number":456,"context_line":"            return False"},{"line_number":457,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"5a5ae5dd_a3d8b254","line":454,"updated":"2016-02-03 16:17:45.000000000","message":"This makes a subtle change to the condition for making a PUT request. \n\nOn master, if the timestamp, t_newest, of the newest local object (result of GETs) is \u003e remote timestamp then the PUT will succeed. \n\nWith this change, if the timestamp in the created_at column is \u003c remote timestamp then no PUT is attempted, but created_at may be \u003c t_newest when container updates have failed.","commit_id":"e7e380ffe5705c7370d3dad5044d3613a8b00477"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b190a481278c57848759864dc41224bf277f9e2c","unresolved":false,"context_lines":[{"line_number":513,"context_line":"                    row, sync_to, user_key, broker, realm,"},{"line_number":514,"context_line":"                    realm_key)"},{"line_number":515,"context_line":"                if object_exists_remotely is True:"},{"line_number":516,"context_line":"                    return True"},{"line_number":517,"context_line":"                part, nodes \u003d \\"},{"line_number":518,"context_line":"                    self.get_object_ring(info[\u0027storage_policy_index\u0027]). \\"},{"line_number":519,"context_line":"                    get_nodes(info[\u0027account\u0027], info[\u0027container\u0027],"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a5ae5dd_d2bb3e0d","line":516,"updated":"2016-02-03 16:17:45.000000000","message":"simplifies to:\n\n  if self._object_in_remote_container(\n      row, sync_to, user_key, broker, realm,\n      realm_key):\n      return True","commit_id":"e7e380ffe5705c7370d3dad5044d3613a8b00477"},{"author":{"_account_id":12193,"name":"Hisashi Osanai","email":"osanai.hisashi@jp.fujitsu.com","username":"ho"},"change_message_id":"20e9fb5a51c7c2e478ebda7e4866396811e53acd","unresolved":false,"context_lines":[{"line_number":431,"context_line":"        \"\"\""},{"line_number":432,"context_line":"        headers \u003d {}"},{"line_number":433,"context_line":"        formated_time \u003d strftime(\"%a, %d %b %Y %H:%M:%S GMT\","},{"line_number":434,"context_line":"                                 gmtime(Timestamp(row[\u0027created_at\u0027])))"},{"line_number":435,"context_line":"        headers[\u0027if-modified-since\u0027] \u003d formated_time"},{"line_number":436,"context_line":"        self.logger.debug(\"modified since header %s\" % formated_time)"},{"line_number":437,"context_line":"        if realm and realm_key:"}],"source_content_type":"text/x-python","patch_set":8,"id":"bae84128_4eea2d75","line":434,"range":{"start_line":434,"start_character":50,"end_line":434,"end_character":67},"updated":"2016-02-24 06:38:12.000000000","message":"Should we use a timestamp which stores in local object?","commit_id":"d63bf01b308aad52c1a869554048c79ec4a2000a"},{"author":{"_account_id":12193,"name":"Hisashi Osanai","email":"osanai.hisashi@jp.fujitsu.com","username":"ho"},"change_message_id":"20e9fb5a51c7c2e478ebda7e4866396811e53acd","unresolved":false,"context_lines":[{"line_number":432,"context_line":"        headers \u003d {}"},{"line_number":433,"context_line":"        formated_time \u003d strftime(\"%a, %d %b %Y %H:%M:%S GMT\","},{"line_number":434,"context_line":"                                 gmtime(Timestamp(row[\u0027created_at\u0027])))"},{"line_number":435,"context_line":"        headers[\u0027if-modified-since\u0027] \u003d formated_time"},{"line_number":436,"context_line":"        self.logger.debug(\"modified since header %s\" % formated_time)"},{"line_number":437,"context_line":"        if realm and realm_key:"},{"line_number":438,"context_line":"            nonce \u003d uuid.uuid4().hex"}],"source_content_type":"text/x-python","patch_set":8,"id":"bae84128_4e850dcd","line":435,"range":{"start_line":435,"start_character":8,"end_line":435,"end_character":52},"updated":"2016-02-24 06:38:12.000000000","message":"Is X-Newest necessary to check other objects?","commit_id":"d63bf01b308aad52c1a869554048c79ec4a2000a"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"868d63c58e768a37521c3c310a6e5c9696015f75","unresolved":false,"context_lines":[{"line_number":420,"context_line":"        :param sync_to: The URL to the remote container."},{"line_number":421,"context_line":"        :param user_key: The X-Container-Sync-Key to use when sending requests"},{"line_number":422,"context_line":"                         to the other container."},{"line_number":423,"context_line":"        :param broker: The local container database broker."},{"line_number":424,"context_line":"        :param realm: The realm from self.realms_conf, if there is one."},{"line_number":425,"context_line":"            If None, fallback to using the older allowed_sync_hosts"},{"line_number":426,"context_line":"            way of syncing."}],"source_content_type":"text/x-python","patch_set":11,"id":"3afc51ec_15cc7134","line":423,"updated":"2016-03-11 18:50:38.000000000","message":"broker is not used in method","commit_id":"a94db65f25633a193490ad9dbe5ae61eaa6c555d"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"64132651f334d9afb26ad550d00afc94fec39919","unresolved":false,"context_lines":[{"line_number":431,"context_line":"        \"\"\""},{"line_number":432,"context_line":"        headers \u003d {}"},{"line_number":433,"context_line":"        formated_time \u003d strftime(\"%a, %d %b %Y %H:%M:%S GMT\","},{"line_number":434,"context_line":"                                 gmtime(Timestamp(row[\u0027created_at\u0027])))"},{"line_number":435,"context_line":"        headers[\u0027if-modified-since\u0027] \u003d formated_time"},{"line_number":436,"context_line":"        self.logger.debug(\"modified since header %s\" % formated_time)"},{"line_number":437,"context_line":"        if realm and realm_key:"}],"source_content_type":"text/x-python","patch_set":11,"id":"5aef4532_91bde36a","line":434,"updated":"2016-03-08 23:31:39.000000000","message":"this blows up with fast-post composite timestamps\n\ncontainer-6021: ERROR Syncing /srv/node2/sdb2/containers/457/dfc/7263e3bf5e3d669bbf0cae915d8b4dfc/7263e3bf5e3d669bbf0cae915d8b4dfc.db {\u0027name\u0027: \u0027object-2bf5a821-5584-4c56-93c6-2b2420b115b6\u0027, \u0027deleted\u0027: 0, \u0027created_at\u0027: \u00271457476621.84434+69b9e+0\u0027, \u0027storage_policy_index\u0027: 0, \u0027etag\u0027: \u002770b68ae721a61941a1a62724dde5d5e4\u0027, \u0027content_type\u0027: \u0027image/jpeg\u0027, \u0027ROWID\u0027: 2, \u0027size\u0027: 9}: #012Traceback (most recent call last):#012  File \"/vagrant/swift/swift/container/sync.py\", line 525, in container_sync_row#012    realm_key):#012  File \"/vagrant/swift/swift/container/sync.py\", line 434, in _object_in_remote_container#012    gmtime(Timestamp(row[\u0027created_at\u0027])))#012  File \"/vagrant/swift/swift/common/utils.py\", line 767, in __init__#012    self.timestamp \u003d float(parts.pop(0))#012ValueError: invalid literal for float(): 1457476621.84434+69b9e+0\n\n\nOn my machine the probetest caught it -\u003e\n\nnosetests swift/test/probe/test_container_sync.py:TestContainerSync.test_sync_with_fast_post","commit_id":"a94db65f25633a193490ad9dbe5ae61eaa6c555d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"868d63c58e768a37521c3c310a6e5c9696015f75","unresolved":false,"context_lines":[{"line_number":431,"context_line":"        \"\"\""},{"line_number":432,"context_line":"        headers \u003d {}"},{"line_number":433,"context_line":"        formated_time \u003d strftime(\"%a, %d %b %Y %H:%M:%S GMT\","},{"line_number":434,"context_line":"                                 gmtime(Timestamp(row[\u0027created_at\u0027])))"},{"line_number":435,"context_line":"        headers[\u0027if-modified-since\u0027] \u003d formated_time"},{"line_number":436,"context_line":"        self.logger.debug(\"modified since header %s\" % formated_time)"},{"line_number":437,"context_line":"        if realm and realm_key:"}],"source_content_type":"text/x-python","patch_set":11,"id":"3afc51ec_d555e91d","line":434,"in_reply_to":"5aef4532_91bde36a","updated":"2016-03-11 18:50:38.000000000","message":"I suggest that instead of passing the row, you pass the ts_meta timestamp and name to this method\n\nor\n\nyou could modify the row using _transform_record() from swift/container/backend.py ... but that could get confusing","commit_id":"a94db65f25633a193490ad9dbe5ae61eaa6c555d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"868d63c58e768a37521c3c310a6e5c9696015f75","unresolved":false,"context_lines":[{"line_number":445,"context_line":"                                                             nonce,"},{"line_number":446,"context_line":"                                                             sig)"},{"line_number":447,"context_line":"        else:"},{"line_number":448,"context_line":"            headers[\u0027x-container-sync-key\u0027] \u003d user_key"},{"line_number":449,"context_line":"        try:"},{"line_number":450,"context_line":"            head_object(sync_to, name\u003drow[\u0027name\u0027],"},{"line_number":451,"context_line":"                        headers\u003dheaders,"}],"source_content_type":"text/x-python","patch_set":11,"id":"3afc51ec_d8e4003d","line":448,"updated":"2016-03-11 18:50:38.000000000","message":"lines 437 to 448: a very similar block of code appears 3 times including here, so would be good to refactor to a helper method","commit_id":"a94db65f25633a193490ad9dbe5ae61eaa6c555d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"868d63c58e768a37521c3c310a6e5c9696015f75","unresolved":false,"context_lines":[{"line_number":453,"context_line":"                        logger\u003dself.logger,"},{"line_number":454,"context_line":"                        retries\u003d0)"},{"line_number":455,"context_line":"            # Object in remote should be updated"},{"line_number":456,"context_line":"            return False"},{"line_number":457,"context_line":"        except ClientException as http_err:"},{"line_number":458,"context_line":"            # Remote has a newer version or updated object"},{"line_number":459,"context_line":"            if http_err.http_status \u003d\u003d 304:"}],"source_content_type":"text/x-python","patch_set":11,"id":"3afc51ec_186828f5","line":456,"updated":"2016-03-11 18:50:38.000000000","message":"response status is 200 if the remote object is newer than if-modified-since, so this should return True??","commit_id":"a94db65f25633a193490ad9dbe5ae61eaa6c555d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"868d63c58e768a37521c3c310a6e5c9696015f75","unresolved":false,"context_lines":[{"line_number":456,"context_line":"            return False"},{"line_number":457,"context_line":"        except ClientException as http_err:"},{"line_number":458,"context_line":"            # Remote has a newer version or updated object"},{"line_number":459,"context_line":"            if http_err.http_status \u003d\u003d 304:"},{"line_number":460,"context_line":"                return True"},{"line_number":461,"context_line":"            # Object not in remote"},{"line_number":462,"context_line":"            if http_err.http_status \u003d\u003d 404:"}],"source_content_type":"text/x-python","patch_set":11,"id":"3afc51ec_5849108b","line":459,"updated":"2016-03-11 18:50:38.000000000","message":"response status is 304 if remote object is at same time as if-modified-since or older","commit_id":"a94db65f25633a193490ad9dbe5ae61eaa6c555d"},{"author":{"_account_id":11317,"name":"Eran Rom","email":"eran@itsonlyme.name","username":"eranr"},"change_message_id":"eef6667782524bcc728eae74817c073d7e5e1ba4","unresolved":false,"context_lines":[{"line_number":456,"context_line":"            return False"},{"line_number":457,"context_line":"        except ClientException as http_err:"},{"line_number":458,"context_line":"            # Remote has a newer version or updated object"},{"line_number":459,"context_line":"            if http_err.http_status \u003d\u003d 304:"},{"line_number":460,"context_line":"                return True"},{"line_number":461,"context_line":"            # Object not in remote"},{"line_number":462,"context_line":"            if http_err.http_status \u003d\u003d 404:"}],"source_content_type":"text/x-python","patch_set":11,"id":"3afc51ec_b85e3be7","line":459,"in_reply_to":"3afc51ec_5849108b","updated":"2016-03-12 21:03:05.000000000","message":"I agree this is problematic. But first lets agree on the desired returned values:\n1. For 200 we need to return True so as NOT to replicate (indeed looks like we have a bug in line 456)\n2. For 304 As Alistair mentions its not decisive: 304 is returned if the remote last-modified \u003c\u003d if-modified-since\nNow, for last-modified \u003c if-modified-since we would like to return False (meaning the object does not exist remotely and we need to replicate)\nand for last-modified \u003d\u003d if-modified-since we want to return True (meaning timestamps are equal no need to replicate).\nSo it is not clear what if-modified-since \u0027buys\u0027 us esp. with the change to SimpleClient we should have access to all the information we need on the remote object:\nX-Timestamp - ts.normal\nX-Backend-Timestamp - ts.internal\nlast_modified - math.ceil(ts)\nLet\u0027s just use them to determine what we need to return.","commit_id":"a94db65f25633a193490ad9dbe5ae61eaa6c555d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"0fe734343bf406c0c823b9ed2d24c10cc066a282","unresolved":false,"context_lines":[{"line_number":456,"context_line":"            return False"},{"line_number":457,"context_line":"        except ClientException as http_err:"},{"line_number":458,"context_line":"            # Remote has a newer version or updated object"},{"line_number":459,"context_line":"            if http_err.http_status \u003d\u003d 304:"},{"line_number":460,"context_line":"                return True"},{"line_number":461,"context_line":"            # Object not in remote"},{"line_number":462,"context_line":"            if http_err.http_status \u003d\u003d 404:"}],"source_content_type":"text/x-python","patch_set":11,"id":"3afc51ec_a27253ab","line":459,"in_reply_to":"3afc51ec_b85e3be7","updated":"2016-03-14 17:17:42.000000000","message":"Agree. I think it was my bad idea to suggest if-modified-since in a previous review. Sorry.","commit_id":"a94db65f25633a193490ad9dbe5ae61eaa6c555d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"868d63c58e768a37521c3c310a6e5c9696015f75","unresolved":false,"context_lines":[{"line_number":489,"context_line":"            start_time \u003d time()"},{"line_number":490,"context_line":"            # extract last modified time from the created_at value"},{"line_number":491,"context_line":"            ts_data, ts_ctype, ts_meta \u003d decode_timestamps("},{"line_number":492,"context_line":"                row[\u0027created_at\u0027])"},{"line_number":493,"context_line":"            if row[\u0027deleted\u0027]:"},{"line_number":494,"context_line":"                # when sync\u0027ing a deleted object, use ts_data - this is the"},{"line_number":495,"context_line":"                # timestamp of the source tombstone"}],"source_content_type":"text/x-python","patch_set":11,"id":"3afc51ec_35188d84","line":492,"updated":"2016-03-11 18:50:38.000000000","message":"here you have the distinct timestamp values, ts_meta being the last modified time\n\nnote that ts_meta is an instance of the Timestamp class","commit_id":"a94db65f25633a193490ad9dbe5ae61eaa6c555d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"868d63c58e768a37521c3c310a6e5c9696015f75","unresolved":false,"context_lines":[{"line_number":517,"context_line":"                self.logger.increment(\u0027deletes\u0027)"},{"line_number":518,"context_line":"                self.logger.timing_since(\u0027deletes.timing\u0027, start_time)"},{"line_number":519,"context_line":"            else:"},{"line_number":520,"context_line":"                if self._object_in_remote_container(row,"},{"line_number":521,"context_line":"                                                    sync_to,"},{"line_number":522,"context_line":"                                                    user_key,"},{"line_number":523,"context_line":"                                                    broker,"}],"source_content_type":"text/x-python","patch_set":11,"id":"3afc51ec_983718e3","line":520,"updated":"2016-03-11 18:50:38.000000000","message":"consider wrapping the line:\n\n  if self._object_in_remote_container(\n      row, sync_to, .... )","commit_id":"a94db65f25633a193490ad9dbe5ae61eaa6c555d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":409,"context_line":"            self.logger.exception(_(\u0027ERROR Syncing %s\u0027),"},{"line_number":410,"context_line":"                                  broker if broker else path)"},{"line_number":411,"context_line":""},{"line_number":412,"context_line":"    def _update_sync_to_headers(self, sync_to, name, realm_key, user_key,"},{"line_number":413,"context_line":"                                realm, method, headers):"},{"line_number":414,"context_line":"        \"\"\""},{"line_number":415,"context_line":"        Updates container sync headers"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa0719c6_42957be2","line":412,"updated":"2016-03-21 20:15:17.000000000","message":"nit: I think it is possible to make this a method that returns a headers dict rather than modify in place the supplied headers. Modify in place is not wrong (I have done it!) but can be harder to follow and therefore maintain.","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":427,"context_line":"        :param method: HTTP method to create sig with"},{"line_number":428,"context_line":"        :param headers: headers to update with container sync headers"},{"line_number":429,"context_line":""},{"line_number":430,"context_line":"        :returns: True if object already exists in remote"},{"line_number":431,"context_line":"        \"\"\""},{"line_number":432,"context_line":"        if realm and realm_key:"},{"line_number":433,"context_line":"            nonce \u003d uuid.uuid4().hex"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa0719c6_917c0774","line":430,"updated":"2016-03-21 20:15:17.000000000","message":"delete the returns doc line","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":438,"context_line":"                                           user_key)"},{"line_number":439,"context_line":"            headers[\u0027x-container-sync-auth\u0027] \u003d \u0027%s %s %s\u0027 % (realm,"},{"line_number":440,"context_line":"                                                             nonce,"},{"line_number":441,"context_line":"                                                             sig)"},{"line_number":442,"context_line":"        else:"},{"line_number":443,"context_line":"            headers[\u0027x-container-sync-key\u0027] \u003d user_key"},{"line_number":444,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"fa0719c6_825833fc","line":441,"updated":"2016-03-21 20:15:17.000000000","message":"unnecessary line wrapping","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":462,"context_line":"            allowed_sync_hosts way of syncing."},{"line_number":463,"context_line":"        :returns: True if object already exists in remote"},{"line_number":464,"context_line":"        \"\"\""},{"line_number":465,"context_line":"        headers \u003d {}"},{"line_number":466,"context_line":"        headers[\u0027x-timestamp\u0027] \u003d timestamp.internal"},{"line_number":467,"context_line":"        self._update_sync_to_headers(sync_to, name, realm_key,"},{"line_number":468,"context_line":"                                     user_key, realm, \u0027HEAD\u0027, headers)"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa0719c6_ecf9284f","line":465,"updated":"2016-03-21 20:15:17.000000000","message":"can be simpler:\n\nheaders{\u0027x-timestamp\u0027: timestamp.internal}","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"},{"author":{"_account_id":11317,"name":"Eran Rom","email":"eran@itsonlyme.name","username":"eranr"},"change_message_id":"598d22d6e53ffbd161aa1ea945cdf581e68b1761","unresolved":false,"context_lines":[{"line_number":463,"context_line":"        :returns: True if object already exists in remote"},{"line_number":464,"context_line":"        \"\"\""},{"line_number":465,"context_line":"        headers \u003d {}"},{"line_number":466,"context_line":"        headers[\u0027x-timestamp\u0027] \u003d timestamp.internal"},{"line_number":467,"context_line":"        self._update_sync_to_headers(sync_to, name, realm_key,"},{"line_number":468,"context_line":"                                     user_key, realm, \u0027HEAD\u0027, headers)"},{"line_number":469,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":12,"id":"1af94dfe_433548f5","line":466,"updated":"2016-03-17 07:49:31.000000000","message":"We may want to put here:\nheaders[\u0027X-Newest\u0027] instead of x-timestamp\nbut lets wait to hear from Alistair and Clay","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":463,"context_line":"        :returns: True if object already exists in remote"},{"line_number":464,"context_line":"        \"\"\""},{"line_number":465,"context_line":"        headers \u003d {}"},{"line_number":466,"context_line":"        headers[\u0027x-timestamp\u0027] \u003d timestamp.internal"},{"line_number":467,"context_line":"        self._update_sync_to_headers(sync_to, name, realm_key,"},{"line_number":468,"context_line":"                                     user_key, realm, \u0027HEAD\u0027, headers)"},{"line_number":469,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa0719c6_ddb6983c","line":466,"in_reply_to":"1af94dfe_433548f5","updated":"2016-03-21 20:15:17.000000000","message":"Seems that x-timestamp must be sent to keep the container sync auth signature checking happy.\n\n\nSetting x-newest is more interesting - it will cause the HEAD to propagate, sequentially, to 2x replica servers in remote cluster. There\u0027s a trade off between the time that takes (and extra intra-cluster request traffic) vs the possibility of sync\u0027ing what turns out to be an obsolete object, resulting in a 409 and costing wasted inter-cluster traffic.\n\nWhen the remote object is self-consistent, then we get a huge reduction in inter-cluster traffic from this patch without sending x-newest.\n\nWhen the remote object is not self-consistent, we may send unnecessary inter-cluster bytes, but that is no worse than on master (well, slightly worse because we did a HEAD and still sent unnecessary bytes).\n\nI think we discussed at hackathon a learning mode - i.e. if the number of 409s from PUTs is excessive, then start using x-newest, because that perhaps suggests that the remote cluster has experienced a failure and has inconsistent objects. But that is for a follow-on patch, if ever!","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":517,"context_line":"                # timestamp of the source tombstone"},{"line_number":518,"context_line":"                try:"},{"line_number":519,"context_line":"                    headers \u003d {\u0027x-timestamp\u0027: ts_data.internal}"},{"line_number":520,"context_line":"                    self._update_sync_to_headers(sync_to, row[\u0027name\u0027],"},{"line_number":521,"context_line":"                                                 realm_key, user_key,"},{"line_number":522,"context_line":"                                                 realm, \u0027DELETE\u0027, headers)"},{"line_number":523,"context_line":"                    delete_object(sync_to, name\u003drow[\u0027name\u0027], headers\u003dheaders,"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa0719c6_825af381","line":520,"updated":"2016-03-21 20:15:17.000000000","message":"nit: it can make code easier to read if the order of args in method signatures within a class or module are somewhat consistent. SO here, although not all args are common to container_sync_row and _update_sync_to_headers, those that are could be in same order:\n\n  _update_sync_to_headers(sync_to, user_key, realm, realm_key, name, method,...)","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":531,"context_line":"                self.logger.increment(\u0027deletes\u0027)"},{"line_number":532,"context_line":"                self.logger.timing_since(\u0027deletes.timing\u0027, start_time)"},{"line_number":533,"context_line":"            else:"},{"line_number":534,"context_line":"                if self._object_in_remote_container(row[\u0027name\u0027], ts_meta,"},{"line_number":535,"context_line":"                                                    sync_to, user_key, realm,"},{"line_number":536,"context_line":"                                                    realm_key):"},{"line_number":537,"context_line":"                    return True"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa0719c6_a251f764","line":534,"updated":"2016-03-21 20:15:17.000000000","message":"nit: as above re. arg ordering","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":536,"context_line":"                                                    realm_key):"},{"line_number":537,"context_line":"                    return True"},{"line_number":538,"context_line":"                # when sync\u0027ing a live object, use ts_meta - this is the time"},{"line_number":539,"context_line":"                # at which the source object was last modified by a PUT or POST"},{"line_number":540,"context_line":"                part, nodes \u003d \\"},{"line_number":541,"context_line":"                    self.get_object_ring(info[\u0027storage_policy_index\u0027]). \\"},{"line_number":542,"context_line":"                    get_nodes(info[\u0027account\u0027], info[\u0027container\u0027],"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa0719c6_629ebf57","line":539,"updated":"2016-03-21 20:15:17.000000000","message":"I think it makes sense to keep this comment right after the else i.e. at line 534","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"c7ddf886a811955341f66bef90df08b1c89378c1","unresolved":false,"context_lines":[{"line_number":448,"context_line":"            allowed_sync_hosts way of syncing."},{"line_number":449,"context_line":"        :returns: True if object already exists in remote"},{"line_number":450,"context_line":"        \"\"\""},{"line_number":451,"context_line":"        headers \u003d {\u0027x-timestamp\u0027:timestamp.internal}"},{"line_number":452,"context_line":"        self._update_sync_to_headers(name, sync_to, user_key, realm,"},{"line_number":453,"context_line":"                                     realm_key, \u0027HEAD\u0027, headers)"},{"line_number":454,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":13,"id":"fa0719c6_702d71a7","line":451,"updated":"2016-03-23 17:56:09.000000000","message":"space needed after :","commit_id":"12f5e75f2fc379177ac0e23324798377f43a1792"}],"test/probe/test_container_sync.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":276,"context_line":""},{"line_number":277,"context_line":"        # upload to dest with same name"},{"line_number":278,"context_line":"        client.put_object(self.url, self.token, dest_container, object_name,"},{"line_number":279,"context_line":"                          \u0027new-test-body\u0027)"},{"line_number":280,"context_line":""},{"line_number":281,"context_line":"        # cycle container-sync"},{"line_number":282,"context_line":"        Manager([\u0027container-sync\u0027]).once()"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa0719c6_9888a324","line":279,"updated":"2016-03-21 20:15:17.000000000","message":"nit: we could do a head here to get the dest timestamp...","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":284,"context_line":"        # verify that the remote object did not change"},{"line_number":285,"context_line":"        resp_headers, body \u003d client.get_object(self.url, self.token,"},{"line_number":286,"context_line":"                                               dest_container, object_name)"},{"line_number":287,"context_line":"        self.assertEqual(body, \u0027new-test-body\u0027)"},{"line_number":288,"context_line":""},{"line_number":289,"context_line":""},{"line_number":290,"context_line":"if __name__ \u003d\u003d \"__main__\":"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa0719c6_788dcf30","line":287,"updated":"2016-03-21 20:15:17.000000000","message":"...and then here we could check that the timestamp is unchanged too.","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"}],"test/unit/common/test_internal_client.py":[{"author":{"_account_id":11317,"name":"Eran Rom","email":"eran@itsonlyme.name","username":"eranr"},"change_message_id":"d2ddea74ede365d631a7444668216d0b5b09702f","unresolved":false,"context_lines":[{"line_number":1192,"context_line":"    return mock_time_value"},{"line_number":1193,"context_line":""},{"line_number":1194,"context_line":""},{"line_number":1195,"context_line":"class TestSimpleClient(unittest.TestCase):"},{"line_number":1196,"context_line":""},{"line_number":1197,"context_line":"    @mock.patch(\u0027eventlet.green.urllib2.urlopen\u0027)"},{"line_number":1198,"context_line":"    @mock.patch(\u0027eventlet.green.urllib2.Request\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"7a5de9d1_711603a9","line":1195,"updated":"2016-02-03 07:01:36.000000000","message":"For regression sake, lets add an explicit test for head, similar to test_get below","commit_id":"e7e380ffe5705c7370d3dad5044d3613a8b00477"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b190a481278c57848759864dc41224bf277f9e2c","unresolved":false,"context_lines":[{"line_number":1192,"context_line":"    return mock_time_value"},{"line_number":1193,"context_line":""},{"line_number":1194,"context_line":""},{"line_number":1195,"context_line":"class TestSimpleClient(unittest.TestCase):"},{"line_number":1196,"context_line":""},{"line_number":1197,"context_line":"    @mock.patch(\u0027eventlet.green.urllib2.urlopen\u0027)"},{"line_number":1198,"context_line":"    @mock.patch(\u0027eventlet.green.urllib2.Request\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a5ae5dd_48ba36e3","line":1195,"in_reply_to":"7a5de9d1_711603a9","updated":"2016-02-03 16:17:45.000000000","message":"+1 good thought since it is the HEAD you really care about returning headers (although maybe not -see my comment about using an if-modified-since header with the HEAD)\n\nYou could investigate parameterising the existing test here,\nso rename test_get as _test_get_head(self, method) then have test_get call self._test_get_head(\u0027GET\u0027) etc...","commit_id":"e7e380ffe5705c7370d3dad5044d3613a8b00477"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":1257,"context_line":""},{"line_number":1258,"context_line":"    @mock.patch(\u0027eventlet.green.urllib2.urlopen\u0027)"},{"line_number":1259,"context_line":"    @mock.patch(\u0027eventlet.green.urllib2.Request\u0027)"},{"line_number":1260,"context_line":"    @mock.patch(\u0027swift.common.internal_client.time\u0027, mock_time)"},{"line_number":1261,"context_line":"    def test_get(self, request, urlopen):"},{"line_number":1262,"context_line":"        self._test_get_head(request, urlopen, \u0027GET\u0027, 0)"},{"line_number":1263,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"fa0719c6_ca1401f7","line":1260,"updated":"2016-03-21 20:15:17.000000000","message":"the mocks could be applied to _test_get_head","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":1265,"context_line":"    @mock.patch(\u0027eventlet.green.urllib2.Request\u0027)"},{"line_number":1266,"context_line":"    @mock.patch(\u0027swift.common.internal_client.time\u0027, mock_time)"},{"line_number":1267,"context_line":"    def test_head(self, request, urlopen):"},{"line_number":1268,"context_line":"        self._test_get_head(request, urlopen, \u0027HEAD\u0027, 12)"},{"line_number":1269,"context_line":""},{"line_number":1270,"context_line":"    @mock.patch(\u0027eventlet.green.urllib2.urlopen\u0027)"},{"line_number":1271,"context_line":"    @mock.patch(\u0027eventlet.green.urllib2.Request\u0027)"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa0719c6_4ac63168","line":1268,"updated":"2016-03-21 20:15:17.000000000","message":"this test fails if run in isolation - it is relying on the previous test to increment the global mocked time 11 times. It should be using its won independent mocked time object.","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"c7ddf886a811955341f66bef90df08b1c89378c1","unresolved":false,"context_lines":[{"line_number":1186,"context_line":""},{"line_number":1187,"context_line":"class TestSimpleClient(unittest.TestCase):"},{"line_number":1188,"context_line":""},{"line_number":1189,"context_line":"    def _test_get_head(self, request, urlopen, method, mocked_time_offset):"},{"line_number":1190,"context_line":""},{"line_number":1191,"context_line":"        mock_time_value \u003d [1401224049.98]"},{"line_number":1192,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"fa0719c6_2d7e0219","line":1189,"updated":"2016-03-23 17:56:09.000000000","message":"please remove mocked_time_offset. It is always set to 0 and the variable makes the test code harder to read.\n\nNow you have moved the mock_time function inside the test method the rest of the changes are unnecessary apart from replacing the None info return value with {\u0027content-length\u0027: \u0027345\u0027}","commit_id":"12f5e75f2fc379177ac0e23324798377f43a1792"}],"test/unit/container/test_sync.py":[{"author":{"_account_id":11317,"name":"Eran Rom","email":"eran@itsonlyme.name","username":"eranr"},"change_message_id":"d2ddea74ede365d631a7444668216d0b5b09702f","unresolved":false,"context_lines":[{"line_number":916,"context_line":"                {\u0027deleted\u0027: False,"},{"line_number":917,"context_line":"                 \u0027name\u0027: \u0027object\u0027,"},{"line_number":918,"context_line":"                 \u0027created_at\u0027: \u00271.2\u0027,"},{"line_number":919,"context_line":"                 \u0027size\u0027: 50}, \u0027http://sync/to/path\u0027,"},{"line_number":920,"context_line":"                \u0027key\u0027, FakeContainerBroker(\u0027broker\u0027),"},{"line_number":921,"context_line":"                {\u0027account\u0027: \u0027a\u0027, \u0027container\u0027: \u0027c\u0027, \u0027storage_policy_index\u0027: 0},"},{"line_number":922,"context_line":"                realm, realm_key))"}],"source_content_type":"text/x-python","patch_set":5,"id":"7a5de9d1_93f2aa6e","line":919,"updated":"2016-02-03 07:01:36.000000000","message":"Seems like the size field is redundant. All tests are happy without it. This is true for all tests (12 instances altogether)","commit_id":"e7e380ffe5705c7370d3dad5044d3613a8b00477"},{"author":{"_account_id":11317,"name":"Eran Rom","email":"eran@itsonlyme.name","username":"eranr"},"change_message_id":"d2ddea74ede365d631a7444668216d0b5b09702f","unresolved":false,"context_lines":[{"line_number":1059,"context_line":"            self.assertEqual(cs.container_puts, 2)"},{"line_number":1060,"context_line":"            self.assertEqual(cs.container_failures, 5)"},{"line_number":1061,"context_line":"            self.assertLogMessage(\u0027error\u0027, \u0027ERROR Syncing\u0027)"},{"line_number":1062,"context_line":""},{"line_number":1063,"context_line":"            # Test the following cases:"},{"line_number":1064,"context_line":"            # head_object has more up to date copy and a put doesnt take place"},{"line_number":1065,"context_line":"            # head_object returns ClientException(404) and a put takes place"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a5ae5dd_fdfe6204","line":1062,"updated":"2016-02-03 07:01:36.000000000","message":"Test cases are good. I suggest to break this into a different function and use mock.patch instead of try - catch - finally pattern when overriding functions. Breaking to a new func will make this much easier.\nAlso, I do not think that we need to mock uuid and shuffle","commit_id":"e7e380ffe5705c7370d3dad5044d3613a8b00477"},{"author":{"_account_id":11317,"name":"Eran Rom","email":"eran@itsonlyme.name","username":"eranr"},"change_message_id":"d2ddea74ede365d631a7444668216d0b5b09702f","unresolved":false,"context_lines":[{"line_number":1061,"context_line":"            self.assertLogMessage(\u0027error\u0027, \u0027ERROR Syncing\u0027)"},{"line_number":1062,"context_line":""},{"line_number":1063,"context_line":"            # Test the following cases:"},{"line_number":1064,"context_line":"            # head_object has more up to date copy and a put doesnt take place"},{"line_number":1065,"context_line":"            # head_object returns ClientException(404) and a put takes place"},{"line_number":1066,"context_line":"            # head_object returns other ClientException put doesnt take place"},{"line_number":1067,"context_line":"            # and we get failure"}],"source_content_type":"text/x-python","patch_set":5,"id":"7a5de9d1_9f4e2b95","line":1064,"updated":"2016-02-03 07:01:36.000000000","message":"There is also head_object has the same time, and put does not take place","commit_id":"e7e380ffe5705c7370d3dad5044d3613a8b00477"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b190a481278c57848759864dc41224bf277f9e2c","unresolved":false,"context_lines":[{"line_number":1068,"context_line":"            # head_object returns other Exception put does not take place"},{"line_number":1069,"context_line":"            # and we get failure"},{"line_number":1070,"context_line":"            def fake_put_object(*args, **kwargs):"},{"line_number":1071,"context_line":"                pass"},{"line_number":1072,"context_line":""},{"line_number":1073,"context_line":"            def fake_head_object(*args, **kwargs):"},{"line_number":1074,"context_line":"                return ({\u0027x-timestamp\u0027: \u00271.2\u0027}, \u0027\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a5ae5dd_754fcf92","line":1071,"updated":"2016-02-03 16:17:45.000000000","message":"see later comment, how about:\n\n  actual_puts \u003d []\n  def fake_put_object(*args, **kwargs):\n      actual_puts.append((args, kwargs))\n\nthen later:\n\n  self.assertEqual(2, actual_puts)\n\n(you can achieve similar using mock.patch and inspecting the patched object\u0027s call_list, but this is probably simpler)","commit_id":"e7e380ffe5705c7370d3dad5044d3613a8b00477"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b190a481278c57848759864dc41224bf277f9e2c","unresolved":false,"context_lines":[{"line_number":1070,"context_line":"            def fake_put_object(*args, **kwargs):"},{"line_number":1071,"context_line":"                pass"},{"line_number":1072,"context_line":""},{"line_number":1073,"context_line":"            def fake_head_object(*args, **kwargs):"},{"line_number":1074,"context_line":"                return ({\u0027x-timestamp\u0027: \u00271.2\u0027}, \u0027\u0027)"},{"line_number":1075,"context_line":""},{"line_number":1076,"context_line":"            sync.put_object \u003d fake_put_object"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a5ae5dd_881d8e68","line":1073,"updated":"2016-02-03 16:17:45.000000000","message":"as above, then assert how many actual_heads were made.","commit_id":"e7e380ffe5705c7370d3dad5044d3613a8b00477"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b190a481278c57848759864dc41224bf277f9e2c","unresolved":false,"context_lines":[{"line_number":1077,"context_line":"            sync.head_object \u003d fake_head_object"},{"line_number":1078,"context_line":"            cs._object_in_remote_container \u003d orig_object_in_rcontainer"},{"line_number":1079,"context_line":"            self.assertTrue(cs.container_sync_row("},{"line_number":1080,"context_line":"                {\u0027deleted\u0027: False,"},{"line_number":1081,"context_line":"                 \u0027name\u0027: \u0027object\u0027,"},{"line_number":1082,"context_line":"                 \u0027created_at\u0027: \u00271.2\u0027,"},{"line_number":1083,"context_line":"                 \u0027size\u0027: 50,"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a5ae5dd_28aa5ad5","line":1080,"updated":"2016-02-03 16:17:45.000000000","message":"many of these row dict definitions appear to be identical - you could define once as a variable","commit_id":"e7e380ffe5705c7370d3dad5044d3613a8b00477"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b190a481278c57848759864dc41224bf277f9e2c","unresolved":false,"context_lines":[{"line_number":1086,"context_line":"                {\u0027account\u0027: \u0027a\u0027, \u0027container\u0027: \u0027c\u0027, \u0027storage_policy_index\u0027: 0},"},{"line_number":1087,"context_line":"                realm, realm_key))"},{"line_number":1088,"context_line":"            # No additional put has taken place"},{"line_number":1089,"context_line":"            self.assertEqual(cs.container_puts, 2)"},{"line_number":1090,"context_line":"            # No additional errors"},{"line_number":1091,"context_line":"            self.assertEqual(cs.container_failures, 5)"},{"line_number":1092,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"5a5ae5dd_f5517f63","line":1089,"updated":"2016-02-03 16:17:45.000000000","message":"I understand that you are following the existing test patterns, but since you have a fake_put_object function, you could capture a list of put requests in that function and then assert that the number is as expected. That is much closer to verifying the number of PUT requests on the wire.\n\nThe assertion of cs.container_puts is ok, but some bug could creep in that causes cs.container_puts to not actually equal the number of PUT requests.","commit_id":"e7e380ffe5705c7370d3dad5044d3613a8b00477"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b190a481278c57848759864dc41224bf277f9e2c","unresolved":false,"context_lines":[{"line_number":1139,"context_line":"                \u0027key\u0027, FakeContainerBroker(\u0027broker\u0027),"},{"line_number":1140,"context_line":"                {\u0027account\u0027: \u0027a\u0027, \u0027container\u0027: \u0027c\u0027, \u0027storage_policy_index\u0027: 0},"},{"line_number":1141,"context_line":"                realm, realm_key))"},{"line_number":1142,"context_line":"            # No additional put has taken place, failures increaed"},{"line_number":1143,"context_line":"            self.assertEqual(cs.container_puts, 3)"},{"line_number":1144,"context_line":"            self.assertEqual(cs.container_failures, 6)"},{"line_number":1145,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"5a5ae5dd_484d5671","line":1142,"updated":"2016-02-03 16:17:45.000000000","message":"s/increaed/increased/\n\nand elsewhere","commit_id":"e7e380ffe5705c7370d3dad5044d3613a8b00477"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":1046,"context_line":"                realm, realm_key))"},{"line_number":1047,"context_line":"            self.assertEqual(cs.container_puts, expected_put_count)"},{"line_number":1048,"context_line":"            excepted_failure_count +\u003d 1"},{"line_number":1049,"context_line":"            self.assertEqual(cs.container_failures, excepted_failure_count)"},{"line_number":1050,"context_line":"            self.assertLogMessage(\u0027info\u0027, \u0027Unauth\u0027)"},{"line_number":1051,"context_line":""},{"line_number":1052,"context_line":"            def fake_put_object(*args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa0719c6_38e357a5","line":1049,"updated":"2016-03-21 20:15:17.000000000","message":"could we make this assertion at every time expected_failures increases, otherwise if this assertion fails we do not know which of the preceding operations failed to register the failure.","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":1081,"context_line":"            excepted_failure_count +\u003d 1"},{"line_number":1082,"context_line":"            self.assertLogMessage(\u0027error\u0027, \u0027ERROR Syncing\u0027)"},{"line_number":1083,"context_line":""},{"line_number":1084,"context_line":"            # Test the following cases:"},{"line_number":1085,"context_line":"            # head_object returns ClientException(404) and a put takes place"},{"line_number":1086,"context_line":"            # head_object returns other ClientException put doesnt take place"},{"line_number":1087,"context_line":"            # and we get failure"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa0719c6_78682ff6","line":1084,"updated":"2016-03-21 20:15:17.000000000","message":"nit: This method is already long, and is getting longer...is there much overhead to making a new test method here? or do the following assertions rely on state that has been created before this point?","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":1101,"context_line":"            def fake_put_object(*args, **kwargs):"},{"line_number":1102,"context_line":"                actual_puts.append((args, kwargs))"},{"line_number":1103,"context_line":""},{"line_number":1104,"context_line":"            def fake_head_object(*args, **kwargs):"},{"line_number":1105,"context_line":"                raise ClientException(\u0027test client exception\u0027, http_status\u003d404)"},{"line_number":1106,"context_line":""},{"line_number":1107,"context_line":"            cs._object_in_remote_container \u003d orig_object_in_rcontainer"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa0719c6_d36ddc05","line":1104,"updated":"2016-03-21 20:15:17.000000000","message":"nit: consider a helper method to create these fake functions","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":1104,"context_line":"            def fake_head_object(*args, **kwargs):"},{"line_number":1105,"context_line":"                raise ClientException(\u0027test client exception\u0027, http_status\u003d404)"},{"line_number":1106,"context_line":""},{"line_number":1107,"context_line":"            cs._object_in_remote_container \u003d orig_object_in_rcontainer"},{"line_number":1108,"context_line":"            sync.put_object \u003d fake_put_object"},{"line_number":1109,"context_line":"            sync.head_object \u003d fake_head_object"},{"line_number":1110,"context_line":"            self.assertTrue(cs.container_sync_row("}],"source_content_type":"text/x-python","patch_set":12,"id":"fa0719c6_d8e85b7f","line":1107,"updated":"2016-03-21 20:15:17.000000000","message":"this should also be reset in the finally clause in case we never reach this point.","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05fb63ddac732f1ad133a8a210583e9bbd6229a5","unresolved":false,"context_lines":[{"line_number":1154,"context_line":"            # Additional put has taken place"},{"line_number":1155,"context_line":"            self.assertEqual(len(actual_puts), 2)"},{"line_number":1156,"context_line":"            # No additional errors"},{"line_number":1157,"context_line":"            self.assertEqual(cs.container_failures, excepted_failure_count)"},{"line_number":1158,"context_line":""},{"line_number":1159,"context_line":"        finally:"},{"line_number":1160,"context_line":"            sync.uuid \u003d orig_uuid"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa0719c6_939b64c9","line":1157,"updated":"2016-03-21 20:15:17.000000000","message":"I was expecting this to go on to test cases where the head info has timestamp less than source timestamp, then equal to, then greater, or did those cases get covered elsewhere?","commit_id":"054d4e6d6b96dcc3098495e22755113e0544ddac"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"c7ddf886a811955341f66bef90df08b1c89378c1","unresolved":false,"context_lines":[{"line_number":1136,"context_line":"            actual_puts \u003d []"},{"line_number":1137,"context_line":""},{"line_number":1138,"context_line":"            def fake_put_object(*args, **kwargs):"},{"line_number":1139,"context_line":"                actual_puts.append((args, kwargs))"},{"line_number":1140,"context_line":""},{"line_number":1141,"context_line":"            def fake_head_object(*args, **kwargs):"},{"line_number":1142,"context_line":"                raise ClientException(\u0027test client exception\u0027, http_status\u003d404)"}],"source_content_type":"text/x-python","patch_set":13,"id":"fa0719c6_b006e9cb","line":1139,"updated":"2016-03-23 17:56:09.000000000","message":"no need to redefine put_object","commit_id":"12f5e75f2fc379177ac0e23324798377f43a1792"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"c7ddf886a811955341f66bef90df08b1c89378c1","unresolved":false,"context_lines":[{"line_number":1181,"context_line":""},{"line_number":1182,"context_line":"            def fake_head_object(*args, **kwargs):"},{"line_number":1183,"context_line":"                return ({\u0027x-timestamp\u0027: \u00271.1\u0027}, \u0027\u0027)"},{"line_number":1184,"context_line":"                return ({}, {})"},{"line_number":1185,"context_line":""},{"line_number":1186,"context_line":"            sync.head_object \u003d fake_head_object"},{"line_number":1187,"context_line":"            self.assertTrue(cs.container_sync_row("}],"source_content_type":"text/x-python","patch_set":13,"id":"fa0719c6_b02c2903","line":1184,"updated":"2016-03-23 17:56:09.000000000","message":"delete line 1184","commit_id":"12f5e75f2fc379177ac0e23324798377f43a1792"}]}