Re: svn commit: r1799736 - /ofbiz/ofbiz-framework/trunk/b
Locked
 
|
This commit is wrong and bad on multiple levels. Please revert
On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: > Author: jleroux > Date: Sat Jun 24 07:56:45 2017 > New Revision: 1799736 > > URL: http://svn.apache.org/viewvc?rev=1799736&view=rev > Log: > No functional change > > Improves terminateOfbiz byt using TERM before KILL > https://fr.wikipedia.org/wiki/Kill_(Unix) > https://unix.stackexchange.com/questions/8916/when- > should-i-not-kill-9-a-process > > Modified: > ofbiz/ofbiz-framework/trunk/build.gradle > > Modified: ofbiz/ofbiz-framework/trunk/build.gradle > URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ > build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff > ============================================================ > ================== > --- ofbiz/ofbiz-framework/trunk/build.gradle (original) > +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 07:56:45 2017 > @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, > standardOutput = processOutput > } > processOutput.toString().split(System.lineSeparator()).each > { line -> > + // Try to terminate cleanly > if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) > { > - exec { commandLine 'kill', '-9', > line.tokenize().first() } > + exec { commandLine 'kill', '-TERM', > line.tokenize().first() } > + } > + // Only kill if needed > + if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) > { > + exec { commandLine 'kill', '-KILL', > line.tokenize().first() } > } > } > } > > > |
Re: svn commit: r1799736 - /ofbiz/ofbiz-framework/trunk/b
Administrator
|
What makes you think it's wrong? I tested it locally using 2 background instances and it cleaned worked.
I also tried with one standard instance (not in background). It works, and you get this message :ofbiz FAILED FAILURE: Build failed with an exception. * What went wrong: Execution failed for task ':ofbiz'. > Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished with non-zero exit value 137 Which I believe is OK because Java does not expect to be killed! Jacques Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit : > This commit is wrong and bad on multiple levels. Please revert > > On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: > >> Author: jleroux >> Date: Sat Jun 24 07:56:45 2017 >> New Revision: 1799736 >> >> URL: http://svn.apache.org/viewvc?rev=1799736&view=rev >> Log: >> No functional change >> >> Improves terminateOfbiz byt using TERM before KILL >> https://fr.wikipedia.org/wiki/Kill_(Unix) >> https://unix.stackexchange.com/questions/8916/when- >> should-i-not-kill-9-a-process >> >> Modified: >> ofbiz/ofbiz-framework/trunk/build.gradle >> >> Modified: ofbiz/ofbiz-framework/trunk/build.gradle >> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >> build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff >> ============================================================ >> ================== >> --- ofbiz/ofbiz-framework/trunk/build.gradle (original) >> +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 07:56:45 2017 >> @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, >> standardOutput = processOutput >> } >> processOutput.toString().split(System.lineSeparator()).each >> { line -> >> + // Try to terminate cleanly >> if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) >> { >> - exec { commandLine 'kill', '-9', >> line.tokenize().first() } >> + exec { commandLine 'kill', '-TERM', >> line.tokenize().first() } >> + } >> + // Only kill if needed >> + if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) >> { >> + exec { commandLine 'kill', '-KILL', >> line.tokenize().first() } >> } >> } >> } >> >> >> |
|
|
As usual, you refuse to revert because you don't understand the code and I
pay the price of spending my time explaining. I hope you will reconsider this time consuming and non-cooperative behavior. The quick version: - copy and paste antipattern - incorrect conditional checking leading to both blocks getting executed or both blocks not executing Your belief that Gradle fails because java does not expect to be killed is amazing! It means you do not understand what this code is doing and what is causing the failure. On Jun 25, 2017 10:42 AM, "Jacques Le Roux" <[hidden email]> wrote: What makes you think it's wrong? I tested it locally using 2 background instances and it cleaned worked. I also tried with one standard instance (not in background). It works, and you get this message :ofbiz FAILED FAILURE: Build failed with an exception. * What went wrong: Execution failed for task ':ofbiz'. > Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished with non-zero exit value 137 Which I believe is OK because Java does not expect to be killed! Jacques Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit : > This commit is wrong and bad on multiple levels. Please revert > > On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: > > Author: jleroux >> Date: Sat Jun 24 07:56:45 2017 >> New Revision: 1799736 >> >> URL: http://svn.apache.org/viewvc?rev=1799736&view=rev >> Log: >> No functional change >> >> Improves terminateOfbiz byt using TERM before KILL >> https://fr.wikipedia.org/wiki/Kill_(Unix) >> https://unix.stackexchange.com/questions/8916/when- >> should-i-not-kill-9-a-process >> >> Modified: >> ofbiz/ofbiz-framework/trunk/build.gradle >> >> Modified: ofbiz/ofbiz-framework/trunk/build.gradle >> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >> build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff >> ============================================================ >> ================== >> --- ofbiz/ofbiz-framework/trunk/build.gradle (original) >> +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 07:56:45 2017 >> @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, >> standardOutput = processOutput >> } >> processOutput.toString().split(System.lineSeparator()).each >> { line -> >> + // Try to terminate cleanly >> if (line ==~ /.*org\.apache\.ofbiz\.base\.s >> tart\.Start.*/) >> { >> - exec { commandLine 'kill', '-9', >> line.tokenize().first() } >> + exec { commandLine 'kill', '-TERM', >> line.tokenize().first() } >> + } >> + // Only kill if needed >> + if (line ==~ /.*org\.apache\.ofbiz\.base\.s >> tart\.Start.*/) >> { >> + exec { commandLine 'kill', '-KILL', >> line.tokenize().first() } >> } >> } >> } >> >> >> >> |
Re: svn commit: r1799736 - /ofbiz/ofbiz-framework/trunk/b
|
Administrator
|
I don't revert without explanations on why I should revert. Sorry, but I don't find your explanations clear.
My explanation, tell my what's wrong: The 2 "if" blocks are executed sequentially for each line containing "a start"and ignore other lines. I did not change the previous logic, just added a new if. Gradle exec spawns a new process and waits till it ends (this point is important). If the line contains "a start" the 1st "if" try to terminate it. If it worked the 2nd "if" does not execute. This is better than before because it allows the "'start" process "a chance to clean up after itself" (cf unix.stackexchange.com below) If the "start" process is not terminated then it's killed by the 2nd "if", like it was done before. As I said it cleanly worked with 2 OFBiz instances running in the background. Now tell me what's wrong? Thanks Jacques Le 25/06/2017 à 11:22, Taher Alkhateeb a écrit : > As usual, you refuse to revert because you don't understand the code and I > pay the price of spending my time explaining. I hope you will reconsider > this time consuming and non-cooperative behavior. > > The quick version: > - copy and paste antipattern > - incorrect conditional checking leading to both blocks getting executed or > both blocks not executing > > Your belief that Gradle fails because java does not expect to be killed is > amazing! It means you do not understand what this code is doing and what is > causing the failure. > > > On Jun 25, 2017 10:42 AM, "Jacques Le Roux" <[hidden email]> > wrote: > > What makes you think it's wrong? I tested it locally using 2 background > instances and it cleaned worked. > > I also tried with one standard instance (not in background). It works, and > you get this message > > :ofbiz FAILED > FAILURE: Build failed with an exception. > * What went wrong: > Execution failed for task ':ofbiz'. >> Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished with > non-zero exit value 137 > > Which I believe is OK because Java does not expect to be killed! > > Jacques > > > > Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit : > >> This commit is wrong and bad on multiple levels. Please revert >> >> On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: >> >> Author: jleroux >>> Date: Sat Jun 24 07:56:45 2017 >>> New Revision: 1799736 >>> >>> URL: http://svn.apache.org/viewvc?rev=1799736&view=rev >>> Log: >>> No functional change >>> >>> Improves terminateOfbiz byt using TERM before KILL >>> https://fr.wikipedia.org/wiki/Kill_(Unix) >>> https://unix.stackexchange.com/questions/8916/when- >>> should-i-not-kill-9-a-process >>> >>> Modified: >>> ofbiz/ofbiz-framework/trunk/build.gradle >>> >>> Modified: ofbiz/ofbiz-framework/trunk/build.gradle >>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>> build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff >>> ============================================================ >>> ================== >>> --- ofbiz/ofbiz-framework/trunk/build.gradle (original) >>> +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 07:56:45 2017 >>> @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, >>> standardOutput = processOutput >>> } >>> processOutput.toString().split(System.lineSeparator()).each >>> { line -> >>> + // Try to terminate cleanly >>> if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>> tart\.Start.*/) >>> { >>> - exec { commandLine 'kill', '-9', >>> line.tokenize().first() } >>> + exec { commandLine 'kill', '-TERM', >>> line.tokenize().first() } >>> + } >>> + // Only kill if needed >>> + if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>> tart\.Start.*/) >>> { >>> + exec { commandLine 'kill', '-KILL', >>> line.tokenize().first() } >>> } >>> } >>> } >>> >>> >>> >>> |
Re: svn commit: r1799736 - /ofbiz/ofbiz-framework/trunk/b
|
Administrator
|
Ah wait, I see line still contains "start" so will be executed twice anyway. OK I'll improve that.
Jacques Le 25/06/2017 à 12:18, Jacques Le Roux a écrit : > I don't revert without explanations on why I should revert. Sorry, but I don't find your explanations clear. > > My explanation, tell my what's wrong: > > The 2 "if" blocks are executed sequentially for each line containing "a start"and ignore other lines. I did not change the previous logic, just > added a new if. > > Gradle exec spawns a new process and waits till it ends (this point is important). > > If the line contains "a start" the 1st "if" try to terminate it. > > If it worked the 2nd "if" does not execute. This is better than before because it allows the "'start" process "a chance to clean up after itself" > (cf unix.stackexchange.com below) > > If the "start" process is not terminated then it's killed by the 2nd "if", like it was done before. > > As I said it cleanly worked with 2 OFBiz instances running in the background. > > Now tell me what's wrong? > > Thanks > > Jacques > > > Le 25/06/2017 à 11:22, Taher Alkhateeb a écrit : >> As usual, you refuse to revert because you don't understand the code and I >> pay the price of spending my time explaining. I hope you will reconsider >> this time consuming and non-cooperative behavior. >> >> The quick version: >> - copy and paste antipattern >> - incorrect conditional checking leading to both blocks getting executed or >> both blocks not executing >> >> Your belief that Gradle fails because java does not expect to be killed is >> amazing! It means you do not understand what this code is doing and what is >> causing the failure. >> >> >> On Jun 25, 2017 10:42 AM, "Jacques Le Roux" <[hidden email]> >> wrote: >> >> What makes you think it's wrong? I tested it locally using 2 background >> instances and it cleaned worked. >> >> I also tried with one standard instance (not in background). It works, and >> you get this message >> >> :ofbiz FAILED >> FAILURE: Build failed with an exception. >> * What went wrong: >> Execution failed for task ':ofbiz'. >>> Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished with >> non-zero exit value 137 >> >> Which I believe is OK because Java does not expect to be killed! >> >> Jacques >> >> >> >> Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit : >> >>> This commit is wrong and bad on multiple levels. Please revert >>> >>> On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: >>> >>> Author: jleroux >>>> Date: Sat Jun 24 07:56:45 2017 >>>> New Revision: 1799736 >>>> >>>> URL: http://svn.apache.org/viewvc?rev=1799736&view=rev >>>> Log: >>>> No functional change >>>> >>>> Improves terminateOfbiz byt using TERM before KILL >>>> https://fr.wikipedia.org/wiki/Kill_(Unix) >>>> https://unix.stackexchange.com/questions/8916/when- >>>> should-i-not-kill-9-a-process >>>> >>>> Modified: >>>> ofbiz/ofbiz-framework/trunk/build.gradle >>>> >>>> Modified: ofbiz/ofbiz-framework/trunk/build.gradle >>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>> build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff >>>> ============================================================ >>>> ================== >>>> --- ofbiz/ofbiz-framework/trunk/build.gradle (original) >>>> +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 07:56:45 2017 >>>> @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, >>>> standardOutput = processOutput >>>> } >>>> processOutput.toString().split(System.lineSeparator()).each >>>> { line -> >>>> + // Try to terminate cleanly >>>> if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>> tart\.Start.*/) >>>> { >>>> - exec { commandLine 'kill', '-9', >>>> line.tokenize().first() } >>>> + exec { commandLine 'kill', '-TERM', >>>> line.tokenize().first() } >>>> + } >>>> + // Only kill if needed >>>> + if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>> tart\.Start.*/) >>>> { >>>> + exec { commandLine 'kill', '-KILL', >>>> line.tokenize().first() } >>>> } >>>> } >>>> } >>>> >>>> >>>> >>>> > > |
|
|
Good, now I kindly ask you to try not to take up _my_ time for _your_
mistakes by reverting instead of arguing. You refuse every time I ask for a revert and end up doing it anyway but after taking up what little time we have by arguing when there is so much work to do. On Sun, Jun 25, 2017 at 1:28 PM, Jacques Le Roux < [hidden email]> wrote: > Ah wait, I see line still contains "start" so will be executed twice > anyway. OK I'll improve that. > > Jacques > > > > Le 25/06/2017 à 12:18, Jacques Le Roux a écrit : > >> I don't revert without explanations on why I should revert. Sorry, but I >> don't find your explanations clear. >> >> My explanation, tell my what's wrong: >> >> The 2 "if" blocks are executed sequentially for each line containing "a >> start"and ignore other lines. I did not change the previous logic, just >> added a new if. >> >> Gradle exec spawns a new process and waits till it ends (this point is >> important). >> >> If the line contains "a start" the 1st "if" try to terminate it. >> >> If it worked the 2nd "if" does not execute. This is better than before >> because it allows the "'start" process "a chance to clean up after itself" >> (cf unix.stackexchange.com below) >> >> If the "start" process is not terminated then it's killed by the 2nd >> "if", like it was done before. >> >> As I said it cleanly worked with 2 OFBiz instances running in the >> background. >> >> Now tell me what's wrong? >> >> Thanks >> >> Jacques >> >> >> Le 25/06/2017 à 11:22, Taher Alkhateeb a écrit : >> >>> As usual, you refuse to revert because you don't understand the code and >>> I >>> pay the price of spending my time explaining. I hope you will reconsider >>> this time consuming and non-cooperative behavior. >>> >>> The quick version: >>> - copy and paste antipattern >>> - incorrect conditional checking leading to both blocks getting executed >>> or >>> both blocks not executing >>> >>> Your belief that Gradle fails because java does not expect to be killed >>> is >>> amazing! It means you do not understand what this code is doing and what >>> is >>> causing the failure. >>> >>> >>> On Jun 25, 2017 10:42 AM, "Jacques Le Roux" < >>> [hidden email]> >>> wrote: >>> >>> What makes you think it's wrong? I tested it locally using 2 background >>> instances and it cleaned worked. >>> >>> I also tried with one standard instance (not in background). It works, >>> and >>> you get this message >>> >>> :ofbiz FAILED >>> FAILURE: Build failed with an exception. >>> * What went wrong: >>> Execution failed for task ':ofbiz'. >>> >>>> Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished with >>>> >>> non-zero exit value 137 >>> >>> Which I believe is OK because Java does not expect to be killed! >>> >>> Jacques >>> >>> >>> >>> Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit : >>> >>> This commit is wrong and bad on multiple levels. Please revert >>>> >>>> On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: >>>> >>>> Author: jleroux >>>> >>>>> Date: Sat Jun 24 07:56:45 2017 >>>>> New Revision: 1799736 >>>>> >>>>> URL: http://svn.apache.org/viewvc?rev=1799736&view=rev >>>>> Log: >>>>> No functional change >>>>> >>>>> Improves terminateOfbiz byt using TERM before KILL >>>>> https://fr.wikipedia.org/wiki/Kill_(Unix) >>>>> https://unix.stackexchange.com/questions/8916/when- >>>>> should-i-not-kill-9-a-process >>>>> >>>>> Modified: >>>>> ofbiz/ofbiz-framework/trunk/build.gradle >>>>> >>>>> Modified: ofbiz/ofbiz-framework/trunk/build.gradle >>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>> build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff >>>>> ============================================================ >>>>> ================== >>>>> --- ofbiz/ofbiz-framework/trunk/build.gradle (original) >>>>> +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 07:56:45 2017 >>>>> @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, >>>>> standardOutput = processOutput >>>>> } >>>>> processOutput.toString().split(System.lineSeparator()).each >>>>> { line -> >>>>> + // Try to terminate cleanly >>>>> if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>> tart\.Start.*/) >>>>> { >>>>> - exec { commandLine 'kill', '-9', >>>>> line.tokenize().first() } >>>>> + exec { commandLine 'kill', '-TERM', >>>>> line.tokenize().first() } >>>>> + } >>>>> + // Only kill if needed >>>>> + if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>> tart\.Start.*/) >>>>> { >>>>> + exec { commandLine 'kill', '-KILL', >>>>> line.tokenize().first() } >>>>> } >>>>> } >>>>> } >>>>> >>>>> >>>>> >>>>> >>>>> >> >> > |
Re: svn commit: r1799736 - /ofbiz/ofbiz-framework/trunk/b
|
Administrator
|
In reply to this post by Jacques Le Roux
Mmm but anyway, I remember now how I thought that.
In the 2nd "if", if the process was terminated, kill will just say that it can't find the process and exec will return. Else the process will be killed. What's wrong then? Jacques Le 25/06/2017 à 12:28, Jacques Le Roux a écrit : > Ah wait, I see line still contains "start" so will be executed twice anyway. OK I'll improve that. > > Jacques > > > Le 25/06/2017 à 12:18, Jacques Le Roux a écrit : >> I don't revert without explanations on why I should revert. Sorry, but I don't find your explanations clear. >> >> My explanation, tell my what's wrong: >> >> The 2 "if" blocks are executed sequentially for each line containing "a start"and ignore other lines. I did not change the previous logic, just >> added a new if. >> >> Gradle exec spawns a new process and waits till it ends (this point is important). >> >> If the line contains "a start" the 1st "if" try to terminate it. >> >> If it worked the 2nd "if" does not execute. This is better than before because it allows the "'start" process "a chance to clean up after itself" >> (cf unix.stackexchange.com below) >> >> If the "start" process is not terminated then it's killed by the 2nd "if", like it was done before. >> >> As I said it cleanly worked with 2 OFBiz instances running in the background. >> >> Now tell me what's wrong? >> >> Thanks >> >> Jacques >> >> >> Le 25/06/2017 à 11:22, Taher Alkhateeb a écrit : >>> As usual, you refuse to revert because you don't understand the code and I >>> pay the price of spending my time explaining. I hope you will reconsider >>> this time consuming and non-cooperative behavior. >>> >>> The quick version: >>> - copy and paste antipattern >>> - incorrect conditional checking leading to both blocks getting executed or >>> both blocks not executing >>> >>> Your belief that Gradle fails because java does not expect to be killed is >>> amazing! It means you do not understand what this code is doing and what is >>> causing the failure. >>> >>> >>> On Jun 25, 2017 10:42 AM, "Jacques Le Roux" <[hidden email]> >>> wrote: >>> >>> What makes you think it's wrong? I tested it locally using 2 background >>> instances and it cleaned worked. >>> >>> I also tried with one standard instance (not in background). It works, and >>> you get this message >>> >>> :ofbiz FAILED >>> FAILURE: Build failed with an exception. >>> * What went wrong: >>> Execution failed for task ':ofbiz'. >>>> Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished with >>> non-zero exit value 137 >>> >>> Which I believe is OK because Java does not expect to be killed! >>> >>> Jacques >>> >>> >>> >>> Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit : >>> >>>> This commit is wrong and bad on multiple levels. Please revert >>>> >>>> On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: >>>> >>>> Author: jleroux >>>>> Date: Sat Jun 24 07:56:45 2017 >>>>> New Revision: 1799736 >>>>> >>>>> URL: http://svn.apache.org/viewvc?rev=1799736&view=rev >>>>> Log: >>>>> No functional change >>>>> >>>>> Improves terminateOfbiz byt using TERM before KILL >>>>> https://fr.wikipedia.org/wiki/Kill_(Unix) >>>>> https://unix.stackexchange.com/questions/8916/when- >>>>> should-i-not-kill-9-a-process >>>>> >>>>> Modified: >>>>> ofbiz/ofbiz-framework/trunk/build.gradle >>>>> >>>>> Modified: ofbiz/ofbiz-framework/trunk/build.gradle >>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>> build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff >>>>> ============================================================ >>>>> ================== >>>>> --- ofbiz/ofbiz-framework/trunk/build.gradle (original) >>>>> +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 07:56:45 2017 >>>>> @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, >>>>> standardOutput = processOutput >>>>> } >>>>> processOutput.toString().split(System.lineSeparator()).each >>>>> { line -> >>>>> + // Try to terminate cleanly >>>>> if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>> tart\.Start.*/) >>>>> { >>>>> - exec { commandLine 'kill', '-9', >>>>> line.tokenize().first() } >>>>> + exec { commandLine 'kill', '-TERM', >>>>> line.tokenize().first() } >>>>> + } >>>>> + // Only kill if needed >>>>> + if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>> tart\.Start.*/) >>>>> { >>>>> + exec { commandLine 'kill', '-KILL', >>>>> line.tokenize().first() } >>>>> } >>>>> } >>>>> } >>>>> >>>>> >>>>> >>>>> >> >> > > |
|
|
OK, I take this question as your refusal to revert and cooperate. I'm done
spending time here and I leave it up to the community to see if they want incorrect code in the code base. On Sun, Jun 25, 2017 at 1:34 PM, Jacques Le Roux < [hidden email]> wrote: > Mmm but anyway, I remember now how I thought that. > > In the 2nd "if", if the process was terminated, kill will just say that it > can't find the process and exec will return. Else the process will be > killed. > > What's wrong then? > > Jacques > > > > Le 25/06/2017 à 12:28, Jacques Le Roux a écrit : > >> Ah wait, I see line still contains "start" so will be executed twice >> anyway. OK I'll improve that. >> >> Jacques >> >> >> Le 25/06/2017 à 12:18, Jacques Le Roux a écrit : >> >>> I don't revert without explanations on why I should revert. Sorry, but I >>> don't find your explanations clear. >>> >>> My explanation, tell my what's wrong: >>> >>> The 2 "if" blocks are executed sequentially for each line containing "a >>> start"and ignore other lines. I did not change the previous logic, just >>> added a new if. >>> >>> Gradle exec spawns a new process and waits till it ends (this point is >>> important). >>> >>> If the line contains "a start" the 1st "if" try to terminate it. >>> >>> If it worked the 2nd "if" does not execute. This is better than before >>> because it allows the "'start" process "a chance to clean up after itself" >>> (cf unix.stackexchange.com below) >>> >>> If the "start" process is not terminated then it's killed by the 2nd >>> "if", like it was done before. >>> >>> As I said it cleanly worked with 2 OFBiz instances running in the >>> background. >>> >>> Now tell me what's wrong? >>> >>> Thanks >>> >>> Jacques >>> >>> >>> Le 25/06/2017 à 11:22, Taher Alkhateeb a écrit : >>> >>>> As usual, you refuse to revert because you don't understand the code >>>> and I >>>> pay the price of spending my time explaining. I hope you will reconsider >>>> this time consuming and non-cooperative behavior. >>>> >>>> The quick version: >>>> - copy and paste antipattern >>>> - incorrect conditional checking leading to both blocks getting >>>> executed or >>>> both blocks not executing >>>> >>>> Your belief that Gradle fails because java does not expect to be killed >>>> is >>>> amazing! It means you do not understand what this code is doing and >>>> what is >>>> causing the failure. >>>> >>>> >>>> On Jun 25, 2017 10:42 AM, "Jacques Le Roux" < >>>> [hidden email]> >>>> wrote: >>>> >>>> What makes you think it's wrong? I tested it locally using 2 background >>>> instances and it cleaned worked. >>>> >>>> I also tried with one standard instance (not in background). It works, >>>> and >>>> you get this message >>>> >>>> :ofbiz FAILED >>>> FAILURE: Build failed with an exception. >>>> * What went wrong: >>>> Execution failed for task ':ofbiz'. >>>> >>>>> Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished with >>>>> >>>> non-zero exit value 137 >>>> >>>> Which I believe is OK because Java does not expect to be killed! >>>> >>>> Jacques >>>> >>>> >>>> >>>> Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit : >>>> >>>> This commit is wrong and bad on multiple levels. Please revert >>>>> >>>>> On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: >>>>> >>>>> Author: jleroux >>>>> >>>>>> Date: Sat Jun 24 07:56:45 2017 >>>>>> New Revision: 1799736 >>>>>> >>>>>> URL: http://svn.apache.org/viewvc?rev=1799736&view=rev >>>>>> Log: >>>>>> No functional change >>>>>> >>>>>> Improves terminateOfbiz byt using TERM before KILL >>>>>> https://fr.wikipedia.org/wiki/Kill_(Unix) >>>>>> https://unix.stackexchange.com/questions/8916/when- >>>>>> should-i-not-kill-9-a-process >>>>>> >>>>>> Modified: >>>>>> ofbiz/ofbiz-framework/trunk/build.gradle >>>>>> >>>>>> Modified: ofbiz/ofbiz-framework/trunk/build.gradle >>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>>> build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff >>>>>> ============================================================ >>>>>> ================== >>>>>> --- ofbiz/ofbiz-framework/trunk/build.gradle (original) >>>>>> +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 07:56:45 2017 >>>>>> @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, >>>>>> standardOutput = processOutput >>>>>> } >>>>>> processOutput.toString().split(System.lineSeparator()).each >>>>>> { line -> >>>>>> + // Try to terminate cleanly >>>>>> if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>> tart\.Start.*/) >>>>>> { >>>>>> - exec { commandLine 'kill', '-9', >>>>>> line.tokenize().first() } >>>>>> + exec { commandLine 'kill', '-TERM', >>>>>> line.tokenize().first() } >>>>>> + } >>>>>> + // Only kill if needed >>>>>> + if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>> tart\.Start.*/) >>>>>> { >>>>>> + exec { commandLine 'kill', '-KILL', >>>>>> line.tokenize().first() } >>>>>> } >>>>>> } >>>>>> } >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>> >>> >> >> > |
Re: svn commit: r1799736 - /ofbiz/ofbiz-framework/trunk/b
|
I's like to propose the following process to get this worked out without
endless ping-pong here on the mailing lists: 1. revert the change as requested 2. provide a Jira with patch and review/discuss it there (what's the problem, how should it be solved etc.) 3. come to a conclusion and a thumbsup by other committers 4. commit the solution. Thanks, Michael Am 25.06.17 um 12:36 schrieb Taher Alkhateeb: > OK, I take this question as your refusal to revert and cooperate. I'm done > spending time here and I leave it up to the community to see if they want > incorrect code in the code base. > > On Sun, Jun 25, 2017 at 1:34 PM, Jacques Le Roux < > [hidden email]> wrote: > >> Mmm but anyway, I remember now how I thought that. >> >> In the 2nd "if", if the process was terminated, kill will just say that it >> can't find the process and exec will return. Else the process will be >> killed. >> >> What's wrong then? >> >> Jacques >> >> >> >> Le 25/06/2017 à 12:28, Jacques Le Roux a écrit : >> >>> Ah wait, I see line still contains "start" so will be executed twice >>> anyway. OK I'll improve that. >>> >>> Jacques >>> >>> >>> Le 25/06/2017 à 12:18, Jacques Le Roux a écrit : >>> >>>> I don't revert without explanations on why I should revert. Sorry, but I >>>> don't find your explanations clear. >>>> >>>> My explanation, tell my what's wrong: >>>> >>>> The 2 "if" blocks are executed sequentially for each line containing "a >>>> start"and ignore other lines. I did not change the previous logic, just >>>> added a new if. >>>> >>>> Gradle exec spawns a new process and waits till it ends (this point is >>>> important). >>>> >>>> If the line contains "a start" the 1st "if" try to terminate it. >>>> >>>> If it worked the 2nd "if" does not execute. This is better than before >>>> because it allows the "'start" process "a chance to clean up after itself" >>>> (cf unix.stackexchange.com below) >>>> >>>> If the "start" process is not terminated then it's killed by the 2nd >>>> "if", like it was done before. >>>> >>>> As I said it cleanly worked with 2 OFBiz instances running in the >>>> background. >>>> >>>> Now tell me what's wrong? >>>> >>>> Thanks >>>> >>>> Jacques >>>> >>>> >>>> Le 25/06/2017 à 11:22, Taher Alkhateeb a écrit : >>>> >>>>> As usual, you refuse to revert because you don't understand the code >>>>> and I >>>>> pay the price of spending my time explaining. I hope you will reconsider >>>>> this time consuming and non-cooperative behavior. >>>>> >>>>> The quick version: >>>>> - copy and paste antipattern >>>>> - incorrect conditional checking leading to both blocks getting >>>>> executed or >>>>> both blocks not executing >>>>> >>>>> Your belief that Gradle fails because java does not expect to be killed >>>>> is >>>>> amazing! It means you do not understand what this code is doing and >>>>> what is >>>>> causing the failure. >>>>> >>>>> >>>>> On Jun 25, 2017 10:42 AM, "Jacques Le Roux" < >>>>> [hidden email]> >>>>> wrote: >>>>> >>>>> What makes you think it's wrong? I tested it locally using 2 background >>>>> instances and it cleaned worked. >>>>> >>>>> I also tried with one standard instance (not in background). It works, >>>>> and >>>>> you get this message >>>>> >>>>> :ofbiz FAILED >>>>> FAILURE: Build failed with an exception. >>>>> * What went wrong: >>>>> Execution failed for task ':ofbiz'. >>>>> >>>>>> Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished with >>>>>> >>>>> non-zero exit value 137 >>>>> >>>>> Which I believe is OK because Java does not expect to be killed! >>>>> >>>>> Jacques >>>>> >>>>> >>>>> >>>>> Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit : >>>>> >>>>> This commit is wrong and bad on multiple levels. Please revert >>>>>> On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: >>>>>> >>>>>> Author: jleroux >>>>>> >>>>>>> Date: Sat Jun 24 07:56:45 2017 >>>>>>> New Revision: 1799736 >>>>>>> >>>>>>> URL: http://svn.apache.org/viewvc?rev=1799736&view=rev >>>>>>> Log: >>>>>>> No functional change >>>>>>> >>>>>>> Improves terminateOfbiz byt using TERM before KILL >>>>>>> https://fr.wikipedia.org/wiki/Kill_(Unix) >>>>>>> https://unix.stackexchange.com/questions/8916/when- >>>>>>> should-i-not-kill-9-a-process >>>>>>> >>>>>>> Modified: >>>>>>> ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>> >>>>>>> Modified: ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>>>> build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff >>>>>>> ============================================================ >>>>>>> ================== >>>>>>> --- ofbiz/ofbiz-framework/trunk/build.gradle (original) >>>>>>> +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 07:56:45 2017 >>>>>>> @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, >>>>>>> standardOutput = processOutput >>>>>>> } >>>>>>> processOutput.toString().split(System.lineSeparator()).each >>>>>>> { line -> >>>>>>> + // Try to terminate cleanly >>>>>>> if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>>> tart\.Start.*/) >>>>>>> { >>>>>>> - exec { commandLine 'kill', '-9', >>>>>>> line.tokenize().first() } >>>>>>> + exec { commandLine 'kill', '-TERM', >>>>>>> line.tokenize().first() } >>>>>>> + } >>>>>>> + // Only kill if needed >>>>>>> + if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>>> tart\.Start.*/) >>>>>>> { >>>>>>> + exec { commandLine 'kill', '-KILL', >>>>>>> line.tokenize().first() } >>>>>>> } >>>>>>> } >>>>>>> } >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>> >>> |
smime.p7s (5K)
Re: svn commit: r1799736 - /ofbiz/ofbiz-framework/trunk/b
|
Administrator
|
But what's wrong? It's not clear to me, do you see it Michael?
Not clearly answering this question just adds more work on my side, I still don't see why I should revert. Jacques Le 25/06/2017 à 13:11, Michael Brohl a écrit : > I's like to propose the following process to get this worked out without endless ping-pong here on the mailing lists: > > 1. revert the change as requested > > 2. provide a Jira with patch and review/discuss it there (what's the problem, how should it be solved etc.) > > 3. come to a conclusion and a thumbsup by other committers > > 4. commit the solution. > > Thanks, > > Michael > > > Am 25.06.17 um 12:36 schrieb Taher Alkhateeb: >> OK, I take this question as your refusal to revert and cooperate. I'm done >> spending time here and I leave it up to the community to see if they want >> incorrect code in the code base. >> >> On Sun, Jun 25, 2017 at 1:34 PM, Jacques Le Roux < >> [hidden email]> wrote: >> >>> Mmm but anyway, I remember now how I thought that. >>> >>> In the 2nd "if", if the process was terminated, kill will just say that it >>> can't find the process and exec will return. Else the process will be >>> killed. >>> >>> What's wrong then? >>> >>> Jacques >>> >>> >>> >>> Le 25/06/2017 à 12:28, Jacques Le Roux a écrit : >>> >>>> Ah wait, I see line still contains "start" so will be executed twice >>>> anyway. OK I'll improve that. >>>> >>>> Jacques >>>> >>>> >>>> Le 25/06/2017 à 12:18, Jacques Le Roux a écrit : >>>> >>>>> I don't revert without explanations on why I should revert. Sorry, but I >>>>> don't find your explanations clear. >>>>> >>>>> My explanation, tell my what's wrong: >>>>> >>>>> The 2 "if" blocks are executed sequentially for each line containing "a >>>>> start"and ignore other lines. I did not change the previous logic, just >>>>> added a new if. >>>>> >>>>> Gradle exec spawns a new process and waits till it ends (this point is >>>>> important). >>>>> >>>>> If the line contains "a start" the 1st "if" try to terminate it. >>>>> >>>>> If it worked the 2nd "if" does not execute. This is better than before >>>>> because it allows the "'start" process "a chance to clean up after itself" >>>>> (cf unix.stackexchange.com below) >>>>> >>>>> If the "start" process is not terminated then it's killed by the 2nd >>>>> "if", like it was done before. >>>>> >>>>> As I said it cleanly worked with 2 OFBiz instances running in the >>>>> background. >>>>> >>>>> Now tell me what's wrong? >>>>> >>>>> Thanks >>>>> >>>>> Jacques >>>>> >>>>> >>>>> Le 25/06/2017 à 11:22, Taher Alkhateeb a écrit : >>>>> >>>>>> As usual, you refuse to revert because you don't understand the code >>>>>> and I >>>>>> pay the price of spending my time explaining. I hope you will reconsider >>>>>> this time consuming and non-cooperative behavior. >>>>>> >>>>>> The quick version: >>>>>> - copy and paste antipattern >>>>>> - incorrect conditional checking leading to both blocks getting >>>>>> executed or >>>>>> both blocks not executing >>>>>> >>>>>> Your belief that Gradle fails because java does not expect to be killed >>>>>> is >>>>>> amazing! It means you do not understand what this code is doing and >>>>>> what is >>>>>> causing the failure. >>>>>> >>>>>> >>>>>> On Jun 25, 2017 10:42 AM, "Jacques Le Roux" < >>>>>> [hidden email]> >>>>>> wrote: >>>>>> >>>>>> What makes you think it's wrong? I tested it locally using 2 background >>>>>> instances and it cleaned worked. >>>>>> >>>>>> I also tried with one standard instance (not in background). It works, >>>>>> and >>>>>> you get this message >>>>>> >>>>>> :ofbiz FAILED >>>>>> FAILURE: Build failed with an exception. >>>>>> * What went wrong: >>>>>> Execution failed for task ':ofbiz'. >>>>>> >>>>>>> Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished with >>>>>>> >>>>>> non-zero exit value 137 >>>>>> >>>>>> Which I believe is OK because Java does not expect to be killed! >>>>>> >>>>>> Jacques >>>>>> >>>>>> >>>>>> >>>>>> Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit : >>>>>> >>>>>> This commit is wrong and bad on multiple levels. Please revert >>>>>>> On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: >>>>>>> >>>>>>> Author: jleroux >>>>>>> >>>>>>>> Date: Sat Jun 24 07:56:45 2017 >>>>>>>> New Revision: 1799736 >>>>>>>> >>>>>>>> URL: http://svn.apache.org/viewvc?rev=1799736&view=rev >>>>>>>> Log: >>>>>>>> No functional change >>>>>>>> >>>>>>>> Improves terminateOfbiz byt using TERM before KILL >>>>>>>> https://fr.wikipedia.org/wiki/Kill_(Unix) >>>>>>>> https://unix.stackexchange.com/questions/8916/when- >>>>>>>> should-i-not-kill-9-a-process >>>>>>>> >>>>>>>> Modified: >>>>>>>> ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>>> >>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>>>>> build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff >>>>>>>> ============================================================ >>>>>>>> ================== >>>>>>>> --- ofbiz/ofbiz-framework/trunk/build.gradle (original) >>>>>>>> +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 07:56:45 2017 >>>>>>>> @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, >>>>>>>> standardOutput = processOutput >>>>>>>> } >>>>>>>> processOutput.toString().split(System.lineSeparator()).each >>>>>>>> { line -> >>>>>>>> + // Try to terminate cleanly >>>>>>>> if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>>>> tart\.Start.*/) >>>>>>>> { >>>>>>>> - exec { commandLine 'kill', '-9', >>>>>>>> line.tokenize().first() } >>>>>>>> + exec { commandLine 'kill', '-TERM', >>>>>>>> line.tokenize().first() } >>>>>>>> + } >>>>>>>> + // Only kill if needed >>>>>>>> + if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>>>> tart\.Start.*/) >>>>>>>> { >>>>>>>> + exec { commandLine 'kill', '-KILL', >>>>>>>> line.tokenize().first() } >>>>>>>> } >>>>>>>> } >>>>>>>> } >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>> >>>> > > |
|
|
I'm completing my answer for the record to show exactly where your code is
faulty and wrong just like in the other previous cases. 1- you have a copy-paste pattern in your code 2- you are calling the kill command twice immediately one after another without checking whether SIGTERM killed the process properly. The whole purpose of doing SIGTERM is to allow cleaning resources before enforcing SIGKILL. You're not giving the JVM an opportunity to call the shutdown hook because you're looking up the same output from the same "ps ax" command for both kill commands. So your code is working almost exactly as if typing: if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) { exec { commandLine 'kill', '-TERM', line.tokenize().first() } exec { commandLine 'kill', '-KILL', line.tokenize().first() } } Now what's happening is that SIGKILL is executing before SIGTERM completes, so the build is successful but OFBiz is killed with SIGKILL not SIGTERM. If SIGTERM finishes very quickly (does not happen with me) then the build system would crash because the exit value of SIGKILL would be 1, not 0 (process not found). So either way it is a problem (either killing with SIGKILL or crashing the build system) Convinced? I hope you can revert now. On Sun, Jun 25, 2017 at 2:20 PM, Jacques Le Roux < [hidden email]> wrote: > But what's wrong? It's not clear to me, do you see it Michael? > > Not clearly answering this question just adds more work on my side, I > still don't see why I should revert. > > Jacques > > > > Le 25/06/2017 à 13:11, Michael Brohl a écrit : > >> I's like to propose the following process to get this worked out without >> endless ping-pong here on the mailing lists: >> >> 1. revert the change as requested >> >> 2. provide a Jira with patch and review/discuss it there (what's the >> problem, how should it be solved etc.) >> >> 3. come to a conclusion and a thumbsup by other committers >> >> 4. commit the solution. >> >> Thanks, >> >> Michael >> >> >> Am 25.06.17 um 12:36 schrieb Taher Alkhateeb: >> >>> OK, I take this question as your refusal to revert and cooperate. I'm >>> done >>> spending time here and I leave it up to the community to see if they want >>> incorrect code in the code base. >>> >>> On Sun, Jun 25, 2017 at 1:34 PM, Jacques Le Roux < >>> [hidden email]> wrote: >>> >>> Mmm but anyway, I remember now how I thought that. >>>> >>>> In the 2nd "if", if the process was terminated, kill will just say that >>>> it >>>> can't find the process and exec will return. Else the process will be >>>> killed. >>>> >>>> What's wrong then? >>>> >>>> Jacques >>>> >>>> >>>> >>>> Le 25/06/2017 à 12:28, Jacques Le Roux a écrit : >>>> >>>> Ah wait, I see line still contains "start" so will be executed twice >>>>> anyway. OK I'll improve that. >>>>> >>>>> Jacques >>>>> >>>>> >>>>> Le 25/06/2017 à 12:18, Jacques Le Roux a écrit : >>>>> >>>>> I don't revert without explanations on why I should revert. Sorry, but >>>>>> I >>>>>> don't find your explanations clear. >>>>>> >>>>>> My explanation, tell my what's wrong: >>>>>> >>>>>> The 2 "if" blocks are executed sequentially for each line containing >>>>>> "a >>>>>> start"and ignore other lines. I did not change the previous logic, >>>>>> just >>>>>> added a new if. >>>>>> >>>>>> Gradle exec spawns a new process and waits till it ends (this point is >>>>>> important). >>>>>> >>>>>> If the line contains "a start" the 1st "if" try to terminate it. >>>>>> >>>>>> If it worked the 2nd "if" does not execute. This is better than before >>>>>> because it allows the "'start" process "a chance to clean up after >>>>>> itself" >>>>>> (cf unix.stackexchange.com below) >>>>>> >>>>>> If the "start" process is not terminated then it's killed by the 2nd >>>>>> "if", like it was done before. >>>>>> >>>>>> As I said it cleanly worked with 2 OFBiz instances running in the >>>>>> background. >>>>>> >>>>>> Now tell me what's wrong? >>>>>> >>>>>> Thanks >>>>>> >>>>>> Jacques >>>>>> >>>>>> >>>>>> Le 25/06/2017 à 11:22, Taher Alkhateeb a écrit : >>>>>> >>>>>> As usual, you refuse to revert because you don't understand the code >>>>>>> and I >>>>>>> pay the price of spending my time explaining. I hope you will >>>>>>> reconsider >>>>>>> this time consuming and non-cooperative behavior. >>>>>>> >>>>>>> The quick version: >>>>>>> - copy and paste antipattern >>>>>>> - incorrect conditional checking leading to both blocks getting >>>>>>> executed or >>>>>>> both blocks not executing >>>>>>> >>>>>>> Your belief that Gradle fails because java does not expect to be >>>>>>> killed >>>>>>> is >>>>>>> amazing! It means you do not understand what this code is doing and >>>>>>> what is >>>>>>> causing the failure. >>>>>>> >>>>>>> >>>>>>> On Jun 25, 2017 10:42 AM, "Jacques Le Roux" < >>>>>>> [hidden email]> >>>>>>> wrote: >>>>>>> >>>>>>> What makes you think it's wrong? I tested it locally using 2 >>>>>>> background >>>>>>> instances and it cleaned worked. >>>>>>> >>>>>>> I also tried with one standard instance (not in background). It >>>>>>> works, >>>>>>> and >>>>>>> you get this message >>>>>>> >>>>>>> :ofbiz FAILED >>>>>>> FAILURE: Build failed with an exception. >>>>>>> * What went wrong: >>>>>>> Execution failed for task ':ofbiz'. >>>>>>> >>>>>>> Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished >>>>>>>> with >>>>>>>> >>>>>>>> non-zero exit value 137 >>>>>>> >>>>>>> Which I believe is OK because Java does not expect to be killed! >>>>>>> >>>>>>> Jacques >>>>>>> >>>>>>> >>>>>>> >>>>>>> Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit : >>>>>>> >>>>>>> This commit is wrong and bad on multiple levels. Please revert >>>>>>> >>>>>>>> On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: >>>>>>>> >>>>>>>> Author: jleroux >>>>>>>> >>>>>>>> Date: Sat Jun 24 07:56:45 2017 >>>>>>>>> New Revision: 1799736 >>>>>>>>> >>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1799736&view=rev >>>>>>>>> Log: >>>>>>>>> No functional change >>>>>>>>> >>>>>>>>> Improves terminateOfbiz byt using TERM before KILL >>>>>>>>> https://fr.wikipedia.org/wiki/Kill_(Unix) >>>>>>>>> https://unix.stackexchange.com/questions/8916/when- >>>>>>>>> should-i-not-kill-9-a-process >>>>>>>>> >>>>>>>>> Modified: >>>>>>>>> ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>>>> >>>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>>>>>> build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff >>>>>>>>> ============================================================ >>>>>>>>> ================== >>>>>>>>> --- ofbiz/ofbiz-framework/trunk/build.gradle (original) >>>>>>>>> +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 07:56:45 >>>>>>>>> 2017 >>>>>>>>> @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, >>>>>>>>> standardOutput = processOutput >>>>>>>>> } >>>>>>>>> processOutput.toString().split(System.lineSeparator()).each >>>>>>>>> { line -> >>>>>>>>> + // Try to terminate cleanly >>>>>>>>> if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>>>>> tart\.Start.*/) >>>>>>>>> { >>>>>>>>> - exec { commandLine 'kill', '-9', >>>>>>>>> line.tokenize().first() } >>>>>>>>> + exec { commandLine 'kill', '-TERM', >>>>>>>>> line.tokenize().first() } >>>>>>>>> + } >>>>>>>>> + // Only kill if needed >>>>>>>>> + if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>>>>> tart\.Start.*/) >>>>>>>>> { >>>>>>>>> + exec { commandLine 'kill', '-KILL', >>>>>>>>> line.tokenize().first() } >>>>>>>>> } >>>>>>>>> } >>>>>>>>> } >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>> >>>>> >> >> > |
|
|
Oh and to prove the second scenario, just add a sleep for a few seconds
after the SIGTERM kill command and observe how the system will crash: if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) { exec { commandLine 'kill', '-TERM', line.tokenize().first() } Thread.sleep(5000) } Again, this shows you how poor the quality of this code is. It introduces a subtle bug, it repeats an if condition unnecessarily, and the comments are wrong "Only kill if needed" On Sun, Jun 25, 2017 at 4:22 PM, Taher Alkhateeb <[hidden email] > wrote: > I'm completing my answer for the record to show exactly where your code is > faulty and wrong just like in the other previous cases. > > 1- you have a copy-paste pattern in your code > 2- you are calling the kill command twice immediately one after another > without checking whether SIGTERM killed the process properly. The whole > purpose of doing SIGTERM is to allow cleaning resources before enforcing > SIGKILL. You're not giving the JVM an opportunity to call the shutdown hook > because you're looking up the same output from the same "ps ax" command for > both kill commands. > > So your code is working almost exactly as if typing: > if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) { > exec { commandLine 'kill', '-TERM', line.tokenize().first() } > exec { commandLine 'kill', '-KILL', line.tokenize().first() } > } > > Now what's happening is that SIGKILL is executing before SIGTERM > completes, so the build is successful but OFBiz is killed with SIGKILL not > SIGTERM. If SIGTERM finishes very quickly (does not happen with me) then > the build system would crash because the exit value of SIGKILL would be 1, > not 0 (process not found). So either way it is a problem (either killing > with SIGKILL or crashing the build system) > > Convinced? I hope you can revert now. > > On Sun, Jun 25, 2017 at 2:20 PM, Jacques Le Roux < > [hidden email]> wrote: > >> But what's wrong? It's not clear to me, do you see it Michael? >> >> Not clearly answering this question just adds more work on my side, I >> still don't see why I should revert. >> >> Jacques >> >> >> >> Le 25/06/2017 à 13:11, Michael Brohl a écrit : >> >>> I's like to propose the following process to get this worked out without >>> endless ping-pong here on the mailing lists: >>> >>> 1. revert the change as requested >>> >>> 2. provide a Jira with patch and review/discuss it there (what's the >>> problem, how should it be solved etc.) >>> >>> 3. come to a conclusion and a thumbsup by other committers >>> >>> 4. commit the solution. >>> >>> Thanks, >>> >>> Michael >>> >>> >>> Am 25.06.17 um 12:36 schrieb Taher Alkhateeb: >>> >>>> OK, I take this question as your refusal to revert and cooperate. I'm >>>> done >>>> spending time here and I leave it up to the community to see if they >>>> want >>>> incorrect code in the code base. >>>> >>>> On Sun, Jun 25, 2017 at 1:34 PM, Jacques Le Roux < >>>> [hidden email]> wrote: >>>> >>>> Mmm but anyway, I remember now how I thought that. >>>>> >>>>> In the 2nd "if", if the process was terminated, kill will just say >>>>> that it >>>>> can't find the process and exec will return. Else the process will be >>>>> killed. >>>>> >>>>> What's wrong then? >>>>> >>>>> Jacques >>>>> >>>>> >>>>> >>>>> Le 25/06/2017 à 12:28, Jacques Le Roux a écrit : >>>>> >>>>> Ah wait, I see line still contains "start" so will be executed twice >>>>>> anyway. OK I'll improve that. >>>>>> >>>>>> Jacques >>>>>> >>>>>> >>>>>> Le 25/06/2017 à 12:18, Jacques Le Roux a écrit : >>>>>> >>>>>> I don't revert without explanations on why I should revert. Sorry, >>>>>>> but I >>>>>>> don't find your explanations clear. >>>>>>> >>>>>>> My explanation, tell my what's wrong: >>>>>>> >>>>>>> The 2 "if" blocks are executed sequentially for each line containing >>>>>>> "a >>>>>>> start"and ignore other lines. I did not change the previous logic, >>>>>>> just >>>>>>> added a new if. >>>>>>> >>>>>>> Gradle exec spawns a new process and waits till it ends (this point >>>>>>> is >>>>>>> important). >>>>>>> >>>>>>> If the line contains "a start" the 1st "if" try to terminate it. >>>>>>> >>>>>>> If it worked the 2nd "if" does not execute. This is better than >>>>>>> before >>>>>>> because it allows the "'start" process "a chance to clean up after >>>>>>> itself" >>>>>>> (cf unix.stackexchange.com below) >>>>>>> >>>>>>> If the "start" process is not terminated then it's killed by the 2nd >>>>>>> "if", like it was done before. >>>>>>> >>>>>>> As I said it cleanly worked with 2 OFBiz instances running in the >>>>>>> background. >>>>>>> >>>>>>> Now tell me what's wrong? >>>>>>> >>>>>>> Thanks >>>>>>> >>>>>>> Jacques >>>>>>> >>>>>>> >>>>>>> Le 25/06/2017 à 11:22, Taher Alkhateeb a écrit : >>>>>>> >>>>>>> As usual, you refuse to revert because you don't understand the code >>>>>>>> and I >>>>>>>> pay the price of spending my time explaining. I hope you will >>>>>>>> reconsider >>>>>>>> this time consuming and non-cooperative behavior. >>>>>>>> >>>>>>>> The quick version: >>>>>>>> - copy and paste antipattern >>>>>>>> - incorrect conditional checking leading to both blocks getting >>>>>>>> executed or >>>>>>>> both blocks not executing >>>>>>>> >>>>>>>> Your belief that Gradle fails because java does not expect to be >>>>>>>> killed >>>>>>>> is >>>>>>>> amazing! It means you do not understand what this code is doing and >>>>>>>> what is >>>>>>>> causing the failure. >>>>>>>> >>>>>>>> >>>>>>>> On Jun 25, 2017 10:42 AM, "Jacques Le Roux" < >>>>>>>> [hidden email]> >>>>>>>> wrote: >>>>>>>> >>>>>>>> What makes you think it's wrong? I tested it locally using 2 >>>>>>>> background >>>>>>>> instances and it cleaned worked. >>>>>>>> >>>>>>>> I also tried with one standard instance (not in background). It >>>>>>>> works, >>>>>>>> and >>>>>>>> you get this message >>>>>>>> >>>>>>>> :ofbiz FAILED >>>>>>>> FAILURE: Build failed with an exception. >>>>>>>> * What went wrong: >>>>>>>> Execution failed for task ':ofbiz'. >>>>>>>> >>>>>>>> Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished >>>>>>>>> with >>>>>>>>> >>>>>>>>> non-zero exit value 137 >>>>>>>> >>>>>>>> Which I believe is OK because Java does not expect to be killed! >>>>>>>> >>>>>>>> Jacques >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit : >>>>>>>> >>>>>>>> This commit is wrong and bad on multiple levels. Please revert >>>>>>>> >>>>>>>>> On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: >>>>>>>>> >>>>>>>>> Author: jleroux >>>>>>>>> >>>>>>>>> Date: Sat Jun 24 07:56:45 2017 >>>>>>>>>> New Revision: 1799736 >>>>>>>>>> >>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1799736&view=rev >>>>>>>>>> Log: >>>>>>>>>> No functional change >>>>>>>>>> >>>>>>>>>> Improves terminateOfbiz byt using TERM before KILL >>>>>>>>>> https://fr.wikipedia.org/wiki/Kill_(Unix) >>>>>>>>>> https://unix.stackexchange.com/questions/8916/when- >>>>>>>>>> should-i-not-kill-9-a-process >>>>>>>>>> >>>>>>>>>> Modified: >>>>>>>>>> ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>>>>> >>>>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>>>>>>> build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff >>>>>>>>>> ============================================================ >>>>>>>>>> ================== >>>>>>>>>> --- ofbiz/ofbiz-framework/trunk/build.gradle (original) >>>>>>>>>> +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 07:56:45 >>>>>>>>>> 2017 >>>>>>>>>> @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, >>>>>>>>>> standardOutput = processOutput >>>>>>>>>> } >>>>>>>>>> processOutput.toString().split(System.lineSeparator()).each >>>>>>>>>> { line -> >>>>>>>>>> + // Try to terminate cleanly >>>>>>>>>> if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>>>>>> tart\.Start.*/) >>>>>>>>>> { >>>>>>>>>> - exec { commandLine 'kill', '-9', >>>>>>>>>> line.tokenize().first() } >>>>>>>>>> + exec { commandLine 'kill', '-TERM', >>>>>>>>>> line.tokenize().first() } >>>>>>>>>> + } >>>>>>>>>> + // Only kill if needed >>>>>>>>>> + if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>>>>>> tart\.Start.*/) >>>>>>>>>> { >>>>>>>>>> + exec { commandLine 'kill', '-KILL', >>>>>>>>>> line.tokenize().first() } >>>>>>>>>> } >>>>>>>>>> } >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>> >>>>>> >>> >>> >> > |
Re: svn commit: r1799736 - /ofbiz/ofbiz-framework/trunk/b
|
Administrator
|
Thanks Taher,
I indeed missed that kill is not an atomic operation. I reverted at r1799852. Before reading your last message I wanted to propose to set a delay between the 2 operations. So SIGTERM fails. I'd really like to send SIGTERM before killing the process and only kill it if it's necessary (ie if SIGTERM did not terminate the process) So we need to catch the SIGTERM exec and send SIGKILL if it fails, right? Jacques Le 25/06/2017 à 16:05, Taher Alkhateeb a écrit : > Oh and to prove the second scenario, just add a sleep for a few seconds > after the SIGTERM kill command and observe how the system will crash: > > if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) { > exec { commandLine 'kill', '-TERM', line.tokenize().first() } > Thread.sleep(5000) > } > > Again, this shows you how poor the quality of this code is. It introduces a > subtle bug, it repeats an if condition unnecessarily, and the comments are > wrong "Only kill if needed" > > On Sun, Jun 25, 2017 at 4:22 PM, Taher Alkhateeb <[hidden email] >> wrote: >> I'm completing my answer for the record to show exactly where your code is >> faulty and wrong just like in the other previous cases. >> >> 1- you have a copy-paste pattern in your code >> 2- you are calling the kill command twice immediately one after another >> without checking whether SIGTERM killed the process properly. The whole >> purpose of doing SIGTERM is to allow cleaning resources before enforcing >> SIGKILL. You're not giving the JVM an opportunity to call the shutdown hook >> because you're looking up the same output from the same "ps ax" command for >> both kill commands. >> >> So your code is working almost exactly as if typing: >> if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) { >> exec { commandLine 'kill', '-TERM', line.tokenize().first() } >> exec { commandLine 'kill', '-KILL', line.tokenize().first() } >> } >> >> Now what's happening is that SIGKILL is executing before SIGTERM >> completes, so the build is successful but OFBiz is killed with SIGKILL not >> SIGTERM. If SIGTERM finishes very quickly (does not happen with me) then >> the build system would crash because the exit value of SIGKILL would be 1, >> not 0 (process not found). So either way it is a problem (either killing >> with SIGKILL or crashing the build system) >> >> Convinced? I hope you can revert now. >> >> On Sun, Jun 25, 2017 at 2:20 PM, Jacques Le Roux < >> [hidden email]> wrote: >> >>> But what's wrong? It's not clear to me, do you see it Michael? >>> >>> Not clearly answering this question just adds more work on my side, I >>> still don't see why I should revert. >>> >>> Jacques >>> >>> >>> >>> Le 25/06/2017 à 13:11, Michael Brohl a écrit : >>> >>>> I's like to propose the following process to get this worked out without >>>> endless ping-pong here on the mailing lists: >>>> >>>> 1. revert the change as requested >>>> >>>> 2. provide a Jira with patch and review/discuss it there (what's the >>>> problem, how should it be solved etc.) >>>> >>>> 3. come to a conclusion and a thumbsup by other committers >>>> >>>> 4. commit the solution. >>>> >>>> Thanks, >>>> >>>> Michael >>>> >>>> >>>> Am 25.06.17 um 12:36 schrieb Taher Alkhateeb: >>>> >>>>> OK, I take this question as your refusal to revert and cooperate. I'm >>>>> done >>>>> spending time here and I leave it up to the community to see if they >>>>> want >>>>> incorrect code in the code base. >>>>> >>>>> On Sun, Jun 25, 2017 at 1:34 PM, Jacques Le Roux < >>>>> [hidden email]> wrote: >>>>> >>>>> Mmm but anyway, I remember now how I thought that. >>>>>> In the 2nd "if", if the process was terminated, kill will just say >>>>>> that it >>>>>> can't find the process and exec will return. Else the process will be >>>>>> killed. >>>>>> >>>>>> What's wrong then? >>>>>> >>>>>> Jacques >>>>>> >>>>>> >>>>>> >>>>>> Le 25/06/2017 à 12:28, Jacques Le Roux a écrit : >>>>>> >>>>>> Ah wait, I see line still contains "start" so will be executed twice >>>>>>> anyway. OK I'll improve that. >>>>>>> >>>>>>> Jacques >>>>>>> >>>>>>> >>>>>>> Le 25/06/2017 à 12:18, Jacques Le Roux a écrit : >>>>>>> >>>>>>> I don't revert without explanations on why I should revert. Sorry, >>>>>>>> but I >>>>>>>> don't find your explanations clear. >>>>>>>> >>>>>>>> My explanation, tell my what's wrong: >>>>>>>> >>>>>>>> The 2 "if" blocks are executed sequentially for each line containing >>>>>>>> "a >>>>>>>> start"and ignore other lines. I did not change the previous logic, >>>>>>>> just >>>>>>>> added a new if. >>>>>>>> >>>>>>>> Gradle exec spawns a new process and waits till it ends (this point >>>>>>>> is >>>>>>>> important). >>>>>>>> >>>>>>>> If the line contains "a start" the 1st "if" try to terminate it. >>>>>>>> >>>>>>>> If it worked the 2nd "if" does not execute. This is better than >>>>>>>> before >>>>>>>> because it allows the "'start" process "a chance to clean up after >>>>>>>> itself" >>>>>>>> (cf unix.stackexchange.com below) >>>>>>>> >>>>>>>> If the "start" process is not terminated then it's killed by the 2nd >>>>>>>> "if", like it was done before. >>>>>>>> >>>>>>>> As I said it cleanly worked with 2 OFBiz instances running in the >>>>>>>> background. >>>>>>>> >>>>>>>> Now tell me what's wrong? >>>>>>>> >>>>>>>> Thanks >>>>>>>> >>>>>>>> Jacques >>>>>>>> >>>>>>>> >>>>>>>> Le 25/06/2017 à 11:22, Taher Alkhateeb a écrit : >>>>>>>> >>>>>>>> As usual, you refuse to revert because you don't understand the code >>>>>>>>> and I >>>>>>>>> pay the price of spending my time explaining. I hope you will >>>>>>>>> reconsider >>>>>>>>> this time consuming and non-cooperative behavior. >>>>>>>>> >>>>>>>>> The quick version: >>>>>>>>> - copy and paste antipattern >>>>>>>>> - incorrect conditional checking leading to both blocks getting >>>>>>>>> executed or >>>>>>>>> both blocks not executing >>>>>>>>> >>>>>>>>> Your belief that Gradle fails because java does not expect to be >>>>>>>>> killed >>>>>>>>> is >>>>>>>>> amazing! It means you do not understand what this code is doing and >>>>>>>>> what is >>>>>>>>> causing the failure. >>>>>>>>> >>>>>>>>> >>>>>>>>> On Jun 25, 2017 10:42 AM, "Jacques Le Roux" < >>>>>>>>> [hidden email]> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>> What makes you think it's wrong? I tested it locally using 2 >>>>>>>>> background >>>>>>>>> instances and it cleaned worked. >>>>>>>>> >>>>>>>>> I also tried with one standard instance (not in background). It >>>>>>>>> works, >>>>>>>>> and >>>>>>>>> you get this message >>>>>>>>> >>>>>>>>> :ofbiz FAILED >>>>>>>>> FAILURE: Build failed with an exception. >>>>>>>>> * What went wrong: >>>>>>>>> Execution failed for task ':ofbiz'. >>>>>>>>> >>>>>>>>> Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished >>>>>>>>>> with >>>>>>>>>> >>>>>>>>>> non-zero exit value 137 >>>>>>>>> Which I believe is OK because Java does not expect to be killed! >>>>>>>>> >>>>>>>>> Jacques >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit : >>>>>>>>> >>>>>>>>> This commit is wrong and bad on multiple levels. Please revert >>>>>>>>> >>>>>>>>>> On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: >>>>>>>>>> >>>>>>>>>> Author: jleroux >>>>>>>>>> >>>>>>>>>> Date: Sat Jun 24 07:56:45 2017 >>>>>>>>>>> New Revision: 1799736 >>>>>>>>>>> >>>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1799736&view=rev >>>>>>>>>>> Log: >>>>>>>>>>> No functional change >>>>>>>>>>> >>>>>>>>>>> Improves terminateOfbiz byt using TERM before KILL >>>>>>>>>>> https://fr.wikipedia.org/wiki/Kill_(Unix) >>>>>>>>>>> https://unix.stackexchange.com/questions/8916/when- >>>>>>>>>>> should-i-not-kill-9-a-process >>>>>>>>>>> >>>>>>>>>>> Modified: >>>>>>>>>>> ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>>>>>> >>>>>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>>>>>>>> build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff >>>>>>>>>>> ============================================================ >>>>>>>>>>> ================== >>>>>>>>>>> --- ofbiz/ofbiz-framework/trunk/build.gradle (original) >>>>>>>>>>> +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 07:56:45 >>>>>>>>>>> 2017 >>>>>>>>>>> @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, >>>>>>>>>>> standardOutput = processOutput >>>>>>>>>>> } >>>>>>>>>>> processOutput.toString().split(System.lineSeparator()).each >>>>>>>>>>> { line -> >>>>>>>>>>> + // Try to terminate cleanly >>>>>>>>>>> if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>>>>>>> tart\.Start.*/) >>>>>>>>>>> { >>>>>>>>>>> - exec { commandLine 'kill', '-9', >>>>>>>>>>> line.tokenize().first() } >>>>>>>>>>> + exec { commandLine 'kill', '-TERM', >>>>>>>>>>> line.tokenize().first() } >>>>>>>>>>> + } >>>>>>>>>>> + // Only kill if needed >>>>>>>>>>> + if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>>>>>>> tart\.Start.*/) >>>>>>>>>>> { >>>>>>>>>>> + exec { commandLine 'kill', '-KILL', >>>>>>>>>>> line.tokenize().first() } >>>>>>>>>>> } >>>>>>>>>>> } >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>> |
|
|
I do not think we should send SIGTERM in the first place. The proper
termination of OFBiz is with the --shutdown command which cleanly stops everything and calls the shutdown hook. The gradle task "terminateOfbiz" should only be used if the shutdown command fails (last resort) as cleary mentioned in the docs. On Jun 26, 2017 9:19 AM, "Jacques Le Roux" <[hidden email]> wrote: Thanks Taher, I indeed missed that kill is not an atomic operation. I reverted at r1799852. Before reading your last message I wanted to propose to set a delay between the 2 operations. So SIGTERM fails. I'd really like to send SIGTERM before killing the process and only kill it if it's necessary (ie if SIGTERM did not terminate the process) So we need to catch the SIGTERM exec and send SIGKILL if it fails, right? Jacques Le 25/06/2017 à 16:05, Taher Alkhateeb a écrit : > Oh and to prove the second scenario, just add a sleep for a few seconds > after the SIGTERM kill command and observe how the system will crash: > > if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) { > exec { commandLine 'kill', '-TERM', line.tokenize().first() } > Thread.sleep(5000) > } > > Again, this shows you how poor the quality of this code is. It introduces a > subtle bug, it repeats an if condition unnecessarily, and the comments are > wrong "Only kill if needed" > > On Sun, Jun 25, 2017 at 4:22 PM, Taher Alkhateeb < > [hidden email] > >> wrote: >> I'm completing my answer for the record to show exactly where your code is >> faulty and wrong just like in the other previous cases. >> >> 1- you have a copy-paste pattern in your code >> 2- you are calling the kill command twice immediately one after another >> without checking whether SIGTERM killed the process properly. The whole >> purpose of doing SIGTERM is to allow cleaning resources before enforcing >> SIGKILL. You're not giving the JVM an opportunity to call the shutdown >> hook >> because you're looking up the same output from the same "ps ax" command >> for >> both kill commands. >> >> So your code is working almost exactly as if typing: >> if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) { >> exec { commandLine 'kill', '-TERM', line.tokenize().first() } >> exec { commandLine 'kill', '-KILL', line.tokenize().first() } >> } >> >> Now what's happening is that SIGKILL is executing before SIGTERM >> completes, so the build is successful but OFBiz is killed with SIGKILL not >> SIGTERM. If SIGTERM finishes very quickly (does not happen with me) then >> the build system would crash because the exit value of SIGKILL would be 1, >> not 0 (process not found). So either way it is a problem (either killing >> with SIGKILL or crashing the build system) >> >> Convinced? I hope you can revert now. >> >> On Sun, Jun 25, 2017 at 2:20 PM, Jacques Le Roux < >> [hidden email]> wrote: >> >> But what's wrong? It's not clear to me, do you see it Michael? >>> >>> Not clearly answering this question just adds more work on my side, I >>> still don't see why I should revert. >>> >>> Jacques >>> >>> >>> >>> Le 25/06/2017 à 13:11, Michael Brohl a écrit : >>> >>> I's like to propose the following process to get this worked out without >>>> endless ping-pong here on the mailing lists: >>>> >>>> 1. revert the change as requested >>>> >>>> 2. provide a Jira with patch and review/discuss it there (what's the >>>> problem, how should it be solved etc.) >>>> >>>> 3. come to a conclusion and a thumbsup by other committers >>>> >>>> 4. commit the solution. >>>> >>>> Thanks, >>>> >>>> Michael >>>> >>>> >>>> Am 25.06.17 um 12:36 schrieb Taher Alkhateeb: >>>> >>>> OK, I take this question as your refusal to revert and cooperate. I'm >>>>> done >>>>> spending time here and I leave it up to the community to see if they >>>>> want >>>>> incorrect code in the code base. >>>>> >>>>> On Sun, Jun 25, 2017 at 1:34 PM, Jacques Le Roux < >>>>> [hidden email]> wrote: >>>>> >>>>> Mmm but anyway, I remember now how I thought that. >>>>> >>>>>> In the 2nd "if", if the process was terminated, kill will just say >>>>>> that it >>>>>> can't find the process and exec will return. Else the process will be >>>>>> killed. >>>>>> >>>>>> What's wrong then? >>>>>> >>>>>> Jacques >>>>>> >>>>>> >>>>>> >>>>>> Le 25/06/2017 à 12:28, Jacques Le Roux a écrit : >>>>>> >>>>>> Ah wait, I see line still contains "start" so will be executed twice >>>>>> >>>>>>> anyway. OK I'll improve that. >>>>>>> >>>>>>> Jacques >>>>>>> >>>>>>> >>>>>>> Le 25/06/2017 à 12:18, Jacques Le Roux a écrit : >>>>>>> >>>>>>> I don't revert without explanations on why I should revert. Sorry, >>>>>>> >>>>>>>> but I >>>>>>>> don't find your explanations clear. >>>>>>>> >>>>>>>> My explanation, tell my what's wrong: >>>>>>>> >>>>>>>> The 2 "if" blocks are executed sequentially for each line containing >>>>>>>> "a >>>>>>>> start"and ignore other lines. I did not change the previous logic, >>>>>>>> just >>>>>>>> added a new if. >>>>>>>> >>>>>>>> Gradle exec spawns a new process and waits till it ends (this point >>>>>>>> is >>>>>>>> important). >>>>>>>> >>>>>>>> If the line contains "a start" the 1st "if" try to terminate it. >>>>>>>> >>>>>>>> If it worked the 2nd "if" does not execute. This is better than >>>>>>>> before >>>>>>>> because it allows the "'start" process "a chance to clean up after >>>>>>>> itself" >>>>>>>> (cf unix.stackexchange.com below) >>>>>>>> >>>>>>>> If the "start" process is not terminated then it's killed by the 2nd >>>>>>>> "if", like it was done before. >>>>>>>> >>>>>>>> As I said it cleanly worked with 2 OFBiz instances running in the >>>>>>>> background. >>>>>>>> >>>>>>>> Now tell me what's wrong? >>>>>>>> >>>>>>>> Thanks >>>>>>>> >>>>>>>> Jacques >>>>>>>> >>>>>>>> >>>>>>>> Le 25/06/2017 à 11:22, Taher Alkhateeb a écrit : >>>>>>>> >>>>>>>> As usual, you refuse to revert because you don't understand the code >>>>>>>> >>>>>>>>> and I >>>>>>>>> pay the price of spending my time explaining. I hope you will >>>>>>>>> reconsider >>>>>>>>> this time consuming and non-cooperative behavior. >>>>>>>>> >>>>>>>>> The quick version: >>>>>>>>> - copy and paste antipattern >>>>>>>>> - incorrect conditional checking leading to both blocks getting >>>>>>>>> executed or >>>>>>>>> both blocks not executing >>>>>>>>> >>>>>>>>> Your belief that Gradle fails because java does not expect to be >>>>>>>>> killed >>>>>>>>> is >>>>>>>>> amazing! It means you do not understand what this code is doing and >>>>>>>>> what is >>>>>>>>> causing the failure. >>>>>>>>> >>>>>>>>> >>>>>>>>> On Jun 25, 2017 10:42 AM, "Jacques Le Roux" < >>>>>>>>> [hidden email]> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>> What makes you think it's wrong? I tested it locally using 2 >>>>>>>>> background >>>>>>>>> instances and it cleaned worked. >>>>>>>>> >>>>>>>>> I also tried with one standard instance (not in background). It >>>>>>>>> works, >>>>>>>>> and >>>>>>>>> you get this message >>>>>>>>> >>>>>>>>> :ofbiz FAILED >>>>>>>>> FAILURE: Build failed with an exception. >>>>>>>>> * What went wrong: >>>>>>>>> Execution failed for task ':ofbiz'. >>>>>>>>> >>>>>>>>> Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished >>>>>>>>> >>>>>>>>>> with >>>>>>>>>> >>>>>>>>>> non-zero exit value 137 >>>>>>>>>> >>>>>>>>> Which I believe is OK because Java does not expect to be killed! >>>>>>>>> >>>>>>>>> Jacques >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit : >>>>>>>>> >>>>>>>>> This commit is wrong and bad on multiple levels. Please revert >>>>>>>>> >>>>>>>>> On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: >>>>>>>>>> >>>>>>>>>> Author: jleroux >>>>>>>>>> >>>>>>>>>> Date: Sat Jun 24 07:56:45 2017 >>>>>>>>>> >>>>>>>>>>> New Revision: 1799736 >>>>>>>>>>> >>>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1799736&view=rev >>>>>>>>>>> Log: >>>>>>>>>>> No functional change >>>>>>>>>>> >>>>>>>>>>> Improves terminateOfbiz byt using TERM before KILL >>>>>>>>>>> https://fr.wikipedia.org/wiki/Kill_(Unix) >>>>>>>>>>> https://unix.stackexchange.com/questions/8916/when- >>>>>>>>>>> should-i-not-kill-9-a-process >>>>>>>>>>> >>>>>>>>>>> Modified: >>>>>>>>>>> ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>>>>>> >>>>>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>>>>>>>> build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff >>>>>>>>>>> ============================================================ >>>>>>>>>>> ================== >>>>>>>>>>> --- ofbiz/ofbiz-framework/trunk/build.gradle (original) >>>>>>>>>>> +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 07:56:45 >>>>>>>>>>> 2017 >>>>>>>>>>> @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, >>>>>>>>>>> standardOutput = processOutput >>>>>>>>>>> } >>>>>>>>>>> processOutput.toString().split(System.lineSeparator()).each >>>>>>>>>>> { line -> >>>>>>>>>>> + // Try to terminate cleanly >>>>>>>>>>> if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>>>>>>> tart\.Start.*/) >>>>>>>>>>> { >>>>>>>>>>> - exec { commandLine 'kill', '-9', >>>>>>>>>>> line.tokenize().first() } >>>>>>>>>>> + exec { commandLine 'kill', '-TERM', >>>>>>>>>>> line.tokenize().first() } >>>>>>>>>>> + } >>>>>>>>>>> + // Only kill if needed >>>>>>>>>>> + if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>>>>>>> tart\.Start.*/) >>>>>>>>>>> { >>>>>>>>>>> + exec { commandLine 'kill', '-KILL', >>>>>>>>>>> line.tokenize().first() } >>>>>>>>>>> } >>>>>>>>>>> } >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>> |
Re: svn commit: r1799736 - /ofbiz/ofbiz-framework/trunk/b
|
Administrator
|
OK, the shutdown command is somehow our SIGTERM, makes sense
Jacques Le 26/06/2017 à 08:45, Taher Alkhateeb a écrit : > I do not think we should send SIGTERM in the first place. The proper > termination of OFBiz is with the --shutdown command which cleanly stops > everything and calls the shutdown hook. The gradle task "terminateOfbiz" > should only be used if the shutdown command fails (last resort) as cleary > mentioned in the docs. > > On Jun 26, 2017 9:19 AM, "Jacques Le Roux" <[hidden email]> > wrote: > > Thanks Taher, > > I indeed missed that kill is not an atomic operation. I reverted at > r1799852. > > Before reading your last message I wanted to propose to set a delay between > the 2 operations. > > So SIGTERM fails. I'd really like to send SIGTERM before killing the > process and only kill it if it's necessary (ie if SIGTERM did not terminate > the process) > > So we need to catch the SIGTERM exec and send SIGKILL if it fails, right? > > Jacques > > > > Le 25/06/2017 à 16:05, Taher Alkhateeb a écrit : > >> Oh and to prove the second scenario, just add a sleep for a few seconds >> after the SIGTERM kill command and observe how the system will crash: >> >> if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) { >> exec { commandLine 'kill', '-TERM', line.tokenize().first() } >> Thread.sleep(5000) >> } >> >> Again, this shows you how poor the quality of this code is. It introduces a >> subtle bug, it repeats an if condition unnecessarily, and the comments are >> wrong "Only kill if needed" >> >> On Sun, Jun 25, 2017 at 4:22 PM, Taher Alkhateeb < >> [hidden email] >> >>> wrote: >>> I'm completing my answer for the record to show exactly where your code is >>> faulty and wrong just like in the other previous cases. >>> >>> 1- you have a copy-paste pattern in your code >>> 2- you are calling the kill command twice immediately one after another >>> without checking whether SIGTERM killed the process properly. The whole >>> purpose of doing SIGTERM is to allow cleaning resources before enforcing >>> SIGKILL. You're not giving the JVM an opportunity to call the shutdown >>> hook >>> because you're looking up the same output from the same "ps ax" command >>> for >>> both kill commands. >>> >>> So your code is working almost exactly as if typing: >>> if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) { >>> exec { commandLine 'kill', '-TERM', line.tokenize().first() } >>> exec { commandLine 'kill', '-KILL', line.tokenize().first() } >>> } >>> >>> Now what's happening is that SIGKILL is executing before SIGTERM >>> completes, so the build is successful but OFBiz is killed with SIGKILL not >>> SIGTERM. If SIGTERM finishes very quickly (does not happen with me) then >>> the build system would crash because the exit value of SIGKILL would be 1, >>> not 0 (process not found). So either way it is a problem (either killing >>> with SIGKILL or crashing the build system) >>> >>> Convinced? I hope you can revert now. >>> >>> On Sun, Jun 25, 2017 at 2:20 PM, Jacques Le Roux < >>> [hidden email]> wrote: >>> >>> But what's wrong? It's not clear to me, do you see it Michael? >>>> Not clearly answering this question just adds more work on my side, I >>>> still don't see why I should revert. >>>> >>>> Jacques >>>> >>>> >>>> >>>> Le 25/06/2017 à 13:11, Michael Brohl a écrit : >>>> >>>> I's like to propose the following process to get this worked out without >>>>> endless ping-pong here on the mailing lists: >>>>> >>>>> 1. revert the change as requested >>>>> >>>>> 2. provide a Jira with patch and review/discuss it there (what's the >>>>> problem, how should it be solved etc.) >>>>> >>>>> 3. come to a conclusion and a thumbsup by other committers >>>>> >>>>> 4. commit the solution. >>>>> >>>>> Thanks, >>>>> >>>>> Michael >>>>> >>>>> >>>>> Am 25.06.17 um 12:36 schrieb Taher Alkhateeb: >>>>> >>>>> OK, I take this question as your refusal to revert and cooperate. I'm >>>>>> done >>>>>> spending time here and I leave it up to the community to see if they >>>>>> want >>>>>> incorrect code in the code base. >>>>>> >>>>>> On Sun, Jun 25, 2017 at 1:34 PM, Jacques Le Roux < >>>>>> [hidden email]> wrote: >>>>>> >>>>>> Mmm but anyway, I remember now how I thought that. >>>>>> >>>>>>> In the 2nd "if", if the process was terminated, kill will just say >>>>>>> that it >>>>>>> can't find the process and exec will return. Else the process will be >>>>>>> killed. >>>>>>> >>>>>>> What's wrong then? >>>>>>> >>>>>>> Jacques >>>>>>> >>>>>>> >>>>>>> >>>>>>> Le 25/06/2017 à 12:28, Jacques Le Roux a écrit : >>>>>>> >>>>>>> Ah wait, I see line still contains "start" so will be executed twice >>>>>>> >>>>>>>> anyway. OK I'll improve that. >>>>>>>> >>>>>>>> Jacques >>>>>>>> >>>>>>>> >>>>>>>> Le 25/06/2017 à 12:18, Jacques Le Roux a écrit : >>>>>>>> >>>>>>>> I don't revert without explanations on why I should revert. Sorry, >>>>>>>> >>>>>>>>> but I >>>>>>>>> don't find your explanations clear. >>>>>>>>> >>>>>>>>> My explanation, tell my what's wrong: >>>>>>>>> >>>>>>>>> The 2 "if" blocks are executed sequentially for each line containing >>>>>>>>> "a >>>>>>>>> start"and ignore other lines. I did not change the previous logic, >>>>>>>>> just >>>>>>>>> added a new if. >>>>>>>>> >>>>>>>>> Gradle exec spawns a new process and waits till it ends (this point >>>>>>>>> is >>>>>>>>> important). >>>>>>>>> >>>>>>>>> If the line contains "a start" the 1st "if" try to terminate it. >>>>>>>>> >>>>>>>>> If it worked the 2nd "if" does not execute. This is better than >>>>>>>>> before >>>>>>>>> because it allows the "'start" process "a chance to clean up after >>>>>>>>> itself" >>>>>>>>> (cf unix.stackexchange.com below) >>>>>>>>> >>>>>>>>> If the "start" process is not terminated then it's killed by the 2nd >>>>>>>>> "if", like it was done before. >>>>>>>>> >>>>>>>>> As I said it cleanly worked with 2 OFBiz instances running in the >>>>>>>>> background. >>>>>>>>> >>>>>>>>> Now tell me what's wrong? >>>>>>>>> >>>>>>>>> Thanks >>>>>>>>> >>>>>>>>> Jacques >>>>>>>>> >>>>>>>>> >>>>>>>>> Le 25/06/2017 à 11:22, Taher Alkhateeb a écrit : >>>>>>>>> >>>>>>>>> As usual, you refuse to revert because you don't understand the code >>>>>>>>> >>>>>>>>>> and I >>>>>>>>>> pay the price of spending my time explaining. I hope you will >>>>>>>>>> reconsider >>>>>>>>>> this time consuming and non-cooperative behavior. >>>>>>>>>> >>>>>>>>>> The quick version: >>>>>>>>>> - copy and paste antipattern >>>>>>>>>> - incorrect conditional checking leading to both blocks getting >>>>>>>>>> executed or >>>>>>>>>> both blocks not executing >>>>>>>>>> >>>>>>>>>> Your belief that Gradle fails because java does not expect to be >>>>>>>>>> killed >>>>>>>>>> is >>>>>>>>>> amazing! It means you do not understand what this code is doing and >>>>>>>>>> what is >>>>>>>>>> causing the failure. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Jun 25, 2017 10:42 AM, "Jacques Le Roux" < >>>>>>>>>> [hidden email]> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> What makes you think it's wrong? I tested it locally using 2 >>>>>>>>>> background >>>>>>>>>> instances and it cleaned worked. >>>>>>>>>> >>>>>>>>>> I also tried with one standard instance (not in background). It >>>>>>>>>> works, >>>>>>>>>> and >>>>>>>>>> you get this message >>>>>>>>>> >>>>>>>>>> :ofbiz FAILED >>>>>>>>>> FAILURE: Build failed with an exception. >>>>>>>>>> * What went wrong: >>>>>>>>>> Execution failed for task ':ofbiz'. >>>>>>>>>> >>>>>>>>>> Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished >>>>>>>>>> >>>>>>>>>>> with >>>>>>>>>>> >>>>>>>>>>> non-zero exit value 137 >>>>>>>>>>> >>>>>>>>>> Which I believe is OK because Java does not expect to be killed! >>>>>>>>>> >>>>>>>>>> Jacques >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit : >>>>>>>>>> >>>>>>>>>> This commit is wrong and bad on multiple levels. Please revert >>>>>>>>>> >>>>>>>>>> On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: >>>>>>>>>>> Author: jleroux >>>>>>>>>>> >>>>>>>>>>> Date: Sat Jun 24 07:56:45 2017 >>>>>>>>>>> >>>>>>>>>>>> New Revision: 1799736 >>>>>>>>>>>> >>>>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1799736&view=rev >>>>>>>>>>>> Log: >>>>>>>>>>>> No functional change >>>>>>>>>>>> >>>>>>>>>>>> Improves terminateOfbiz byt using TERM before KILL >>>>>>>>>>>> https://fr.wikipedia.org/wiki/Kill_(Unix) >>>>>>>>>>>> https://unix.stackexchange.com/questions/8916/when- >>>>>>>>>>>> should-i-not-kill-9-a-process >>>>>>>>>>>> >>>>>>>>>>>> Modified: >>>>>>>>>>>> ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>>>>>>> >>>>>>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>>>>>>>>> build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff >>>>>>>>>>>> ============================================================ >>>>>>>>>>>> ================== >>>>>>>>>>>> --- ofbiz/ofbiz-framework/trunk/build.gradle (original) >>>>>>>>>>>> +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 07:56:45 >>>>>>>>>>>> 2017 >>>>>>>>>>>> @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, >>>>>>>>>>>> standardOutput = processOutput >>>>>>>>>>>> } >>>>>>>>>>>> processOutput.toString().split(System.lineSeparator()).each >>>>>>>>>>>> { line -> >>>>>>>>>>>> + // Try to terminate cleanly >>>>>>>>>>>> if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>>>>>>>> tart\.Start.*/) >>>>>>>>>>>> { >>>>>>>>>>>> - exec { commandLine 'kill', '-9', >>>>>>>>>>>> line.tokenize().first() } >>>>>>>>>>>> + exec { commandLine 'kill', '-TERM', >>>>>>>>>>>> line.tokenize().first() } >>>>>>>>>>>> + } >>>>>>>>>>>> + // Only kill if needed >>>>>>>>>>>> + if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>>>>>>>> tart\.Start.*/) >>>>>>>>>>>> { >>>>>>>>>>>> + exec { commandLine 'kill', '-KILL', >>>>>>>>>>>> line.tokenize().first() } >>>>>>>>>>>> } >>>>>>>>>>>> } >>>>>>>>>>>> } >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> |
A problem with terminateOfbiz Gradle task (was [Re: svn commit: r1799736 - /ofbiz/ofbiz-framework/trunk/b
|
Administrator
|
We still have a problem with terminateOfbiz: it's not portoffset aware.
So when you run it, it closes all OFBiz instances running on a machine (eg on VM demo) I guess we only need to improve terminateOfbiz by passing a portoffset parameter I'll create a Jira and provide a patch for review Jacques Le 26/06/2017 à 09:37, Jacques Le Roux a écrit : > OK, the shutdown command is somehow our SIGTERM, makes sense > > Jacques > > > Le 26/06/2017 à 08:45, Taher Alkhateeb a écrit : >> I do not think we should send SIGTERM in the first place. The proper >> termination of OFBiz is with the --shutdown command which cleanly stops >> everything and calls the shutdown hook. The gradle task "terminateOfbiz" >> should only be used if the shutdown command fails (last resort) as cleary >> mentioned in the docs. >> >> On Jun 26, 2017 9:19 AM, "Jacques Le Roux" <[hidden email]> >> wrote: >> >> Thanks Taher, >> >> I indeed missed that kill is not an atomic operation. I reverted at >> r1799852. >> >> Before reading your last message I wanted to propose to set a delay between >> the 2 operations. >> >> So SIGTERM fails. I'd really like to send SIGTERM before killing the >> process and only kill it if it's necessary (ie if SIGTERM did not terminate >> the process) >> >> So we need to catch the SIGTERM exec and send SIGKILL if it fails, right? >> >> Jacques >> >> >> >> Le 25/06/2017 à 16:05, Taher Alkhateeb a écrit : >> >>> Oh and to prove the second scenario, just add a sleep for a few seconds >>> after the SIGTERM kill command and observe how the system will crash: >>> >>> if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) { >>> exec { commandLine 'kill', '-TERM', line.tokenize().first() } >>> Thread.sleep(5000) >>> } >>> >>> Again, this shows you how poor the quality of this code is. It introduces a >>> subtle bug, it repeats an if condition unnecessarily, and the comments are >>> wrong "Only kill if needed" >>> >>> On Sun, Jun 25, 2017 at 4:22 PM, Taher Alkhateeb < >>> [hidden email] >>> >>>> wrote: >>>> I'm completing my answer for the record to show exactly where your code is >>>> faulty and wrong just like in the other previous cases. >>>> >>>> 1- you have a copy-paste pattern in your code >>>> 2- you are calling the kill command twice immediately one after another >>>> without checking whether SIGTERM killed the process properly. The whole >>>> purpose of doing SIGTERM is to allow cleaning resources before enforcing >>>> SIGKILL. You're not giving the JVM an opportunity to call the shutdown >>>> hook >>>> because you're looking up the same output from the same "ps ax" command >>>> for >>>> both kill commands. >>>> >>>> So your code is working almost exactly as if typing: >>>> if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) { >>>> exec { commandLine 'kill', '-TERM', line.tokenize().first() } >>>> exec { commandLine 'kill', '-KILL', line.tokenize().first() } >>>> } >>>> >>>> Now what's happening is that SIGKILL is executing before SIGTERM >>>> completes, so the build is successful but OFBiz is killed with SIGKILL not >>>> SIGTERM. If SIGTERM finishes very quickly (does not happen with me) then >>>> the build system would crash because the exit value of SIGKILL would be 1, >>>> not 0 (process not found). So either way it is a problem (either killing >>>> with SIGKILL or crashing the build system) >>>> >>>> Convinced? I hope you can revert now. >>>> >>>> On Sun, Jun 25, 2017 at 2:20 PM, Jacques Le Roux < >>>> [hidden email]> wrote: >>>> >>>> But what's wrong? It's not clear to me, do you see it Michael? >>>>> Not clearly answering this question just adds more work on my side, I >>>>> still don't see why I should revert. >>>>> >>>>> Jacques >>>>> >>>>> >>>>> >>>>> Le 25/06/2017 à 13:11, Michael Brohl a écrit : >>>>> >>>>> I's like to propose the following process to get this worked out without >>>>>> endless ping-pong here on the mailing lists: >>>>>> >>>>>> 1. revert the change as requested >>>>>> >>>>>> 2. provide a Jira with patch and review/discuss it there (what's the >>>>>> problem, how should it be solved etc.) >>>>>> >>>>>> 3. come to a conclusion and a thumbsup by other committers >>>>>> >>>>>> 4. commit the solution. >>>>>> >>>>>> Thanks, >>>>>> >>>>>> Michael >>>>>> >>>>>> >>>>>> Am 25.06.17 um 12:36 schrieb Taher Alkhateeb: >>>>>> >>>>>> OK, I take this question as your refusal to revert and cooperate. I'm >>>>>>> done >>>>>>> spending time here and I leave it up to the community to see if they >>>>>>> want >>>>>>> incorrect code in the code base. >>>>>>> >>>>>>> On Sun, Jun 25, 2017 at 1:34 PM, Jacques Le Roux < >>>>>>> [hidden email]> wrote: >>>>>>> >>>>>>> Mmm but anyway, I remember now how I thought that. >>>>>>> >>>>>>>> In the 2nd "if", if the process was terminated, kill will just say >>>>>>>> that it >>>>>>>> can't find the process and exec will return. Else the process will be >>>>>>>> killed. >>>>>>>> >>>>>>>> What's wrong then? >>>>>>>> >>>>>>>> Jacques >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Le 25/06/2017 à 12:28, Jacques Le Roux a écrit : >>>>>>>> >>>>>>>> Ah wait, I see line still contains "start" so will be executed twice >>>>>>>> >>>>>>>>> anyway. OK I'll improve that. >>>>>>>>> >>>>>>>>> Jacques >>>>>>>>> >>>>>>>>> >>>>>>>>> Le 25/06/2017 à 12:18, Jacques Le Roux a écrit : >>>>>>>>> >>>>>>>>> I don't revert without explanations on why I should revert. Sorry, >>>>>>>>> >>>>>>>>>> but I >>>>>>>>>> don't find your explanations clear. >>>>>>>>>> >>>>>>>>>> My explanation, tell my what's wrong: >>>>>>>>>> >>>>>>>>>> The 2 "if" blocks are executed sequentially for each line containing >>>>>>>>>> "a >>>>>>>>>> start"and ignore other lines. I did not change the previous logic, >>>>>>>>>> just >>>>>>>>>> added a new if. >>>>>>>>>> >>>>>>>>>> Gradle exec spawns a new process and waits till it ends (this point >>>>>>>>>> is >>>>>>>>>> important). >>>>>>>>>> >>>>>>>>>> If the line contains "a start" the 1st "if" try to terminate it. >>>>>>>>>> >>>>>>>>>> If it worked the 2nd "if" does not execute. This is better than >>>>>>>>>> before >>>>>>>>>> because it allows the "'start" process "a chance to clean up after >>>>>>>>>> itself" >>>>>>>>>> (cf unix.stackexchange.com below) >>>>>>>>>> >>>>>>>>>> If the "start" process is not terminated then it's killed by the 2nd >>>>>>>>>> "if", like it was done before. >>>>>>>>>> >>>>>>>>>> As I said it cleanly worked with 2 OFBiz instances running in the >>>>>>>>>> background. >>>>>>>>>> >>>>>>>>>> Now tell me what's wrong? >>>>>>>>>> >>>>>>>>>> Thanks >>>>>>>>>> >>>>>>>>>> Jacques >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Le 25/06/2017 à 11:22, Taher Alkhateeb a écrit : >>>>>>>>>> >>>>>>>>>> As usual, you refuse to revert because you don't understand the code >>>>>>>>>> >>>>>>>>>>> and I >>>>>>>>>>> pay the price of spending my time explaining. I hope you will >>>>>>>>>>> reconsider >>>>>>>>>>> this time consuming and non-cooperative behavior. >>>>>>>>>>> >>>>>>>>>>> The quick version: >>>>>>>>>>> - copy and paste antipattern >>>>>>>>>>> - incorrect conditional checking leading to both blocks getting >>>>>>>>>>> executed or >>>>>>>>>>> both blocks not executing >>>>>>>>>>> >>>>>>>>>>> Your belief that Gradle fails because java does not expect to be >>>>>>>>>>> killed >>>>>>>>>>> is >>>>>>>>>>> amazing! It means you do not understand what this code is doing and >>>>>>>>>>> what is >>>>>>>>>>> causing the failure. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Jun 25, 2017 10:42 AM, "Jacques Le Roux" < >>>>>>>>>>> [hidden email]> >>>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> What makes you think it's wrong? I tested it locally using 2 >>>>>>>>>>> background >>>>>>>>>>> instances and it cleaned worked. >>>>>>>>>>> >>>>>>>>>>> I also tried with one standard instance (not in background). It >>>>>>>>>>> works, >>>>>>>>>>> and >>>>>>>>>>> you get this message >>>>>>>>>>> >>>>>>>>>>> :ofbiz FAILED >>>>>>>>>>> FAILURE: Build failed with an exception. >>>>>>>>>>> * What went wrong: >>>>>>>>>>> Execution failed for task ':ofbiz'. >>>>>>>>>>> >>>>>>>>>>> Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished >>>>>>>>>>> >>>>>>>>>>>> with >>>>>>>>>>>> >>>>>>>>>>>> non-zero exit value 137 >>>>>>>>>>>> >>>>>>>>>>> Which I believe is OK because Java does not expect to be killed! >>>>>>>>>>> >>>>>>>>>>> Jacques >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit : >>>>>>>>>>> >>>>>>>>>>> This commit is wrong and bad on multiple levels. Please revert >>>>>>>>>>> >>>>>>>>>>> On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: >>>>>>>>>>>> Author: jleroux >>>>>>>>>>>> >>>>>>>>>>>> Date: Sat Jun 24 07:56:45 2017 >>>>>>>>>>>> >>>>>>>>>>>>> New Revision: 1799736 >>>>>>>>>>>>> >>>>>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1799736&view=rev >>>>>>>>>>>>> Log: >>>>>>>>>>>>> No functional change >>>>>>>>>>>>> >>>>>>>>>>>>> Improves terminateOfbiz byt using TERM before KILL >>>>>>>>>>>>> https://fr.wikipedia.org/wiki/Kill_(Unix) >>>>>>>>>>>>> https://unix.stackexchange.com/questions/8916/when- >>>>>>>>>>>>> should-i-not-kill-9-a-process >>>>>>>>>>>>> >>>>>>>>>>>>> Modified: >>>>>>>>>>>>> ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>>>>>>>> >>>>>>>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>>>>>>>>>> build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff >>>>>>>>>>>>> ============================================================ >>>>>>>>>>>>> ================== >>>>>>>>>>>>> --- ofbiz/ofbiz-framework/trunk/build.gradle (original) >>>>>>>>>>>>> +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 07:56:45 >>>>>>>>>>>>> 2017 >>>>>>>>>>>>> @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, >>>>>>>>>>>>> standardOutput = processOutput >>>>>>>>>>>>> } >>>>>>>>>>>>> processOutput.toString().split(System.lineSeparator()).each >>>>>>>>>>>>> { line -> >>>>>>>>>>>>> + // Try to terminate cleanly >>>>>>>>>>>>> if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>>>>>>>>> tart\.Start.*/) >>>>>>>>>>>>> { >>>>>>>>>>>>> - exec { commandLine 'kill', '-9', >>>>>>>>>>>>> line.tokenize().first() } >>>>>>>>>>>>> + exec { commandLine 'kill', '-TERM', >>>>>>>>>>>>> line.tokenize().first() } >>>>>>>>>>>>> + } >>>>>>>>>>>>> + // Only kill if needed >>>>>>>>>>>>> + if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>>>>>>>>> tart\.Start.*/) >>>>>>>>>>>>> { >>>>>>>>>>>>> + exec { commandLine 'kill', '-KILL', >>>>>>>>>>>>> line.tokenize().first() } >>>>>>>>>>>>> } >>>>>>>>>>>>> } >>>>>>>>>>>>> } >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> > > |
|
|
I think terminateOfbiz should NOT be portoffset-aware nor should it
kill only one instance. terminateOfbiz is a convenience task to kill all instances which is mostly used during development. That's the only purpose it has. I believe it should NOT be used in any automation suite or buildbot or anything like that. I have already spent a lot of time correcting your errors in that task [1]. Please let's try to avoid repeating this time consuming process. [1] https://s.apache.org/fvBB On Tue, Jun 27, 2017 at 3:39 PM, Jacques Le Roux <[hidden email]> wrote: > We still have a problem with terminateOfbiz: it's not portoffset aware. > > So when you run it, it closes all OFBiz instances running on a machine (eg > on VM demo) > > I guess we only need to improve terminateOfbiz by passing a portoffset > parameter > > I'll create a Jira and provide a patch for review > > Jacques > > > Le 26/06/2017 à 09:37, Jacques Le Roux a écrit : >> >> OK, the shutdown command is somehow our SIGTERM, makes sense >> >> Jacques >> >> >> Le 26/06/2017 à 08:45, Taher Alkhateeb a écrit : >>> >>> I do not think we should send SIGTERM in the first place. The proper >>> termination of OFBiz is with the --shutdown command which cleanly stops >>> everything and calls the shutdown hook. The gradle task "terminateOfbiz" >>> should only be used if the shutdown command fails (last resort) as cleary >>> mentioned in the docs. >>> >>> On Jun 26, 2017 9:19 AM, "Jacques Le Roux" <[hidden email]> >>> wrote: >>> >>> Thanks Taher, >>> >>> I indeed missed that kill is not an atomic operation. I reverted at >>> r1799852. >>> >>> Before reading your last message I wanted to propose to set a delay >>> between >>> the 2 operations. >>> >>> So SIGTERM fails. I'd really like to send SIGTERM before killing the >>> process and only kill it if it's necessary (ie if SIGTERM did not >>> terminate >>> the process) >>> >>> So we need to catch the SIGTERM exec and send SIGKILL if it fails, right? >>> >>> Jacques >>> >>> >>> >>> Le 25/06/2017 à 16:05, Taher Alkhateeb a écrit : >>> >>>> Oh and to prove the second scenario, just add a sleep for a few seconds >>>> after the SIGTERM kill command and observe how the system will crash: >>>> >>>> if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) { >>>> exec { commandLine 'kill', '-TERM', line.tokenize().first() } >>>> Thread.sleep(5000) >>>> } >>>> >>>> Again, this shows you how poor the quality of this code is. It >>>> introduces a >>>> subtle bug, it repeats an if condition unnecessarily, and the comments >>>> are >>>> wrong "Only kill if needed" >>>> >>>> On Sun, Jun 25, 2017 at 4:22 PM, Taher Alkhateeb < >>>> [hidden email] >>>> >>>>> wrote: >>>>> I'm completing my answer for the record to show exactly where your code >>>>> is >>>>> faulty and wrong just like in the other previous cases. >>>>> >>>>> 1- you have a copy-paste pattern in your code >>>>> 2- you are calling the kill command twice immediately one after another >>>>> without checking whether SIGTERM killed the process properly. The whole >>>>> purpose of doing SIGTERM is to allow cleaning resources before >>>>> enforcing >>>>> SIGKILL. You're not giving the JVM an opportunity to call the shutdown >>>>> hook >>>>> because you're looking up the same output from the same "ps ax" command >>>>> for >>>>> both kill commands. >>>>> >>>>> So your code is working almost exactly as if typing: >>>>> if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) { >>>>> exec { commandLine 'kill', '-TERM', line.tokenize().first() } >>>>> exec { commandLine 'kill', '-KILL', line.tokenize().first() } >>>>> } >>>>> >>>>> Now what's happening is that SIGKILL is executing before SIGTERM >>>>> completes, so the build is successful but OFBiz is killed with SIGKILL >>>>> not >>>>> SIGTERM. If SIGTERM finishes very quickly (does not happen with me) >>>>> then >>>>> the build system would crash because the exit value of SIGKILL would be >>>>> 1, >>>>> not 0 (process not found). So either way it is a problem (either >>>>> killing >>>>> with SIGKILL or crashing the build system) >>>>> >>>>> Convinced? I hope you can revert now. >>>>> >>>>> On Sun, Jun 25, 2017 at 2:20 PM, Jacques Le Roux < >>>>> [hidden email]> wrote: >>>>> >>>>> But what's wrong? It's not clear to me, do you see it Michael? >>>>>> >>>>>> Not clearly answering this question just adds more work on my side, I >>>>>> still don't see why I should revert. >>>>>> >>>>>> Jacques >>>>>> >>>>>> >>>>>> >>>>>> Le 25/06/2017 à 13:11, Michael Brohl a écrit : >>>>>> >>>>>> I's like to propose the following process to get this worked out >>>>>> without >>>>>>> >>>>>>> endless ping-pong here on the mailing lists: >>>>>>> >>>>>>> 1. revert the change as requested >>>>>>> >>>>>>> 2. provide a Jira with patch and review/discuss it there (what's the >>>>>>> problem, how should it be solved etc.) >>>>>>> >>>>>>> 3. come to a conclusion and a thumbsup by other committers >>>>>>> >>>>>>> 4. commit the solution. >>>>>>> >>>>>>> Thanks, >>>>>>> >>>>>>> Michael >>>>>>> >>>>>>> >>>>>>> Am 25.06.17 um 12:36 schrieb Taher Alkhateeb: >>>>>>> >>>>>>> OK, I take this question as your refusal to revert and cooperate. I'm >>>>>>>> >>>>>>>> done >>>>>>>> spending time here and I leave it up to the community to see if they >>>>>>>> want >>>>>>>> incorrect code in the code base. >>>>>>>> >>>>>>>> On Sun, Jun 25, 2017 at 1:34 PM, Jacques Le Roux < >>>>>>>> [hidden email]> wrote: >>>>>>>> >>>>>>>> Mmm but anyway, I remember now how I thought that. >>>>>>>> >>>>>>>>> In the 2nd "if", if the process was terminated, kill will just say >>>>>>>>> that it >>>>>>>>> can't find the process and exec will return. Else the process will >>>>>>>>> be >>>>>>>>> killed. >>>>>>>>> >>>>>>>>> What's wrong then? >>>>>>>>> >>>>>>>>> Jacques >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Le 25/06/2017 à 12:28, Jacques Le Roux a écrit : >>>>>>>>> >>>>>>>>> Ah wait, I see line still contains "start" so will be executed >>>>>>>>> twice >>>>>>>>> >>>>>>>>>> anyway. OK I'll improve that. >>>>>>>>>> >>>>>>>>>> Jacques >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Le 25/06/2017 à 12:18, Jacques Le Roux a écrit : >>>>>>>>>> >>>>>>>>>> I don't revert without explanations on why I should revert. Sorry, >>>>>>>>>> >>>>>>>>>>> but I >>>>>>>>>>> don't find your explanations clear. >>>>>>>>>>> >>>>>>>>>>> My explanation, tell my what's wrong: >>>>>>>>>>> >>>>>>>>>>> The 2 "if" blocks are executed sequentially for each line >>>>>>>>>>> containing >>>>>>>>>>> "a >>>>>>>>>>> start"and ignore other lines. I did not change the previous >>>>>>>>>>> logic, >>>>>>>>>>> just >>>>>>>>>>> added a new if. >>>>>>>>>>> >>>>>>>>>>> Gradle exec spawns a new process and waits till it ends (this >>>>>>>>>>> point >>>>>>>>>>> is >>>>>>>>>>> important). >>>>>>>>>>> >>>>>>>>>>> If the line contains "a start" the 1st "if" try to terminate it. >>>>>>>>>>> >>>>>>>>>>> If it worked the 2nd "if" does not execute. This is better than >>>>>>>>>>> before >>>>>>>>>>> because it allows the "'start" process "a chance to clean up >>>>>>>>>>> after >>>>>>>>>>> itself" >>>>>>>>>>> (cf unix.stackexchange.com below) >>>>>>>>>>> >>>>>>>>>>> If the "start" process is not terminated then it's killed by the >>>>>>>>>>> 2nd >>>>>>>>>>> "if", like it was done before. >>>>>>>>>>> >>>>>>>>>>> As I said it cleanly worked with 2 OFBiz instances running in the >>>>>>>>>>> background. >>>>>>>>>>> >>>>>>>>>>> Now tell me what's wrong? >>>>>>>>>>> >>>>>>>>>>> Thanks >>>>>>>>>>> >>>>>>>>>>> Jacques >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Le 25/06/2017 à 11:22, Taher Alkhateeb a écrit : >>>>>>>>>>> >>>>>>>>>>> As usual, you refuse to revert because you don't understand the >>>>>>>>>>> code >>>>>>>>>>> >>>>>>>>>>>> and I >>>>>>>>>>>> pay the price of spending my time explaining. I hope you will >>>>>>>>>>>> reconsider >>>>>>>>>>>> this time consuming and non-cooperative behavior. >>>>>>>>>>>> >>>>>>>>>>>> The quick version: >>>>>>>>>>>> - copy and paste antipattern >>>>>>>>>>>> - incorrect conditional checking leading to both blocks getting >>>>>>>>>>>> executed or >>>>>>>>>>>> both blocks not executing >>>>>>>>>>>> >>>>>>>>>>>> Your belief that Gradle fails because java does not expect to be >>>>>>>>>>>> killed >>>>>>>>>>>> is >>>>>>>>>>>> amazing! It means you do not understand what this code is doing >>>>>>>>>>>> and >>>>>>>>>>>> what is >>>>>>>>>>>> causing the failure. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Jun 25, 2017 10:42 AM, "Jacques Le Roux" < >>>>>>>>>>>> [hidden email]> >>>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> What makes you think it's wrong? I tested it locally using 2 >>>>>>>>>>>> background >>>>>>>>>>>> instances and it cleaned worked. >>>>>>>>>>>> >>>>>>>>>>>> I also tried with one standard instance (not in background). It >>>>>>>>>>>> works, >>>>>>>>>>>> and >>>>>>>>>>>> you get this message >>>>>>>>>>>> >>>>>>>>>>>> :ofbiz FAILED >>>>>>>>>>>> FAILURE: Build failed with an exception. >>>>>>>>>>>> * What went wrong: >>>>>>>>>>>> Execution failed for task ':ofbiz'. >>>>>>>>>>>> >>>>>>>>>>>> Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished >>>>>>>>>>>> >>>>>>>>>>>>> with >>>>>>>>>>>>> >>>>>>>>>>>>> non-zero exit value 137 >>>>>>>>>>>>> >>>>>>>>>>>> Which I believe is OK because Java does not expect to be killed! >>>>>>>>>>>> >>>>>>>>>>>> Jacques >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit : >>>>>>>>>>>> >>>>>>>>>>>> This commit is wrong and bad on multiple levels. Please revert >>>>>>>>>>>> >>>>>>>>>>>> On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> Author: jleroux >>>>>>>>>>>>> >>>>>>>>>>>>> Date: Sat Jun 24 07:56:45 2017 >>>>>>>>>>>>> >>>>>>>>>>>>>> New Revision: 1799736 >>>>>>>>>>>>>> >>>>>>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1799736&view=rev >>>>>>>>>>>>>> Log: >>>>>>>>>>>>>> No functional change >>>>>>>>>>>>>> >>>>>>>>>>>>>> Improves terminateOfbiz byt using TERM before KILL >>>>>>>>>>>>>> https://fr.wikipedia.org/wiki/Kill_(Unix) >>>>>>>>>>>>>> https://unix.stackexchange.com/questions/8916/when- >>>>>>>>>>>>>> should-i-not-kill-9-a-process >>>>>>>>>>>>>> >>>>>>>>>>>>>> Modified: >>>>>>>>>>>>>> ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>>>>>>>>> >>>>>>>>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>>>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>>>>>>>>>>> build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff >>>>>>>>>>>>>> ============================================================ >>>>>>>>>>>>>> ================== >>>>>>>>>>>>>> --- ofbiz/ofbiz-framework/trunk/build.gradle (original) >>>>>>>>>>>>>> +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 >>>>>>>>>>>>>> 07:56:45 >>>>>>>>>>>>>> 2017 >>>>>>>>>>>>>> @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, >>>>>>>>>>>>>> standardOutput = processOutput >>>>>>>>>>>>>> } >>>>>>>>>>>>>> processOutput.toString().split(System.lineSeparator()).each >>>>>>>>>>>>>> { line -> >>>>>>>>>>>>>> + // Try to terminate cleanly >>>>>>>>>>>>>> if (line ==~ >>>>>>>>>>>>>> /.*org\.apache\.ofbiz\.base\.s >>>>>>>>>>>>>> tart\.Start.*/) >>>>>>>>>>>>>> { >>>>>>>>>>>>>> - exec { commandLine 'kill', '-9', >>>>>>>>>>>>>> line.tokenize().first() } >>>>>>>>>>>>>> + exec { commandLine 'kill', '-TERM', >>>>>>>>>>>>>> line.tokenize().first() } >>>>>>>>>>>>>> + } >>>>>>>>>>>>>> + // Only kill if needed >>>>>>>>>>>>>> + if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>>>>>>>>>> tart\.Start.*/) >>>>>>>>>>>>>> { >>>>>>>>>>>>>> + exec { commandLine 'kill', '-KILL', >>>>>>>>>>>>>> line.tokenize().first() } >>>>>>>>>>>>>> } >>>>>>>>>>>>>> } >>>>>>>>>>>>>> } >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> >> >> > |
Re: A problem with terminateOfbiz Gradle task (was [Re: svn commit: r1799736 - /ofbiz/ofbiz-framework/trunk/b
|
Administrator
|
OK, I think nobody else cares because you indeed rarely run several OFBiz instances on the same machine, apart in demos and maybe in development.
So I'll create a script specific to demos where I need to sometimes kill only one of the 3 instances and not all. Jacques Le 27/06/2017 à 15:12, Taher Alkhateeb a écrit : > I think terminateOfbiz should NOT be portoffset-aware nor should it > kill only one instance. terminateOfbiz is a convenience task to kill > all instances which is mostly used during development. That's the only > purpose it has. I believe it should NOT be used in any automation > suite or buildbot or anything like that. > > I have already spent a lot of time correcting your errors in that task > [1]. Please let's try to avoid repeating this time consuming process. > > [1] https://s.apache.org/fvBB > > On Tue, Jun 27, 2017 at 3:39 PM, Jacques Le Roux > <[hidden email]> wrote: >> We still have a problem with terminateOfbiz: it's not portoffset aware. >> >> So when you run it, it closes all OFBiz instances running on a machine (eg >> on VM demo) >> >> I guess we only need to improve terminateOfbiz by passing a portoffset >> parameter >> >> I'll create a Jira and provide a patch for review >> >> Jacques >> >> >> Le 26/06/2017 à 09:37, Jacques Le Roux a écrit : >>> OK, the shutdown command is somehow our SIGTERM, makes sense >>> >>> Jacques >>> >>> >>> Le 26/06/2017 à 08:45, Taher Alkhateeb a écrit : >>>> I do not think we should send SIGTERM in the first place. The proper >>>> termination of OFBiz is with the --shutdown command which cleanly stops >>>> everything and calls the shutdown hook. The gradle task "terminateOfbiz" >>>> should only be used if the shutdown command fails (last resort) as cleary >>>> mentioned in the docs. >>>> >>>> On Jun 26, 2017 9:19 AM, "Jacques Le Roux" <[hidden email]> >>>> wrote: >>>> >>>> Thanks Taher, >>>> >>>> I indeed missed that kill is not an atomic operation. I reverted at >>>> r1799852. >>>> >>>> Before reading your last message I wanted to propose to set a delay >>>> between >>>> the 2 operations. >>>> >>>> So SIGTERM fails. I'd really like to send SIGTERM before killing the >>>> process and only kill it if it's necessary (ie if SIGTERM did not >>>> terminate >>>> the process) >>>> >>>> So we need to catch the SIGTERM exec and send SIGKILL if it fails, right? >>>> >>>> Jacques >>>> >>>> >>>> >>>> Le 25/06/2017 à 16:05, Taher Alkhateeb a écrit : >>>> >>>>> Oh and to prove the second scenario, just add a sleep for a few seconds >>>>> after the SIGTERM kill command and observe how the system will crash: >>>>> >>>>> if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) { >>>>> exec { commandLine 'kill', '-TERM', line.tokenize().first() } >>>>> Thread.sleep(5000) >>>>> } >>>>> >>>>> Again, this shows you how poor the quality of this code is. It >>>>> introduces a >>>>> subtle bug, it repeats an if condition unnecessarily, and the comments >>>>> are >>>>> wrong "Only kill if needed" >>>>> >>>>> On Sun, Jun 25, 2017 at 4:22 PM, Taher Alkhateeb < >>>>> [hidden email] >>>>> >>>>>> wrote: >>>>>> I'm completing my answer for the record to show exactly where your code >>>>>> is >>>>>> faulty and wrong just like in the other previous cases. >>>>>> >>>>>> 1- you have a copy-paste pattern in your code >>>>>> 2- you are calling the kill command twice immediately one after another >>>>>> without checking whether SIGTERM killed the process properly. The whole >>>>>> purpose of doing SIGTERM is to allow cleaning resources before >>>>>> enforcing >>>>>> SIGKILL. You're not giving the JVM an opportunity to call the shutdown >>>>>> hook >>>>>> because you're looking up the same output from the same "ps ax" command >>>>>> for >>>>>> both kill commands. >>>>>> >>>>>> So your code is working almost exactly as if typing: >>>>>> if (line ==~ /.*org\.apache\.ofbiz\.base\.start\.Start.*/) { >>>>>> exec { commandLine 'kill', '-TERM', line.tokenize().first() } >>>>>> exec { commandLine 'kill', '-KILL', line.tokenize().first() } >>>>>> } >>>>>> >>>>>> Now what's happening is that SIGKILL is executing before SIGTERM >>>>>> completes, so the build is successful but OFBiz is killed with SIGKILL >>>>>> not >>>>>> SIGTERM. If SIGTERM finishes very quickly (does not happen with me) >>>>>> then >>>>>> the build system would crash because the exit value of SIGKILL would be >>>>>> 1, >>>>>> not 0 (process not found). So either way it is a problem (either >>>>>> killing >>>>>> with SIGKILL or crashing the build system) >>>>>> >>>>>> Convinced? I hope you can revert now. >>>>>> >>>>>> On Sun, Jun 25, 2017 at 2:20 PM, Jacques Le Roux < >>>>>> [hidden email]> wrote: >>>>>> >>>>>> But what's wrong? It's not clear to me, do you see it Michael? >>>>>>> Not clearly answering this question just adds more work on my side, I >>>>>>> still don't see why I should revert. >>>>>>> >>>>>>> Jacques >>>>>>> >>>>>>> >>>>>>> >>>>>>> Le 25/06/2017 à 13:11, Michael Brohl a écrit : >>>>>>> >>>>>>> I's like to propose the following process to get this worked out >>>>>>> without >>>>>>>> endless ping-pong here on the mailing lists: >>>>>>>> >>>>>>>> 1. revert the change as requested >>>>>>>> >>>>>>>> 2. provide a Jira with patch and review/discuss it there (what's the >>>>>>>> problem, how should it be solved etc.) >>>>>>>> >>>>>>>> 3. come to a conclusion and a thumbsup by other committers >>>>>>>> >>>>>>>> 4. commit the solution. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> >>>>>>>> Michael >>>>>>>> >>>>>>>> >>>>>>>> Am 25.06.17 um 12:36 schrieb Taher Alkhateeb: >>>>>>>> >>>>>>>> OK, I take this question as your refusal to revert and cooperate. I'm >>>>>>>>> done >>>>>>>>> spending time here and I leave it up to the community to see if they >>>>>>>>> want >>>>>>>>> incorrect code in the code base. >>>>>>>>> >>>>>>>>> On Sun, Jun 25, 2017 at 1:34 PM, Jacques Le Roux < >>>>>>>>> [hidden email]> wrote: >>>>>>>>> >>>>>>>>> Mmm but anyway, I remember now how I thought that. >>>>>>>>> >>>>>>>>>> In the 2nd "if", if the process was terminated, kill will just say >>>>>>>>>> that it >>>>>>>>>> can't find the process and exec will return. Else the process will >>>>>>>>>> be >>>>>>>>>> killed. >>>>>>>>>> >>>>>>>>>> What's wrong then? >>>>>>>>>> >>>>>>>>>> Jacques >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Le 25/06/2017 à 12:28, Jacques Le Roux a écrit : >>>>>>>>>> >>>>>>>>>> Ah wait, I see line still contains "start" so will be executed >>>>>>>>>> twice >>>>>>>>>> >>>>>>>>>>> anyway. OK I'll improve that. >>>>>>>>>>> >>>>>>>>>>> Jacques >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Le 25/06/2017 à 12:18, Jacques Le Roux a écrit : >>>>>>>>>>> >>>>>>>>>>> I don't revert without explanations on why I should revert. Sorry, >>>>>>>>>>> >>>>>>>>>>>> but I >>>>>>>>>>>> don't find your explanations clear. >>>>>>>>>>>> >>>>>>>>>>>> My explanation, tell my what's wrong: >>>>>>>>>>>> >>>>>>>>>>>> The 2 "if" blocks are executed sequentially for each line >>>>>>>>>>>> containing >>>>>>>>>>>> "a >>>>>>>>>>>> start"and ignore other lines. I did not change the previous >>>>>>>>>>>> logic, >>>>>>>>>>>> just >>>>>>>>>>>> added a new if. >>>>>>>>>>>> >>>>>>>>>>>> Gradle exec spawns a new process and waits till it ends (this >>>>>>>>>>>> point >>>>>>>>>>>> is >>>>>>>>>>>> important). >>>>>>>>>>>> >>>>>>>>>>>> If the line contains "a start" the 1st "if" try to terminate it. >>>>>>>>>>>> >>>>>>>>>>>> If it worked the 2nd "if" does not execute. This is better than >>>>>>>>>>>> before >>>>>>>>>>>> because it allows the "'start" process "a chance to clean up >>>>>>>>>>>> after >>>>>>>>>>>> itself" >>>>>>>>>>>> (cf unix.stackexchange.com below) >>>>>>>>>>>> >>>>>>>>>>>> If the "start" process is not terminated then it's killed by the >>>>>>>>>>>> 2nd >>>>>>>>>>>> "if", like it was done before. >>>>>>>>>>>> >>>>>>>>>>>> As I said it cleanly worked with 2 OFBiz instances running in the >>>>>>>>>>>> background. >>>>>>>>>>>> >>>>>>>>>>>> Now tell me what's wrong? >>>>>>>>>>>> >>>>>>>>>>>> Thanks >>>>>>>>>>>> >>>>>>>>>>>> Jacques >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Le 25/06/2017 à 11:22, Taher Alkhateeb a écrit : >>>>>>>>>>>> >>>>>>>>>>>> As usual, you refuse to revert because you don't understand the >>>>>>>>>>>> code >>>>>>>>>>>> >>>>>>>>>>>>> and I >>>>>>>>>>>>> pay the price of spending my time explaining. I hope you will >>>>>>>>>>>>> reconsider >>>>>>>>>>>>> this time consuming and non-cooperative behavior. >>>>>>>>>>>>> >>>>>>>>>>>>> The quick version: >>>>>>>>>>>>> - copy and paste antipattern >>>>>>>>>>>>> - incorrect conditional checking leading to both blocks getting >>>>>>>>>>>>> executed or >>>>>>>>>>>>> both blocks not executing >>>>>>>>>>>>> >>>>>>>>>>>>> Your belief that Gradle fails because java does not expect to be >>>>>>>>>>>>> killed >>>>>>>>>>>>> is >>>>>>>>>>>>> amazing! It means you do not understand what this code is doing >>>>>>>>>>>>> and >>>>>>>>>>>>> what is >>>>>>>>>>>>> causing the failure. >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On Jun 25, 2017 10:42 AM, "Jacques Le Roux" < >>>>>>>>>>>>> [hidden email]> >>>>>>>>>>>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> What makes you think it's wrong? I tested it locally using 2 >>>>>>>>>>>>> background >>>>>>>>>>>>> instances and it cleaned worked. >>>>>>>>>>>>> >>>>>>>>>>>>> I also tried with one standard instance (not in background). It >>>>>>>>>>>>> works, >>>>>>>>>>>>> and >>>>>>>>>>>>> you get this message >>>>>>>>>>>>> >>>>>>>>>>>>> :ofbiz FAILED >>>>>>>>>>>>> FAILURE: Build failed with an exception. >>>>>>>>>>>>> * What went wrong: >>>>>>>>>>>>> Execution failed for task ':ofbiz'. >>>>>>>>>>>>> >>>>>>>>>>>>> Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished >>>>>>>>>>>>> >>>>>>>>>>>>>> with >>>>>>>>>>>>>> >>>>>>>>>>>>>> non-zero exit value 137 >>>>>>>>>>>>>> >>>>>>>>>>>>> Which I believe is OK because Java does not expect to be killed! >>>>>>>>>>>>> >>>>>>>>>>>>> Jacques >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit : >>>>>>>>>>>>> >>>>>>>>>>>>> This commit is wrong and bad on multiple levels. Please revert >>>>>>>>>>>>> >>>>>>>>>>>>> On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: >>>>>>>>>>>>>> Author: jleroux >>>>>>>>>>>>>> >>>>>>>>>>>>>> Date: Sat Jun 24 07:56:45 2017 >>>>>>>>>>>>>> >>>>>>>>>>>>>>> New Revision: 1799736 >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1799736&view=rev >>>>>>>>>>>>>>> Log: >>>>>>>>>>>>>>> No functional change >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Improves terminateOfbiz byt using TERM before KILL >>>>>>>>>>>>>>> https://fr.wikipedia.org/wiki/Kill_(Unix) >>>>>>>>>>>>>>> https://unix.stackexchange.com/questions/8916/when- >>>>>>>>>>>>>>> should-i-not-kill-9-a-process >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Modified: >>>>>>>>>>>>>>> ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/build.gradle >>>>>>>>>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>>>>>>>>>>>> build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff >>>>>>>>>>>>>>> ============================================================ >>>>>>>>>>>>>>> ================== >>>>>>>>>>>>>>> --- ofbiz/ofbiz-framework/trunk/build.gradle (original) >>>>>>>>>>>>>>> +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 >>>>>>>>>>>>>>> 07:56:45 >>>>>>>>>>>>>>> 2017 >>>>>>>>>>>>>>> @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, >>>>>>>>>>>>>>> standardOutput = processOutput >>>>>>>>>>>>>>> } >>>>>>>>>>>>>>> processOutput.toString().split(System.lineSeparator()).each >>>>>>>>>>>>>>> { line -> >>>>>>>>>>>>>>> + // Try to terminate cleanly >>>>>>>>>>>>>>> if (line ==~ >>>>>>>>>>>>>>> /.*org\.apache\.ofbiz\.base\.s >>>>>>>>>>>>>>> tart\.Start.*/) >>>>>>>>>>>>>>> { >>>>>>>>>>>>>>> - exec { commandLine 'kill', '-9', >>>>>>>>>>>>>>> line.tokenize().first() } >>>>>>>>>>>>>>> + exec { commandLine 'kill', '-TERM', >>>>>>>>>>>>>>> line.tokenize().first() } >>>>>>>>>>>>>>> + } >>>>>>>>>>>>>>> + // Only kill if needed >>>>>>>>>>>>>>> + if (line ==~ /.*org\.apache\.ofbiz\.base\.s >>>>>>>>>>>>>>> tart\.Start.*/) >>>>>>>>>>>>>>> { >>>>>>>>>>>>>>> + exec { commandLine 'kill', '-KILL', >>>>>>>>>>>>>>> line.tokenize().first() } >>>>>>>>>>>>>>> } >>>>>>>>>>>>>>> } >>>>>>>>>>>>>>> } >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>> |
Re: svn commit: r1799736 - /ofbiz/ofbiz-framework/trunk/b
|
|
In reply to this post by taher
Hi Taher,
I can understand your frustration, but this: > This commit is wrong and bad on multiple levels. Please revert is an unproductive and unreasonable request. Whether you want to or not, you have to justify the request. Obviously the developer doesn't know what is wrong or they wouldn't have committed it in the first place. Asking them to revert without giving even a brief explanation of what you find wrong leaves the committer completely in the dark and it isn't how vetoes are supposed to work. Asking for blind obedience to your demands to save you the time of explaining them will never fly with any committer. Again, I understand your frustration and I've been there myself many times in the past but being hostile accomplishes nothing other than your ability to work productively with other committers in the future. I can say from bitter experience that it does nothing to improve the behavior of whoever you're communicating with and if you're being obviously unreasonable it will damage your standing with other contributors as well. Regards Scott On 25 June 2017 at 21:22, Taher Alkhateeb <[hidden email]> wrote: > As usual, you refuse to revert because you don't understand the code and I > pay the price of spending my time explaining. I hope you will reconsider > this time consuming and non-cooperative behavior. > > The quick version: > - copy and paste antipattern > - incorrect conditional checking leading to both blocks getting executed or > both blocks not executing > > Your belief that Gradle fails because java does not expect to be killed is > amazing! It means you do not understand what this code is doing and what is > causing the failure. > > > On Jun 25, 2017 10:42 AM, "Jacques Le Roux" <[hidden email]> > wrote: > > What makes you think it's wrong? I tested it locally using 2 background > instances and it cleaned worked. > > I also tried with one standard instance (not in background). It works, and > you get this message > > :ofbiz FAILED > FAILURE: Build failed with an exception. > * What went wrong: > Execution failed for task ':ofbiz'. > > Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished with > non-zero exit value 137 > > Which I believe is OK because Java does not expect to be killed! > > Jacques > > > > Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit : > > > This commit is wrong and bad on multiple levels. Please revert > > > > On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: > > > > Author: jleroux > >> Date: Sat Jun 24 07:56:45 2017 > >> New Revision: 1799736 > >> > >> URL: http://svn.apache.org/viewvc?rev=1799736&view=rev > >> Log: > >> No functional change > >> > >> Improves terminateOfbiz byt using TERM before KILL > >> https://fr.wikipedia.org/wiki/Kill_(Unix) > >> https://unix.stackexchange.com/questions/8916/when- > >> should-i-not-kill-9-a-process > >> > >> Modified: > >> ofbiz/ofbiz-framework/trunk/build.gradle > >> > >> Modified: ofbiz/ofbiz-framework/trunk/build.gradle > >> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ > >> build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff > >> ============================================================ > >> ================== > >> --- ofbiz/ofbiz-framework/trunk/build.gradle (original) > >> +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 07:56:45 2017 > >> @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, > >> standardOutput = processOutput > >> } > >> processOutput.toString().split(System.lineSeparator()). > each > >> { line -> > >> + // Try to terminate cleanly > >> if (line ==~ /.*org\.apache\.ofbiz\.base\.s > >> tart\.Start.*/) > >> { > >> - exec { commandLine 'kill', '-9', > >> line.tokenize().first() } > >> + exec { commandLine 'kill', '-TERM', > >> line.tokenize().first() } > >> + } > >> + // Only kill if needed > >> + if (line ==~ /.*org\.apache\.ofbiz\.base\.s > >> tart\.Start.*/) > >> { > >> + exec { commandLine 'kill', '-KILL', > >> line.tokenize().first() } > >> } > >> } > >> } > >> > >> > >> > >> > |
|
|
Hi Scott,
I really appreciate your feedback, especially with respect to how other contributors might react. It is important to stress to all contributors and committers that I'm a deep believer in community work and I look forward to learning from and working with all of you. I warmly remember the initiative that Sharan and I took with the community in [1]. I also raise my hand as the first person to make mistakes, both in code and also in communicating. My language might be a bit harsh, and I will work to tone it down and be more objective. However, I also ask you all to look at things in context. This is not a thread in isolation from what has been going on for a long time. There is a reason and history behind our reactions. To avoid repeating what we discussed in [2] I will just get to the point: Jacques, using his own words "Commits a lot". This machine-gun commit pattern means more quantity and less quality which is the reason why he gets vetoes. In fact, I cannot even remember when / if I vetoed others. Committing faulty / bad quality code in addition to being degrading to the project, is also time consuming for other contributors / committers to review. A time which could be spent writing code, refactoring and doing useful stuff. If you are providing quality commits consistently and then you make an error then I wouldn't react the same way to my reaction here. My perception of the reason you said you "I understand your frustration" is that you see this pattern as clearly as I do. In many occasions, I make an objection with (using your words) a "brief description" for a certain commit, but end up spending lots of time arguing because Jacques would refuse to revert. So you would expect after all these incidents that either a) he will be more careful or b) he would be more willing to revert and cooperate; neither seems to be happening. Jacques has an immense amount of energy that I really wish we can positively utilize in the community by just slowing down and being a bit more careful. I do not enjoy nor do I find it the best use of my time to review commits and veto them, and it would be actually much easier to just ignore all this stuff and say to hell with it. So please try understand where I'm coming from. Finally, I apologize for anyone who was displeased with my tone, and again I look forward to working with all of you. Cheers, [1] https://cwiki.apache.org/confluence/display/OFBIZ/Re-Factor+To-Do+List [2] https://s.apache.org/jB5f Taher Alkhateeb On Thu, Jun 29, 2017 at 1:13 AM, Scott Gray <[hidden email]> wrote: > Hi Taher, > > I can understand your frustration, but this: > >> This commit is wrong and bad on multiple levels. Please revert > > > is an unproductive and unreasonable request. Whether you want to or not, > you have to justify the request. Obviously the developer doesn't know what > is wrong or they wouldn't have committed it in the first place. Asking > them to revert without giving even a brief explanation of what you find > wrong leaves the committer completely in the dark and it isn't how vetoes > are supposed to work. Asking for blind obedience to your demands to save > you the time of explaining them will never fly with any committer. > > Again, I understand your frustration and I've been there myself many times > in the past but being hostile accomplishes nothing other than your ability > to work productively with other committers in the future. I can say from > bitter experience that it does nothing to improve the behavior of whoever > you're communicating with and if you're being obviously unreasonable it > will damage your standing with other contributors as well. > > Regards > Scott > > On 25 June 2017 at 21:22, Taher Alkhateeb <[hidden email]> > wrote: > >> As usual, you refuse to revert because you don't understand the code and I >> pay the price of spending my time explaining. I hope you will reconsider >> this time consuming and non-cooperative behavior. >> >> The quick version: >> - copy and paste antipattern >> - incorrect conditional checking leading to both blocks getting executed or >> both blocks not executing >> >> Your belief that Gradle fails because java does not expect to be killed is >> amazing! It means you do not understand what this code is doing and what is >> causing the failure. >> >> >> On Jun 25, 2017 10:42 AM, "Jacques Le Roux" <[hidden email]> >> wrote: >> >> What makes you think it's wrong? I tested it locally using 2 background >> instances and it cleaned worked. >> >> I also tried with one standard instance (not in background). It works, and >> you get this message >> >> :ofbiz FAILED >> FAILURE: Build failed with an exception. >> * What went wrong: >> Execution failed for task ':ofbiz'. >> > Process 'command '/usr/lib/jvm/java-8-oracle/bin/java'' finished with >> non-zero exit value 137 >> >> Which I believe is OK because Java does not expect to be killed! >> >> Jacques >> >> >> >> Le 24/06/2017 à 20:04, Taher Alkhateeb a écrit : >> >> > This commit is wrong and bad on multiple levels. Please revert >> > >> > On Sat, Jun 24, 2017 at 10:56 AM, <[hidden email]> wrote: >> > >> > Author: jleroux >> >> Date: Sat Jun 24 07:56:45 2017 >> >> New Revision: 1799736 >> >> >> >> URL: http://svn.apache.org/viewvc?rev=1799736&view=rev >> >> Log: >> >> No functional change >> >> >> >> Improves terminateOfbiz byt using TERM before KILL >> >> https://fr.wikipedia.org/wiki/Kill_(Unix) >> >> https://unix.stackexchange.com/questions/8916/when- >> >> should-i-not-kill-9-a-process >> >> >> >> Modified: >> >> ofbiz/ofbiz-framework/trunk/build.gradle >> >> >> >> Modified: ofbiz/ofbiz-framework/trunk/build.gradle >> >> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >> >> build.gradle?rev=1799736&r1=1799735&r2=1799736&view=diff >> >> ============================================================ >> >> ================== >> >> --- ofbiz/ofbiz-framework/trunk/build.gradle (original) >> >> +++ ofbiz/ofbiz-framework/trunk/build.gradle Sat Jun 24 07:56:45 2017 >> >> @@ -332,8 +332,13 @@ task terminateOfbiz(group: ofbizServer, >> >> standardOutput = processOutput >> >> } >> >> processOutput.toString().split(System.lineSeparator()). >> each >> >> { line -> >> >> + // Try to terminate cleanly >> >> if (line ==~ /.*org\.apache\.ofbiz\.base\.s >> >> tart\.Start.*/) >> >> { >> >> - exec { commandLine 'kill', '-9', >> >> line.tokenize().first() } >> >> + exec { commandLine 'kill', '-TERM', >> >> line.tokenize().first() } >> >> + } >> >> + // Only kill if needed >> >> + if (line ==~ /.*org\.apache\.ofbiz\.base\.s >> >> tart\.Start.*/) >> >> { >> >> + exec { commandLine 'kill', '-KILL', >> >> line.tokenize().first() } >> >> } >> >> } >> >> } >> >> >> >> >> >> >> >> >> |
«
Return to OFBiz - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |