Code review comment for lp:~gholt/swift/lobjects4

Revision history for this message
clayg (clay-gerrard) wrote :

great work gholt.

In SegmentedIterable, _load_next_segment, __iter__ and app_iter_range all log the same message ("ERROR: While processing manifest") - if the exception happens in _load_next_segment it gets logged twice.

Eitherway, the posthooklogger will log the status of the response as successful, even though the client likely blew up cause it didn't get all the expected data.

Just for the purpose of logging, since SegmentedIterable has a backref to the Response - I would suggest we update the status_int to a 503. Even though we can't do anything about the 200 we already sent to the client, we know that whatever bytes were transferred so far won't be billable.

Just a thought - personal preference in control structure - I don't care for the except Exception: if not isisntance(StopIteration): log, raise - I would prefer except StopIteration: pass except Exception: log, raise.

Also in SegmentedIterable, instead of initializing self.response to None, how about an empty Response(). It *should* be overridden (assuming the caller read the docstring) - but initializing to "something" removes the need for an extra test in all the "if self.response: update attribute on response" code. Alternatively you could make response a required named parameter to __init__ and then just require the calling method update it's response's app_iter instead of its new SegmentedIterable's backref.

resp = Response()
resp.app_iter = SegIter(resp)

^ does that work?

The history section mentions the "implied user manifest" that we have now with the consistency issue where the proxy can't ever know if it has all of the users intended objects, but not the rejected "explicit user manifest" when the body of the manifest file includes all of the objects in the order the should be glued together - like amazon uses. Is it worth mentioning why our solution is better?

Thanks for working on this essential feature, you deserve much karama!

« Back to merge proposal