buffer(int) is mostly called from value vectors, but there are an additional 10+ places where it's being used and most of them don't bother checking if the returned buffer is null.
On Tue, Jun 30, 2015 at 3:39 PM, Jacques Nadeau <[email protected]> wrote: > What is your sense of the use of this interface? I would hope new uses > would be scarce. Do you think this will be a frequent occurrence that most > devs will experience? > On Jun 30, 2015 3:35 PM, "Abdel Hakim Deneche" <[email protected]> > wrote: > > > I started this discussion while fixing DRILL-3312 > > <https://issues.apache.org/jira/browse/DRILL-3312>, it took me quite > some > > time to debug and find the root of an error I was seeing. I first thought > > about fixing those few places that call buffer() without checking if the > > buffer is null or not, but this won't prevent new code from repeating the > > same mistakes. > > > > > > On Tue, Jun 30, 2015 at 3:19 PM, Jacques Nadeau <[email protected]> > > wrote: > > > > > Lots of different comments on this thread. Here are few of my > thoughts: > > > > > > I think you need to differentiate the different tiers of interfaces a > > > little bit more. > > > > > > BufferAllocator.buffer() was meant to be a low level advanced > interface. > > > It should be used rarely and when done so, only with extreme care. > Most > > > devs shouldn't use it. Having an advanced interface seems fine here. > > This > > > was how the interface was envisioned. It looks like there are > currently > > > 16 classes that reference this code not including vectors. (There > > probably > > > should be less, it looks like some code duplication contributes to > this.) > > > > > > All the vector classes also access this code. Vectors have two > methods: > > > allocateNew and allocateNewSafe. allocateNew has an exception behavior > > and > > > allocateNewSafe has a return value behavior. These are main APIs that > > are > > > used extensively throughout the code and the ones we should be most > > focused > > > on. I like giving the developer the choice of behavior (thus the two > > > options). It sounds like others are saying to remove this choice > > (because > > > code is not managing it correctly). > > > > > > If there are bugs in the 16 classes on how they use the advanced > > interface, > > > this doesn't seem like a big challenge to correct rather than bulk > > > modifying the API. If people are using the wrong method on the vector > > > classes (or the vector classes aren't correctly behaving), that doesn't > > > seem like something we should address at the allocator API level. > > > > > > Hakim, can you be more specific about your concern? It doesn't seem > like > > > we're talking about thousands of places here. > > > > > > Regarding checked versus unchecked... I'm strongly in support of using > > > checked exceptions for most things. However, I'm against it for an out > > of > > > memory condition. (Note how the JVM also treats this as an unchecked > > > exception.) It causes early catching when, in most cases, we really > want > > to > > > bubble way up the stack and fail the query. I know of very few places > > > where we can actually locally manage an out of memory condition so why > > > write huge amounts of code the bubble the exception all the way to > where > > we > > > can actually handle it. > > > > > > A quick sidebar to Jason's "Hash agg out of memory" comment > > > Two thoughts here. I'm not sure this is a meaningful message. Where > we > > > run out of memory may have nothing to do with where we are using most > > > memory. And if we did to decide this was useful, this can be done just > > as > > > easily with unchecked exceptions as checked exception. What I'm want > to > > > avoid is adding throws OutOfMemoryException in 1000 places. The > reality > > is > > > everybody should assume that an OutOfMemory can occur pretty much > > anytime. > > > > > > TL;DR > > > > > > In general, I think this discussion is too abstract and thus has the > > danger > > > of trending towards a religious discussion (I already see a bit of that > > > fervor in some of the responses.). The number of places we are talking > > > about are not that large and are easy to find. Let's solve this by > > looking > > > at specifics. For example, if people think checked would be better, > > > someone should go through and make an example changeset for everyone to > > > review. My guess is, the only that it works will be if very quickly, > the > > > handling code converts the checked exception into an unchecked > > > UserException. But I'm more than happy to be proven wrong. > > > > > > On Tue, Jun 30, 2015 at 2:16 PM, Daniel Barclay <[email protected] > > > > > wrote: > > > > > > > Hanifi GUNES wrote: > > > > > > > >> - 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. > > > >> > > > > > > > > +(1 times <some large number>) > > > > > > > > Daniel > > > > > > > > > > > > > > > >> > > > >> - 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 < > [email protected] > > >: > > > >> > > > >> +1 to Hanifi's > > > >>> > > > >>> On Tue, Jun 30, 2015 at 1:38 PM, Jason Altekruse < > > > >>> [email protected] > > > >>> > > > >>>> > > > >>>> 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 <[email protected]> > > 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 < > > > [email protected] > > > >>>>> > > > >>>> : > > > >>>> > > > >>>>> > > > >>>>> 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 < > > > >>>>>> > > > >>>>> [email protected]> > > > >>>> > > > >>>>> 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 < > > > >>>>>>> [email protected] > > > >>>>>>> > > > >>>>>>>> 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 > > > >>> > > > >>>> > > > >>>> > > > >>> > > > >> > > > > > > > > -- > > > > Daniel Barclay > > > > MapR Technologies > > > > > > > > > > > > > > > -- > > > > 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>
