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 Sun, 24 Nov 2013 00:12:22 GMT
On 23 November 2013 23:43, Philippe Mouawad <philippe.mouawad@gmail.com> wrote:
> On Sun, Nov 24, 2013 at 12:10 AM, sebb <sebbaz@gmail.com> wrote:
>
>> 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.
>>
>> It was just to point to reported response times when Include is checked.
> In Test plan I simulate processing that takes more than 1 second, and you
> can see impact on reported response time.

Well yes, I understand that.

>
>> > 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 usually use (And I don't think I am alone) TC as a wrapper to samplers
> (not always, but whenever I have more than one sample in a business
> Transaction).
> If I don't uncheck Include... then it would mean I would be reporting
> response time degraded by Post Processors behaviour (which are always
> present for Response Assertions for example).

It's actually very easy to change the default for NEW TC elements:

Index: TransactionControllerGui.java
===================================================================
--- TransactionControllerGui.java    (revision 1537781)
+++ TransactionControllerGui.java    (working copy)
@@ -48,6 +48,7 @@
     @Override
     public TestElement createTestElement() {
         TransactionController lc = new TransactionController();
+        lc.setIncludeTimers(false); // change default for new test elements
         configureTestElement(lc);
         return lc;
     }

This won't change existing behaviour or JMX file contents when saving
an amended test plan.

It remains to be decided whether the value should be controlled via a
property with the default of false, or whether it is better to just
clear the checkbox unconditionally (as per the patch above).

In either case, the change needs to be documented in changes.xml and
component_reference

>>
>> > 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.
>>
>
>
>
> --
> Cordialement.
> Philippe Mouawad.

Mime
View raw message