Hello everyone, So I didn't get feedback for quite a while, probably because the patch is large. However, I think given that this is only a refactoring / cleanup exercise (plus the feature in this thread) I will go ahead and commit this work soon. I've tested it on my machine and things seem to be going smooth. If anyone wants to review before I commit please go ahead and raise your hand!
Cheers, Taher Alkhateeb On Thu, Jul 20, 2017 at 9:34 AM, Taher Alkhateeb <[email protected]> wrote: > Hi folks, I know it's a big patch, but it would be really great if > someone can take a look at [1]? Specifically, I have added the logic > for "continue-on-failure" plus adding old paramters for --load-data > that might not be necessary anymore? I even documented them in > README.md. This includes flags like: create-constraints, > drop-constraints, create-pks, drop-pks and so on. I would like to > remove them, but kept them because I'm not sure if people are using > them still? > > [1] https://issues.apache.org/jira/browse/OFBIZ-9441 > > On Tue, Jul 11, 2017 at 1:10 PM, Jacques Le Roux > <[email protected]> wrote: >> Yes, that's why it's a long task. I have to consider all cases carefully. >> >> That's also why I added this last comment (quoted below) in OFBIZ-8341 >> >> "I'll sometimes create subtasks or new Jira issues to differentiate cases >> that need to be discussed. >> It would be good for instance for a type of exception and a type of file >> (service, event, helper/handler/test/etc.) to use and adopt a same type of >> exception handling." >> >> Having patterns would help everybody, when creating, reviewing, refactoring, >> etc. >> >> Jacques >> >> >> >> Le 11/07/2017 à 09:47, Taher Alkhateeb a écrit : >>> >>> 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] >>>>>>>>>>>> >>
