jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: svn commit: r1532085 - /jmeter/trunk/src/core/org/apache/jmeter/control/TransactionController.java
Date Sat, 23 Nov 2013 23:10:28 GMT
On 23 November 2013 22:59, Philippe Mouawad <philippe.mouawad@gmail.com> wrote:
> Hello sebb,
> My answers below.
>
>
> On Sat, Nov 23, 2013 at 6:17 PM, sebb <sebbaz@gmail.com> wrote:
>
>> On 23 November 2013 16:22, Philippe Mouawad <philippe.mouawad@gmail.com>
>> wrote:
>> > On Sat, Nov 23, 2013 at 4:41 PM, sebb <sebbaz@gmail.com> wrote:
>> >
>> >> On 23 November 2013 14:03, Philippe Mouawad <philippe.mouawad@gmail.com
>> >
>> >> wrote:
>> >> > Hello,
>> >> > What about introducing a property called:
>> >> >
>> >> >    - transaction_controller.include_processing_and_timers
>> >> >
>> >> >
>> >> > It would allow users who have existing plans to stay with existing
>> >> > behaviour by setting:
>> >> >
>> >> >    - transaction_controller.include_processing_and_timers=true
>> >> >
>> >> >
>> >> > And default value would be false.
>> >>
>> >> That would change the behaviour for existing tests.
>> >>
>> > No if user sets
>>  transaction_controller.include_processing_and_timers=true,
>> > everything remains as currently.
>> > We add a Incompatible change in changes.xml and everything is fine.
>>
>> Yes, I understand that, but it still requires existing users to update
>> their installation.
>> Whereas if the default is true, only users who want the new behaviour
>> need to change their settings.
>>
>
> See https://issues.apache.org/bugzilla/show_bug.cgi?id=55816 attached Test
> Plan.

Not sure that's relevant here. Seems like an obvious bug to me if the
TC does not honour the setting of the include check-box.

> If the default is true, do you find it OK that users need to update their
> installation to have the best behaviour from JMeter when using TC ?

I don't agree that it is necessarily best behaviour to ignore timers.

> I don't. Default should be the best ones, and backward compatibility should
> not be more important than having the right behaviour.

Again, I don't agree that the proposed behaviour is more correct than
the current behaviour.

> I find it already very good that we introduce a special property to keep
> old behaviour.

Fine, but unless the old behaviour is clearly wrong - which I dispute
- the default should be to keep the old behaviour.

