This thread is a good example of refactoring. So mass fixing of swallowed exceptions is not ideal IMHO because sometimes we want to log things, sometimes we want to re-throw, sometimes we want a different path. Hence each item should be refactored slowly and in isolation because if you just throw a log in there then people would think this code is probably okay and doesn't need review.
Anyway, again I appreciate all the help in your reviews, the feature is more or less implemented in OFBIZ-9441. On Tue, Jul 11, 2017 at 12:35 AM, Jacques Le Roux <[email protected]> wrote: > I'll refrain to speak about swallowed exceptions ;) > > I still want to continue on OFBIZ-8341 but accepted to get sidetracked in > multi ways :) > > Jacques > > > > Le 10/07/2017 à 21:36, Taher Alkhateeb a écrit : >> >> Fixed it in the JIRA, the EntitySaxReader (should be the next class to >> refactor) is logging an error but suppressing an exception. The logic >> for "continue-on-error" had to go 3 classes deep to work correctly. I >> always underestimate how much spaghetti code we have :) >> >> On Mon, Jul 10, 2017 at 8:14 PM, Taher Alkhateeb >> <[email protected]> wrote: >>> >>> Quick update, to my surprise an exception is swallowed >>> deeeeeeeeeeeeeeeeeeeeeeeeeeeep in the call stack. So getting this >>> feature might require some intrusive changes. I'm still working on it >>> and will keep you posted, but as of right now, no exception is >>> bubbling up to be caught with "continue-on-failure" >>> >>> On Mon, Jul 10, 2017 at 2:14 PM, Taher Alkhateeb >>> <[email protected]> wrote: >>>> >>>> Thank you everyone for your feedback, I will let this discussion >>>> continue for a few days before committing anything (testing is going >>>> to take some time anyway). >>>> >>>> Now, I need help, I have a big patch in [1] that does what we >>>> discussed in this thread and a whole lot more. If you have the time, I >>>> really need your help! Most useful help is testing, there are so many >>>> properties and combinations to use, so this requires thorough testing. >>>> Also for those familiar with the core framework API, a second look at >>>> the code would help. >>>> >>>> [1] https://issues.apache.org/jira/browse/OFBIZ-9441 >>>> >>>> On Mon, Jul 10, 2017 at 1:45 PM, Devanshu Vyas >>>> <[email protected]> wrote: >>>>> >>>>> I agree with option #3 and the 'continue-on-failure' flag with default >>>>> value=false. :) >>>>> >>>>> Thanks & Regards, >>>>> Devanshu Vyas. >>>>> >>>>> On Mon, Jul 10, 2017 at 3:07 PM, Taher Alkhateeb >>>>> <[email protected] >>>>>> >>>>>> wrote: >>>>>> Hi Rishi, >>>>>> >>>>>> So my suggestion is that if anything does not load, then immediately >>>>>> fail. >>>>>> >>>>>> Why am I suggesting this? >>>>>> - You have to intentionally ignore data failure after being aware of >>>>>> it (it does not slip between the cracks) >>>>>> - The data will automatically get cleaned by committers because no >>>>>> failing data will be committed to the code base. >>>>>> >>>>>> I suspect we will actually catch some data loading failures that exist >>>>>> in the code base and we are maybe unaware of. >>>>>> >>>>>> On Mon, Jul 10, 2017 at 12:04 PM, Rishi Solanki >>>>>> <[email protected]> >>>>>> wrote: >>>>>>> >>>>>>> I'm good to go with option #3 and continue-on-failure. >>>>>>> >>>>>>> Just wanted to mention one thing here; for which type of data we will >>>>>>> be >>>>>>> failing build. That means, we have several options seed, ext, demo. >>>>>>> Do we >>>>>>> need to discuss these points or we are fine for all type of data. >>>>>>> Like >>>>>> >>>>>> demo >>>>>>> >>>>>>> data fails only affect a process for that data set only, and for that >>>>>>> failing build is okay or not (as on data load we get logs if any file >>>>>>> didn't load). >>>>>>> >>>>>>> >>>>>>> Btw, I'm good with the proposal, just sharing a thought in case we >>>>>>> should >>>>>>> discuss or may be we can simply ignore if we are good with that. >>>>>>> >>>>>>> Thaks! >>>>>>> >>>>>>> >>>>>>> >>>>>>> Rishi Solanki >>>>>>> Sr Manager, Enterprise Software Development >>>>>>> HotWax Systems Pvt. Ltd. >>>>>>> Direct: +91-9893287847 >>>>>>> http://www.hotwaxsystems.com >>>>>>> >>>>>>> On Mon, Jul 10, 2017 at 2:15 PM, Deepak Dixit < >>>>>>> [email protected]> wrote: >>>>>>> >>>>>>>>> Historically the data loader boolean props are false if ommitted >>>>>>>>> and >>>>>> >>>>>> the >>>>>>>>> >>>>>>>>> code expects that, but you have a point about the double negative. >>>>>>>>> We >>>>>> >>>>>> can >>>>>>>>> >>>>>>>>> instead call it "continue-on-failure" for example. >>>>>>>>> >>>>>>>> +1 continue-on-failure with default value false >>>>>>>> >>>>>>>> Thanks & Regards >>>>>>>> -- >>>>>>>> Deepak Dixit >>>>>>>> www.hotwaxsystems.com >>>>>>>> www.hotwax.co >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> On Jul 10, 2017 3:48 AM, "Paul Foxworthy" <[email protected]> >>>>>> >>>>>> wrote: >>>>>>>>> >>>>>>>>> Hi all, >>>>>>>>> >>>>>>>>> I agree with option 3. I recall in my own work I once needed to add >>>>>>>>> a >>>>>>>> >>>>>>>> throw >>>>>>>>> >>>>>>>>> where there was none to track down a problem. >>>>>>>>> >>>>>>>>> However ignore-failure leads to a double negative. How about >>>>>>>>> "stop-on-failure", default value true? >>>>>>>>> >>>>>>>>> Cheers >>>>>>>>> >>>>>>>>> Paul Foxworthy >>>>>>>>> >>>>>>>>> >>>>>>>>> On 10 July 2017 at 05:27, Taher Alkhateeb >>>>>>>>> <[email protected] >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> Correction: on item (2) in my post: fail immediately, not after >>>>>>>>>> loading all files, otherwise there's no point. >>>>>>>>>> >>>>>>>>>> On Sun, Jul 9, 2017 at 10:18 PM, Taher Alkhateeb >>>>>>>>>> <[email protected]> wrote: >>>>>>>>>>> >>>>>>>>>>> Hello Everyone, >>>>>>>>>>> >>>>>>>>>>> For a long time I was annoyed by something in OFBiz: the build >>>>>> >>>>>> system >>>>>>>>>>> >>>>>>>>>>> does not fail if data loading fails for some files. I spend hours >>>>>>>>>>> hunting bugs only to discover that the data simply did not load. >>>>>>>>>>> >>>>>>>>>>> Given that I'm working on refactoring the data loading container, >>>>>> >>>>>> I >>>>>>>>>>> >>>>>>>>>>> believe this issue should resolved. However, I'm not sure if the >>>>>>>>>>> community is interested in making such a change. >>>>>>>>>>> >>>>>>>>>>> So I list below 3 options to select from: >>>>>>>>>>> >>>>>>>>>>> 1- Leave it as is, do not fail the build if some files do not >>>>>>>>>>> load >>>>>>>>>>> 2- Continue loading until all files are done and then fail the >>>>>> >>>>>> build >>>>>>>>>>> >>>>>>>>>>> 3- Provide a flag e.g. ignore-failure that tells the system >>>>>> >>>>>> whether >>>>>>>> >>>>>>>> to >>>>>>>>>>> >>>>>>>>>>> fail or not with a default value of "false". >>>>>>>>>>> >>>>>>>>>>> My personal preference is for (3) >>>>>>>>>>> >>>>>>>>>>> WDYT? >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Coherent Software Australia Pty Ltd >>>>>>>>> PO Box 2773 >>>>>>>>> Cheltenham Vic 3192 >>>>>>>>> Australia >>>>>>>>> >>>>>>>>> Phone: +61 3 9585 6788 >>>>>>>>> Web: http://www.coherentsoftware.com.au/ >>>>>>>>> Email: [email protected] >>>>>>>>> >
