Starting a JIRA more than a month ago, putting 5 patches and asking for reviews multiple times on the mailing list is not CTR.
On Fri, Aug 4, 2017 at 10:21 AM, Jacques Le Roux <[email protected]> wrote: > Hi Taher, > > My last reviews of your work (previous commits) let me think that we can go > in a CTR mode :) > > I'll try to do a review today though... > > About the "old paramters for --load-data" did you check that they are not > indirectly be used by webtools I mean > https://demo-trunk.ofbiz.apache.org/webtools/control/view/checkdb > > It should not be (something else is used I guess) but I did not check > > Jacques > > > > Le 04/08/2017 à 08:52, Taher Alkhateeb a écrit : >> >> 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] >>>>>>>>>>>>>> >