>
>>
>> >
>> >> I'm not keen on changing the behaviour unless it is clearly incorrect.
>> >> I don't think that is the case here.
>> >>
>> > It is clearly incorrect because it reports in overall response time of
>> > Transaction Controller the time taken by post processors (I agree if it
>> > reported only timers it would be acceptable) .
>> >
>>
>> It's still the overall time for the TC children.
>>
>
> Yes but do you agree that a user only wants cumulated response time
> excluding any JMeter processing.
>
>
>> If PostProcessors are taking a significant time to run, then that is
>> something to be looked at separately.
>> And there may be no PostProcessors.
>>
>>
>
>
>> >
>> >
>> >>
>> >> > By the way this impact should leed us to change something we've been
>> >> doing
>> >> > up till now which is not to write the value of a property if it is
>> equal
>> >> to
>> >> > default.
>> >> > If we had not done this up until now , we could have changed this
>> default
>> >> > with no impact .
>> >>
>> >> I don't think that is true.
>> >>
>> >> AFAICT it's possible for the JMX to have one default and the property
>> >> another.
>> >> However it would probably require the code to check whether the test
>> >> element is brand new or has been created from the JMX.
>> >>
>> >
>> > My proposition is to always write the value wether equals or not to
>> > default.
>>
>> Which makes the JMX files much larger.
>> Also when the JMX file is saved it will change; if users store their
>> JMX files in an SCM system, it will generate unnecessary changes.
>>
>> >>
>> >> It would also have to be carefully documented as we have not done this
>> so
>> >> far.
>> >>
>> >> > Regards
>> >> > Philippe
>> >> >
>> >> >
>> >> >
>> >> > On Tue, Oct 15, 2013 at 4:46 PM, sebb <sebbaz@gmail.com> wrote:
>> >> >
>> >> >> On 14 October 2013 22:05, Philippe Mouawad <
>> philippe.mouawad@gmail.com>
>> >> >> wrote:
>> >> >> > The problem is that if you change it , then automatically
it will
>> >> change
>> >> >> > behaviour of current scripts I think.
>> >> >>
>> >> >> Yes.
>> >> >>
>> >> >> > As when reloading existing script,
>> >> >> > getPropertyAsBoolean(INCLUDE_TIMERS,
>> >> DEFAULT_VALUE_FOR_INCLUDE_TIMERS);
>> >> >> > will return DEFAULT_VALUE_FOR_INCLUDE_TIMERS , which will
be false
>> >> while
>> >> >> in
>> >> >> > fact while its previous meaning when user saved his script
was
>> true.
>> >> >>
>> >> >> The value of the default cannot be changed without affecting existing
>> >> >> test plans.
>> >> >>
>> >> >> > See https://issues.apache.org/bugzilla/show_bug.cgi?id=55498
>> >> >> >
>> >> >> > In my opinion this should be changed adding an Incompatible
change
>> >> but we
>> >> >> > would need to accept that users who really need it have to
modify
>> >> their
>> >> >> > script.
>> >> >> >
>> >> >> > +1 for this breaking change.
>> >> >> >
>> >> >> > Regards
>> >> >> > Philippe
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> > On Mon, Oct 14, 2013 at 10:56 PM, Milamber <milamber@apache.org>
>> >> wrote:
>> >> >> >
>> >> >> >>
>> >> >> >> Le 14/10/2013 21:47, pmouawad@apache.org a ecrit :
>> >> >> >>
>> >> >> >>  Author: pmouawad
>> >> >> >>> Date: Mon Oct 14 20:47:58 2013
>> >> >> >>> New Revision: 1532085
>> >> >> >>>
>> >> >> >>> URL: http://svn.apache.org/r1532085
>> >> >> >>> Log:
>> >> >> >>> Add constant
>> >> >> >>>
>> >> >> >>
>> >> >> >> Perhaps, It would be great to transform this default value
to a
>> >> property
>> >> >> >> for allow user to change this.
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >>> Modified:
>> >> >> >>>      jmeter/trunk/src/core/org/**apache/jmeter/control/**
>> >> >> >>> TransactionController.java
>> >> >> >>>
>> >> >> >>> Modified: jmeter/trunk/src/core/org/**apache/jmeter/control/**
>> >> >> >>> TransactionController.java
>> >> >> >>> URL: http://svn.apache.org/viewvc/**jmeter/trunk/src/core/org/**
>> >> >> >>> apache/jmeter/control/**TransactionController.java?**
>> >> >> >>> rev=1532085&r1=1532084&r2=**1532085&view=diff<
>> >> >>
>> >>
>> http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/control/TransactionController.java?rev=1532085&r1=1532084&r2=1532085&view=diff
>> >> >> >
>> >> >> >>> ==============================**==============================**
>> >> >> >>> ==================
>> >> >> >>> ---
>> >> >>
>> >>
>> jmeter/trunk/src/core/org/**apache/jmeter/control/**TransactionController.java
>> >> >> >>> (original)
>> >> >> >>> +++
>> >> >>
>> >>
>> jmeter/trunk/src/core/org/**apache/jmeter/control/**TransactionController.java
>> >> >> >>> Mon Oct 14 20:47:58 2013
>> >> >> >>> @@ -53,6 +53,8 @@ public class TransactionController
exten
>> >> >> >>>             private static final Logger log = LoggingManager.**
>> >> >> >>> getLoggerForClass();
>> >> >> >>>   +    private static final boolean
>> >> DEFAULT_VALUE_FOR_INCLUDE_**TIMERS
>> >> >> =
>> >> >> >>> true; // default true for compatibility
>> >> >> >>> +
>> >> >> >>>       /**
>> >> >> >>>        * Only used in parent Mode
>> >> >> >>>        */
>> >> >> >>> @@ -293,7 +295,7 @@ public class TransactionController
exten
>> >> >> >>>        * @param includeTimers
>> >> >> >>>        */
>> >> >> >>>       public void setIncludeTimers(boolean includeTimers)
{
>> >> >> >>> -        setProperty(INCLUDE_TIMERS, includeTimers,
true); //
>> >> default
>> >> >> >>> true for compatibility
>> >> >> >>> +        setProperty(INCLUDE_TIMERS, includeTimers,
>> >> >> >>> DEFAULT_VALUE_FOR_INCLUDE_**TIMERS);
>> >> >> >>>       }
>> >> >> >>>         /**
>> >> >> >>> @@ -302,6 +304,6 @@ public class TransactionController
exten
>> >> >> >>>        * @return boolean (defaults to true for backwards
>> >> compatibility)
>> >> >> >>>        */
>> >> >> >>>       public boolean isIncludeTimers() {
>> >> >> >>> -        return getPropertyAsBoolean(INCLUDE_**TIMERS,
true);
>> >> >> >>> +        return getPropertyAsBoolean(INCLUDE_**TIMERS,
>> >> >> >>> DEFAULT_VALUE_FOR_INCLUDE_**TIMERS);
>> >> >> >>>       }
>> >> >> >>>   }
>> >> >> >>>
>> >> >> >>>
>> >> >> >>>
>> >> >> >>>
>> >> >> >>
>> >> >> >
>> >> >> >
>> >> >> > --
>> >> >> > Cordialement.
>> >> >> > Philippe Mouawad.
>> >> >>
>> >> >
>> >> >
>> >> >
>> >> > --
>> >> > Cordialement.
>> >> > Philippe Mouawad.
>> >>
>> >
>> >
>> >
>> > --
>> > Cordialement.
>> > Philippe Mouawad.
>>
>
>
>
> --
> Cordialement.
> Philippe Mouawad.

Mime
View raw message