Adding extra data to Kudu::Status

[Andrew Wong <aw...@cloudera.com>]

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

Re: Adding extra data to Kudu::Status

[Andrew Wong <aw...@cloudera.com>]

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 <da...@apache.org> 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 <da...@gmail.com>
> 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 <as...@cloudera.com>
> > 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

Re: Adding extra data to Kudu::Status

[Dan Burkert <da...@apache.org>]

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 <da...@gmail.com> 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 <as...@cloudera.com>
> 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
> >>
> >>
> >
>

Re: Adding extra data to Kudu::Status

[David Alves <da...@gmail.com>]

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 <as...@cloudera.com>
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
>>
>>
>

Re: Adding extra data to Kudu::Status

[Alexey Serbin <as...@cloudera.com>]

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
>