Do you think we should be reaching that deep in the other places? On Jun 30, 2015 3:55 PM, "Abdel Hakim Deneche" <[email protected]> wrote:
> 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 > > >
