You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by sebb <se...@gmail.com> on 2014/10/09 03:32:45 UTC
Re: svn commit: r1630232 - /jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java
On 8 October 2014 22:01, <pm...@apache.org> wrote:
> Author: pmouawad
> Date: Wed Oct 8 21:01:52 2014
> New Revision: 1630232
>
> URL: http://svn.apache.org/r1630232
> Log:
> Throw when configuration error (related to 57068)
>
> Modified:
> jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java
>
> Modified: jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java
> URL: http://svn.apache.org/viewvc/jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java?rev=1630232&r1=1630231&r2=1630232&view=diff
> ==============================================================================
> --- jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java (original)
> +++ jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java Wed Oct 8 21:01:52 2014
> @@ -159,8 +159,10 @@ public class SyncTimer extends AbstractT
> try {
> if(timeoutInMs==0) {
> arrival = this.barrier.await();
> - } else {
> + } else if(timeoutInMs > 0){
> arrival = this.barrier.await(timeoutInMs, TimeUnit.MILLISECONDS);
> + } else {
> + throw new IllegalArgumentException("Negative value for timeout:"+timeoutInMs+" in Synchronizing Timer "+getName());
-1
This is the wrong place to check the value. It should be done in the
GUI and/or the setter method, not at run-time.
> }
> } catch (InterruptedException e) {
> return 0;
>
>
Re: svn commit: r1630232 - /jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java
Posted by sebb <se...@gmail.com>.
On 9 October 2014 20:39, sebb <se...@gmail.com> wrote:
> On 9 October 2014 20:27, Philippe Mouawad <ph...@gmail.com> wrote:
>> On Thu, Oct 9, 2014 at 3:32 AM, sebb <se...@gmail.com> wrote:
>>
>>> On 8 October 2014 22:01, <pm...@apache.org> wrote:
>>> > Author: pmouawad
>>> > Date: Wed Oct 8 21:01:52 2014
>>> > New Revision: 1630232
>>> >
>>> > URL: http://svn.apache.org/r1630232
>>> > Log:
>>> > Throw when configuration error (related to 57068)
>>> >
>>> > Modified:
>>> > jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java
>>> >
>>> > Modified:
>>> jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java
>>> > URL:
>>> http://svn.apache.org/viewvc/jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java?rev=1630232&r1=1630231&r2=1630232&view=diff
>>> >
>>> ==============================================================================
>>> > --- jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java
>>> (original)
>>> > +++ jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java
>>> Wed Oct 8 21:01:52 2014
>>> > @@ -159,8 +159,10 @@ public class SyncTimer extends AbstractT
>>> > try {
>>> > if(timeoutInMs==0) {
>>> > arrival = this.barrier.await();
>>> > - } else {
>>> > + } else if(timeoutInMs > 0){
>>> > arrival = this.barrier.await(timeoutInMs,
>>> TimeUnit.MILLISECONDS);
>>> > + } else {
>>> > + throw new IllegalArgumentException("Negative value
>>> for timeout:"+timeoutInMs+" in Synchronizing Timer "+getName());
>>>
>>> -1
>>>
>>> This is the wrong place to check the value. It should be done in the
>>> GUI and/or the setter method, not at run-time.
>>>
>>> Setter does not seem to be a right place for me, setter should only be a
>> stupid method
>> Gui won't catch existing misconfiguration.
>
> But that does not mean we should allow the GUI to set a negative number.
>
>> The performance impact is low considering it should only happen in wrong
>> case.
>
> The performance impact is not at issue here.
> The point is that the code should try and catch the bug as early as possible.
>
> It's hard work trying to link an error in the log file to a GUI field.
There's another aspect to this - why should the incorrect value cause
the test to fail?
Would it not be better to just log the error and continue, especially
if the GUI does not provide validation?
>> Feel free to find a better way.
>>
>>> > }
>>> > } catch (InterruptedException e) {
>>> > return 0;
>>> >
>>> >
>>>
>>
>>
>>
>> --
>> Cordialement.
>> Philippe Mouawad.
Re: svn commit: r1630232 - /jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java
Posted by sebb <se...@gmail.com>.
On 9 October 2014 20:27, Philippe Mouawad <ph...@gmail.com> wrote:
> On Thu, Oct 9, 2014 at 3:32 AM, sebb <se...@gmail.com> wrote:
>
>> On 8 October 2014 22:01, <pm...@apache.org> wrote:
>> > Author: pmouawad
>> > Date: Wed Oct 8 21:01:52 2014
>> > New Revision: 1630232
>> >
>> > URL: http://svn.apache.org/r1630232
>> > Log:
>> > Throw when configuration error (related to 57068)
>> >
>> > Modified:
>> > jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java
>> >
>> > Modified:
>> jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java
>> > URL:
>> http://svn.apache.org/viewvc/jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java?rev=1630232&r1=1630231&r2=1630232&view=diff
>> >
>> ==============================================================================
>> > --- jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java
>> (original)
>> > +++ jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java
>> Wed Oct 8 21:01:52 2014
>> > @@ -159,8 +159,10 @@ public class SyncTimer extends AbstractT
>> > try {
>> > if(timeoutInMs==0) {
>> > arrival = this.barrier.await();
>> > - } else {
>> > + } else if(timeoutInMs > 0){
>> > arrival = this.barrier.await(timeoutInMs,
>> TimeUnit.MILLISECONDS);
>> > + } else {
>> > + throw new IllegalArgumentException("Negative value
>> for timeout:"+timeoutInMs+" in Synchronizing Timer "+getName());
>>
>> -1
>>
>> This is the wrong place to check the value. It should be done in the
>> GUI and/or the setter method, not at run-time.
>>
>> Setter does not seem to be a right place for me, setter should only be a
> stupid method
> Gui won't catch existing misconfiguration.
But that does not mean we should allow the GUI to set a negative number.
> The performance impact is low considering it should only happen in wrong
> case.
The performance impact is not at issue here.
The point is that the code should try and catch the bug as early as possible.
It's hard work trying to link an error in the log file to a GUI field.
> Feel free to find a better way.
>
>> > }
>> > } catch (InterruptedException e) {
>> > return 0;
>> >
>> >
>>
>
>
>
> --
> Cordialement.
> Philippe Mouawad.
Re: svn commit: r1630232 - /jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java
Posted by Philippe Mouawad <ph...@gmail.com>.
On Thu, Oct 9, 2014 at 3:32 AM, sebb <se...@gmail.com> wrote:
> On 8 October 2014 22:01, <pm...@apache.org> wrote:
> > Author: pmouawad
> > Date: Wed Oct 8 21:01:52 2014
> > New Revision: 1630232
> >
> > URL: http://svn.apache.org/r1630232
> > Log:
> > Throw when configuration error (related to 57068)
> >
> > Modified:
> > jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java
> >
> > Modified:
> jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java?rev=1630232&r1=1630231&r2=1630232&view=diff
> >
> ==============================================================================
> > --- jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java
> (original)
> > +++ jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java
> Wed Oct 8 21:01:52 2014
> > @@ -159,8 +159,10 @@ public class SyncTimer extends AbstractT
> > try {
> > if(timeoutInMs==0) {
> > arrival = this.barrier.await();
> > - } else {
> > + } else if(timeoutInMs > 0){
> > arrival = this.barrier.await(timeoutInMs,
> TimeUnit.MILLISECONDS);
> > + } else {
> > + throw new IllegalArgumentException("Negative value
> for timeout:"+timeoutInMs+" in Synchronizing Timer "+getName());
>
> -1
>
> This is the wrong place to check the value. It should be done in the
> GUI and/or the setter method, not at run-time.
>
> Setter does not seem to be a right place for me, setter should only be a
stupid method
Gui won't catch existing misconfiguration.
The performance impact is low considering it should only happen in wrong
case.
Feel free to find a better way.
> > }
> > } catch (InterruptedException e) {
> > return 0;
> >
> >
>
--
Cordialement.
Philippe Mouawad.