)]}'
{"Engine/swift/storlet_gateway/gateways/docker/runtime.py":[{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"86fc8b41aee7665f89ac29af415cc631b80f5324","unresolved":false,"context_lines":[{"line_number":812,"context_line":"            yield writer"},{"line_number":813,"context_line":"        except OSError as e:"},{"line_number":814,"context_line":"            if self.logger:"},{"line_number":815,"context_line":"                self.logger.exception(\u0027opening fd failed: %s\u0027, e.message)"},{"line_number":816,"context_line":"            raise"},{"line_number":817,"context_line":"        finally:"},{"line_number":818,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a8bc5a2_de3086eb","line":815,"range":{"start_line":815,"start_character":56,"end_line":815,"end_character":60},"updated":"2016-09-28 08:04:23.000000000","message":"Do you mind also remove this printing of e.message? It is included in trackback, and we don\u0027t need to show it here.","commit_id":"23161926e5b434e9ff1ec00ef7c86266bb4f81d8"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"226bebff19ec7f7b89548c1c322a174f3111b075","unresolved":false,"context_lines":[{"line_number":812,"context_line":"            yield writer"},{"line_number":813,"context_line":"        except OSError as e:"},{"line_number":814,"context_line":"            if self.logger:"},{"line_number":815,"context_line":"                self.logger.exception(\u0027opening fd failed: %s\u0027, e.message)"},{"line_number":816,"context_line":"            raise"},{"line_number":817,"context_line":"        finally:"},{"line_number":818,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a8bc5a2_fe29c2f3","line":815,"range":{"start_line":815,"start_character":56,"end_line":815,"end_character":60},"in_reply_to":"5a8bc5a2_de3086eb","updated":"2016-09-28 08:13:00.000000000","message":"I\u0027m not sure if we need the e.message. oh, wait, if we reriase this error, anyone catches this error?\n\nPerhaps, we might need a big try/except block wraps writer.close()","commit_id":"23161926e5b434e9ff1ec00ef7c86266bb4f81d8"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"05017705bcb64fceb1ae26ac86ad58f6ed68d2c1","unresolved":false,"context_lines":[{"line_number":812,"context_line":"            yield writer"},{"line_number":813,"context_line":"        except OSError as e:"},{"line_number":814,"context_line":"            if self.logger:"},{"line_number":815,"context_line":"                self.logger.exception(\u0027opening fd failed: %s\u0027, e.message)"},{"line_number":816,"context_line":"            raise"},{"line_number":817,"context_line":"        finally:"},{"line_number":818,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a8bc5a2_3e432a6e","line":815,"range":{"start_line":815,"start_character":56,"end_line":815,"end_character":60},"in_reply_to":"5a8bc5a2_fe29c2f3","updated":"2016-09-28 08:16:29.000000000","message":"\u003e I\u0027m not sure if we need the e.message. oh, wait, if we reriase this error, anyone catches this error?\n\nAt least, we don\u0027t need to show doubled e.massage here, and just the following sample works.\n\n    self.logger.exception(\u0027opening fd failed\u0027)\n\n\u003e Perhaps, we might need a big try/except block wraps writer.close()\nagree.\nMaybe we also need to reconsider where we log this error.","commit_id":"23161926e5b434e9ff1ec00ef7c86266bb4f81d8"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"86fc8b41aee7665f89ac29af415cc631b80f5324","unresolved":false,"context_lines":[{"line_number":817,"context_line":"        finally:"},{"line_number":818,"context_line":"            try:"},{"line_number":819,"context_line":"                writer.close()"},{"line_number":820,"context_line":"            except:"},{"line_number":821,"context_line":"                pass"},{"line_number":822,"context_line":"        # NOTE(takashi): writer.close() also closes fd, so we don\u0027t have to"},{"line_number":823,"context_line":"        #                close fd again."}],"source_content_type":"text/x-python","patch_set":1,"id":"5a8bc5a2_7e6772ea","line":820,"range":{"start_line":820,"start_character":12,"end_line":820,"end_character":19},"updated":"2016-09-28 08:04:23.000000000","message":"I don\u0027t like using just \u0027except:\u0027, because it can eat up unexpected exceptions like SysExit.\n\nDoes \u0027except OSError\u0027 work here?","commit_id":"23161926e5b434e9ff1ec00ef7c86266bb4f81d8"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"226bebff19ec7f7b89548c1c322a174f3111b075","unresolved":false,"context_lines":[{"line_number":817,"context_line":"        finally:"},{"line_number":818,"context_line":"            try:"},{"line_number":819,"context_line":"                writer.close()"},{"line_number":820,"context_line":"            except:"},{"line_number":821,"context_line":"                pass"},{"line_number":822,"context_line":"        # NOTE(takashi): writer.close() also closes fd, so we don\u0027t have to"},{"line_number":823,"context_line":"        #                close fd again."}],"source_content_type":"text/x-python","patch_set":1,"id":"5a8bc5a2_de724649","line":820,"range":{"start_line":820,"start_character":12,"end_line":820,"end_character":19},"in_reply_to":"5a8bc5a2_7e6772ea","updated":"2016-09-28 08:13:00.000000000","message":"I think any exception is ok here. The main reason is because I\u0027d avoid the un-catched exception in green thread.","commit_id":"23161926e5b434e9ff1ec00ef7c86266bb4f81d8"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"69fd726de99878bb9286792de13ff92b617b567e","unresolved":false,"context_lines":[{"line_number":830,"context_line":"            # so that we should catch and suppress it here"},{"line_number":831,"context_line":"            if self.logger:"},{"line_number":832,"context_line":"                self.logger.exception("},{"line_number":833,"context_line":"                    \u0027error at writing input data %s, %s, %s\u0027 %"},{"line_number":834,"context_line":"                    (exc_type, exc_value, exc_traceback))"},{"line_number":835,"context_line":"            pass"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a8bc5a2_87f22e05","line":834,"range":{"start_line":833,"start_character":48,"end_line":834,"end_character":56},"updated":"2016-09-28 09:10:26.000000000","message":"This information about the exception (class name, message and traceback) is included in the log line when you use logger.exception, so you don\u0027t need to explicitly include them in log message here.","commit_id":"c428283ef5fd4eb62ca9fc76d97ab784246dc472"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"083dcb7a7267765d3239792f1d72d3ad90ae4e3b","unresolved":false,"context_lines":[{"line_number":830,"context_line":"            # so that we should catch and suppress it here"},{"line_number":831,"context_line":"            if self.logger:"},{"line_number":832,"context_line":"                self.logger.exception("},{"line_number":833,"context_line":"                    \u0027error at writing input data %s, %s, %s\u0027 %"},{"line_number":834,"context_line":"                    (exc_type, exc_value, exc_traceback))"},{"line_number":835,"context_line":"            pass"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a8bc5a2_e73bf236","line":834,"range":{"start_line":833,"start_character":48,"end_line":834,"end_character":56},"in_reply_to":"5a8bc5a2_87f22e05","updated":"2016-09-28 09:15:31.000000000","message":"Ok, will do.","commit_id":"c428283ef5fd4eb62ca9fc76d97ab784246dc472"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"070762ad477a05720227cc7a9ff9d00d830caf83","unresolved":false,"context_lines":[{"line_number":811,"context_line":"            writer \u003d os.fdopen(fd, \u0027w\u0027)"},{"line_number":812,"context_line":"            yield writer"},{"line_number":813,"context_line":"        except OSError:"},{"line_number":814,"context_line":"            if self.logger:"},{"line_number":815,"context_line":"                self.logger.exception(\u0027opening fd failed\u0027)"},{"line_number":816,"context_line":"            raise"},{"line_number":817,"context_line":"        finally:"},{"line_number":818,"context_line":"            writer.close()"}],"source_content_type":"text/x-python","patch_set":3,"id":"5a8bc5a2_355fc40f","line":815,"range":{"start_line":814,"start_character":12,"end_line":815,"end_character":58},"updated":"2016-09-28 09:47:34.000000000","message":"I noticed that this causes duplicated traceback log about OSError. Can we just remove logging here and log only at _write_input_data?","commit_id":"95e9bdf4f5c75c748a681c5253bf3b9b5d7afb99"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"070762ad477a05720227cc7a9ff9d00d830caf83","unresolved":false,"context_lines":[{"line_number":815,"context_line":"                self.logger.exception(\u0027opening fd failed\u0027)"},{"line_number":816,"context_line":"            raise"},{"line_number":817,"context_line":"        finally:"},{"line_number":818,"context_line":"            writer.close()"},{"line_number":819,"context_line":"        # NOTE(takashi): writer.close() also closes fd, so we don\u0027t have to"},{"line_number":820,"context_line":"        #                close fd again."},{"line_number":821,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"5a8bc5a2_353184ed","line":818,"range":{"start_line":818,"start_character":12,"end_line":818,"end_character":26},"updated":"2016-09-28 09:47:34.000000000","message":"If we put fdopen inside try block, we get NameError when OSError is raised in fdopen.\n\nMaybe we need to fix this like the following.\n(note) I\u0027m using \u0027is not None\u0027 to be more safe, but it may be unnecessary\n\nwriter \u003d None\ntry:\n    writer \u003d os.fdopen(fd, \u0027w\u0027)\n    yield writer\nfinally:\n    if writer is not None:\n        writer.close()","commit_id":"95e9bdf4f5c75c748a681c5253bf3b9b5d7afb99"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"65f1a9cecea47c4c976955709709b61193c14354","unresolved":false,"context_lines":[{"line_number":815,"context_line":"                self.logger.exception(\u0027opening fd failed\u0027)"},{"line_number":816,"context_line":"            raise"},{"line_number":817,"context_line":"        finally:"},{"line_number":818,"context_line":"            writer.close()"},{"line_number":819,"context_line":"        # NOTE(takashi): writer.close() also closes fd, so we don\u0027t have to"},{"line_number":820,"context_line":"        #                close fd again."},{"line_number":821,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"3a98d1f4_aa89accc","line":818,"range":{"start_line":818,"start_character":12,"end_line":818,"end_character":26},"in_reply_to":"5a8bc5a2_353184ed","updated":"2016-09-29 02:34:30.000000000","message":"it might be just\n try:\n     writer \u003d os.fdopen(fd, \u0027w\u0027)\n     yield writer\n except:\n     # snip\n else:\n     writer.close()\n\n?","commit_id":"95e9bdf4f5c75c748a681c5253bf3b9b5d7afb99"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"070762ad477a05720227cc7a9ff9d00d830caf83","unresolved":false,"context_lines":[{"line_number":828,"context_line":"            # _write_input_data is designed to run eventlet thread"},{"line_number":829,"context_line":"            # so that we should catch and suppress it here"},{"line_number":830,"context_line":"            if self.logger:"},{"line_number":831,"context_line":"                self.logger.exception(\u0027Unexpected error at writing input data\u0027)"},{"line_number":832,"context_line":"            pass"}],"source_content_type":"text/x-python","patch_set":3,"id":"5a8bc5a2_553610b5","line":831,"range":{"start_line":831,"start_character":39,"end_line":831,"end_character":77},"updated":"2016-09-28 09:47:34.000000000","message":"If we merge logging into this one, we can change this to \u0027Error at writing input data\u0027","commit_id":"95e9bdf4f5c75c748a681c5253bf3b9b5d7afb99"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"f158ef3d8101ad833e6bba38b4cbab5351a7ef5d","unresolved":false,"context_lines":[{"line_number":814,"context_line":"            if self.logger:"},{"line_number":815,"context_line":"                self.logger.exception(\u0027opening fd failed\u0027)"},{"line_number":816,"context_line":"        else:"},{"line_number":817,"context_line":"            writer.close()"},{"line_number":818,"context_line":"        # NOTE(takashi): writer.close() also closes fd, so we don\u0027t have to"},{"line_number":819,"context_line":"        #                close fd again."},{"line_number":820,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"3a98d1f4_9d77d8d3","line":817,"range":{"start_line":817,"start_character":12,"end_line":817,"end_character":26},"updated":"2016-09-29 03:14:11.000000000","message":"I\u0027m afraid this is never called if we get unhanded exception inside context manager.","commit_id":"b9fb59c20078a4648baefe798cc308093df2db42"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"ab4a9f6c234e8382c53c784a154c994a787b0af5","unresolved":false,"context_lines":[{"line_number":814,"context_line":"            if self.logger:"},{"line_number":815,"context_line":"                self.logger.exception(\u0027opening fd failed\u0027)"},{"line_number":816,"context_line":"        else:"},{"line_number":817,"context_line":"            writer.close()"},{"line_number":818,"context_line":"        # NOTE(takashi): writer.close() also closes fd, so we don\u0027t have to"},{"line_number":819,"context_line":"        #                close fd again."},{"line_number":820,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"3a98d1f4_dbade832","line":817,"range":{"start_line":817,"start_character":12,"end_line":817,"end_character":26},"in_reply_to":"3a98d1f4_3b2b2ced","updated":"2016-09-29 04:42:27.000000000","message":"\u003e I\u0027m not sure but if os.close(fd) works well?\n\nAs we find that caution in the doc, I think we should TRY to close the fd at least. How about this? Maybe another solution is that we do not touch that fd here, and let upper layer to close the fd in its exception handling. \nNOTE: I divided the previous one try-except block to two blocks, to show the aim for exception handling more clearly.\n\n\ntry:\n    writer \u003d os.fdopen(fd, \u0027w\u0027)\nexcept OSError:\n    exc_type, exc_value, exc_traceback \u003d sys.exc_info()\n    # It seems that we failed to open the fd, which\n    # is currently untouched. We need to confirm it\n    # is surely closed\n    try:\n        os.fdclose(fd)\n    except OSError:\n        self.logger.exception(\u0027Failed to close the fd\u0027)\n    six.reraise(exc_type, exc_value, exc_traceback)\n\ntry:\n   yield writer\nfinally:\n   writer.close()","commit_id":"b9fb59c20078a4648baefe798cc308093df2db42"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"e6e8a1ae5e44d978bc9c56e16202d243c89101eb","unresolved":false,"context_lines":[{"line_number":814,"context_line":"            if self.logger:"},{"line_number":815,"context_line":"                self.logger.exception(\u0027opening fd failed\u0027)"},{"line_number":816,"context_line":"        else:"},{"line_number":817,"context_line":"            writer.close()"},{"line_number":818,"context_line":"        # NOTE(takashi): writer.close() also closes fd, so we don\u0027t have to"},{"line_number":819,"context_line":"        #                close fd again."},{"line_number":820,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"3a98d1f4_3b2b2ced","line":817,"range":{"start_line":817,"start_character":12,"end_line":817,"end_character":26},"in_reply_to":"3a98d1f4_9d77d8d3","updated":"2016-09-29 04:24:25.000000000","message":"Looking at the docs for os.fdopen[1], the change you suggested in the previous patch \n\n writer \u003d None\n try:\n   writer \u003d os.fdopen(fd, \u0027w\u0027)\n   yield writer\n finally:\n   if writer:\n      writer.close()\n\nseems also unsafe.\n\nThe docs says \"If fdopen() raises an exception, it leaves fd untouched (unclosed).\" That means, fdopen will raise an exception and in the python, writer will be assigned nothing. Thus, we can never call close against to the fd.\n\nI\u0027m not sure but if os.close(fd) works well?\n\n1: https://docs.python.org/2.7/library/os.html#os.fdopen","commit_id":"b9fb59c20078a4648baefe798cc308093df2db42"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"f4fea7467be0223e5d5d742d79172b44ee8f4493","unresolved":false,"context_lines":[{"line_number":508,"context_line":"                                as data stream"},{"line_number":509,"context_line":"    \"\"\""},{"line_number":510,"context_line":"    def __init__(self, srequest, storlet_pipe_path, storlet_logger_path,"},{"line_number":511,"context_line":"                 timeout, logger, extra_sources\u003dNone):"},{"line_number":512,"context_line":"        self.srequest \u003d srequest"},{"line_number":513,"context_line":"        self.storlet_pipe_path \u003d storlet_pipe_path"},{"line_number":514,"context_line":"        self.storlet_logger_path \u003d storlet_logger_path"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a98d1f4_660194ae","line":511,"range":{"start_line":511,"start_character":26,"end_line":511,"end_character":32},"updated":"2016-09-29 08:30:30.000000000","message":"Makes much sense!","commit_id":"ff4b61b7bc93a52622c91621d705ab947e90a0fa"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"f4fea7467be0223e5d5d742d79172b44ee8f4493","unresolved":false,"context_lines":[{"line_number":804,"context_line":"            writer \u003d os.fdopen(fd, \u0027w\u0027)"},{"line_number":805,"context_line":"        except (OSError, TypeError, ValueError):"},{"line_number":806,"context_line":"            exc_type, exc_value, exc_traceback \u003d sys.exc_info()"},{"line_number":807,"context_line":"            self.logger.exception(\u0027fdopen failed\u0027)"},{"line_number":808,"context_line":"            try:"},{"line_number":809,"context_line":"                os.close(fd)"},{"line_number":810,"context_line":"            except Exception:"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a98d1f4_e649248d","line":807,"range":{"start_line":807,"start_character":12,"end_line":807,"end_character":50},"updated":"2016-09-29 08:30:30.000000000","message":"If you log this here, you get duplicated log about the raised exception generated here and generated in L840","commit_id":"ff4b61b7bc93a52622c91621d705ab947e90a0fa"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"e2cf6abb895b95af81d2ae564585a69651113719","unresolved":false,"context_lines":[{"line_number":804,"context_line":"            writer \u003d os.fdopen(fd, \u0027w\u0027)"},{"line_number":805,"context_line":"        except (OSError, TypeError, ValueError):"},{"line_number":806,"context_line":"            exc_type, exc_value, exc_traceback \u003d sys.exc_info()"},{"line_number":807,"context_line":"            self.logger.exception(\u0027fdopen failed\u0027)"},{"line_number":808,"context_line":"            try:"},{"line_number":809,"context_line":"                os.close(fd)"},{"line_number":810,"context_line":"            except Exception:"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a98d1f4_4d7590f7","line":807,"range":{"start_line":807,"start_character":12,"end_line":807,"end_character":50},"in_reply_to":"3a98d1f4_e649248d","updated":"2016-09-29 10:53:31.000000000","message":"Maybe no? AFAIK, sys.exc_info will steal the recent exception so that logger.exception should not get the stacktrace here... it needs double check though.","commit_id":"ff4b61b7bc93a52622c91621d705ab947e90a0fa"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"f4fea7467be0223e5d5d742d79172b44ee8f4493","unresolved":false,"context_lines":[{"line_number":814,"context_line":"            six.reraise(exc_type, exc_value, exc_traceback)"},{"line_number":815,"context_line":"        try:"},{"line_number":816,"context_line":"            yield writer"},{"line_number":817,"context_line":"        except StorletTimeout:"},{"line_number":818,"context_line":"            self.logger.exception("},{"line_number":819,"context_line":"                \u0027Timeout (%s)s with writing fd %s\u0027 % (self.timeout, fd))"},{"line_number":820,"context_line":"        except IOError:"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a98d1f4_2675ccca","line":817,"range":{"start_line":817,"start_character":15,"end_line":817,"end_character":29},"updated":"2016-09-29 08:30:30.000000000","message":"IMO, we should not eat up exceptions in context manager, and should do \u0027raise\u0027.","commit_id":"ff4b61b7bc93a52622c91621d705ab947e90a0fa"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"e2cf6abb895b95af81d2ae564585a69651113719","unresolved":false,"context_lines":[{"line_number":814,"context_line":"            six.reraise(exc_type, exc_value, exc_traceback)"},{"line_number":815,"context_line":"        try:"},{"line_number":816,"context_line":"            yield writer"},{"line_number":817,"context_line":"        except StorletTimeout:"},{"line_number":818,"context_line":"            self.logger.exception("},{"line_number":819,"context_line":"                \u0027Timeout (%s)s with writing fd %s\u0027 % (self.timeout, fd))"},{"line_number":820,"context_line":"        except IOError:"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a98d1f4_28bf52fc","line":817,"range":{"start_line":817,"start_character":15,"end_line":817,"end_character":29},"in_reply_to":"3a98d1f4_2675ccca","updated":"2016-09-29 10:53:31.000000000","message":"If we raise the exception here, who else catches/handles that? this writer and _write_input_data are designed to be called in greenthread. Thus, the timeout won\u0027t be up to the thread caller. if we reraises this here, the catcher is only except block at L836.\n\nIMO, the except block at L836 should be the *LAST RESORT* for unexpected error so I\u0027d vote this should be as this is because there is no benefit from reraising.","commit_id":"ff4b61b7bc93a52622c91621d705ab947e90a0fa"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"1da09ad752dc14bf7a2eb266e71e449b6a74b316","unresolved":false,"context_lines":[{"line_number":814,"context_line":"            six.reraise(exc_type, exc_value, exc_traceback)"},{"line_number":815,"context_line":"        try:"},{"line_number":816,"context_line":"            yield writer"},{"line_number":817,"context_line":"        except StorletTimeout:"},{"line_number":818,"context_line":"            self.logger.exception("},{"line_number":819,"context_line":"                \u0027Timeout (%s)s with writing fd %s\u0027 % (self.timeout, fd))"},{"line_number":820,"context_line":"        except IOError:"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a98d1f4_d4884425","line":817,"range":{"start_line":817,"start_character":15,"end_line":817,"end_character":29},"in_reply_to":"3a98d1f4_28bf52fc","updated":"2016-09-29 11:51:35.000000000","message":"I\u0027m afraid that it\u0027s it not obvious that there are some exceptions absorbed in context manager. We can not find at a glance that StorletTimeout raised at L834 does not come to L836, and is eaten up in this context manager.\n\nI agree that we need the final catching point.","commit_id":"ff4b61b7bc93a52622c91621d705ab947e90a0fa"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"5ff0b790f0e4bd30aefb3419e4f9411d3eba222e","unresolved":false,"context_lines":[{"line_number":814,"context_line":"            six.reraise(exc_type, exc_value, exc_traceback)"},{"line_number":815,"context_line":"        try:"},{"line_number":816,"context_line":"            yield writer"},{"line_number":817,"context_line":"        except StorletTimeout:"},{"line_number":818,"context_line":"            self.logger.exception("},{"line_number":819,"context_line":"                \u0027Timeout (%s)s with writing fd %s\u0027 % (self.timeout, fd))"},{"line_number":820,"context_line":"        except IOError:"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a98d1f4_5a429d3f","line":817,"range":{"start_line":817,"start_character":15,"end_line":817,"end_character":29},"in_reply_to":"3a98d1f4_d4884425","updated":"2016-09-29 12:13:12.000000000","message":"Fair point. Probably, the exception should be caught where close point as possible. I\u0027m getting another solution for you. will make a new patch set.","commit_id":"ff4b61b7bc93a52622c91621d705ab947e90a0fa"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"f4fea7467be0223e5d5d742d79172b44ee8f4493","unresolved":false,"context_lines":[{"line_number":816,"context_line":"            yield writer"},{"line_number":817,"context_line":"        except StorletTimeout:"},{"line_number":818,"context_line":"            self.logger.exception("},{"line_number":819,"context_line":"                \u0027Timeout (%s)s with writing fd %s\u0027 % (self.timeout, fd))"},{"line_number":820,"context_line":"        except IOError:"},{"line_number":821,"context_line":"            # this will happen at sort of broken pipe while writer.write"},{"line_number":822,"context_line":"            self.logger.exception(\u0027IOError with writing fd %s\u0027 % fd)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a98d1f4_a609dc97","line":819,"range":{"start_line":819,"start_character":54,"end_line":819,"end_character":66},"updated":"2016-09-29 08:30:30.000000000","message":"(nit) It seems that StorletTimeout (which inherits eventlet.Timeout) has \u0027seconds\u0027 parameter which stores timeout seconds. Does it make sense to use it instead?\n\nexcept StorletTimeout as to:\n    self.logger.exception(... % (to.seconds, fd))","commit_id":"ff4b61b7bc93a52622c91621d705ab947e90a0fa"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"1da09ad752dc14bf7a2eb266e71e449b6a74b316","unresolved":false,"context_lines":[{"line_number":816,"context_line":"            yield writer"},{"line_number":817,"context_line":"        except StorletTimeout:"},{"line_number":818,"context_line":"            self.logger.exception("},{"line_number":819,"context_line":"                \u0027Timeout (%s)s with writing fd %s\u0027 % (self.timeout, fd))"},{"line_number":820,"context_line":"        except IOError:"},{"line_number":821,"context_line":"            # this will happen at sort of broken pipe while writer.write"},{"line_number":822,"context_line":"            self.logger.exception(\u0027IOError with writing fd %s\u0027 % fd)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a98d1f4_d41204b4","line":819,"range":{"start_line":819,"start_character":54,"end_line":819,"end_character":66},"in_reply_to":"3a98d1f4_880a26d1","updated":"2016-09-29 11:51:35.000000000","message":"I just intended to say that we can get the timeout value really passed to the Timeout instance, although it has the same value as self.timeout.","commit_id":"ff4b61b7bc93a52622c91621d705ab947e90a0fa"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"e2cf6abb895b95af81d2ae564585a69651113719","unresolved":false,"context_lines":[{"line_number":816,"context_line":"            yield writer"},{"line_number":817,"context_line":"        except StorletTimeout:"},{"line_number":818,"context_line":"            self.logger.exception("},{"line_number":819,"context_line":"                \u0027Timeout (%s)s with writing fd %s\u0027 % (self.timeout, fd))"},{"line_number":820,"context_line":"        except IOError:"},{"line_number":821,"context_line":"            # this will happen at sort of broken pipe while writer.write"},{"line_number":822,"context_line":"            self.logger.exception(\u0027IOError with writing fd %s\u0027 % fd)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a98d1f4_880a26d1","line":819,"range":{"start_line":819,"start_character":54,"end_line":819,"end_character":66},"in_reply_to":"3a98d1f4_a609dc97","updated":"2016-09-29 10:53:31.000000000","message":"No? self.timeout is just an integer. Please double check the definition at [1] [2]\n\n\n1: https://review.openstack.org/#/c/378326/5/Engine/swift/storlet_gateway/gateways/docker/runtime.py@518\n2: https://review.openstack.org/#/c/378326/5/Engine/swift/storlet_gateway/gateways/docker/runtime.py@504","commit_id":"ff4b61b7bc93a52622c91621d705ab947e90a0fa"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"f4fea7467be0223e5d5d742d79172b44ee8f4493","unresolved":false,"context_lines":[{"line_number":836,"context_line":"        except Exception:"},{"line_number":837,"context_line":"            # _write_input_data is designed to run eventlet thread"},{"line_number":838,"context_line":"            # so that we should catch and suppress it here"},{"line_number":839,"context_line":"            if self.logger:"},{"line_number":840,"context_line":"                self.logger.exception(\u0027Unexpected error at writing input data\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a98d1f4_66e354a4","line":839,"range":{"start_line":839,"start_character":12,"end_line":839,"end_character":27},"updated":"2016-09-29 08:30:30.000000000","message":"We don\u0027t need this any more","commit_id":"ff4b61b7bc93a52622c91621d705ab947e90a0fa"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"e2cf6abb895b95af81d2ae564585a69651113719","unresolved":false,"context_lines":[{"line_number":836,"context_line":"        except Exception:"},{"line_number":837,"context_line":"            # _write_input_data is designed to run eventlet thread"},{"line_number":838,"context_line":"            # so that we should catch and suppress it here"},{"line_number":839,"context_line":"            if self.logger:"},{"line_number":840,"context_line":"                self.logger.exception(\u0027Unexpected error at writing input data\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a98d1f4_8dccf836","line":839,"range":{"start_line":839,"start_character":12,"end_line":839,"end_character":27},"in_reply_to":"3a98d1f4_66e354a4","updated":"2016-09-29 10:53:31.000000000","message":"Good catch. Will fix.","commit_id":"ff4b61b7bc93a52622c91621d705ab947e90a0fa"},{"author":{"_account_id":11317,"name":"Eran Rom","email":"eran@itsonlyme.name","username":"eranr"},"change_message_id":"a68c554737cd4549485d261fc77e4cdcaf6a022c","unresolved":false,"context_lines":[{"line_number":822,"context_line":"    def _write_input_data(self, fd, data_iter):"},{"line_number":823,"context_line":"        try:"},{"line_number":824,"context_line":"            # double try/except block saving from unexpected errors"},{"line_number":825,"context_line":"            try:"},{"line_number":826,"context_line":"                with self._open_writer(fd) as writer:"},{"line_number":827,"context_line":"                    for chunk in data_iter:"},{"line_number":828,"context_line":"                        with StorletTimeout(self.timeout):"}],"source_content_type":"text/x-python","patch_set":7,"id":"1a95cdbc_5f3fe6d5","line":825,"updated":"2016-10-05 18:29:41.000000000","message":"This is not very important, and mostly for my understanding (so I am anyway +2). Do we really want to use context manager only for the case where the fdopen fails? Doesn\u0027t it make sense to move all the inner exception handling (lines 830-837) to the context manager generator function?","commit_id":"9cbf813de2c82438aae163e05266a930db9b8998"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"451fef0a7f973b1c31434a823941e1889ccbe2ab","unresolved":false,"context_lines":[{"line_number":833,"context_line":"                # this will happen at sort of broken pipe while writer.write"},{"line_number":834,"context_line":"                self.logger.exception(\u0027IOError with writing fd %s\u0027 % fd)"},{"line_number":835,"context_line":"            except StorletTimeout:"},{"line_number":836,"context_line":"                self.logger.exception("},{"line_number":837,"context_line":"                    \u0027Timeout (%s)s with writing fd %s\u0027 % (self.timeout, fd))"},{"line_number":838,"context_line":"        except Exception:"},{"line_number":839,"context_line":"            # _write_input_data is designed to run eventlet thread"},{"line_number":840,"context_line":"            # so that we should catch and suppress it here"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a98d1f4_69e875e3","line":837,"range":{"start_line":836,"start_character":16,"end_line":837,"end_character":75},"updated":"2016-09-29 13:34:59.000000000","message":"It seems that the timeout class name and given timeout seconds are automatically added to log if we use logger generated by swift.common.utils.get_logger [1],\nso this causes duplicated logging around Timeout(%s)\nI don\u0027t know we can depend on the implementation of swift, but currently while we only use swift it seems a little redundant. I\u0027d like to ask your opinion (or opinions from other core reviewers)\n\n[1] https://github.com/openstack/swift/blob/1e5c5c35bd02ae9bf3866141727f0e8fdd9b8fb4/swift/common/utils.py#L1680-L1686","commit_id":"9cbf813de2c82438aae163e05266a930db9b8998"},{"author":{"_account_id":11317,"name":"Eran Rom","email":"eran@itsonlyme.name","username":"eranr"},"change_message_id":"a68c554737cd4549485d261fc77e4cdcaf6a022c","unresolved":false,"context_lines":[{"line_number":833,"context_line":"                # this will happen at sort of broken pipe while writer.write"},{"line_number":834,"context_line":"                self.logger.exception(\u0027IOError with writing fd %s\u0027 % fd)"},{"line_number":835,"context_line":"            except StorletTimeout:"},{"line_number":836,"context_line":"                self.logger.exception("},{"line_number":837,"context_line":"                    \u0027Timeout (%s)s with writing fd %s\u0027 % (self.timeout, fd))"},{"line_number":838,"context_line":"        except Exception:"},{"line_number":839,"context_line":"            # _write_input_data is designed to run eventlet thread"},{"line_number":840,"context_line":"            # so that we should catch and suppress it here"}],"source_content_type":"text/x-python","patch_set":7,"id":"1a95cdbc_bf30ea02","line":837,"range":{"start_line":836,"start_character":16,"end_line":837,"end_character":75},"in_reply_to":"3a98d1f4_655c11ee","updated":"2016-10-05 18:29:41.000000000","message":"+1","commit_id":"9cbf813de2c82438aae163e05266a930db9b8998"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"1c04e16736e919bdbb33c76ad3a8e17b1c55d383","unresolved":false,"context_lines":[{"line_number":833,"context_line":"                # this will happen at sort of broken pipe while writer.write"},{"line_number":834,"context_line":"                self.logger.exception(\u0027IOError with writing fd %s\u0027 % fd)"},{"line_number":835,"context_line":"            except StorletTimeout:"},{"line_number":836,"context_line":"                self.logger.exception("},{"line_number":837,"context_line":"                    \u0027Timeout (%s)s with writing fd %s\u0027 % (self.timeout, fd))"},{"line_number":838,"context_line":"        except Exception:"},{"line_number":839,"context_line":"            # _write_input_data is designed to run eventlet thread"},{"line_number":840,"context_line":"            # so that we should catch and suppress it here"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a98d1f4_655c11ee","line":837,"range":{"start_line":836,"start_character":16,"end_line":837,"end_character":75},"in_reply_to":"3a98d1f4_69e875e3","updated":"2016-09-29 15:01:57.000000000","message":"I don\u0027t have strong opinion that, but it seems not duplicated, just redundant a bit though. In my feeling, saying  \u0027we got timeout with x seconds\u0027 here explicitly is worth to us to know the error reason and the method spec and the string format (i.e. sort of Timeout.__str__ impl) could be changed. In addition, that exception should not appear so frequently so that it seems ok to show the redundant ones.","commit_id":"9cbf813de2c82438aae163e05266a930db9b8998"}]}