drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Hanifi GUNES <hanifigu...@gmail.com>
Subject Re: [DISCUSSION] should BufferAllocator.buffer() throw an OutOfMemoryException ?
Date Tue, 30 Jun 2015 20:58:18 GMT
- We would end up having to add it to almost everything everywhere
Why would one propagate the checked exception for no reason? And why would
one not propagate the exception for a good reason like robustness? I agree
that one has to avoid propagating the checked exception for no reason
however I support propagating it for a good reason.

The added benefit of raising a checked exception is reminding as well as
enforcing devs to handle it and be more cautious about this particular
event. I find this compile-time safety check invaluable for robustness.


- Or constantly wrapping it with RuntimeException to get around that
If it has to be done, I would recommend relying on a helper to do so. There
is not much of man-work involved here.


2015-06-30 13:53 GMT-07:00 Abdel Hakim Deneche <adeneche@maprtech.com>:

> +1 to Hanifi's
>
> On Tue, Jun 30, 2015 at 1:38 PM, Jason Altekruse <altekrusejason@gmail.com
> >
> wrote:
>
> > +1 to Hanifi's comments, I think it makes much more sense to have a
> number
> > of sites where the operators are explicitly catching a checked OOM
> > exception and either decide to handle it or produce a message like "Hash
> > Aggregate does not support our of memory conditions". This would be
> > particularly useful for debugging queries, as the user exception can
> > provide context information about the current operation. This way users
> can
> > have some idea about the part of their query that might be causing an
> > excessive strain on system resources. I understand that there are also
> > cases where operators competing for memory can make it a toss up to which
> > will actually fail, but this would at least be a step to give more
> detailed
> > information to users.
> >
> > On Tue, Jun 30, 2015 at 1:28 PM, Hanifi GUNES <hg@apache.org> wrote:
> >
> > > I would propose throwing a checked exception encouraging explicit and
> > > consistent handling of this event. Each sub-system has liberty to
> decide
> > if
> > > an OOM failure is fatal or non-fatal depending on its capabilities.
> Also
> > if
> > > at some point a sub-system needs to communicate with its callers via a
> > > different mechanism such like using flags (boolean, enum etc) or
> raising
> > an
> > > unchecked exception that's still fine, just handle the exception. If
> > there
> > > is a need to suppress the checked exception that's fine too, just use a
> > > helper method.
> > >
> > > Either way, returning *null* sounds problematic in many ways i) it is
> > > implicit ii) unsafe iii) its handling logic is repetitive iv) it is
> > > semantically unclean to make null mean something - even worse something
> > > context specific.
> > >
> > >
> > > -Hanifi
> > >
> > > 2015-06-30 12:23 GMT-07:00 Abdel Hakim Deneche <adeneche@maprtech.com
> >:
> > >
> > > > I guess that would fix the issue too. But we may still run into
> > > situations
> > > > where the caller will pass a flag to "mute" the exception and not
> > handle
> > > > the case anyway.
> > > >
> > > > If .buffer() unconditionally throws an exception, can't the caller,
> who
> > > > wants to, just catch that and handle it properly ?
> > > >
> > > > On Tue, Jun 30, 2015 at 12:13 PM, Chris Westin <
> > chriswestin42@gmail.com>
> > > > wrote:
> > > >
> > > > > No, but we should do something close to that.
> > > > >
> > > > > There are cases where the caller can handle the inability to get
> more
> > > > > memory, and may be able to go to disk. However, you are correct
> that
> > > > there
> > > > > are many that can't handle an OOM, and that fail to check.
> > > > >
> > > > > Instead of unconditionally throwing OutOfMemoryRuntimeException,
I
> > > would
> > > > > suggest that the buffer() call take a flag that indicates whether
> or
> > > not
> > > > it
> > > > > should throw if it is unable to fulfill the request. This way, the
> > call
> > > > > sites that can handle an OOM can pass in the flag to return null,
> and
> > > the
> > > > > rest can pass in the flag value to throw, and not have to have any
> > > > checking
> > > > > code.
> > > > >
> > > > >
> > > > > On Tue, Jun 30, 2015 at 12:06 PM, Abdel Hakim Deneche <
> > > > > adeneche@maprtech.com
> > > > > > wrote:
> > > > >
> > > > > > our current implementations of BufferAllocator.buffer(int, int)
> > > returns
> > > > > > null when it cannot allocate the buffer.
> > > > > >
> > > > > > But looking through the code, there are many places that don't
> > check
> > > if
> > > > > the
> > > > > > allocated buffer is null before trying to access it which will
> > throw
> > > a
> > > > > > NullPointerException.
> > > > > >
> > > > > > ValueVectors' allocateNewSafe() seem to be the only place that
> > handle
> > > > the
> > > > > > null in a specific manner.
> > > > > >
> > > > > > Should we update the allocators' implementation to throw an
> > > > > > OutOfMemoryRuntimeException instead of returning null ? this
has
> > the
> > > > > added
> > > > > > benefit of displaying a proper out of memory error message to
the
> > > user.
> > > > > >
> > > > > > Thanks!
> > > > > >
> > > > > > --
> > > > > >
> > > > > > Abdelhakim Deneche
> > > > > >
> > > > > > Software Engineer
> > > > > >
> > > > > >   <http://www.mapr.com/>
> > > > > >
> > > > > >
> > > > > > Now Available - Free Hadoop On-Demand Training
> > > > > > <
> > > > > >
> > > > >
> > > >
> > >
> >
> http://www.mapr.com/training?utm_source=Email&utm_medium=Signature&utm_campaign=Free%20available
> > > > > > >
> > > > > >
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > >
> > > > Abdelhakim Deneche
> > > >
> > > > Software Engineer
> > > >
> > > >   <http://www.mapr.com/>
> > > >
> > > >
> > > > Now Available - Free Hadoop On-Demand Training
> > > > <
> > > >
> > >
> >
> http://www.mapr.com/training?utm_source=Email&utm_medium=Signature&utm_campaign=Free%20available
> > > > >
> > > >
> > >
> >
>
>
>
> --
>
> Abdelhakim Deneche
>
> Software Engineer
>
>   <http://www.mapr.com/>
>
>
> Now Available - Free Hadoop On-Demand Training
> <
> http://www.mapr.com/training?utm_source=Email&utm_medium=Signature&utm_campaign=Free%20available
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message