[Discussion] Failing the build if data loading fails

[taher]

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
<[hidden email]> 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
>> <[hidden email]> 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
>>>> <[hidden email]> 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
>>>>> <[hidden email]> 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
>>>>>> <[hidden email]> 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
>>>>>>> <[hidden email]
>>>>>>>>
>>>>>>>> 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
>>>>>>>> <[hidden email]>
>>>>>>>> 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: [phone]
>>>>>>>>> http://www.hotwaxsystems.com
>>>>>>>>>
>>>>>>>>> On Mon, Jul 10, 2017 at 2:15 PM, Deepak Dixit <
>>>>>>>>> [hidden email]> 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" <[hidden email]>
>>>>>>>>
>>>>>>>> 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
>>>>>>>>>>> <[hidden email]
>>>>>>>>>>> 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
>>>>>>>>>>>> <[hidden email]> 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: [phone]
>>>>>>>>>>> Web: http://www.coherentsoftware.com.au/
>>>>>>>>>>> Email: [hidden email]
>>>>>>>>>>>
>

[taher]

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
<[hidden email]> 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
> <[hidden email]> 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
>>> <[hidden email]> 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
>>>>> <[hidden email]> 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
>>>>>> <[hidden email]> 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
>>>>>>> <[hidden email]> 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
>>>>>>>> <[hidden email]
>>>>>>>>>
>>>>>>>>> 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
>>>>>>>>> <[hidden email]>
>>>>>>>>> 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: [phone]
>>>>>>>>>> http://www.hotwaxsystems.com
>>>>>>>>>>
>>>>>>>>>> On Mon, Jul 10, 2017 at 2:15 PM, Deepak Dixit <
>>>>>>>>>> [hidden email]> 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" <[hidden email]>
>>>>>>>>>
>>>>>>>>> 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
>>>>>>>>>>>> <[hidden email]
>>>>>>>>>>>> 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
>>>>>>>>>>>>> <[hidden email]> 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: [phone]
>>>>>>>>>>>> Web: http://www.coherentsoftware.com.au/
>>>>>>>>>>>> Email: [hidden email]
>>>>>>>>>>>>
>>

[Jacques Le Roux]

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
> <[hidden email]> 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
>> <[hidden email]> 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
>>>> <[hidden email]> 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
>>>>>> <[hidden email]> 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
>>>>>>> <[hidden email]> 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
>>>>>>>> <[hidden email]> 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
>>>>>>>>> <[hidden email]
>>>>>>>>>> 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
>>>>>>>>>> <[hidden email]>
>>>>>>>>>> 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: [phone]
>>>>>>>>>>> http://www.hotwaxsystems.com
>>>>>>>>>>>
>>>>>>>>>>> On Mon, Jul 10, 2017 at 2:15 PM, Deepak Dixit <
>>>>>>>>>>> [hidden email]> 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" <[hidden email]>
>>>>>>>>>> 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
>>>>>>>>>>>>> <[hidden email]
>>>>>>>>>>>>> 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
>>>>>>>>>>>>>> <[hidden email]> 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: [phone]
>>>>>>>>>>>>> Web: http://www.coherentsoftware.com.au/
>>>>>>>>>>>>> Email: [hidden email]
>>>>>>>>>>>>>

[taher]

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
<[hidden email]> 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
>> <[hidden email]> 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
>>> <[hidden email]> 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
>>>>> <[hidden email]> 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
>>>>>>> <[hidden email]> 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
>>>>>>>> <[hidden email]> 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
>>>>>>>>> <[hidden email]> 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
>>>>>>>>>> <[hidden email]
>>>>>>>>>>>
>>>>>>>>>>> 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
>>>>>>>>>>> <[hidden email]>
>>>>>>>>>>> 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: [phone]
>>>>>>>>>>>> http://www.hotwaxsystems.com
>>>>>>>>>>>>
>>>>>>>>>>>> On Mon, Jul 10, 2017 at 2:15 PM, Deepak Dixit <
>>>>>>>>>>>> [hidden email]> 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"
>>>>>>>>>>>>>> <[hidden email]>
>>>>>>>>>>>
>>>>>>>>>>> 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
>>>>>>>>>>>>>> <[hidden email]
>>>>>>>>>>>>>> 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
>>>>>>>>>>>>>>> <[hidden email]> 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: [phone]
>>>>>>>>>>>>>> Web: http://www.coherentsoftware.com.au/
>>>>>>>>>>>>>> Email: [hidden email]
>>>>>>>>>>>>>>
>

[Jacques Le Roux]

Yes, I meant that for such next endeavours you might commit directly. Especially if you can commit smaller pieces... Anyway it's up to you...

Jacques


Le 04/08/2017 à 09:35, Taher Alkhateeb a écrit :

> 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
> <[hidden email]> 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
>>> <[hidden email]> 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
>>>> <[hidden email]> 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
>>>>>> <[hidden email]> 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
>>>>>>>> <[hidden email]> 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
>>>>>>>>> <[hidden email]> 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
>>>>>>>>>> <[hidden email]> 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
>>>>>>>>>>> <[hidden email]
>>>>>>>>>>>> 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
>>>>>>>>>>>> <[hidden email]>
>>>>>>>>>>>> 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: [phone]
>>>>>>>>>>>>> http://www.hotwaxsystems.com
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Mon, Jul 10, 2017 at 2:15 PM, Deepak Dixit <
>>>>>>>>>>>>> [hidden email]> 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"
>>>>>>>>>>>>>>> <[hidden email]>
>>>>>>>>>>>> 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
>>>>>>>>>>>>>>> <[hidden email]
>>>>>>>>>>>>>>> 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
>>>>>>>>>>>>>>>> <[hidden email]> 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: [phone]
>>>>>>>>>>>>>>> Web: http://www.coherentsoftware.com.au/
>>>>>>>>>>>>>>> Email: [hidden email]
>>>>>>>>>>>>>>>