For now, getting by with out params is fine. As for piping in this UUID case, proper layering should be able to mitigate a lot of the issues.
That said, still interesting to think about. I think templatizing would only work if we could pattern-match on the type of output we want. E.g. something like status.HasExtra<uint16_t>(), but not sure if that's possible in C++. The point about heap-allocating given copies is pretty valid, but I don't think the overhead of shared_ptr would be a deal-breaker. If we "unwrap" the Status (handle StatusExtra extras) at the right layers, I could see it being almost equivalent to having an out-param, just attached to the Status instead of the function signature. On Mon, Apr 24, 2017 at 2:58 PM, Dan Burkert <[email protected]> wrote: > Between RVO and move semantics we shouldn't be (deep) copying Status > objects very much. That being said, while I agree that the Status pattern > is not optimal in a lot of ways, I think changing it to be templated is > going to be very painful (if not impossible due to public API concerns). > Piping through an additional out-param seems like a prudent approach > without boiling the ocean. > > FWIW Rust does have a templated result type which is used pervasively in > the standard library and elsewhere, and I do think it's a great pattern. > It has one difference though - it templates on both a success type _and_ an > error type, which avoids the need to have an out-param in almost all > cases. I'd be in favor of moving to something like that, but it requires > something like C++ variants to do correctly (boost might have this, not > sure). I bring this up only because I think there is an opportunity to > improve our error handling pattern a lot, but I don't think adding an error > param is necessarily going far enough for the amount of effort it would > require. > > - Dan > > On Mon, Apr 24, 2017 at 1:02 PM, David Alves <[email protected]> > wrote: > > > I'd be pro having an _optional_ PB or some other optional way to keep > extra > > metadata in a status. > > In most cases (no extra metadata) this wouldn't have an additional cost > > when we're copying statuses around and the PB would have the obvious > > advantage of being easy to send to clients, when we need more error > > information to handle things properly. > > I don't think extra args are going to be scalable since we'd need to > > basically pipe them everywhere we could get an EIO. > > Regarding the templated option, how would the caller know that it can be > > downcast? Is it always in the same type of error? Would you need rtti? > > > > -david > > > > On Mon, Apr 24, 2017 at 12:46 PM, Alexey Serbin <[email protected]> > > wrote: > > > > > Hi Andrew, > > > > > > The instances of Status objects are copied around very often, so > putting > > > something heap-allocated into the Status class would require dealing > with > > > that (wrapping those into shared_ptr? But I'm not sure it's worth it). > > > > > > As for down-casting: would it be possible in your case to limit the > > > exchange of ExtendedStatus instances only to some particular methods > and > > > make sure your methods always return the ExternedStatus? That would > > allow > > > to avoid downcasting. > > > > > > I considered a similar approach while propagating RPC error codes along > > > with Status in the code around connection negotiation, but eventually I > > > decided to go with an additional output parameter for a couple of > > methods. > > > > > > > > > Best regards, > > > > > > Alexey > > > > > > > > > On 4/24/17 12:33 PM, Andrew Wong wrote: > > > > > >> Hi everyone, > > >> > > >> In my work on disk failure handling, it would be very useful to pass > > extra > > >> information along with Statuses. To add some context, I would like to > > >> propagate the uuid of a failed disk to the high-level operations that > > may > > >> have caused the failure (e.g. FlushDeltaMemStoresOp.Perform(), etc.), > > >> which > > >> can then error-handle based on this uuid (e.g. tell the FsManager, > > >> DataDirManager, etc. that disk <uuid> is dead) instead of failing a > > check. > > >> > > >> Since this is such a widely used class, this sort of change warrants > > some > > >> discussion, particularly if the functionality would be useful > elsewhere. > > >> > > >> A couple possible implementations: > > >> > > >> 1. Have a template class that extends Status with a pointer to a > > >> heap-allocated T. When handling errors, call a templatized function > that > > >> downcasts the Status to a StatusWithExtra<T> and returns the T. This > is > > >> moderately flexible, and wouldn't need to be changed as it gets used > > more > > >> often, but downcasting will warrant careful consideration in terms of > > >> type-safety. > > >> > > >> 2. Have Status have a StatusExtrasPB message that has a number of > > objects > > >> within. As passing data through Status becomes used in other areas of > > >> code, > > >> the StatusExtrasPB can be extended. This would avoid downcasting, > > >> extending > > >> Status directly. > > >> > > >> These are just a couple of ideas, but I'd be interested in seeing what > > >> people think and whether anyone had other ideas for this sort of > > >> message-passing. > > >> > > >> > > >> Andrew > > >> > > >> > > > > > > -- Andrew Wong
